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> <d83950c4-7458-aeea-f341-327c163704a8@arm.com>
 <CABN1KC+_HDi_i2zzpZVbqiUP5-QB9YrE5wzLqr==_wOemaCXzA@mail.gmail.com>
 <bf8f21be-7249-fc27-9704-211d0f5a12b1@arm.com> <CAKfTPtAgkyE1xntn-4u9o8DFhH9iGq54c-QXYr0cE+zvoPx9Gw@mail.gmail.com>
In-Reply-To: <CAKfTPtAgkyE1xntn-4u9o8DFhH9iGq54c-QXYr0cE+zvoPx9Gw@mail.gmail.com>
From:   Saravana Kannan <saravanak@google.com>
Date:   Thu, 20 Apr 2023 09:25:23 -0700
Message-ID: <CAGETcx-suryHeB3wpaTSZBiw6+VwA7pe=GnrbtizSVj+C9Smtg@mail.gmail.com>
Subject: Re: [RFC PATCH v1] sched/uclamp: Introduce SCHED_FLAG_RESET_UCLAMP_ON_FORK
 flag
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     Dietmar Eggemann <dietmar.eggemann@arm.com>,
        David Dai <davidai@google.com>, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.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 Thu, Apr 20, 2023 at 6:44=E2=80=AFAM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Thu, 20 Apr 2023 at 11:37, Dietmar Eggemann <dietmar.eggemann@arm.com>=
 wrote:
> >
> > On 20/04/2023 03:11, David Dai wrote:
> > > On Tue, Apr 18, 2023 at 10:18=E2=80=AFPM Dietmar Eggemann
> > > <dietmar.eggemann@arm.com> wrote:
> > >>
> > >
> > > Hi Dietmar, thanks for your time,
> > >
> > >> On 16/04/2023 23:34, David Dai wrote:
> > >>> A userspace service may manage uclamp dynamically for individual ta=
sks and
> > >>> a child task will unintentionally inherit a pesudo-random uclamp se=
tting.
> > >>> This could result in the child task being stuck with a static uclam=
p value
> > >>
> > >> Could you explain this with a little bit more detail? Why isn't the
> > >> child task also managed by the userspace service?
> > >
> > > See Qais=E2=80=99 reply that contains more detail on how it=E2=80=99s=
 being used in
> > > Android. In general, if a dynamic userspace service will adjust uclam=
p
> > > on the fly for a given task, but has no knowledge or control over if
> > > or when a task forks. Depending on the timing of the fork, a child
> > > task may inherit a very large or a small uclamp_min or uclamp_max
> > > value. The intent of this patch is to provide more flexibility to the
> > > uclamp APIs such that child tasks do not get stuck with a poor uclamp
> > > value when spawned while retaining other sched attributes. When
> > > RESET_ON_FORK is set on the parent task, it will reset uclamp values
> > > for the child but also reset other sched attributes as well.
> >
> > OK, in this case, why not just change behavior and always reset the
> > uclamp values at fork?
> >
> > Do we anticipate a use-case in which uclamp inheritance would be requir=
ed?
> >
> > Let's not over-complicate the sched_[sg]etattr() unnecessarily.
>
> I was about to ask the same question and I'm aligned with Dietmar.
> Use RESET_ON_FORK and set all attributes

That's racy though. If we have an external service (that's only
responsible for setting uclamp) setting all the attributes, the forked
thread could also be trying to set some of the attributes. Also, how
is this external service going to keep track of all the threads being
forked and set the right attributes for all of them?

If it's not considered a UAPI breakage, I'd rather we never inherit uclamp.

-Saravana

>
> >
> > [...]
> >
> > >> Does this issue happen with uclamp mainline or only with Android's
> > >> slightly different version (max- vs. sum aggregation)?
> > >
> > > I=E2=80=99m using the version of uclamp that=E2=80=99s in Linus=E2=80=
=99 tree. How uclamp is
> > > aggregated is unrelated to the problem I=E2=80=99m trying to solve wi=
th this
> > > patch. Which is to extend the uclamp APIs to have finer control for
> > > the uclamp inheritance of child tasks.
> >
> > OK, I see.