| Message ID | 20171130114723.29210-7-patrick.bellasi@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/6] cpufreq: schedutil: reset sg_cpus's flags at IDLE enter | expand |
Hi, On 30/11/17 11:47, Patrick Bellasi wrote: > In system where multiple CPUs shares the same frequency domain a small > workload on a CPU can still be subject to frequency spikes, generated by > the activation of the sugov's kthread. > > Since the sugov kthread is a special RT task, which goal is just that to > activate a frequency transition, it does not make sense for it to bias > the schedutil's frequency selection policy. > > This patch exploits the information related to the current task to silently > ignore cpufreq_update_this_cpu() calls, coming from the RT scheduler, while > the sugov kthread is running. > > Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com> > Reviewed-by: Dietmar Eggemann <dietmar.eggemann@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 > > --- > Changes from v2: > - rebased on v4.15-rc1 > - moved at the end of the stack since considered more controversial > Changes from v1: > - move check before policy spinlock (JuriL) > > Change-Id: I4d749458229b6496dd24a8c357be42cd35a739fd > --- > kernel/sched/cpufreq_schedutil.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 3eea8884e61b..a93ad5b0c40d 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -270,6 +270,10 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, > bool rt_mode; > bool busy; > > + /* Skip updates generated by sugov kthreads */ > + if (unlikely(current == sg_policy->thread)) > + return; > + > sugov_set_iowait_boost(sg_cpu, time, flags); > sg_cpu->last_update = time; > > @@ -356,6 +360,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > unsigned int next_f; > bool rt_mode; > > + /* Skip updates generated by sugov kthreads */ > + if (unlikely(current == sg_policy->thread)) > + return; > + > raw_spin_lock(&sg_policy->update_lock); > > sugov_get_util(&util, &max, sg_cpu->cpu); If the DL changes (which I shall post again as soon as tip/sched/core is bumped up to 4.15-rc1) get in first, this is going to be useless (as the DL kthread gets ignored by the scheduling class itself). But, this looks good to me "in the meantime". Reviewed-by: Juri Lelli <juri.lelli@redhat.com> Best, Juri
On 30/11/17 16:02, Patrick Bellasi wrote: > On 30-Nov 14:41, Juri Lelli wrote: [...] > > If the DL changes (which I shall post again as soon as tip/sched/core is > > bumped up to 4.15-rc1) get in first, this is going to be useless (as the > > DL kthread gets ignored by the scheduling class itself). But, this looks > > good to me "in the meantime". > > Just to better understand, you mean that the DL kthread does not send > out schedutil updates? It doesn't have a "proper" bandwidth (utilization) :/. So it gets "unnoticed" and schedutil updates are not triggered. > > If that's the case I agree we can discard this patch... that's also > one of the reasons why I move it at the end of this series. Not sure about this though, not my call :). I guess this still helps until we get the DL changes in. Best, Juri
On 30-Nov 17:12, Juri Lelli wrote: > On 30/11/17 16:02, Patrick Bellasi wrote: > > On 30-Nov 14:41, Juri Lelli wrote: > > [...] > > > > If the DL changes (which I shall post again as soon as tip/sched/core is > > > bumped up to 4.15-rc1) get in first, this is going to be useless (as the > > > DL kthread gets ignored by the scheduling class itself). But, this looks > > > good to me "in the meantime". > > > > Just to better understand, you mean that the DL kthread does not send > > out schedutil updates? > > It doesn't have a "proper" bandwidth (utilization) :/. So it gets > "unnoticed" and schedutil updates are not triggered. Ah, right... that was the "let CBS ignore this DL task" solution! ;-) > > > > If that's the case I agree we can discard this patch... that's also > > one of the reasons why I move it at the end of this series. > > Not sure about this though, not my call :). I guess this still helps > until we get the DL changes in. Yes, agree... well Viresh had some concerns about this patch. Let see what he thinks. > Best, > > Juri -- #include <best/regards.h> Patrick Bellasi
On 30-11-17, 16:42, Patrick Bellasi wrote: > On 30-Nov 17:12, Juri Lelli wrote: > > Not sure about this though, not my call :). I guess this still helps > > until we get the DL changes in. > > Yes, agree... well Viresh had some concerns about this patch. > Let see what he thinks. And the problem is that we never got to a conclusion in V2 for this patch as well, as you never responded to the last set of queries I had. Unless we finish the ongoing conversations, we will have the same queries again. IOW, can you explain the exact scenario where this patch will be helpful ? I am not sure if this patch is that helpful at all :) -- viresh
On 07-Dec 14:54, Viresh Kumar wrote: > On 30-11-17, 16:42, Patrick Bellasi wrote: > > On 30-Nov 17:12, Juri Lelli wrote: > > > Not sure about this though, not my call :). I guess this still helps > > > until we get the DL changes in. > > > > Yes, agree... well Viresh had some concerns about this patch. > > Let see what he thinks. > > And the problem is that we never got to a conclusion in V2 for this > patch as well, as you never responded to the last set of queries I > had. Unless we finish the ongoing conversations, we will have the same > queries again. > > IOW, can you explain the exact scenario where this patch will be > helpful ? I am not sure if this patch is that helpful at all :) My use case is considering a small FAIR task which triggers an OPP change while running on a CPU which is different from the one running the schedutil kthread. If we go for a "clear flags semantics", where the RT class clears its flag as soon as there are not more RT tasks runnable, then this patch likely is not more required. Otherwise, there can be corner cases in which the RT flag remain set in the kthread CPU thus affecting OPP decisions even then there are not RT tasks around. Consider for example this scenarios: CPU0: | SUGOV | | SUGOV | small FAIR | | small FAIR | | flags = RT ^ ^ | | (A) | (B) | sugov_update_util() | | | sugov_update_util() CPU1: medium growing FAIR task | another FAIR enqueued In this case: - in A we set the RT flag - in B we pick the MAX OPP even if there are only small tasks. This will not happen with a "clear flags semantics" because we ensure to release the RT flag every time an RT task exit the CPU. Thus, if we go for that semantics, we can skip this patch. -- #include <best/regards.h> Patrick Bellasi
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 3eea8884e61b..a93ad5b0c40d 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -270,6 +270,10 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, bool rt_mode; bool busy; + /* Skip updates generated by sugov kthreads */ + if (unlikely(current == sg_policy->thread)) + return; + sugov_set_iowait_boost(sg_cpu, time, flags); sg_cpu->last_update = time; @@ -356,6 +360,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, unsigned int next_f; bool rt_mode; + /* Skip updates generated by sugov kthreads */ + if (unlikely(current == sg_policy->thread)) + return; + raw_spin_lock(&sg_policy->update_lock); sugov_get_util(&util, &max, sg_cpu->cpu);