From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Patrick Bellasi <patrick.bellasi@arm.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Juri Lelli <juri.lelli@arm.com>, Joel Fernandes <joelaf@google.com>,
        Andres Oportus <andresoportus@google.com>,
        Todd Kjos <tkjos@android.com>,
        Morten Rasmussen <morten.rasmussen@arm.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>
Subject: Re: [PATCH v2 1/6] cpufreq: schedutil: ignore sugov kthreads
Date: Fri, 07 Jul 2017 00:18:21 +0200
Message-ID: <2882907.BM3Q4OeCm1@aspire.rjw.lan>
User-Agent: KMail/4.14.10 (Linux/4.12.0-rc1+; KDE/4.14.9; x86_64; ; )
In-Reply-To: <20170705113834.GB2659@e110439-lin>
References: <1499189651-18797-1-git-send-email-patrick.bellasi@arm.com> <20170705050056.GN3532@vireshk-i7> <20170705113834.GB2659@e110439-lin>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3630
Lines: 85

On Wednesday, July 05, 2017 12:38:34 PM Patrick Bellasi wrote:
> On 05-Jul 10:30, Viresh Kumar wrote:
> > On 04-07-17, 18:34, 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>
> > > 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 v1:
> > > - move check before policy spinlock (JuriL)
> > > ---
> > >  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 c982dd0..eaba6d6 100644
> > > --- a/kernel/sched/cpufreq_schedutil.c
> > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > @@ -218,6 +218,10 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> > >  	unsigned int next_f;
> > >  	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;
> > >  
> > > @@ -290,6 +294,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
> > >  	unsigned long util, max;
> > >  	unsigned int next_f;
> > >  
> > > +	/* Skip updates generated by sugov kthreads */
> > > +	if (unlikely(current == sg_policy->thread))
> > > +		return;
> > > +
> > >  	sugov_get_util(&util, &max);
> > 
> > Yes we discussed this last time as well (I looked again at those discussions and
> > am still confused a bit), but wanted to clarify one more time.
> > 
> > After the 2nd patch of this series is applied, why will we still have this
> > problem? As we concluded it last time, the problem wouldn't happen until the
> > time the sugov RT thread is running (Hint: work_in_progress). And once the sugov
> > RT thread is gone, one of the other scheduling classes will take over and should
> > update the flag pretty quickly.
> > 
> > Are we worried about the time between the sugov RT thread finishes and when the
> > CFS or IDLE sched class call the util handler again? If yes, then we will still
> > have that problem for any normal RT/DL task. Isn't it ?
> 
> Yes, we are worried about that time, without this we can generate
> spikes to the max OPP even when only relatively small FAIR tasks are
> running.
> 
> The same problem is not there for the other "normal RT/DL" tasks, just
> because for those tasks this is the expected behavior: we wanna go to
> max.
> 
> To the contrary the sugov kthread, although being a RT task, is just
> functional to the "machinery" to work, it's an actuator. Thus, IMO it
> makes no sense from a design standpoint for it to interfere whatsoever
> with what the "machinery" is doing.

How is this related to the Juri's series?

Thanks,
Rafael