| Message ID | 20171102113840.17439-1-chris.redpath@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] cpufreq: schedutil: Examine the correct CPU when we update util | expand |
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. > 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@arm.com> > Reviewed-by: Patrick Bellasi <patrick.bellasi@arm.com> > Reviewed-by: Brendan Jackman <brendan.jackman@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > --- > 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 -- viresh
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@arm.com> >> Reviewed-by: Patrick Bellasi <patrick.bellasi@arm.com> >> Reviewed-by: Brendan Jackman <brendan.jackman@arm.com> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Peter Zijlstra <peterz@infradead.org> >> --- >> 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.
On 02-11-17, 12:06, Chris Redpath wrote: > 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 understand what you want to convey here but the log says something else according to me. It says: "Since: 4296f23ed cpufreq: schedutil: Fix per-CPU structure initialization in sugov_start(), We lost the value of sg_cpu->cpu which is assigned during sugov_register." Reading this line it looks like sg_cpu->cpu was screwed up by 4296f23ed, which happened in 4.9. But the sg_cpu->cpu field itself got added in 4.14 and so I think we should write it differently. If I were you, I wouldn't mention 4296f23ed here at all but just say that memset() is clearing the value of sg_cpu->cpu, fix that. Thanks. -- viresh
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);
Since: 4296f23ed cpufreq: schedutil: Fix per-CPU structure initialization in sugov_start() 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@arm.com> Reviewed-by: Patrick Bellasi <patrick.bellasi@arm.com> Reviewed-by: Brendan Jackman <brendan.jackman@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/sched/cpufreq_schedutil.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) -- 2.13.1.449.g02a2850