Re: [PATCH v11 2/5] sched/core: uclamp: Propagate parent clamps

From: Patrick Bellasi <patrick.bellasi@arm.com>
To: "Michal Koutný" <mkoutny@suse.com>
Cc: Alessio Balsini <balsini@android.com>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Morten Rasmussen <morten.rasmussen@arm.com>,
	Quentin Perret <quentin.perret@arm.com>,
	Joel Fernandes <joelaf@google.com>, Paul Turner <pjt@google.com>,
	Steve Muckle <smuckle@google.com>,
	Suren Baghdasaryan <surenb@google.com>,
	Todd Kjos <tkjos@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	Tejun Heo <tj@kernel.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Ingo Molnar <mingo@redhat.com>,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v11 2/5] sched/core: uclamp: Propagate parent clamps
Date: Tue, 16 Jul 2019 18:55:42 +0100	[thread overview]
Message-ID: <20190716175542.p7vs2muslyuez6lq@e110439-lin> (raw)
In-Reply-To: <20190716152929.GD32540@blackbody.suse.cz>

On 16-Jul 17:29, Michal Koutný wrote:
> On Tue, Jul 16, 2019 at 03:07:06PM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > That note comes from the previous review cycle and it's based on a
> > request from Tejun to align uclamp behaviors with the way the
> > delegation model is supposed to work.
> I saw and hopefully understood that reasoning -- uclamp.min has the
> protection semantics and uclamp.max the limit semantics.
> 
> However, what took me some time to comprehend when the effected
> uclamp.min and uclamp.max cross over, i.e. that uclamp.min is then bound
> by uclamp.max (besides parent's uclamp.min). Your commit message
> explains that and I think it's relevant for the kernel docs file
> itself.

Right, I've just added a paragraph to the cpu.uclamp.min documentation.

> > You right, the synchronization is introduced by a later patch:
> > 
> >    sched/core: uclamp: Update CPU's refcount on TG's clamp changes
> I saw that lock but didn't realize __setscheduler_uclamp() touches only
> task's struct uclamp_se, none of task_group's/css's (which is under
> uclamp_mutex). That seems correct.

Right, the mutex is used only on the cgroup side. That's because the
CGroup API can affect multiple tasks running on different CPUs, thus
we wanna make sure we don't come up with race conditions. In that
path we can also afford to go a bit slower.

In the fast path instead we rely on the rq-locks to ensure
serialization on RUNNABLE tasks clamp updates.

Coming from the __setscheduler_uclamp() side however we don't sync
RUNNABLE tasks immediately. We delay the update to next enqueue
opportunity.

Cheers,
Patrick

-- 
#include <best/regards.h>

Patrick Bellasi