Re: [PATCH v2] cpufreq: schedutil: Examine the correct CPU when we update util

[Chris Redpath]

Hi Viresh

On 02/11/17 11:40, Viresh Kumar wrote:
> On 02-11-17, 11:38, Chris Redpath wrote:
> > Since:
> > 4296f23ed cpufreq: schedutil: Fix per-CPU structure initialization in
> >   sugov_start()
>
> This is still incorrect. This BUG has nothing to do with 4296f23ed
> AFAICT.

According to my diff, this was the commit which switched from assigning
the values directly (and not overwriting the cpu member, which was
introduced in the other commit you reference) to using a memset and
clearing the whole struct. I figured that the fix (which was for a
different issue) inadvertently exposed this, which was what I was trying
to convey here. I can remove this and just reference 674e75411fc2. It's
clear that what's broken is the remote update.

I'll reply with a v3 shortly.

--Chris


> > We lost the value of sg_cpu->cpu which is assigned during
> > sugov_register. The memset in sugov_start overwrites it with zero.
> >
> > The change here was triggered by the commit adding the remote update
> > functionality.
> > 674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks")
> >
> > This leads to always looking at the utilization of CPU0 instead of
> > the one we just updated when we do a utilization update callback.
> >
> > Let's fix this by consolidating the initialization code into
> > sugov_start().
> >
> > Fixes: 674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks")
> > Signed-off-by: Chris Redpath <chris.redpath@xxxxxxx>
> > Reviewed-by: Patrick Bellasi <patrick.bellasi@xxxxxxx>
> > Reviewed-by: Brendan Jackman <brendan.jackman@xxxxxxx>
> > Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > ---
> >   kernel/sched/cpufreq_schedutil.c | 6 +-----
> >   1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 6c1a7fcfa2a7..dc68a1ccdb33 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -728,6 +728,7 @@ static int sugov_start(struct cpufreq_policy *policy)
> >              struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
> >
> >              memset(sg_cpu, 0, sizeof(*sg_cpu));
> > +            sg_cpu->cpu = cpu;
> >              sg_cpu->sg_policy = sg_policy;
> >              sg_cpu->flags = SCHED_CPUFREQ_RT;
> >              sg_cpu->iowait_boost_max = policy->cpuinfo.max_freq;
> > @@ -793,11 +794,6 @@ struct cpufreq_governor *cpufreq_default_governor(void)
> >
> >   static int __init sugov_register(void)
> >   {
> > -    int cpu;
> > -
> > -    for_each_possible_cpu(cpu)
> > -            per_cpu(sugov_cpu, cpu).cpu = cpu;
> > -
> >      return cpufreq_register_governor(&schedutil_gov);
> >   }
> >   fs_initcall(sugov_register);
> > --
> > 2.13.1.449.g02a2850
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.