RE: [PATCH] cpufreq: governor: Fix prev_load initialization in cpufreq_governor_start()

[Chen, Yu C]

Hi,

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
> Sent: Monday, April 25, 2016 9:08 AM
> To: Linux PM list
> Cc: Linux Kernel Mailing List; Viresh Kumar; Chen, Yu C
> Subject: [PATCH] cpufreq: governor: Fix prev_load initialization in
> cpufreq_governor_start()
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> The way cpufreq_governor_start() initializes j_cdbs->prev_load is questionable.
> 
> First off, j_cdbs->prev_cpu_wall used as a denominator in the computation may
> be zero.  The case this happens is when
> get_cpu_idle_time_us() returns -1 and get_cpu_idle_time_jiffy() used to return
> that number is called exactly at the jiffies_64 wrap time.  It is rather hard to
> trigger that error, but it is not impossible and it will just crash the kernel then.
> 
> Second, j_cdbs->prev_load is computed as the average load during the entire
> time since the system started and it may not reflect the load in the previous
> sampling period (as it is expected to).
> That doesn't play well with the way dbs_update() uses that value.
> Namely, if the update time delta (wall_time) happens do be greater than twice
happens s/do/to be?
> the sampling rate on the first invocation of it, the initial value of j_cdbs-
> >prev_load (which may be completely off) will be returned to the caller as the
> current load (unless it is equal to zero and unless another CPU sharing the same
> policy object has a greater load value).
> 
> For this reason, notice that the prev_load field of struct cpu_dbs_info is only
> used by dbs_update() and only in that one place, so if
> cpufreq_governor_start() is modified to always initialize it to 0, it will make
> dbs_update() always compute the actual load first time it checks the update
> time delta against the doubled sampling rate (after initialization) and there
> won't be any side effects of it.
> 
> Consequently, modify cpufreq_governor_start() as described.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---

Acked-by: Chen Yu <yu.c.chen@xxxxxxxxx>

thanks,
Yu