Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Doug Anderson <dianders@chromium.org>]

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:
> > >
> > > uclamp_fork() resets the uclamp values to their default when the
> > > reset-on-fork flag is set. It also checks whether the task has a RT
> > > policy, and sets its uclamp.min to 1024 accordingly. However, during
> > > reset-on-fork, the task's policy is lowered to SCHED_NORMAL right after,
> > > hence leading to an erroneous uclamp.min setting for the new task if it
> > > was forked from RT.
> > >
> > > Fix this by removing the unnecessary check on rt_policy() in
> > > uclamp_fork() as this doesn't make sense if the reset-on-fork flag is
> > > set.
> > >
> > > Reported-by: Chitti Babu Theegala <ctheegal@codeaurora.org>
> > > Signed-off-by: Quentin Perret <qperret@google.com>
> > > ---
> > >  kernel/sched/core.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > 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.

-Doug