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
>
> [...]
