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

[Qais Yousef]

Hi Yun

Sorry for chipping in late.

On 10/25/20 15:36, Yun Hsiang wrote:
> If the user wants to stop controlling uclamp and let the task inherit
> the value from the group, we need a method to reset.
> 
> Add SCHED_FLAG_UTIL_CLAMP_RESET flag to allow the user to reset uclamp via
> sched_setattr syscall.
> 
> 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
> 
> Signed-off-by: Yun Hsiang <hsiang023167@xxxxxxxxx>
> ---
>  include/uapi/linux/sched.h |  7 +++++--
>  kernel/sched/core.c        | 41 +++++++++++++++++++++++++++++++-------
>  2 files changed, 39 insertions(+), 9 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
>  
>  #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)

Is it safe to change this define in a uapi header without a potential
consequence?

FWIW I still have concerns about this approach. We're doing a reset to control
cgroup behavior, I don't see any correlation between the two. Besides the
difference between RESET and setting uclamp_min=0 without RESET is not obvious
nor intuitive for someone who didn't look at the code.

I propose something like the below which is more explicit about what is being
requested and delivered here. And if we decide to deprecate this behavior,
it'd be much easier to just ignore this flag.

You must set this flag with your uclamp request to retain the cgroup
inheritance behavior. If the flag is not set, we automatically clear it.

Only compile tested.

Thanks

--
Qais Yousef


--------->8-----------