Re: [PATCH][v3] cpufreq: governor: Fix overflow when calculating idle time

[Rafael J. Wysocki]

On Wed, Apr 20, 2016 at 3:26 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Tuesday, April 19, 2016 11:57:32 AM Chen Yu wrote:
>> It was reported that after Commit 0df35026c6a5 ("cpufreq: governor:
>> Fix negative idle_time when configured with CONFIG_HZ_PERIODIC"),
>> cpufreq ondemand governor started to act oddly. Without any load,
>> with freshly booted system, it pumped cpu frequency up to maximum
>> at some point of time and stayed there. The problem is caused by
>> jiffies overflow in get_cpu_idle_time:
>>
>> After booting up 5 minutes, the jiffies will round up to zero.
>> As a result, the following condition in cpu governor will always be
>> true:
>>       if (cur_idle_time <= j_cdbs->prev_cpu_idle)
>>               idle_time = 0;
>>
>> which caused problems.
>>
>> For example, once cur_idle_time has rounded up to zero, meanwhile
>> prev_cpu_idle still remains negative(because of jiffies initial value
>> of -300HZ, which is very big after converted to unsigned), thus above
>> condition is met, thus we get a zero of idle running time during
>> this sample, which causes a high busy time, thus governor always
>> requests for the highest freq.
>>
>> This patch fixes this problem by updating prev_cpu_idle for
>> each sample period, even if prev_cpu_idle is bigger than
>> cur_idle_time, thus to prevent the scenario of 'prev_cpu_idle always
>> bigger than cur_idle_time' from happening.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=115261
>> Reported-by: Timo Valtoaho <timo.valtoaho@xxxxxxxxx>
>> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
>
> This looks better than the previous versions to me, but ->
>
>> ---
>> v3:
>>  - Do not use INITIAL_JIFFIES because it should be transparent
>>    to user, meanwhile keep original semanteme to use delta
>>    of time slice.
>> ---
>> v2:
>>  - Send this patch to a wider scope, including timing-system maintainers,
>>    as well as some modifications in the commit message to make it more clear.
>> ---
>>  drivers/cpufreq/cpufreq.c          | 4 ++++
>>  drivers/cpufreq/cpufreq_governor.c | 8 +++++++-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index b87596b..b0479b3 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -132,6 +132,10 @@ struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
>>  }
>>  EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
>>
>> +/**
>> + * The wall time and idle time are both possible to round up,
>
> That's difficult to parse.  I guess you wanted to say that they may overflow?
>
>> + * people should use delta rather than the value itself.
>> + */
>
> -> this new comment doesn't really belong to the fix.  You can send a separate
> patch adding it.
>
> Moreover -->
>
>>  static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
>>  {
>>       u64 idle_time;
>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>> index 10a5cfe..8de3fba 100644
>> --- a/drivers/cpufreq/cpufreq_governor.c
>> +++ b/drivers/cpufreq/cpufreq_governor.c
>> @@ -197,8 +197,14 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
>>                       idle_time = 0;
>>               } else {
>>                       idle_time = cur_idle_time - j_cdbs->prev_cpu_idle;
>> -                     j_cdbs->prev_cpu_idle = cur_idle_time;
>>               }
>> +             /*
>> +              * It is possible prev_cpu_idle being bigger than cur_idle_time,
>> +              * when 32bit rounds up if !CONFIG_VIRT_CPU_ACCOUNTING,
>> +              * thus get a 0% idle estimation. So update prev_cpu_idle during
>> +              * each sample period to avoid this situation lasting too long.
>> +              */
>> +             j_cdbs->prev_cpu_idle = cur_idle_time;
>
> --> it looks like the bug is that we are comparing signed values as unsigned.
>
>>
>>               if (ignore_nice) {
>>                       u64 cur_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
>>
>
> So what about the simple change below?

Well, it doesn't make sense, sorry about the confusion.

> ---
>  drivers/cpufreq/cpufreq_governor.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -146,7 +146,7 @@ unsigned int dbs_update(struct cpufreq_p
>                 wall_time = cur_wall_time - j_cdbs->prev_cpu_wall;
>                 j_cdbs->prev_cpu_wall = cur_wall_time;
>
> -               if (cur_idle_time <= j_cdbs->prev_cpu_idle) {
> +               if ((s64)cur_idle_time <= (s64)j_cdbs->prev_cpu_idle) {
>                         idle_time = 0;
>                 } else {
>                         idle_time = cur_idle_time - j_cdbs->prev_cpu_idle;
>