Re: [PATCH] sched: make p->prio independent of p->mm

From: Steven Rostedt
Date: Thu Apr 23 2020 - 11:01:18 EST


On Thu, 23 Apr 2020 22:16:09 +0800
Hillf Danton <hdanton@xxxxxxxx> wrote:

> On Thu, 23 Apr 2020 09:44:03 -0400 Steven Rostedt wrote:
> >
> > On Thu, 23 Apr 2020 11:26:20 +0200
> > Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > > On Thu, Apr 23, 2020 at 12:01:28PM +0800, Hillf Danton wrote:
> > > > --- a/kernel/sched/core.c
> > > > +++ b/kernel/sched/core.c
> > > > @@ -4796,13 +4796,19 @@ recheck:
> > > > return -EINVAL;
> > > >
> > > > /*
> > > > - * Valid priorities for SCHED_FIFO and SCHED_RR are
> > > > - * 1..MAX_USER_RT_PRIO-1, valid priority for SCHED_NORMAL,
> > > > - * SCHED_BATCH and SCHED_IDLE is 0.
> > > > + * The MAX_USER_RT_PRIO value allows the actual maximum
> > > > + * RT priority to be separate from the value exported to
> > > > + * user-space. This allows kernel threads to set their
> > > > + * priority to a value higher than any user task.
> > > > */
> > > > - if ((p->mm && attr->sched_priority > MAX_USER_RT_PRIO-1) ||
> > > > - (!p->mm && attr->sched_priority > MAX_RT_PRIO-1))
> > > > - return -EINVAL;
> > > > + if (p->flags & PF_KTHREAD) {
> > > > + if (attr->sched_priority > MAX_RT_PRIO - 1)
> > > > + return -EINVAL;
> > > > + } else {
> > > > + if (attr->sched_priority > MAX_USER_RT_PRIO - 1)
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > >
> > > Arguably we can do away with the check entirely, MAX_RT_PRIO ==
> > > MAX_USER_RT_PRIO.
> >
> > Heh, that was one of my first patches accepted in the mainline kernel! :-)
> >
> > And the reason we added it, was because there was a small time when the RT
> > patch (or my variation of it) had MAX_USER_RT_PRIO and MAX_RT_PRIO different
> > values, and would crash in that case here.
> >
> > d46523ea32a79 ("fix MAX_USER_RT_PRIO and MAX_RT_PRIO")
> >
> > I would say if we get rid of that check, get rid of the MAX_USER_RT_PRIO
> > with it, and make everything use MAX_RT_PRIO.
>
> BTW the newprio compuation at the beginning of the function looks
> questionable if that check is axed without anything added, because
> it's then used in the case of pi boost.


I believe Peter meant axing the double check, not the check together.

That is, instead of:

if (p->flags & PF_KTHREAD) {
if (attr->sched_priority > MAX_RT_PRIO - 1)
return -EINVAL;
} else {
if (attr->sched_priority > MAX_USER_RT_PRIO - 1)
return -EINVAL;
}

Just have:

if (attr->sched_priority > MAX_RT_PRIO -1)
return -EINVAL;

-- Steve