cpufreq.chart.py: add more accurate cpufreq_stats-based calculations

There were two major problems with this module:

- The 'cpuN' names weren't accurate. The 'self.paths.sort()' was trying
  to compensate for os.walk() enumerating the CPUs out of order (as any
  directory enumeration will do). Unfortunately the sort() function is
  alphabetical, so it would result in a list of paths like this:

    [
        '/sys/.../cpu0/...'
        '/sys/.../cpu1/...'
        '/sys/.../cpu11/...'
        '/sys/.../cpu12/...'
        ...
    ]

  So the chart for cpu2 would actually map to cpu11's stats.

  This can be corrected by extracting the 'cpuN' value that's already
  inside the path anyway.

- The scaling_cur_freq value is an instantaneous value. It only
  represents the current processor P-state at the time it was read, and
  doesn't account for the other 999ms that netdata wasn't looking at the
  value.

  This can be corrected by using data from cpufreq_stats, which includes
  P-state residency statistics. Note that the values in cpufreq_stats
  aren't always valid (e.g. if the cpufreq governor is set to
  'schedutil', the statistic files exist but are are empty), so we can
  just fall back to the inaccurate scaling_cur_freq method if necessary.

Signed-off-by: Steven Noonan <steven@uplinklabs.net>

diff --git a/python.d/README.md b/python.d/README.md
index b7fb4e9f4706a04f9116f20f7cc4ef2ec61d7227..9e18f3eb8c9231d179074cf9e2bdf1abc6706fd8 100644 (file)
--- a/python.d/README.md
+++ b/python.d/README.md
@@ -186,13 +186,21 @@ If no configuration is given, module will attempt to read named.stats file  at `
 
 ---
 
-
 # cpufreq
 
-Module shows current cpu frequency by looking at appropriate files in /sys/devices
+This module shows the current CPU frequency as set by the cpufreq kernel
+module.
 
 **Requirement:**
-Processor which presents data scaling frequency data
+You need to have `CONFIG_CPU_FREQ` and (optionally) `CONFIG_CPU_FREQ_STAT`
+enabled in your kernel.
+
+This module tries to read from one of two possible locations. On
+initialization, it tries to read the `time_in_state` files provided by
+cpufreq\_stats. If this file does not exist, or doesn't contain valid data, it
+falls back to using the more inaccurate `scaling_cur_freq` file (which only
+represents the **current** CPU frequency, and doesn't account for any state
+changes which happen between updates).
 
 It produces one chart with multiple lines (one line per core).
 
@@ -204,7 +212,7 @@ Sample:
 sys_dir: "/sys/devices"
 ```
 
-If no configuration is given, module will search for `scaling_cur_freq` files in `/sys/devices` directory.
+If no configuration is given, module will search for cpufreq files in `/sys/devices` directory.
 Directory is also prefixed with `NETDATA_HOST_PREFIX` if specified.
 
 ---

diff --git a/python.d/cpufreq.chart.py b/python.d/cpufreq.chart.py
index a9de5ceddcf7c010c214c7463919e23a9d1117b8..ef10c425143e76a42c7caccdc218e8d565e42e28 100644 (file)
--- a/python.d/cpufreq.chart.py
+++ b/python.d/cpufreq.chart.py
@@ -1,7 +1,8 @@
 # -*- coding: utf-8 -*-
 # Description: cpufreq netdata python.d module
-# Author: Pawel Krupa (paulfantom)
+# Author: Pawel Krupa (paulfantom) and Steven Noonan (tycho)
 
+import glob
 import os
 from base import SimpleService
 
@@ -18,29 +19,31 @@ CHARTS = {
         ]}
 }
 
-
 class Service(SimpleService):
     def __init__(self, configuration=None, name=None):
         prefix = os.getenv('NETDATA_HOST_PREFIX', "")
         if prefix.endswith('/'):
             prefix = prefix[:-1]
         self.sys_dir = prefix + "/sys/devices"
-        self.filename = "scaling_cur_freq"
         SimpleService.__init__(self, configuration=configuration, name=name)
         self.order = ORDER
         self.definitions = CHARTS
         self._orig_name = ""
         self.assignment = {}
-        self.paths = []
+        self.accurate = True
 
     def _get_data(self):
-        raw = {}
-        for path in self.paths:
-            with open(path, 'r') as f:
-                raw[path] = f.read()
         data = {}
-        for path in self.paths:
-            data[self.assignment[path]] = raw[path]
+        if self.accurate:
+            for name, path in self.assignment.items():
+                total = 0
+                for line in open(path, 'r'):
+                    line = list(map(int, line.split()))
+                    total += (line[0] * line[1]) / 100
+                data[name] = total
+        else:
+            for name, path in self.assignment.items():
+                data[name] = open(path, 'r').read()
         return data
 
     def check(self):
@@ -51,23 +54,35 @@ class Service(SimpleService):
 
         self._orig_name = self.chart_name
 
-        for dirpath, _, filenames in os.walk(self.sys_dir):
-            if self.filename in filenames:
-                self.paths.append(dirpath + "/" + self.filename)
-
-        if len(self.paths) == 0:
-            self.error("cannot find", self.filename)
+        for path in glob.glob(self.sys_dir + '/system/cpu/cpu*/cpufreq/stats/time_in_state'):
+            if len(open(path, 'rb').read().rstrip()) == 0:
+                self.alert("time_in_state is empty, broken cpufreq_stats data")
+                self.assignment = {}
+                break
+            path_elem = path.split('/')
+            cpu = path_elem[-4]
+            self.assignment[cpu] = path
+
+        if len(self.assignment) == 0:
+            self.alert("trying less accurate scaling_cur_freq method")
+            self.accurate = False
+
+            for path in glob.glob(self.sys_dir + '/system/cpu/cpu*/cpufreq/scaling_cur_freq'):
+                path_elem = path.split('/')
+                cpu = path_elem[-3]
+                self.assignment[cpu] = path
+
+        if len(self.assignment) == 0:
+            self.error("couldn't find a method to read cpufreq statistics")
             return False
 
-        self.paths.sort()
-        i = 0
-        for path in self.paths:
-            self.assignment[path] = "cpu" + str(i)
-            i += 1
+        if self.accurate:
+            algo = 'incremental'
+        else:
+            algo = 'absolute'
 
-        for name in self.assignment:
-            dim = self.assignment[name]
-            self.definitions[ORDER[0]]['lines'].append([dim, dim, 'absolute', 1, 1000])
+        for name in self.assignment.keys():
+            self.definitions[ORDER[0]]['lines'].append([name, name, algo, 1, 1000])
 
         return True