| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Nov 2020 11:36:13 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Rafael Wysocki <rjw@...ysocki.net>,
Jonathan Corbet <corbet@....net>, linux-pm@...r.kernel.org,
Vincent Guittot <vincent.guittot@...aro.org>,
Thomas Renninger <trenn@...e.com>,
Shuah Khan <shuah@...nel.org>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: stats: Switch to ktime and msec instead of
jiffies and usertime
On 11/10/20 11:07 AM, Viresh Kumar wrote:
> The cpufreq and thermal core, both provide sysfs statistics to help
> userspace learn about the behavior of frequencies and cooling states.
>
> This is how they look:
>
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:208000 11
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:432000 147
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:729000 1600
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:960000 879
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:1200000 399
>
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state0 4097
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state1 8932
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state2 15868
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state3 1384
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state4 103
>
> Here, state0 of thermal corresponds to the highest frequency of the CPU,
> i.e. 1200000 and state4 to the lowest one.
>
> While both of these try to show similar kind of data (which can still be
> very much different from each other), the values looked different (by a
> factor of 10, i.e. thermal's time_in_state is almost 10 times that of
> cpufreq time_in_state).
>
> This comes from the fact that cpufreq core displays the time in usertime
> units (10 ms).
>
> It would be better if both the frameworks displayed times in the same
> unit as the users may need to correlate between them and different
> scales just make it awkward. And the choice of thermal core for that
> (msec) seems to be a better choice as it is easier to read.
>
> The thermal core also does the stats calculations using ktime, which is
> much more accurate as compared to jiffies used by cpufreq core.
>
> This patch updates the cpufreq core to use ktime for the internal
> calculations and changes the units of time_in_state to msec.
>
> The results look like this after this commit:
>
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:208000 13
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:432000 790
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:729000 12492
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:960000 13259
> /sys/devices/system/cpu/cpufreq/policy0/stats/time_in_state:1200000 3830
>
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state0 3888
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state1 13432
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state2 12336
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state3 740
> /sys/class/thermal/cooling_device0/stats/time_in_state_ms:state4 0
>
> FWIW, tools/power/cpupower/ does consume the time_in_state values from
> the sysfs files but it is independent of the unit of the time and didn't
> require an update.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
> Documentation/cpu-freq/cpufreq-stats.rst | 5 +--
> drivers/cpufreq/cpufreq_stats.c | 47 +++++++++++++-----------
> 2 files changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/cpu-freq/cpufreq-stats.rst b/Documentation/cpu-freq/cpufreq-stats.rst
> index 9ad695b1c7db..9f94012a882f 100644
> --- a/Documentation/cpu-freq/cpufreq-stats.rst
> +++ b/Documentation/cpu-freq/cpufreq-stats.rst
> @@ -64,9 +64,8 @@ need for a reboot.
>
> This gives the amount of time spent in each of the frequencies supported by
> this CPU. The cat output will have "<frequency> <time>" pair in each line, which
> -will mean this CPU spent <time> usertime units of time at <frequency>. Output
> -will have one line for each of the supported frequencies. usertime units here
> -is 10mS (similar to other time exported in /proc).
> +will mean this CPU spent <time> msec of time at <frequency>. Output will have
> +one line for each of the supported frequencies.
>
> ::
>
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 6cd5c8ab5d49..e054ada291e7 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -14,35 +14,38 @@
>
> struct cpufreq_stats {
> unsigned int total_trans;
> - unsigned long long last_time;
> + ktime_t last_time;
> unsigned int max_state;
> unsigned int state_num;
> unsigned int last_index;
> - u64 *time_in_state;
> + ktime_t *time_in_state;
> unsigned int *freq_table;
> unsigned int *trans_table;
>
> /* Deferred reset */
> unsigned int reset_pending;
> - unsigned long long reset_time;
> + ktime_t reset_time;
> };
>
> -static void cpufreq_stats_update(struct cpufreq_stats *stats,
> - unsigned long long time)
> +static void cpufreq_stats_update(struct cpufreq_stats *stats, ktime_t time)
> {
> - unsigned long long cur_time = get_jiffies_64();
> + ktime_t cur_time = ktime_get(), delta;
>
> - stats->time_in_state[stats->last_index] += cur_time - time;
> + delta = ktime_sub(cur_time, time);
> + stats->time_in_state[stats->last_index] =
> + ktime_add(stats->time_in_state[stats->last_index], delta);
> stats->last_time = cur_time;
> }
>
> static void cpufreq_stats_reset_table(struct cpufreq_stats *stats)
> {
> - unsigned int count = stats->max_state;
> + unsigned int count = stats->max_state, i;
> +
> + for (i = 0; i < count; i++)
> + stats->time_in_state[i] = ktime_set(0, 0);
>
> - memset(stats->time_in_state, 0, count * sizeof(u64));
> memset(stats->trans_table, 0, count * count * sizeof(int));
> - stats->last_time = get_jiffies_64();
> + stats->last_time = ktime_get();
> stats->total_trans = 0;
>
> /* Adjust for the time elapsed since reset was requested */
> @@ -70,7 +73,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
> {
> struct cpufreq_stats *stats = policy->stats;
> bool pending = READ_ONCE(stats->reset_pending);
> - unsigned long long time;
> + ktime_t time, now = ktime_get(), delta;
> ssize_t len = 0;
> int i;
>
> @@ -82,18 +85,20 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
> * before the reset_pending read above.
> */
> smp_rmb();
> - time = get_jiffies_64() - READ_ONCE(stats->reset_time);
> + time = ktime_sub(now, READ_ONCE(stats->reset_time));
> } else {
> - time = 0;
> + time = ktime_set(0, 0);;
> }
> } else {
> time = stats->time_in_state[i];
> - if (i == stats->last_index)
> - time += get_jiffies_64() - stats->last_time;
> + if (i == stats->last_index) {
> + delta = ktime_sub(now, stats->last_time);
> + time = ktime_add(delta, time);
> + }
> }
>
> len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
> - jiffies_64_to_clock_t(time));
> + ktime_to_ms(time));
> }
> return len;
> }
> @@ -109,7 +114,7 @@ static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
> * Defer resetting of stats to cpufreq_stats_record_transition() to
> * avoid races.
> */
> - WRITE_ONCE(stats->reset_time, get_jiffies_64());
> + WRITE_ONCE(stats->reset_time, ktime_get());
> /*
> * The memory barrier below is to prevent the readers of reset_time from
> * seeing a stale or partially updated value.
> @@ -228,9 +233,9 @@ void cpufreq_stats_create_table(struct cpufreq_policy *policy)
> if (!stats)
> return;
>
> - alloc_size = count * sizeof(int) + count * sizeof(u64);
> -
> - alloc_size += count * count * sizeof(int);
> + alloc_size = count * sizeof(*stats->time_in_state);
> + alloc_size += count * sizeof(*stats->freq_table);
> + alloc_size += count * count * sizeof(*stats->trans_table);
>
> /* Allocate memory for time_in_state/freq_table/trans_table in one go */
> stats->time_in_state = kzalloc(alloc_size, GFP_KERNEL);
> @@ -249,7 +254,7 @@ void cpufreq_stats_create_table(struct cpufreq_policy *policy)
> stats->freq_table[i++] = pos->frequency;
>
> stats->state_num = i;
> - stats->last_time = get_jiffies_64();
> + stats->last_time = ktime_get();
> stats->last_index = freq_table_get_index(stats, policy->cur);
>
> policy->stats = stats;
>
I am not sure if these ktime_get() are not too heavy in the code path
visited by the scheduler.
How about local_clock()?
It's used in ./drivers/cpuidle/cpuidle.c to do similar accounting.
Regards,
Lukasz
Powered by blists - more mailing lists