Re: [PATCH v8 04/16] sched/core: uclamp: Add system default clamps

[Peter Zijlstra <peterz@infradead.org>]

On Thu, May 09, 2019 at 10:10:57AM +0100, Patrick Bellasi wrote:
> On 08-May 21:15, Peter Zijlstra wrote:
> > On Wed, May 08, 2019 at 09:07:33PM +0200, Peter Zijlstra wrote:
> > > On Tue, Apr 02, 2019 at 11:41:40AM +0100, Patrick Bellasi wrote:
> > > > +static inline struct uclamp_se
> > > > +uclamp_eff_get(struct task_struct *p, unsigned int clamp_id)
> > > > +{
> > > > +	struct uclamp_se uc_req = p->uclamp_req[clamp_id];
> > > > +	struct uclamp_se uc_max = uclamp_default[clamp_id];
> > > > +
> > > > +	/* System default restrictions always apply */
> > > > +	if (unlikely(uc_req.value > uc_max.value))
> > > > +		return uc_max;
> > > > +
> > > > +	return uc_req;
> > > > +}
> > > > +
> > > > +static inline unsigned int
> > > > +uclamp_eff_bucket_id(struct task_struct *p, unsigned int clamp_id)
> > > > +{
> > > > +	struct uclamp_se uc_eff;
> > > > +
> > > > +	/* Task currently refcounted: use back-annotated (effective) bucket */
> > > > +	if (p->uclamp[clamp_id].active)
> > > > +		return p->uclamp[clamp_id].bucket_id;
> > > > +
> > > > +	uc_eff = uclamp_eff_get(p, clamp_id);
> > > > +
> > > > +	return uc_eff.bucket_id;
> > > > +}
> > > > +
> > > > +unsigned int uclamp_eff_value(struct task_struct *p, unsigned int clamp_id)
> > > > +{
> > > > +	struct uclamp_se uc_eff;
> > > > +
> > > > +	/* Task currently refcounted: use back-annotated (effective) value */
> > > > +	if (p->uclamp[clamp_id].active)
> > > > +		return p->uclamp[clamp_id].value;
> > > > +
> > > > +	uc_eff = uclamp_eff_get(p, clamp_id);
> > > > +
> > > > +	return uc_eff.value;
> > > > +}
> > > 
> > > This is 'wrong' because:
> > > 
> > >   uclamp_eff_value(p,id) := uclamp_eff(p,id).value
> > 
> > Clearly I means to say the above does not hold with the given
> > implementation, while the naming would suggest it does.
> 
> Not sure to completely get your point...

the point is that uclamp_eff_get() doesn't do the back annotate thing
and therefore returns something entirely different from
uclamp_eff_{bucket_id,value}(), where the naming would suggest it in
fact returns the same thing.

> > > Which seems to suggest the uclamp_eff_*() functions want another name.
> 
> That function returns the effective value of a task, which is either:
>  1. the back annotated value for a RUNNABLE task
> or
>  2. the aggregation of task-specific, system-default and cgroup values
>     for a non RUNNABLE task.

Right, but uclamp_eff_get() doesn't do 1, while the other two do do it.
And that is confusing.

> > > Also, suppose the above would be true; does GCC really generate better
> > > code for the LHS compared to the RHS?
> 
> It generate "sane" code which implements the above logic and allows
> to know that whenever we call uclamp_eff_value(p,id) we get the most
> updated effective value for a task, independently from its {!}RUNNABLE
> state.
> 
> I would keep the function but, since Suren also complained also about
> the name... perhaps I should come up with a better name? Proposals?

Right, so they should move to the patch where they're needed, but I was
wondering why you'd not written something like:

static inline
struct uclamp_se uclamp_active(struct task_struct *p, unsigned int clamp_id)
{
	if (p->uclamp[clamp_id].active)
		return p->uclamp[clamp_id];

	return uclamp_eff(p, clamp_id);
}

And then used:

	uclamp_active(p, id).{value,bucket_id}

- OR -

have uclamp_eff() include the active thing, afaict the callsite in
uclamp_rq_inc_id() guarantees !active.

In any case, I'm thinking the foo().member notation saves us from having
to have two almost identical functions and the 'inline' part should get
GCC to generate sane code.