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

[Steven Rostedt]

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