Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 3 May 2023 14:29:43 +0100
From:   Qais Yousef <qyousef@layalina.io>
To:     Saravana Kannan <saravanak@google.com>
Cc:     David Dai <davidai@google.com>, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        Qais Yousef <qyousef@google.com>,
        Quentin Perret <qperret@google.com>, kernel-team@android.com,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1] sched/uclamp: Introduce
 SCHED_FLAG_RESET_UCLAMP_ON_FORK flag
Message-ID: <20230503132943.sz22x4zlln4xagjj@airbuntu>
References: <20230416213406.2966521-1-davidai@google.com>
 <20230419175414.ycnlaperdkjxgypx@airbuntu>
 <20230428115755.7uklfd4fmpoee4dp@airbuntu>
 <CAGETcx9xjLzQ2b3FraPsmG=w6=4pYU-tzW-na0Fd5qyrU+C+Yw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAGETcx9xjLzQ2b3FraPsmG=w6=4pYU-tzW-na0Fd5qyrU+C+Yw@mail.gmail.com>
Precedence: bulk

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