Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20201012163140.371688-1-hsiang023167@gmail.com>
 <87blh6iljc.derkling@matbug.net>
 <20201013102951.orcr6m4q2cb7y6zx@e107158-lin>
User-agent: mu4e 1.4.13; emacs 27.1
From:   Patrick Bellasi <patrick.bellasi@matbug.net>
To:     Qais Yousef <qais.yousef@arm.com>
Cc:     Yun Hsiang <hsiang023167@gmail.com>, dietmar.eggemann@arm.com,
        peterz@infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET
 flag to reset uclamp
In-reply-to: <20201013102951.orcr6m4q2cb7y6zx@e107158-lin>
Message-ID: <875z7eic14.derkling@matbug.net>
Date:   Tue, 13 Oct 2020 13:46:31 +0200
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Tue, Oct 13, 2020 at 12:29:51 +0200, Qais Yousef <qais.yousef@arm.com> wrote...

> On 10/13/20 10:21, Patrick Bellasi wrote:
>> 

[...]

>> > +#define SCHED_FLAG_UTIL_CLAMP_RESET (SCHED_FLAG_UTIL_CLAMP_RESET_MIN | \
>> > +					SCHED_FLAG_UTIL_CLAMP_RESET_MAX)
>> > +
>> >  #define SCHED_FLAG_ALL	(SCHED_FLAG_RESET_ON_FORK	| \
>> >  			 SCHED_FLAG_RECLAIM		| \
>> >  			 SCHED_FLAG_DL_OVERRUN		| \
>> >  			 SCHED_FLAG_KEEP_ALL		| \
>> > -			 SCHED_FLAG_UTIL_CLAMP)
>> > +			 SCHED_FLAG_UTIL_CLAMP		| \
>> > +			 SCHED_FLAG_UTIL_CLAMP_RESET)
>> 
>> 
>> ... and use it in conjunction with the existing _CLAMP_{MIN,MAX} to know
>> which clamp should be reset?
>
> I think the RESET should restore *both* MIN and MAX and reset the user_defined
> flag. Since the latter is the main purpose of this interface, I don't think you
> can reset the user_defined flag without resetting both MIN and MAX to
> uclamp_none[UCLAMP_MIN/MAX].

We can certainly set one clamp and not the other, and indeed the
user_defined flag is per-clamp_id, thus we can reset one clamp while
still keeping user-defined the other one.


>> >  #endif /* _UAPI_LINUX_SCHED_H */
>> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> > index 9a2fbf98fd6f..ed4cb412dde7 100644
>> > --- a/kernel/sched/core.c
>> > +++ b/kernel/sched/core.c
>> > @@ -1207,15 +1207,22 @@ static void __setscheduler_uclamp(struct task_struct *p,
>> >  		uclamp_se_set(uc_se, clamp_value, false);
>> >  	}
>> >  
>> > -	if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
>> > +	if (likely(!(attr->sched_flags &
>> > +			(SCHED_FLAG_UTIL_CLAMP | SCHED_FLAG_UTIL_CLAMP_RESET))))
>> >  		return;
>> 
>> This check will not be changed, while we will have to add a bypass in
>> uclamp_validate().
>> 
>> >  
>> > -	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
>> > +	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_RESET_MIN) {
>> > +		uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
>> > +			      0, false);
>> > +	} else if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
>> >  		uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
>> >  			      attr->sched_util_min, true);
>> >  	}
>> >
>> 
>> These checks also will have to be updated to check _RESET and
>> _{MIN,MAX} combinations.
>> 
>> Bonus point would be to be possible to pass in just the _RESET flag if
>> we want to reset both clamps. IOW, passing in _RESET only should be
>> consumed as if we passed in _RESET|_MIN|_MAX.
>> 
>> Caveat, RT tasks have got a special 'reset value' for _MIN.
>> We should check and ensure __uclamp_update_uti_min_rt_default() is
>> property called for those tasks, which likely will require some
>> additional refactoring :/
>
> Hmm I am probably missing something. But if the SCHED_FLAG_UTIL_CLAMP_RESET is
> set, just reset uc_se->user_defined in the loop in __setscheduler_uclamp().
> This should take care of doing the reset properly then. Including for
> RT tasks.

Yes and no. Yes because in principle we can just reset the flag for a
clamp_id without updating the request values, as it is done by the
snippets above, and the internals should work.

However, we will end up reporting the old values when reading from
user-space. We should better check all those reporting code paths or...
just update the requested values as Yun is proposing above.

I like better Yun approach so that we keep internal data structures
aligned with features.

> So IMO you just need a single SCHED_FLAG_UTIL_CLAMP_RESET that if set in the
> attr, you just execute that loop in __setscheduler_uclamp() + reset
> uc_se->user_defined.
>
> It should be invalid to pass the SCHED_FLAG_UTIL_CLAMP_RESET with
> SCHED_FLAG_UTIL_CLAMP_MIN/MAX. Both have contradictory meaning IMO.
> If user passes both we should return an EINVAL error.

Passing in  _CLAMP_RESET|_CLAMP_MIN will mean reset the min value while
keeping the max at whatever it is. I think there could be cases where
this support could be on hand.

However, as in my previous email, by passing in only _CLAMP_RESET, we
should go an reset both.