Message-ID: <CAGETcx9RnVwz3fO2U4x-CkZcMoECee9kUXb_vm0kDK9kXLXapQ@mail.gmail.com>
Date:   Wed, 3 May 2023 08:55:13 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Qais Yousef <qyousef@...alina.io>
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 Wed, May 3, 2023 at 6:29 AM Qais Yousef <qyousef@...alina.io> wrote:
>
> 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).

My comment about over designing/hypothetical is about the "running out
of bits" concern you are trying to solve. Totally agree that we'll
likely need finer reset control and never disagreed with that. But we
can add those as we go to the existing flag field instead of
introducing a new one.

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

Nothing to say to any of the stuff above because I didn't disagree on
this in the first place.

> > 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 ;-)

I think the fact I could misunderstand the API and you had to explain
it makes it a bit more complicated (agree, not a lot) over just using
the existing bits. And I think that's what the maintainers are pushing
back on (in addition to even having finer reset control).

-Saravana

Powered by blists - more mailing lists