Re: [PATCH 2/6] cpufreq: schedutil: ignore the sugov kthread for frequencies selections

[Patrick Bellasi]

On 03-Mar 10:49, Viresh Kumar wrote:
> On 02-03-17, 15:45, Patrick Bellasi wrote:
> > In system where multiple CPUs shares the same frequency domain a small
> > workload on a CPU can still be subject 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.
> > 
> > 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@xxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: linux-pm@xxxxxxxxxxxxxxx
> > ---
> >  kernel/sched/cpufreq_schedutil.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 084a98b..a3fe5e4 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -274,6 +274,8 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
> >  {
> >  	struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> >  	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> > +	unsigned int cpu = smp_processor_id();
> > +	struct task_struct *curr = cpu_curr(cpu);
> 
> Maybe merge these two as you have done in the next patch.

Yes, better.

> >  	unsigned long util, max;
> >  	unsigned int next_f;
> >  
> > @@ -287,6 +289,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
> >  		goto done;
> >  	}
> >  
> > +	/* Skip updates generated by sugov kthreads */
> > +	if (curr == sg_policy->thread)
> > +		goto done;
> > +
> 
> I always wanted to avoid such hacks when I moved to the RT thread :(

Indeed, it is a bit of an hack... but still it's true that this is a
"special" RT thread which must not bias OPP selection.

> I was discussing the exact same problem with Vincent few days back and
> one of the ideas we had was to clear the flags when any RT task is
> dequeued from a rq.

That's a possible solution, however at dequeue time we don't know
what's going on right after. We can end up picking up another RT task.
Thus we can reset the flag when it's not really required, and this
open for possible races...

> AFAIU, the problem we are discussing here
> shouldn't normally occur while the sugov RT thread is running as the
> work_in_progress flag makes sure we don't reevaluate the load while
> the RT thread is updating the frequency.

True...

> The problem perhaps occurs as the flag for CPU X is never cleared,
> and so on the next callback from the scheduler (after the frequency
> is updated and work_in_progress is cleared) we move to the highest
> frequency.

... right.

> So what about clearing the flags, just like the previous patch, when
> the RT or DL task has finished?

As a general goal I think it would be useful to feed input only when
the scheduler knows something is going to happen. That's why in the
last patch of these series I'm proposing to remove updates from
update_curr_rt() and replace it with calls in most interesting events,
like enqueue/pickup instead of dequeue.
 
> Sorry for the noise if it was all nonsense :)

That's absolutely not nonsense, thanks for the feedback.

I agree with you that the solution proposed by this patch sound a bit
of an hack, but still we can argue that using an RT task to change an
OPP is by itself a sort-of an hack.

The main downside I see is the condition check for each and every
update. I don't completely like it, but I'm also not completely
convinced by the "always reset" policy at RT tasks dequeue.

 
> -- 
> viresh

Cheers Patrick

-- 
#include <best/regards.h>

Patrick Bellasi