Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 15 Apr 2020 19:47:18 +0200
From:   Patrick Bellasi <patrick.bellasi@matbug.net>
To:     Doug Anderson <dianders@chromium.org>
Cc:     Quentin Perret <qperret@google.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>, juri.lelli@redhat.com,
        Vincent Guittot <vincent.guittot@linaro.org>,
        dietmar.eggemann@arm.com, Steven Rostedt <rostedt@goodmis.org>,
        Benjamin Segall <bsegall@google.com>, mgorman@suse.de,
        ctheegal@codeaurora.org, valentin.schneider@arm.com,
        qais.yousef@arm.com, LKML <linux-kernel@vger.kernel.org>,
        kernel-team@android.com
Subject: Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp
Message-ID: <20200415174718.GA30565@darkstar>
References: <20200414161320.251897-1-qperret@google.com>
 <CAD=FV=Vo4h43vS1K1+ziAJhQ3UG+Zrx8JN8Q1tkMWU1Oh6OavA@mail.gmail.com>
 <20200415082001.GA256573@google.com>
 <CAD=FV=UB7OZzQO46dV7KOHGqRkgbtaSgLfq55yddyx0L_e37Fg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAD=FV=UB7OZzQO46dV7KOHGqRkgbtaSgLfq55yddyx0L_e37Fg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 15-Apr 09:31, Doug Anderson wrote:
> Hi,
> 
> On Wed, Apr 15, 2020 at 1:20 AM Quentin Perret <qperret@google.com> wrote:
> >
> > Hi Doug,
> > On Tuesday 14 Apr 2020 at 13:45:03 (-0700), Doug Anderson wrote:
> > > Hi,
> > > On Tue, Apr 14, 2020 at 9:13 AM Quentin Perret <qperret@google.com> wrote:

[...]

> > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > > index 3a61a3b8eaa9..9ea3e484eea2 100644
> > > > --- a/kernel/sched/core.c
> > > > +++ b/kernel/sched/core.c
> > > > @@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
> > > >         for_each_clamp_id(clamp_id) {
> > > >                 unsigned int clamp_value = uclamp_none(clamp_id);
> > > >
> > > > -               /* By default, RT tasks always get 100% boost */
> > > > -               if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> > > > -                       clamp_value = uclamp_none(UCLAMP_MAX);
> > > > -
> > > >                 uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
> > >
> > > The local variable "clamp_value" doesn't have a lot of value anymore,
> > > does it?  (Pun intended).
> >
> > :)
> >
> > > Remove it?
> >
> > Right, but I figured the generated code should be similar, and
> > 'uclamp_se_set(&p->uclamp_req[clamp_id], uclamp_none(clamp_id), false);'
> > doesn't fit in 80 cols at this identation level, so I kept the local
> > var. No strong opinion, though.
> 
> OK.  It's definitely a bikeshed color issue and since you'll spend
> more time in this bikeshed than I will I'll leave it to you to pick
> the color.
> 
> I'm not at all an expert on this code but it sure looks sane to me.
> If you think my review tag is worth anything in this context feel free
> to add it, but since you already have Patrick's mine probably adds
> very little value.

Honestly I was almost there to ask for the same cleanup. :)

If you get rid of the useless variable we can go back to the exact
same code modified by the commit we are fixing, i.e. 1a00d999971c.

So, not strong opinions here too, but if it's not a big problem I
would post a cleaned up v2.

Best,
Patrick

-- 
#include <best/regards.h>

Patrick Bellasi