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

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

[Peter Zijlstra]

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.

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

[Steven Rostedt]

On Thu, 23 Apr 2020 11:26:20 +0200
Peter Zijlstra <peterz@infradead.org> 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.

-- Steve

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

[Steven Rostedt]

On Thu, 23 Apr 2020 22:16:09 +0800
Hillf Danton <hdanton@sina.com> 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@infradead.org> 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

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

[Steven Rostedt]

On Fri, 24 Apr 2020 08:30:28 +0800
Hillf Danton <hdanton@sina.com> wrote:

> > 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;
> >   
> Got it, thank you :) Spin in couple of days.

I still think getting rid of MAX_USER_RT_PRIO would be good too.

-- Steve

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

[Steven Rostedt]

On Fri, 24 Apr 2020 10:12:31 +0800
Hillf Danton <hdanton@sina.com> wrote:

> Yes and if you agree, we send it home during the 5.8 cycle, or not before
> fifo is reclaimed from modules.
> 
> In the spin it now looks like
> 
> --- a/include/linux/sched/prio.h
> +++ b/include/linux/sched/prio.h
> @@ -12,15 +12,12 @@
>   * tasks are in the range MAX_RT_PRIO..MAX_PRIO-1. Priority
>   * values are inverted: lower p->prio value means higher priority.
>   *
> - * 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. Note:
> - * MAX_RT_PRIO must not be smaller than MAX_USER_RT_PRIO.
> + * Note: MAX_USER_RT_PRIO will be removed as early as 5.8,
> + * don't use it in new code.
>   */
>  
> -#define MAX_USER_RT_PRIO	100
> -#define MAX_RT_PRIO		MAX_USER_RT_PRIO
> +#define MAX_RT_PRIO		100
> +#define MAX_USER_RT_PRIO	MAX_RT_PRIO
>  
>  #define MAX_PRIO		(MAX_RT_PRIO + NICE_WIDTH)
>  #define DEFAULT_PRIO		(MAX_RT_PRIO + NICE_WIDTH / 2)

No one has used it in years, I don't think we need this change. Just delete
it in one go.

Is making p->prio independent from p->mm needed for other changes? If not,
then we can hold off this change until we do so, otherwise, I would keep
your original patch as is, and then remove the extra check when we remove
MAX_USER_RT_PRIO.

-- Steve