Message-ID: <20201107182447.GA1056076@ubuntu>
Date:   Sun, 8 Nov 2020 02:24:47 +0800
From:   Yun Hsiang <hsiang023167@...il.com>
To:     Dietmar Eggemann <dietmar.eggemann@....com>
Cc:     Qais Yousef <qais.yousef@....com>, peterz@...radead.org,
        linux-kernel@...r.kernel.org, patrick.bellasi@...bug.net,
        kernel test robot <lkp@...el.com>,
        Juri Lelli <juri.lelli@...hat.com>
Subject: Re: [PATCH v5 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET
 flag to reset uclamp

On Wed, Nov 04, 2020 at 10:45:09AM +0100, Dietmar Eggemann wrote:
> On 03/11/2020 14:48, Qais Yousef wrote:
> > Oops, +Juri for real this time.
> > 
> > On 11/03/20 13:46, Qais Yousef wrote:
> >> Hi Yun
> >>
> >> +Juri (A question for you below)
> >>
> >> On 11/03/20 10:37, Yun Hsiang wrote:
> 
> [...]
> 
> >>>  include/uapi/linux/sched.h |  7 +++--
> >>>  kernel/sched/core.c        | 59 ++++++++++++++++++++++++++++----------
> >>>  2 files changed, 49 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
> >>> index 3bac0a8ceab2..6c823ddb1a1e 100644
> >>> --- a/include/uapi/linux/sched.h
> >>> +++ b/include/uapi/linux/sched.h
> >>> @@ -132,17 +132,20 @@ 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
> >>
> >> The new flag needs documentation about how it should be used. It has a none
> >> obvious policy that we can't expect users to just get it.
> 
> See (1) further below.
> 
> >>>  #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)
> >>
> >> Either do this..
> >>
> >>>  
> >>>  #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)
> >>
> >> Or this.
> >>
> >> I checked glibc and it seems they don't use the sched.h from linux and more
> >> surprisingly they don't seem to have a wrapper for sched_setattr(). bionic libc
> >> from Android does take sched.h from linux, but didn't find any user. So we
> >> might be okay with modifying these here.
> 
> Schould be package linux-libc-dev. Debian 10 (buster-backports)
> 5.8.10-1~bpo10+1 contains the uclamp bits as well.
> 
> /usr/include/linux/sched/types.h
> /usr/include/linux/sched.h
> 
> /usr/include/linux/sched.h contains SCHED_FLAG_UTIL_CLAMP and
> SCHED_FLAG_ALL.
> 
> But there is no glibc wrapper for sched_[sg]etattr so syscall wrapping
> is still needed.
> 
> >> I still would like us to document better what we expect from these defines.
> >> Are they for internal kernel use or really for user space? If the former we
> >> should take them out of here. If the latter, then adding the RESET is dangerous
> >> as it'll cause an observable change in behavior; that is if an application was
> >> using SCHED_FLAG_ALL or SCHED_FLAG_UTIL_CLAMP to update the UTIL_MIN/MAX of
> >> a task, existing binaries will find out now that instead of modifying the value
> >> they're actually resetting them.
> 
> I doubt that any application uses SCHED_FLAG_ALL so far since it already
> mixes e.g. DL and UCLAMP stuff. AFAIK the only tools supporting uclamp
> so far is rt-app and uclampset, which both use their own files for DL
> and uclamp definition.
> 

I think SCHED_FLAG_ALL is for internal kernel use. So is it better to
make it within an #ifdef __KERNEL__ #endif block as Patrick said?

> [..]
> 
> >> Add the policy part of the commit message as a documentation to this function
> >> please.
> >>
> >> ie:
> >>
> >> 	/*
> >> 	 * The policy is
> >> 	 * _CLAMP_RESET                           => reset both min and max
> >> 	 * _CLAMP_RESET | _CLAMP_MIN              => reset min value
> >> 	 * _CLAMP_RESET | _CLAMP_MAX              => reset max value
> >> 	 * _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max
> >> 	 */
> >>
> 
> (1) Related to documentation, wouldn't it be better to document in
> include/uapi/linux/sched.h, i.e. where the flags are defined, so it gets
> exported via linux-libc-dev?
> Like it's done for struct sched_attr members in
> include/uapi/linux/sched/types.h.
> 

Ok, I'll put the comment for_CLAMP_MIN/_CLAMP_MAX/CLAMP_RESET in
include/uapi/linux/sched.h.

> [...]
> 
> >>> -	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)
> >>>  		return;
> 
> Another multi line statement so the 'return' could go with braces.
> 
> >>>  
> >>> -	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> >>> +	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN)
> >>>  		uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
> >>> -			      attr->sched_util_min, true);
> >>> -	}
> >>> +				attr->sched_util_min, true);
> >>>  
> >>> -	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX) {
> >>> +	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX)
> >>>  		uclamp_se_set(&p->uclamp_req[UCLAMP_MAX],
> >>> -			      attr->sched_util_max, true);
> >>> -	}
> >>> +				attr->sched_util_max, true);
> >>
> >> These two hunks seem unrelated too. Multi line statement should still have
> >> braces AFAIK. Why change it?

Sorry for the wrong coding style, I'll fix it. And I'll split the
modifications to different patches if I want to do some cleanup.

> +1
> 
> [...]

Powered by blists - more mailing lists