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;
MIME-Version: 1.0
References: <20230416213406.2966521-1-davidai@google.com> <20230419175414.ycnlaperdkjxgypx@airbuntu>
 <20230428115755.7uklfd4fmpoee4dp@airbuntu>
In-Reply-To: <20230428115755.7uklfd4fmpoee4dp@airbuntu>
From:   Saravana Kannan <saravanak@google.com>
Date:   Fri, 28 Apr 2023 11:12:35 -0700
Message-ID: <CAGETcx9xjLzQ2b3FraPsmG=w6=4pYU-tzW-na0Fd5qyrU+C+Yw@mail.gmail.com>
Subject: Re: [RFC PATCH v1] sched/uclamp: Introduce SCHED_FLAG_RESET_UCLAMP_ON_FORK
 flag
To:     Qais Yousef <qyousef@layalina.io>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Apr 28, 2023 at 4:57=E2=80=AFAM Qais Yousef <qyousef@layalina.io> w=
rote:
>
> On 04/19/23 18:54, Qais Yousef wrote:
>
> [...]
>
> > I was considering to have something a bit more generic that allows sele=
cting
> > 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 ex=
tensible
> > if we have other similar use cases in the future. The downside it *migh=
t*
> > require to be done in a separate syscall to the one that sets these par=
ameter.
> > But it should be done once.
> >
> > Maybe there's a better interface, but I think it makes sense to do it i=
n a way
> > that we won't have to do this again. Would be good to hear from maintai=
ners
> > 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 for=
k.
> When this value is 0 (which it should be by default), it means reset
> everything.

Isn't he default NOT to reset everything?

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

-Saravana