Re: [RFC PATCH v1] sched/uclamp: Introduce SCHED_FLAG_RESET_UCLAMP_ON_FORK flag

[Qais Yousef]

On 04/28/23 11:12, Saravana Kannan wrote:
> On Fri, Apr 28, 2023 at 4:57 AM Qais Yousef <qyousef@xxxxxxxxxxx> wrote:
> >
> > On 04/19/23 18:54, Qais Yousef wrote:
> >
> > [...]
> >
> > > I was considering to have something a bit more generic that allows selecting
> > > which attributes to reset.
> > >
> > > For example a syscall with SCHED_FLAG_RESET_ON_FORK_SEL combined with
> > > SCHED_FLAG_UCLAMP_MIN/MAX will only reset those. This should make it extensible
> > > if we have other similar use cases in the future. The downside it *might*
> > > require to be done in a separate syscall to the one that sets these parameter.
> > > But it should be done once.
> > >
> > > Maybe there's a better interface, but I think it makes sense to do it in a way
> > > that we won't have to do this again. Would be good to hear from maintainers
> > > first before you take my word for it ;-)
> >
> > Actually I think we can do a better and simpler generic interface. We don't
> > need a new flag. We can just add a new parameter for what to reset on fork.
> > When this value is 0 (which it should be by default), it means reset
> > everything.
> 
> Isn't he default NOT to reset everything?

The default when the RESET_ON_FORK flag is set. This field will not be used
otherwise. Like what happens for the other params.

> 
> > // pseudo code
> >
> > #define RESET_ON_FORK_ALL       0
> > #define RESET_ON_FORK_POLICY    BIT(1) // implies resetting priority
> > #define RESET_ON_FORK_PRIORITY  BIT(2)
> > #define RESET_ON_FORK_UCLAMP    BIT(3)
> >
> > struct sched_attr {
> >         ...
> >         __u64 sched_reset_on_fork_flags;
> > };
> >
> 
> Also, honestly I think this is over designing for a hypothetical. We

latency_nice is coming next and most likely to require something similar. It's
not hypothetical nor over designing. I think it's worthwhile spending time to
plan for the future. More interfaces are confusing to the end users. And glibc
already complained about evolution of sched_setattr, that's why we don't have
a wrapper there yet (beside none of us pushed that hard to resolve the concerns
due to lack of bandwidth).

https://public-inbox.org/libc-alpha/87va826rsb.fsf@xxxxxxxxxxxxxxxxx/

(this thread reminded me linux-api must be CCed)

And there has been various discussions of the need of higher level
wrappers/libraries that exposes simpler interface to app developers. So I'm
actually expecting this to repeat. I think that was at LPC by Len Brown. I can
find this thread on libc mailing list.

https://public-inbox.org/libc-alpha/CAMe9rOpUh1pjfEUqf_hNxce8ZX=4mg6W=n+BbdZSNFHLi7wtkw@xxxxxxxxxxxxxx/

These QoS hints might imply manipulating nice values and I can see ending up
with a similar situation where we need to reset nice on fork without resetting
other params.

Generally I don't think we should restrict users to self-managed model.
A delegated model does make sense, and the latter implies the need for finer
control on what to reset.

There's rtkit by the way which already an example of a delegating model to
enable creating RT tasks by non privileged users.

Should rtkit force resetting uclamp when on fork? I think it's a grey area and
I learn towards it shouldn't.

> have approximately 53 unused bits. By the time we run out of those,
> we'd have added at least 20-50 more fields. At that point, we can
> always add a flags2 field if we need it. I like David's patch as is --
> it's clear and simple. Add a flag for explicitly what we are trying to
> do and extend as needed.

Fair enough. As I said if the maintainers are okay with current proposal, no
objection from my side. Based on my experience I didn't expect them to be. And
I do think a generic solution is not really complicated and is the better
option. You can consider this as a backup plan ;-)


Cheers

--
Qais Yousef