Messages in this thread | | | Subject | Re: [PATCH v5 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp | From | Dietmar Eggemann <> | Date | Wed, 4 Nov 2020 10:45:09 +0100 |
| |
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.
[..]
>> 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.
[...]
>>> - 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?
+1
[...]
| |