Re: [PATCH] cpufreq: ondemand: Change the calculation of target frequency

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, May 31, 2013 07:33:06 PM Stratos Karafotis wrote:
> On 05/31/2013 11:51 AM, Viresh Kumar wrote:
> >> ---
> >>   arch/x86/include/asm/processor.h   | 29 ----------------------
> >>   drivers/cpufreq/Makefile           |  2 +-
> >>   drivers/cpufreq/acpi-cpufreq.c     |  5 ----
> >>   drivers/cpufreq/cpufreq.c          | 21 ----------------
> >>   drivers/cpufreq/cpufreq_governor.c | 10 +-------
> >>   drivers/cpufreq/cpufreq_governor.h |  1 -
> >>   drivers/cpufreq/cpufreq_ondemand.c | 39 ++++++-----------------------
> >>   drivers/cpufreq/mperf.c            | 51 --------------------------------------
> >>   drivers/cpufreq/mperf.h            |  9 -------
> >>   include/linux/cpufreq.h            |  6 -----
> >>   10 files changed, 9 insertions(+), 164 deletions(-)
> >>   delete mode 100644 drivers/cpufreq/mperf.c
> >>   delete mode 100644 drivers/cpufreq/mperf.h
> > 
> > I believe you should have removed other users of getavg() in a separate
> > patch and also cc'd relevant people so that you can some review comments
> > from  them.
> 
> I will split the patch in two. If it's OK, I will keep the removal of 
> __cpufreq_driver_getavg in the original patch and move the clean up of
> APERF/MPERF support in a second patch. I will also cc relevant people.
> 
> 
> >>          /* Check for frequency increase */
> >> -       if (load_freq > od_tuners->up_threshold * policy->cur) {
> >> +       if (load > od_tuners->up_threshold) {
> > 
> > Chances of this getting hit are minimal now.. I don't know if keeping
> > this will change anything now :)
> 
> Actually, no. This getting hit pretty often.
> Please find attached the cpufreq statistics - trans_table during build
> of 3.4 kernel. With default up_threshold (95), the transition to max
> happened many times because of load was greater than up_threshold.
> I also thought to keep this code to leave up_threshold functionality unaffected.
>  
> On 05/31/2013 03:42 PM, Rafael J. Wysocki wrote:
> > On Friday, May 31, 2013 02:24:59 PM Viresh Kumar wrote:
> >>> +       } else {
> >>> +               /* Calculate the next frequency proportional to load */
> >>>                  unsigned int freq_next;
> >>> -               freq_next = load_freq / od_tuners->adj_up_threshold;
> >>> +               freq_next = load * policy->max / 100;
> >>
> >> Rafael asked why you believe this is the right formula and I really couldn't
> >> find an appropriate answer to that, sorry :(
> > 
> > Right, it would be good to explain that.
> > 
> > "Proportional to load" means C * load, so why is "policy->max / 100" *the* right C?
> > 
> 
> I think, finally(?) I see your point. The right C should be "policy->cpuinfo.max_freq / 100".
> This way the target frequency will be proportional to load and the calculation will
> "map" the load to CPU freq table.

That seems to mean "take the percentage of policy->cpuinfo.max_freq proportional
to the current load and use the available frequency closest to that".  Is that
correct?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.