Re: [PATCH v3 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp

[Yun Hsiang <hsiang023167@gmail.com>]

Hi Patrick,

On Wed, Oct 28, 2020 at 11:11:07AM +0100, Patrick Bellasi wrote:
> 
> Hi Dietmar, Yun,
> I hope I'm not too late before v4 posting ;)
> 
> I think the overall approach is sound, I just added in a couple of
> cleanups and a possible fix (user_defined reset).
> 
> Best,
> Patrick
> 
> 
> On Tue, Oct 27, 2020 at 16:58:13 +0100, Yun Hsiang <hsiang023167@gmail.com> wrote...
> 
> > Hi Diet mar,
> > On Mon, Oct 26, 2020 at 08:00:48PM +0100, Dietmar Eggemann wrote:
> >> On 26/10/2020 16:45, Yun Hsiang wrote:
> 
> [...]
> 
> >> I thought about something like this. Only lightly tested. 
> >> 
> >> ---8<---
> >> 
> >> From: Dietmar Eggemann <dietmar.eggemann@arm.com>
> >> Date: Mon, 26 Oct 2020 13:52:23 +0100
> >> Subject: [PATCH] SCHED_FLAG_UTIL_CLAMP_RESET
> >> 
> >> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> >> ---
> >>  include/uapi/linux/sched.h |  4 +++-
> >>  kernel/sched/core.c        | 31 +++++++++++++++++++++++++++----
> >>  2 files changed, 30 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
> >> index 3bac0a8ceab2..0dd890822751 100644
> >> --- a/include/uapi/linux/sched.h
> >> +++ b/include/uapi/linux/sched.h
> >> @@ -132,12 +132,14 @@ struct clone_args {
> >>  #define SCHED_FLAG_KEEP_PARAMS		0x10
> >>  #define SCHED_FLAG_UTIL_CLAMP_MIN	0x20
> >>  #define SCHED_FLAG_UTIL_CLAMP_MAX	0x40
> >> +#define SCHED_FLAG_UTIL_CLAMP_RESET	0x80
> >>  
> >>  #define SCHED_FLAG_KEEP_ALL	(SCHED_FLAG_KEEP_POLICY | \
> >>  				 SCHED_FLAG_KEEP_PARAMS)
> >>  
> >>  #define SCHED_FLAG_UTIL_CLAMP	(SCHED_FLAG_UTIL_CLAMP_MIN | \
> >> -				 SCHED_FLAG_UTIL_CLAMP_MAX)
> >> +				 SCHED_FLAG_UTIL_CLAMP_MAX | \
> >> +				 SCHED_FLAG_UTIL_CLAMP_RESET)
> >>  
> >>  #define SCHED_FLAG_ALL	(SCHED_FLAG_RESET_ON_FORK	| \
> >>  			 SCHED_FLAG_RECLAIM		| \
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index 3dc415f58bd7..717b1cf5cf1f 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -1438,6 +1438,23 @@ static int uclamp_validate(struct task_struct *p,
> >>  	return 0;
> >>  }
> >>  
> >> +static bool uclamp_reset(enum uclamp_id clamp_id, unsigned long flags)
> >> +{
> 
> Maybe we can add in some comments?
>
I'll add these comment.
> 
>         /* No _UCLAMP_RESET flag set: do not reset */
> >> +	if (!(flags & SCHED_FLAG_UTIL_CLAMP_RESET))
> >> +		return false;
> >> +
> 
>         /* Only _UCLAMP_RESET flag set: reset both clamps */
> >> +	if (!(flags & (SCHED_FLAG_UTIL_CLAMP_MIN | SCHED_FLAG_UTIL_CLAMP_MAX)))
> >> +		return true;
> >> +
>         /* Both _UCLAMP_RESET and _UCLAMP_MIN flags are set: reset only min */
> >> +	if ((flags & SCHED_FLAG_UTIL_CLAMP_MIN) && clamp_id == UCLAMP_MIN)
> >> +		return true;
> >> +
> 
>         /* Both _UCLAMP_RESET and _UCLAMP_MAX flags are set: reset only max */
> >> +	if ((flags & SCHED_FLAG_UTIL_CLAMP_MAX) && clamp_id == UCLAMP_MAX)
> >> +		return true;
> 
> Since the evaluation ordering is important, do we have to better
> _always_ use a READ_ONCE() for all flags accesses above, to ensure it is
> preserved?
>

Is this mean that we want to use READ_ONCE to avoid compiler reordering these
conditions?

> >> +
> >> +	return false;
> >> +}
> >> +
> >>  static void __setscheduler_uclamp(struct task_struct *p,
> >>  				  const struct sched_attr *attr)
> >>  {
> >> @@ -1449,24 +1466,30 @@ static void __setscheduler_uclamp(struct task_struct *p,
> >>  	 */
> 
> Perhaps we should update the comment above this loop with something
> like:
> 
>         /*
>          * Reset to default clamps on forced _UCLAMP_RESET (always) and
>          * for tasks without a task-specific value (on scheduling class change).
>          */
> >>  	for_each_clamp_id(clamp_id) {
> >>  		struct uclamp_se *uc_se = &p->uclamp_req[clamp_id];
> >> +		unsigned int value;
> >>  
> >>  		/* Keep using defined clamps across class changes */
> >> -		if (uc_se->user_defined)
> >> +		if (!uclamp_reset(clamp_id, attr->sched_flags) &&
> >> +		    uc_se->user_defined) {
> >>  			continue;
> >> +		}
> 
> I think we miss to reset the user_defined flag here.
> 
> What about replacing the above chunk with:
> 
>                 if (uclamp_reset(clamp_id, attr->sched_flags))
>                         uc_se->user_defined = false;
>                 if (uc-se->user_defined)
>                         continue;
> 
> ?

user_defined flag will be reset later by uclamp_se_set(uc_se, value,
false). But I agree to split it to two condition because it seems
clearer.

> 
> 
> >>  
> >>  		/*
> >>  		 * RT by default have a 100% boost value that could be modified
> >>  		 * at runtime.
> >>  		 */
> >>  		if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> >> -			__uclamp_update_util_min_rt_default(p);
> >> +			value = sysctl_sched_uclamp_util_min_rt_default;
> 
> By removing this usage of __uclamp_updadate_util_min_rt_default(p),
> the only other usage remaining is the call from:
>    uclamp_udpate_util_min_rt_default().
> 
> What about an additional cleanup by in-lining the only surviving usage?
> 
> 
> >>  		else
> >> -			uclamp_se_set(uc_se, uclamp_none(clamp_id), false);
> >> +			value = uclamp_none(clamp_id);
> >>  
> >> +		uclamp_se_set(uc_se, value, false);
> >>  	}
> >>  
> >> -	if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
> >> +	if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)) ||
> >> +	    attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_RESET) {
> 
> The likely() above should not wrap both conditions to be effective?

Got it.
> 
> >>  		return;
> >> +	}
> >>  
> >>  	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> >>  		uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],