linux-kernel - Re: [RFC PATCH v1] sched/uclamp: Introduce SCHED_FLAG_RESET_UCLAMP_ON

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230503132943.sz22x4zlln4xagjj@airbuntu>
Date:   Wed, 3 May 2023 14:29:43 +0100
From:   Qais Yousef <qyousef@...alina.io>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     David Dai <davidai@...gle.com>, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Valentin Schneider <vschneid@...hat.com>,
        Qais Yousef <qyousef@...gle.com>,
        Quentin Perret <qperret@...gle.com>, kernel-team@...roid.com,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1] sched/uclamp: Introduce
 SCHED_FLAG_RESET_UCLAMP_ON_FORK flag

On 04/28/23 11:12, Saravana Kannan wrote:
> On Fri, Apr 28, 2023 at 4:57 AM Qais Yousef <qyousef@...alina.io> 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@mid.deneb.enyo.de/

(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@mail.gmail.com/

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