| Message ID | 1499189651-18797-5-git-send-email-patrick.bellasi@arm.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
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
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
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
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
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/
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;
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(-)