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

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

[Chris Redpath]

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.

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: 4296f23ed49a ("cpufreq: schedutil: Fix per-CPU structure initialization in sugov_start()")
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>
Cc: 4.9+ <stable@vger.kernel.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..eeb7e0a1d861 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -731,6 +731,7 @@ static int sugov_start(struct cpufreq_policy *policy)
 		sg_cpu->sg_policy = sg_policy;
 		sg_cpu->flags = SCHED_CPUFREQ_RT;
 		sg_cpu->iowait_boost_max = policy->cpuinfo.max_freq;
+		sg_cpu->cpu = cpu;
 	}
 
 	for_each_cpu(cpu, policy->cpus) {
@@ -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

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

[Viresh Kumar]

On 02-11-17, 10:54, Chris Redpath wrote:
> Since:
> 
> 4296f23ed cpufreq: schedutil: Fix per-CPU structure initialization in
>  sugov_start()

Good catch but you pointed out to the wrong commit really.

674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks"

is the real culprit as that's where the 'cpu' field is added.

> We lost the value of sg_cpu->cpu which is assigned during
> sugov_register. The memset in sugov_start overwrites it with zero.
> 
> 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: 4296f23ed49a ("cpufreq: schedutil: Fix per-CPU structure initialization in 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>
> Cc: 4.9+ <stable@vger.kernel.org>

This wouldn't be required now as it got merged in 4.14 only and Rafael
should be able to get that as part of 4.14 itself.

> ---
>  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..eeb7e0a1d861 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -731,6 +731,7 @@ static int sugov_start(struct cpufreq_policy *policy)
>  		sg_cpu->sg_policy = sg_policy;
>  		sg_cpu->flags = SCHED_CPUFREQ_RT;
>  		sg_cpu->iowait_boost_max = policy->cpuinfo.max_freq;
> +		sg_cpu->cpu = cpu;

Maybe just move this below memset.

>  	}
>  
>  	for_each_cpu(cpu, policy->cpus) {
> @@ -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);

Diff looks fine though.

-- 
viresh

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

[Chris Redpath]

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(-)

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

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

[Viresh Kumar]

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

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@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.

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

[Viresh Kumar]

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

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

[Chris Redpath]

After
674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks")

We stopped always reading utilization for the cpu we are running
the governor on, and instead read it for the cpu which we've been
told has updated utilization. This is stored in sugov_cpu->cpu.

The value is set in sugov_register but we clear it in sugov_start
which leads to always looking at the utilization of CPU0 instead
of the correct one.

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

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

[Viresh Kumar]

On 03-11-17, 13:36, Chris Redpath wrote:
> After
> 674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks")
> 
> We stopped always reading utilization for the cpu we are running
> the governor on, and instead read it for the cpu which we've been
> told has updated utilization. This is stored in sugov_cpu->cpu.
> 
> The value is set in sugov_register but we clear it in sugov_start
> which leads to always looking at the utilization of CPU0 instead
> of the correct one.
> 
> 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(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

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

[Chris Redpath]

Hi Viresh, Rafael,

Without this patch, schedutil is totally broken for us - is
there any chance at all this could go in 4.14 or is it just
too late?

Best Regards,
Chris

On 03/11/17 15:45, Viresh Kumar wrote:
> On 03-11-17, 13:36, Chris Redpath wrote:
>> After
>> 674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks")
>>
>> We stopped always reading utilization for the cpu we are running
>> the governor on, and instead read it for the cpu which we've been
>> told has updated utilization. This is stored in sugov_cpu->cpu.
>>
>> The value is set in sugov_register but we clear it in sugov_start
>> which leads to always looking at the utilization of CPU0 instead
>> of the correct one.
>>
>> 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(-)
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
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.

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

[Viresh Kumar]

On 07-11-17, 09:49, Chris Redpath wrote:
> Hi Viresh, Rafael,
> 
> Without this patch, schedutil is totally broken for us - is
> there any chance at all this could go in 4.14 or is it just
> too late?

I see that Rafael has already applied it, but not sure if he is planning to send
it for 4.14 (though he should IMHO, as it is a critical fix).

commit d62d813c0d71 ("cpufreq: schedutil: Examine the correct CPU when we update
util")

--
viresh

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

[Rafael J. Wysocki]

On Tue, Nov 7, 2017 at 10:59 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 07-11-17, 09:49, Chris Redpath wrote:
>> Hi Viresh, Rafael,
>>
>> Without this patch, schedutil is totally broken for us - is
>> there any chance at all this could go in 4.14 or is it just
>> too late?
>
> I see that Rafael has already applied it, but not sure if he is planning to send
> it for 4.14 (though he should IMHO, as it is a critical fix).

Yes, I am.

Thanks,
Rafael

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

[Chris Redpath]

Thanks guys, really appreciated!

--Chris

On 07/11/17 10:09, Rafael J. Wysocki wrote:
> On Tue, Nov 7, 2017 at 10:59 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 07-11-17, 09:49, Chris Redpath wrote:
>>> Hi Viresh, Rafael,
>>>
>>> Without this patch, schedutil is totally broken for us - is
>>> there any chance at all this could go in 4.14 or is it just
>>> too late?
>>
>> I see that Rafael has already applied it, but not sure if he is planning to send
>> it for 4.14 (though he should IMHO, as it is a critical fix).
> 
> Yes, I am.
> 
> Thanks,
> Rafael
>