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

[Viresh Kumar]

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.

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

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

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

Sorry for the noise if it was all nonsense :)

-- 
viresh