Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681105 > unrolled thread

[PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used Patrick Bellasi <patrick.bellasi@arm.com> - 2017-07-04 19:40 +0200
    Re: [PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used Joel Fernandes <joelaf@google.com> - 2017-07-07 08:00 +0200
    Re: [PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used Vikram Mulukutla <markivx@codeaurora.org> - 2017-07-07 08:50 +0200
      Re: [PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used Joel Fernandes <joelaf@google.com> - 2017-07-08 08:20 +0200
        Re: [PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used Vikram Mulukutla <markivx@codeaurora.org> - 2017-07-10 20:00 +0200
          Re: [PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used Joel Fernandes <joelaf@google.com> - 2017-07-11 07:30 +0200

#1681105 — [PATCH v2 4/6] cpufreq: schedutil: update CFS util only if used

Currently the utilization of the FAIR class is collected before locking
the policy. Although that should not be a big issue for most cases, we
also don't really know how much latency there can be between the
utilization reading and its usage.

Let's get the FAIR utilization right before its usage to be better in
sync with the current status of a CPU.

Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
---
 kernel/sched/cpufreq_schedutil.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 98704d8..df433f1 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -308,10 +308,9 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	if (unlikely(current == sg_policy->thread))
 		return;
 
-	sugov_get_util(&util, &max);
-
 	raw_spin_lock(&sg_policy->update_lock);
 
+	sugov_get_util(&util, &max);
 	sg_cpu->util = util;
 	sg_cpu->max = max;
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1682937

On Tue, Jul 4, 2017 at 10:34 AM, Patrick Bellasi
<patrick.bellasi@arm.com> wrote:
> Currently the utilization of the FAIR class is collected before locking
> the policy. Although that should not be a big issue for most cases, we
> also don't really know how much latency there can be between the
> utilization reading and its usage.
>
> Let's get the FAIR utilization right before its usage to be better in
> sync with the current status of a CPU.
>
> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> ---
>  kernel/sched/cpufreq_schedutil.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 98704d8..df433f1 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -308,10 +308,9 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
>         if (unlikely(current == sg_policy->thread))
>                 return;
>
> -       sugov_get_util(&util, &max);
> -
>         raw_spin_lock(&sg_policy->update_lock);
>
> +       sugov_get_util(&util, &max);
>         sg_cpu->util = util;
>         sg_cpu->max = max;

Is your concern that there will we spinlock contention before calling
sugov_get_util?

If that's the case, with your patch it seems to me such contention
(and hence spinning) itself could cause the utilization to be inflated
- thus calling sugov_get_util after acquiring the lock will not be as
accurate as before. In that case it seems to me its better to let
sugov_get_util be called before acquiring the lock (as before).

thanks,

-Joel

[toc] | [prev] | [next] | [standalone]

#1682956

On 2017-07-04 10:34, Patrick Bellasi wrote:
> Currently the utilization of the FAIR class is collected before locking
> the policy. Although that should not be a big issue for most cases, we
> also don't really know how much latency there can be between the
> utilization reading and its usage.
> 
> Let's get the FAIR utilization right before its usage to be better in
> sync with the current status of a CPU.
> 
> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> ---
>  kernel/sched/cpufreq_schedutil.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c 
> b/kernel/sched/cpufreq_schedutil.c
> index 98704d8..df433f1 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -308,10 +308,9 @@ static void sugov_update_shared(struct
> update_util_data *hook, u64 time,
>  	if (unlikely(current == sg_policy->thread))
>  		return;
> 
> -	sugov_get_util(&util, &max);
> -
>  	raw_spin_lock(&sg_policy->update_lock);
> 
> +	sugov_get_util(&util, &max);
>  	sg_cpu->util = util;
>  	sg_cpu->max = max;

Given that the utilization update hooks are called with the per-cpu rq 
lock held (for all classes), I don't think PELT utilization can change 
throughout the lifetime of the cpufreq_update_{util,this_cpu} call? Even 
with Viresh's remote cpu callback series we'd still have to hold the rq 
lock across cpufreq_update_util..  what can change today is 'max' 
(arch_scale_cpu_capacity) when a cpufreq policy is shared, so the patch 
is still needed for that reason I think?

Thanks,
Vikram

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1683547

Hi Vikram,

On Thu, Jul 6, 2017 at 11:44 PM, Vikram Mulukutla
<markivx@codeaurora.org> wrote:
> On 2017-07-04 10:34, Patrick Bellasi wrote:
>>
>> Currently the utilization of the FAIR class is collected before locking
>> the policy. Although that should not be a big issue for most cases, we
>> also don't really know how much latency there can be between the
>> utilization reading and its usage.
>>
>> Let's get the FAIR utilization right before its usage to be better in
>> sync with the current status of a CPU.
>>
>> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-pm@vger.kernel.org
>> ---
>>  kernel/sched/cpufreq_schedutil.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/cpufreq_schedutil.c
>> b/kernel/sched/cpufreq_schedutil.c
>> index 98704d8..df433f1 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -308,10 +308,9 @@ static void sugov_update_shared(struct
>> update_util_data *hook, u64 time,
>>         if (unlikely(current == sg_policy->thread))
>>                 return;
>>
>> -       sugov_get_util(&util, &max);
>> -
>>         raw_spin_lock(&sg_policy->update_lock);
>>
>> +       sugov_get_util(&util, &max);
>>         sg_cpu->util = util;
>>         sg_cpu->max = max;
>
>
> Given that the utilization update hooks are called with the per-cpu rq lock
> held (for all classes), I don't think PELT utilization can change throughout
> the lifetime of the cpufreq_update_{util,this_cpu} call? Even with Viresh's
> remote cpu callback series we'd still have to hold the rq lock across
> cpufreq_update_util..  what can change today is 'max'
> (arch_scale_cpu_capacity) when a cpufreq policy is shared, so the patch is
> still needed for that reason I think?
>

I didn't follow, Could you elaborate more why you think the patch
helps with the case where max changes while the per-cpu rq lock held?

thanks,

-Joel

[toc] | [prev] | [next] | [standalone]

#1684547

On 2017-07-07 23:14, Joel Fernandes wrote:
> Hi Vikram,
> 
> On Thu, Jul 6, 2017 at 11:44 PM, Vikram Mulukutla
> <markivx@codeaurora.org> wrote:
>> On 2017-07-04 10:34, Patrick Bellasi wrote:
>>> 
>>> Currently the utilization of the FAIR class is collected before 
>>> locking
>>> the policy. Although that should not be a big issue for most cases, 
>>> we
>>> also don't really know how much latency there can be between the
>>> utilization reading and its usage.
>>> 
>>> Let's get the FAIR utilization right before its usage to be better in
>>> sync with the current status of a CPU.
>>> 
>>> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>

<snip>

>> Given that the utilization update hooks are called with the per-cpu rq 
>> lock
>> held (for all classes), I don't think PELT utilization can change 
>> throughout
>> the lifetime of the cpufreq_update_{util,this_cpu} call? Even with 
>> Viresh's
>> remote cpu callback series we'd still have to hold the rq lock across
>> cpufreq_update_util..  what can change today is 'max'
>> (arch_scale_cpu_capacity) when a cpufreq policy is shared, so the 
>> patch is
>> still needed for that reason I think?
>> 
> 
> I didn't follow, Could you elaborate more why you think the patch
> helps with the case where max changes while the per-cpu rq lock held?
> 

So going by Patrick's commit text, the concern was a TOC/TOU
problem, but since we agree that CFS utilization can't change
within an rq-locked critical section, the only thing that can
change is 'max'. So you might be the 8th cpu in line waiting
for the sg-policy lock, and after you get the lock, the max may
be outdated.

But come to think of it max changes should be triggering schedutil
updates and those shouldn't be rate-throttled, so maybe we don't
need this at all? It's still somewhat future-proof in case there
is some stat that we read in sugov_get_util that can be updated
asynchronously. However we can put it in when we need it...

> thanks,
> 
> -Joel

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1684781

On Mon, Jul 10, 2017 at 10:49 AM, Vikram Mulukutla
<markivx@codeaurora.org> wrote:
[..]
>
>>> Given that the utilization update hooks are called with the per-cpu rq
>>> lock
>>> held (for all classes), I don't think PELT utilization can change
>>> throughout
>>> the lifetime of the cpufreq_update_{util,this_cpu} call? Even with
>>> Viresh's
>>> remote cpu callback series we'd still have to hold the rq lock across
>>> cpufreq_update_util..  what can change today is 'max'
>>> (arch_scale_cpu_capacity) when a cpufreq policy is shared, so the patch
>>> is
>>> still needed for that reason I think?
>>>
>>
>> I didn't follow, Could you elaborate more why you think the patch
>> helps with the case where max changes while the per-cpu rq lock held?
>>
>
> So going by Patrick's commit text, the concern was a TOC/TOU
> problem, but since we agree that CFS utilization can't change
> within an rq-locked critical section, the only thing that can
> change is 'max'. So you might be the 8th cpu in line waiting
> for the sg-policy lock, and after you get the lock, the max may
> be outdated.
>
> But come to think of it max changes should be triggering schedutil
> updates and those shouldn't be rate-throttled, so maybe we don't
> need this at all? It's still somewhat future-proof in case there
> is some stat that we read in sugov_get_util that can be updated
> asynchronously. However we can put it in when we need it...

It looks like Juri's patch [1] to split signals already cleaned it up
so we should be all set ;-)

Thanks,

-Joel

[1] https://patchwork.kernel.org/patch/9826201/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web