linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Bellasi <patrick.bellasi@arm.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-api@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
	Tejun Heo <tj@kernel.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>
Subject: Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
Date: Wed, 13 Mar 2019 17:09:40 +0000	[thread overview]
Message-ID: <20190313170940.ngiafkmiijryhl2k@e110439-lin> (raw)
In-Reply-To: <20190313143240.GH5922@hirez.programming.kicks-ass.net>

On 13-Mar 15:32, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 45460e7a3eee..447261cd23ba 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -584,14 +584,32 @@ struct sched_dl_entity {
> >   * Utilization clamp for a scheduling entity
> >   * @value:		clamp value "requested" by a se
> >   * @bucket_id:		clamp bucket corresponding to the "requested" value
> > + * @effective:		clamp value and bucket actually "assigned" to the se
> > + * @active:		the se is currently refcounted in a rq's bucket
> >   *
> > + * Both bucket_id and effective::bucket_id are the index of the clamp bucket
> > + * matching the corresponding clamp value which are pre-computed and stored to
> > + * avoid expensive integer divisions from the fast path.
> > + *
> > + * The active bit is set whenever a task has got an effective::value assigned,
> > + * which can be different from the user requested clamp value. This allows to
> > + * know a task is actually refcounting the rq's effective::bucket_id bucket.
> >   */
> >  struct uclamp_se {
> > +	/* Clamp value "requested" by a scheduling entity */
> >  	unsigned int value		: bits_per(SCHED_CAPACITY_SCALE);
> >  	unsigned int bucket_id		: bits_per(UCLAMP_BUCKETS);
> > +	unsigned int active		: 1;
> > +	/*
> > +	 * Clamp value "obtained" by a scheduling entity.
> > +	 *
> > +	 * This cache the actual clamp value, possibly enforced by system
> > +	 * default clamps, a task is subject to while enqueued in a rq.
> > +	 */
> > +	struct {
> > +		unsigned int value	: bits_per(SCHED_CAPACITY_SCALE);
> > +		unsigned int bucket_id	: bits_per(UCLAMP_BUCKETS);
> > +	} effective;
> 
> I still think that this effective thing is backwards.
> 
> The existing code already used @value and @bucket_id as 'effective' and
> you're now changing all that again. This really doesn't make sense to
> me.

With respect to the previous v6, I've now moved this concept to the
patch where we actually use it for the first time.

In this patch we add system default values, thus a task is now subject
to two possible constraints: the task specific (TS) one or the system
default (SD) one.

The most restrictive of the two must be enforced but we also want to
keep track of the task specific value, while system defaults are
enforce, to restore it when the system defaults are relaxed.

For example:

  TS:   |.............. 20 .................|
  SD:   |... 0 ....|..... 40 .....|... 0 ...|
  Time: |..........|..............|.........|
        t0         t1             t2        t3

Despite the task asking always only for a 20% boost:
 - in [t1,t2] we want to boost it to 40% but, right after...
 - in [t2,t3] we want to go back to the 20% boost.

The "effective" values allows to efficiently enforce the most
restrictive clamp value for a task at enqueue time by:
 - not loosing track of the original request
 - don't caring about updating non runnable tasks

> Also; if you don't add it inside struct uclamp_se, but add a second
> instance,
> 
> >  };
> >  #endif /* CONFIG_UCLAMP_TASK */
> >  
> 
> 
> > @@ -803,6 +811,70 @@ static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id,
> >  	WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> >  }
> >  
> > +/*
> > + * The effective clamp bucket index of a task depends on, by increasing
> > + * priority:
> > + * - the task specific clamp value, when explicitly requested from userspace
> > + * - the system default clamp value, defined by the sysadmin
> > + *
> > + * As a side effect, update the task's effective value:
> > + *    task_struct::uclamp::effective::value
> > + * to represent the clamp value of the task effective bucket index.
> > + */
> > +static inline void
> > +uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> > +		     unsigned int *clamp_value, unsigned int *bucket_id)
> > +{
> > +	/* Task specific clamp value */
> > +	*bucket_id = p->uclamp[clamp_id].bucket_id;
> > +	*clamp_value = p->uclamp[clamp_id].value;
> > +
> > +	/* Always apply system default restrictions */
> > +	if (unlikely(*clamp_value > uclamp_default[clamp_id].value)) {
> > +		*clamp_value = uclamp_default[clamp_id].value;
> > +		*bucket_id = uclamp_default[clamp_id].bucket_id;
> > +	}
> > +}
> 
> you can avoid horrors like this and simply return a struct uclamp_se by
> value.

Yes, that should be possible... will look into splitting this out in
v8 to have something like:

---8<---
struct uclamp_req {
	/* Clamp value "requested" by a scheduling entity */
	unsigned int value		: bits_per(SCHED_CAPACITY_SCALE);
	unsigned int bucket_id		: bits_per(UCLAMP_BUCKETS);
	unsigned int active		: 1;
	unsigned int user_defined	: 1;
}

struct uclamp_eff {
	unsigned int value	        : bits_per(SCHED_CAPACITY_SCALE);
	unsigned int bucket_id	        : bits_per(UCLAMP_BUCKETS);
}

struct task_struct {
        // ...
        #ifdef CONFIG_UCLAMP_TASK
                struct uclamp_req uclamp_req[UCLAMP_CNT];
                struct uclamp_eff uclamp_eff[UCLAMP_CNT];
        #endif
        // ...
}

static inline struct uclamp_eff
uclamp_eff_get(struct task_struct *p, unsigned int clamp_id)
{
        struct uclamp_eff uc_eff = p->uclamp_eff[clamp_id];

	uc_eff.bucket_id = p->uclamp_req[clamp_id].bucket_id;
	uc_eff.clamp_value = p->uclamp_req[clamp_id].value;

	if (unlikely(uc_eff.clamp_value > uclamp_default[clamp_id].value)) {
		uc_eff.clamp_value = uclamp_default[clamp_id].value;
		uc_eff.bucket_id = uclamp_default[clamp_id].bucket_id;
	}

        return uc_eff;
}

static inline void
uclamp_eff_set(struct task_struct *p, unsigned int clamp_id)
{
        p->uclamp_eff[clamp_id] = uclamp_eff_get(p, clamp_id);
}
---8<---

Is that what you mean ?

-- 
#include <best/regards.h>

Patrick Bellasi

  reply	other threads:[~2019-03-13 17:09 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-08 10:05 [PATCH v7 00/15] Add utilization clamping support Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting Patrick Bellasi
2019-03-12 12:52   ` Dietmar Eggemann
2019-03-13 15:15     ` Patrick Bellasi
2019-03-13 21:01       ` Suren Baghdasaryan
2019-03-14 14:54         ` Patrick Bellasi
2019-03-14 15:00       ` Patrick Bellasi
2019-03-12 15:20   ` Peter Zijlstra
2019-03-12 15:50     ` Patrick Bellasi
2019-03-13  8:19       ` Peter Zijlstra
2019-03-13 11:37         ` Patrick Bellasi
2019-03-13 13:40   ` Peter Zijlstra
2019-03-13 16:12     ` Patrick Bellasi
2019-03-13 17:22       ` Peter Zijlstra
2019-03-13 18:22         ` Patrick Bellasi
2019-03-13 19:48       ` Peter Zijlstra
2019-03-14 12:13         ` Patrick Bellasi
2019-03-14 13:32           ` Peter Zijlstra
2019-03-14 15:07             ` Patrick Bellasi
2019-03-14 19:18               ` Peter Zijlstra
2019-03-13 13:52   ` Peter Zijlstra
2019-03-13 15:59     ` Patrick Bellasi
2019-03-13 19:30       ` Peter Zijlstra
2019-03-14 11:03         ` Patrick Bellasi
2019-03-14 13:27           ` Peter Zijlstra
2019-03-13 19:39       ` Peter Zijlstra
2019-03-14 11:18         ` Patrick Bellasi
2019-03-13 21:23     ` Suren Baghdasaryan
2019-03-14 12:43       ` Patrick Bellasi
2019-03-13 14:06   ` Peter Zijlstra
2019-03-13 15:28     ` Patrick Bellasi
2019-03-13 14:09   ` Peter Zijlstra
2019-03-13 15:23     ` Patrick Bellasi
2019-03-13 19:46       ` Peter Zijlstra
2019-03-13 21:08         ` Suren Baghdasaryan
2019-03-14 12:22           ` Patrick Bellasi
2019-03-14 11:45         ` Patrick Bellasi
2019-03-13 21:32   ` Suren Baghdasaryan
2019-03-14 14:46     ` Patrick Bellasi
2019-03-14 15:29       ` Suren Baghdasaryan
2019-03-14 15:40         ` Patrick Bellasi
2019-03-14 16:39           ` Suren Baghdasaryan
2019-02-08 10:05 ` [PATCH v7 02/15] sched/core: uclamp: Enforce last task UCLAMP_MAX Patrick Bellasi
2019-03-13 14:10   ` Peter Zijlstra
2019-03-13 16:20     ` Patrick Bellasi
2019-03-13 17:29       ` Peter Zijlstra
2019-03-13 18:29         ` Patrick Bellasi
2019-03-13 14:12   ` Peter Zijlstra
2019-03-13 16:16     ` Patrick Bellasi
2019-03-14  0:29       ` Suren Baghdasaryan
2019-03-14 17:06         ` Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 03/15] sched/core: uclamp: Add system default clamps Patrick Bellasi
2019-03-13 14:32   ` Peter Zijlstra
2019-03-13 17:09     ` Patrick Bellasi [this message]
2019-03-13 19:58       ` Peter Zijlstra
2019-03-13 20:10       ` Peter Zijlstra
2019-03-15 13:41         ` Patrick Bellasi
2019-03-13 20:13   ` Peter Zijlstra
2019-03-13 20:18   ` Peter Zijlstra
2019-03-18 12:18     ` Patrick Bellasi
2019-03-18 13:10       ` Peter Zijlstra
2019-03-18 14:21         ` Patrick Bellasi
2019-03-18 14:29           ` Peter Zijlstra
2019-02-08 10:05 ` [PATCH v7 04/15] sched/core: Allow sched_setattr() to use the current policy Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 05/15] sched/core: uclamp: Extend sched_setattr() to support utilization clamping Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 06/15] sched/core: uclamp: Reset uclamp values on RESET_ON_FORK Patrick Bellasi
2019-03-13 20:52   ` Peter Zijlstra
2019-03-18 12:58     ` Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 07/15] sched/core: uclamp: Set default clamps for RT tasks Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 08/15] sched/cpufreq: uclamp: Add clamps for FAIR and " Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 09/15] sched/core: uclamp: Add uclamp_util_with() Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 10/15] sched/fair: uclamp: Add uclamp support to energy_compute() Patrick Bellasi
2019-03-06 17:21   ` Quentin Perret
2019-03-18 15:19     ` Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 11/15] sched/core: uclamp: Extend CPU's cgroup controller Patrick Bellasi
2019-02-14 15:48   ` Tejun Heo
2019-03-19 10:00     ` Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 12/15] sched/core: uclamp: Propagate parent clamps Patrick Bellasi
2019-03-14 16:17   ` Suren Baghdasaryan
2019-03-18 16:54     ` Patrick Bellasi
2019-03-18 16:58       ` Suren Baghdasaryan
2019-02-08 10:05 ` [PATCH v7 13/15] sched/core: uclamp: Propagate system defaults to root group Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 14/15] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps Patrick Bellasi
2019-02-08 10:05 ` [PATCH v7 15/15] sched/core: uclamp: Update CPU's refcount on TG's clamp changes Patrick Bellasi

Reply instructions:

You may reply publicly 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=20190313170940.ngiafkmiijryhl2k@e110439-lin \
    --to=patrick.bellasi@arm.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=joelaf@google.com \
    --cc=juri.lelli@redhat.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=morten.rasmussen@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=tj@kernel.org \
    --cc=tkjos@google.com \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --subject='Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).