Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Patrick Bellasi <patrick.bellasi@arm.com>
Cc: 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>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Paul Turner <pjt@google.com>,
	Quentin Perret <quentin.perret@arm.com>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Morten Rasmussen <morten.rasmussen@arm.com>,
	Juri Lelli <juri.lelli@redhat.com>, Todd Kjos <tkjos@google.com>,
	Joel Fernandes <joelaf@google.com>,
	Steve Muckle <smuckle@google.com>,
	Suren Baghdasaryan <surenb@google.com>,
	Alessio Balsini <balsini@android.com>
Subject: Re: [PATCH v10 12/16] sched/core: uclamp: Extend CPU's cgroup controller
Date: Mon, 24 Jun 2019 10:52:15 -0700
Message-ID: <20190624175215.GR657710@devbig004.ftw2.facebook.com> (raw)
In-Reply-To: <20190624172906.3d3w6352ji4izjgo@e110439-lin>

Hey, Patrick.

On Mon, Jun 24, 2019 at 06:29:06PM +0100, Patrick Bellasi wrote:
> > I kinda wonder whether the term bandwidth is a bit confusing because
> > it's also used for cpu.max/min.  Would just calling it frequency be
> > clearer?
> 
> Maybe I should find a better way to express the concept above.
> 
> I agree that bandwidth is already used by cpu.{max,min}, what I want
> to call out is that clamps allows to enrich that concept.
> 
> By hinting the scheduler on min/max required utilization we can better
> defined the amount of actual CPU cycles required/allowed.
> That's a bit more precise bandwidth control compared to just rely on
> temporal runnable/period limits.

I see.  I wonder whether it's overloading the same term too subtly
tho.  It's great to document how they interact but it *might* be
easier for readers if a different term is used even if the meaning is
essentially the same.  Anyways, it's a nitpick.  Please feel free to
ignore.

> > > +	tg = css_tg(of_css(of));
> > > +	if (tg == &root_task_group) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > 
> > I don't think you need the above check.
> 
> Don't we want to forbid attributes tuning from the root group?

Yeah, that's enforced by NOT_ON_ROOT flag, right?

> > So, uclamp.max limits the maximum freq% can get and uclamp.min limits
> > hte maximum freq% protection can get in the subtree.  Let's say
> > uclamp.max is 50% and uclamp.min is 100%.
> 
> That's not possible, in the current implementation we always enforce
> the limit (uclamp.max) to be _not smaller_ then the protection
> (uclamp.min).
> 
> Indeed, in principle, it does not make sense to ask for a minimum
> utilization (i.e. frequency boosting) which is higher then the
> maximum allowed utilization (i.e. frequency capping).

Yeah, I'm trying to explain actually it does.

> > It means that protection is not limited but the actual freq% is
> > limited upto 50%, which isn't necessarily invalid.
> > For a simple example, a user might be saying
> > that they want to get whatever protection they can get from its parent
> > but wanna limit eventual freq at 50% and it isn't too difficult to
> > imagine cases where the two knobs are configured separately especially
> > configuration is being managed hierarchically / automatically.
> 
> That's not my understanding, in v10 by default when we create a
> subgroup we assign it uclamp.min=0%, meaning that we don't boost
> frequencies.
> 
> It seems instead that you are asking to set uclamp.min=100% by
> default, so that the effective value will give us whatever the father
> allow. Is that correct?

No, the defaults are fine.  I'm trying to say that min/max
configurations don't need to be coupled like this and there are valid
use cases where the configured min is higher than max when
configurations are nested and managed automatically.

Limits always trump protection in effect of course but please don't
limit what can be configured.

Thanks.

-- 
tejun

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-21  8:42 [PATCH v10 00/16] Add utilization clamping support Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 01/16] sched/core: uclamp: Add CPU's clamp buckets refcounting Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 02/16] sched/core: uclamp: Add bucket local max tracking Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 03/16] sched/core: uclamp: Enforce last task's UCLAMP_MAX Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 04/16] sched/core: uclamp: Add system default clamps Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 05/16] sched/core: Allow sched_setattr() to use the current policy Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 06/16] sched/core: uclamp: Extend sched_setattr() to support utilization clamping Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 07/16] sched/core: uclamp: Reset uclamp values on RESET_ON_FORK Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 08/16] sched/core: uclamp: Set default clamps for RT tasks Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 09/16] sched/cpufreq: uclamp: Add clamps for FAIR and " Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 10/16] sched/core: uclamp: Add uclamp_util_with() Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 11/16] sched/fair: uclamp: Add uclamp support to energy_compute() Patrick Bellasi
2019-06-21 14:01   ` Peter Zijlstra
2019-06-21 14:47     ` Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 12/16] sched/core: uclamp: Extend CPU's cgroup controller Patrick Bellasi
2019-06-22 15:03   ` Tejun Heo
2019-06-24 17:29     ` Patrick Bellasi
2019-06-24 17:52       ` Tejun Heo [this message]
2019-06-25  9:31         ` Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 13/16] sched/core: uclamp: Propagate parent clamps Patrick Bellasi
2019-06-22 15:07   ` Tejun Heo
2019-06-24 17:34     ` Patrick Bellasi
2019-06-24 17:46       ` Tejun Heo
2019-06-21  8:42 ` [PATCH v10 14/16] sched/core: uclamp: Propagate system defaults to root group Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 15/16] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps Patrick Bellasi
2019-06-21  8:42 ` [PATCH v10 16/16] sched/core: uclamp: Update CPU's refcount on TG's clamp changes Patrick Bellasi
2019-06-21 14:55 ` [PATCH v10 00/16] Add utilization clamping support Patrick Bellasi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190624175215.GR657710@devbig004.ftw2.facebook.com \
    --to=tj@kernel.org \
    --cc=balsini@android.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=joelaf@google.com \
    --cc=juri.lelli@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=morten.rasmussen@arm.com \
    --cc=patrick.bellasi@arm.com \
    --cc=peterz@infradead.org \
    --cc=pjt@google.com \
    --cc=quentin.perret@arm.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=smuckle@google.com \
    --cc=surenb@google.com \
    --cc=tkjos@google.com \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox