Ignore negative deltas in cpu times when calculating percentages (#1210) (#1214)

2 files changed, 86 insertions, 31 deletions

diff --git a/psutil/__init__.py b/psutil/__init__.py
index a11988ba..1b0f2c04 100644
--- a/psutil/__init__.py
+++ b/psutil/__init__.py
@@ -1655,6 +1655,27 @@ def _cpu_busy_time(times):
     return busy
 
 
+def _cpu_times_deltas(t1, t2):
+    assert t1._fields == t2._fields, (t1, t2)
+    field_deltas = []
+    for field in _psplatform.scputimes._fields:
+        field_delta = getattr(t2, field) - getattr(t1, field)
+        # CPU times are always supposed to increase over time
+        # or at least remain the same and that's because time
+        # cannot go backwards.
+        # Surprisingly sometimes this might not be the case (at
+        # least on Windows and Linux), see:
+        # https://github.com/giampaolo/psutil/issues/392
+        # https://github.com/giampaolo/psutil/issues/645
+        # https://github.com/giampaolo/psutil/issues/1210
+        # Trim negative deltas to zero to ignore decreasing fields.
+        # top does the same. Reference:
+        # https://gitlab.com/procps-ng/procps/blob/v3.3.12/top/top.c#L5063
+        field_delta = max(0, field_delta)
+        field_deltas.append(field_delta)
+    return _psplatform.scputimes(*field_deltas)
+
+
 def cpu_percent(interval=None, percpu=False):
     """Return a float representing the current system-wide CPU
     utilization as a percentage.
@@ -1697,18 +1718,11 @@ def cpu_percent(interval=None, percpu=False):
         raise ValueError("interval is not positive (got %r)" % interval)
 
     def calculate(t1, t2):
-        t1_all = _cpu_tot_time(t1)
-        t1_busy = _cpu_busy_time(t1)
+        times_delta = _cpu_times_deltas(t1, t2)
 
-        t2_all = _cpu_tot_time(t2)
-        t2_busy = _cpu_busy_time(t2)
+        all_delta = _cpu_tot_time(times_delta)
+        busy_delta = _cpu_busy_time(times_delta)
 
-        # this usually indicates a float precision issue
-        if t2_busy <= t1_busy:
-            return 0.0
-
-        busy_delta = t2_busy - t1_busy
-        all_delta = t2_all - t1_all
         try:
             busy_perc = (busy_delta / all_delta) * 100
         except ZeroDivisionError:
@@ -1777,28 +1791,18 @@ def cpu_times_percent(interval=None, percpu=False):
 
     def calculate(t1, t2):
         nums = []
-        all_delta = _cpu_tot_time(t2) - _cpu_tot_time(t1)
-        for field in t1._fields:
-            field_delta = getattr(t2, field) - getattr(t1, field)
-            try:
-                field_perc = (100 * field_delta) / all_delta
-            except ZeroDivisionError:
-                field_perc = 0.0
+        times_delta = _cpu_times_deltas(t1, t2)
+        all_delta = _cpu_tot_time(times_delta)
+        # "scale" is the value to multiply each delta with to get percentages.
+        # We use "max" to avoid division by zero (if all_delta is 0, then all
+        # fields are 0 so percentages will be 0 too. all_delta cannot be a
+        # fraction because cpu times are integers)
+        scale = 100.0 / max(1, all_delta)
+        for field_delta in times_delta:
+            field_perc = field_delta * scale
             field_perc = round(field_perc, 1)
-            # CPU times are always supposed to increase over time
-            # or at least remain the same and that's because time
-            # cannot go backwards.
-            # Surprisingly sometimes this might not be the case (at
-            # least on Windows and Linux), see:
-            # https://github.com/giampaolo/psutil/issues/392
-            # https://github.com/giampaolo/psutil/issues/645
-            # I really don't know what to do about that except
-            # forcing the value to 0 or 100.
-            if field_perc > 100.0:
-                field_perc = 100.0
-            # `<=` because `-0.0 == 0.0` evaluates to True
-            elif field_perc <= 0.0:
-                field_perc = 0.0
+            # make sure we don't return negative values or values over 100%
+            field_perc = min(max(0.0, field_perc), 100.0)
             nums.append(field_perc)
         return _psplatform.scputimes(*nums)
 
diff --git a/psutil/tests/test_linux.py b/psutil/tests/test_linux.py
index 139b32ab..09063416 100755
--- a/psutil/tests/test_linux.py
+++ b/psutil/tests/test_linux.py
@@ -1090,6 +1090,57 @@ class TestMisc(unittest.TestCase):
 
         self.assertEqual(psutil.PROCFS_PATH, '/proc')
 
+    def test_cpu_steal_decrease(self):
+        # Test cumulative cpu stats decrease. We should ignore this.
+        # See issue #1210.
+
+        def open_mock(name, *args, **kwargs):
+            if name == "/proc/stat":
+                return io.BytesIO(textwrap.dedent("""\
+                    cpu   0 0 0 0 0 0 0 1 0 0
+                    cpu0  0 0 0 0 0 0 0 1 0 0
+                    cpu1  0 0 0 0 0 0 0 1 0 0
+                    """).encode())
+            return orig_open(name, *args, **kwargs)
+
+        orig_open = open
+        patch_point = 'builtins.open' if PY3 else '__builtin__.open'
+
+        with mock.patch(patch_point, create=True, side_effect=open_mock) as m:
+            # first call to "percent" functions should read the new stat file
+            # and compare to the "real" file read at import time - so the
+            # values are meaningless
+            psutil.cpu_percent()
+            assert m.called
+            psutil.cpu_percent(percpu=True)
+            psutil.cpu_times_percent()
+            psutil.cpu_times_percent(percpu=True)
+
+        def open_mock(name, *args, **kwargs):
+            if name == "/proc/stat":
+                return io.BytesIO(textwrap.dedent("""\
+                    cpu   1 0 0 0 0 0 0 0 0 0
+                    cpu0  1 0 0 0 0 0 0 0 0 0
+                    cpu1  1 0 0 0 0 0 0 0 0 0
+                    """).encode())
+            return orig_open(name, *args, **kwargs)
+
+        with mock.patch(patch_point, create=True, side_effect=open_mock) as m:
+            # Increase "user" while steal goes "backwards" to zero.
+            cpu_percent = psutil.cpu_percent()
+            assert m.called
+            cpu_percent_percpu = psutil.cpu_percent(percpu=True)
+            cpu_times_percent = psutil.cpu_times_percent()
+            cpu_times_percent_percpu = psutil.cpu_times_percent(percpu=True)
+            self.assertNotEqual(cpu_percent, 0)
+            self.assertNotEqual(sum(cpu_percent_percpu), 0)
+            self.assertNotEqual(sum(cpu_times_percent), 0)
+            self.assertNotEqual(sum(cpu_times_percent), 100.0)
+            self.assertNotEqual(sum(map(sum, cpu_times_percent_percpu)), 0)
+            self.assertNotEqual(sum(map(sum, cpu_times_percent_percpu)), 100.0)
+            self.assertEqual(cpu_times_percent.steal, 0)
+            self.assertNotEqual(cpu_times_percent.user, 0)
+
     def test_boot_time_mocked(self):
         with mock.patch('psutil._pslinux.open', create=True) as m:
             self.assertRaises(