[PATCH linux-next] sched: Fix broken setscheduler()

[PATCH linux-next] sched: Fix broken setscheduler()

[Steven Rostedt]

I decided to run my tests on linux-next, and my wakeup_rt tracer was
broken. After running a bisect, I found that the problem commit was:

   linux-next commit c365c292d059
   "sched: Consider pi boosting in setscheduler()"

And the reason the wake_rt tracer test was failing, was because it had
no RT task to trace. I first noticed this when running with
sched_switch event and saw that my RT task still had normal SCHED_OTHER
priority. Looking at the problem commit, I found:

 -       p->normal_prio = normal_prio(p);
 -       p->prio = rt_mutex_getprio(p);

With no

 +       p->normal_prio = normal_prio(p);
 +       p->prio = rt_mutex_getprio(p);

Reading what the commit is suppose to do, I realize that the p->prio
can't be set if the task is boosted with a higher prio, but the
p->normal_prio still needs to be set regardless, otherwise, when the
task is deboosted, it wont get the new priority.

The p->prio has to be set before "check_class_changed()" is called,
otherwise the class wont be changed.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4600bca..b1cc871 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3198,6 +3198,7 @@ static void __setscheduler_params(struct task_struct *p,
 	 * getparam()/getattr() don't report silly values for !rt tasks.
 	 */
 	p->rt_priority = attr->sched_priority;
+	p->normal_prio = normal_prio(p);
 	set_load_weight(p);
 }
 
@@ -3207,6 +3208,8 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
 {
 	__setscheduler_params(p, attr);
 
+	p->prio = rt_mutex_getprio(p);
+
 	if (dl_prio(p->prio))
 		p->sched_class = &dl_sched_class;
 	else if (rt_prio(p->prio))

Re: [PATCH linux-next] sched: Fix broken setscheduler()

[Peter Zijlstra]

On Wed, Mar 05, 2014 at 11:29:31PM -0500, Steven Rostedt wrote:
> I decided to run my tests on linux-next, and my wakeup_rt tracer was
> broken. After running a bisect, I found that the problem commit was:
> 
>    linux-next commit c365c292d059
>    "sched: Consider pi boosting in setscheduler()"
> 
> And the reason the wake_rt tracer test was failing, was because it had
> no RT task to trace. I first noticed this when running with
> sched_switch event and saw that my RT task still had normal SCHED_OTHER
> priority. Looking at the problem commit, I found:
> 
>  -       p->normal_prio = normal_prio(p);
>  -       p->prio = rt_mutex_getprio(p);
> 
> With no
> 
>  +       p->normal_prio = normal_prio(p);
>  +       p->prio = rt_mutex_getprio(p);
> 
> Reading what the commit is suppose to do, I realize that the p->prio
> can't be set if the task is boosted with a higher prio, but the
> p->normal_prio still needs to be set regardless, otherwise, when the
> task is deboosted, it wont get the new priority.
> 
> The p->prio has to be set before "check_class_changed()" is called,
> otherwise the class wont be changed.

So Juri had a different patch for this problem:

  http://lkml.kernel.org/r/20140301191838.d15d03112b2598a671dac22c@gmail.com

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 4600bca..b1cc871 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3198,6 +3198,7 @@ static void __setscheduler_params(struct task_struct *p,
>  	 * getparam()/getattr() don't report silly values for !rt tasks.
>  	 */
>  	p->rt_priority = attr->sched_priority;
> +	p->normal_prio = normal_prio(p);
>  	set_load_weight(p);
>  }

Now; if I'm reading things right, normal_prio is the unboosted priority
of a task. And we should indeed keep setting that, otherwise the unboost
doesn't know where it should go.

Juri put that in __setscheduler(), but I think that's wrong because the
rt_mutex_check_prio() case in __sched_setscheduler() still needs to
update this.

> @@ -3207,6 +3208,8 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
>  {
>  	__setscheduler_params(p, attr);
>  
> +	p->prio = rt_mutex_getprio(p);
> +
>  	if (dl_prio(p->prio))
>  		p->sched_class = &dl_sched_class;
>  	else if (rt_prio(p->prio))
> 

And when we call this we're sure to not be boosted; so this is
effectively the same as Juri has:

  p->prio = p->normal_prio = normal_prio(p)

Seeing how rt_mutex_getprio() and normal_prio() are the same under these
conditions.

Re: [PATCH linux-next] sched: Fix broken setscheduler()

[Juri Lelli]

On Thu, 6 Mar 2014 12:58:25 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Mar 05, 2014 at 11:29:31PM -0500, Steven Rostedt wrote:
> > I decided to run my tests on linux-next, and my wakeup_rt tracer was
> > broken. After running a bisect, I found that the problem commit was:
> > 
> >    linux-next commit c365c292d059
> >    "sched: Consider pi boosting in setscheduler()"
> > 
> > And the reason the wake_rt tracer test was failing, was because it had
> > no RT task to trace. I first noticed this when running with
> > sched_switch event and saw that my RT task still had normal SCHED_OTHER
> > priority. Looking at the problem commit, I found:
> > 
> >  -       p->normal_prio = normal_prio(p);
> >  -       p->prio = rt_mutex_getprio(p);
> > 
> > With no
> > 
> >  +       p->normal_prio = normal_prio(p);
> >  +       p->prio = rt_mutex_getprio(p);
> > 
> > Reading what the commit is suppose to do, I realize that the p->prio
> > can't be set if the task is boosted with a higher prio, but the
> > p->normal_prio still needs to be set regardless, otherwise, when the
> > task is deboosted, it wont get the new priority.
> > 
> > The p->prio has to be set before "check_class_changed()" is called,
> > otherwise the class wont be changed.
> 
> So Juri had a different patch for this problem:
> 
>   http://lkml.kernel.org/r/20140301191838.d15d03112b2598a671dac22c@gmail.com
> 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 4600bca..b1cc871 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -3198,6 +3198,7 @@ static void __setscheduler_params(struct task_struct *p,
> >  	 * getparam()/getattr() don't report silly values for !rt tasks.
> >  	 */
> >  	p->rt_priority = attr->sched_priority;
> > +	p->normal_prio = normal_prio(p);
> >  	set_load_weight(p);
> >  }
> 
> Now; if I'm reading things right, normal_prio is the unboosted priority
> of a task. And we should indeed keep setting that, otherwise the unboost
> doesn't know where it should go.
> 
> Juri put that in __setscheduler(), but I think that's wrong because the
> rt_mutex_check_prio() case in __sched_setscheduler() still needs to
> update this.
> 

Oh, right. Missed that.

> > @@ -3207,6 +3208,8 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
> >  {
> >  	__setscheduler_params(p, attr);
> >  
> > +	p->prio = rt_mutex_getprio(p);
> > +
> >  	if (dl_prio(p->prio))
> >  		p->sched_class = &dl_sched_class;
> >  	else if (rt_prio(p->prio))
> > 
> 
> And when we call this we're sure to not be boosted; so this is
> effectively the same as Juri has:
> 
>   p->prio = p->normal_prio = normal_prio(p)
> 
> Seeing how rt_mutex_getprio() and normal_prio() are the same under these
> conditions.
> 
> 

Yes. I think you can go with

 p->prio = p->normal_prio

and save a few checks in rt_mutex_getprio().

Thanks,

- Juri

[PATCH v2] sched: Fix broken setscheduler()

[Steven Rostedt]

On Thu, 6 Mar 2014 13:20:40 +0100
Juri Lelli <juri.lelli@gmail.com> wrote:

> Yes. I think you can go with
> 
>  p->prio = p->normal_prio
> 
> and save a few checks in rt_mutex_getprio().
> 

You're right! Thanks.

-----

I decided to run my tests on linux-next, and my wakeup_rt tracer was
broken. After running a bisect, I found that the problem commit was:

   linux-next commit c365c292d059
   "sched: Consider pi boosting in setscheduler()"

And the reason the wake_rt tracer test was failing, was because it had
no RT task to trace. I first noticed this when running with
sched_switch event and saw that my RT task still had normal SCHED_OTHER
priority. Looking at the problem commit, I found:

 -       p->normal_prio = normal_prio(p);
 -       p->prio = rt_mutex_getprio(p);

With no

 +       p->normal_prio = normal_prio(p);
 +       p->prio = rt_mutex_getprio(p);

Reading what the commit is suppose to do, I realize that the p->prio
can't be set if the task is boosted with a higher prio, but the
p->normal_prio still needs to be set regardless, otherwise, when the
task is deboosted, it wont get the new priority.

The p->prio has to be set before "check_class_changed()" is called,
otherwise the class wont be changed.

Link: http://lkml.kernel.org/r/20140305232931.449eef8c@gandalf.local.home

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4600bca..3134756 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3198,6 +3198,7 @@ static void __setscheduler_params(struct task_struct *p,
 	 * getparam()/getattr() don't report silly values for !rt tasks.
 	 */
 	p->rt_priority = attr->sched_priority;
+	p->normal_prio = normal_prio(p);
 	set_load_weight(p);
 }
 
@@ -3207,6 +3208,12 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
 {
 	__setscheduler_params(p, attr);
 
+	/*
+	 * If we get here, there was no pi waiters boosting the
+	 * task. It is safe to use the normal prio.
+	 */
+	p->prio = normal_prio(p);
+
 	if (dl_prio(p->prio))
 		p->sched_class = &dl_sched_class;
 	else if (rt_prio(p->prio))
-- 
1.8.1.4

Re: [PATCH v2] sched: Fix broken setscheduler()

[Thomas Gleixner]

On Thu, 6 Mar 2014, Steven Rostedt wrote:

> On Thu, 6 Mar 2014 13:20:40 +0100
> Juri Lelli <juri.lelli@gmail.com> wrote:
> 
> > Yes. I think you can go with
> > 
> >  p->prio = p->normal_prio
> > 
> > and save a few checks in rt_mutex_getprio().
> > 
> 
> You're right! Thanks.
> 
> -----
> 
> I decided to run my tests on linux-next, and my wakeup_rt tracer was
> broken. After running a bisect, I found that the problem commit was:
> 
>    linux-next commit c365c292d059
>    "sched: Consider pi boosting in setscheduler()"
> 
> And the reason the wake_rt tracer test was failing, was because it had
> no RT task to trace. I first noticed this when running with
> sched_switch event and saw that my RT task still had normal SCHED_OTHER
> priority. Looking at the problem commit, I found:
> 
>  -       p->normal_prio = normal_prio(p);
>  -       p->prio = rt_mutex_getprio(p);
> 
> With no
> 
>  +       p->normal_prio = normal_prio(p);
>  +       p->prio = rt_mutex_getprio(p);
> 
> Reading what the commit is suppose to do, I realize that the p->prio
> can't be set if the task is boosted with a higher prio, but the
> p->normal_prio still needs to be set regardless, otherwise, when the
> task is deboosted, it wont get the new priority.

Indeed.
 
> The p->prio has to be set before "check_class_changed()" is called,
> otherwise the class wont be changed.
> 
> Link: http://lkml.kernel.org/r/20140305232931.449eef8c@gandalf.local.home
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/sched/core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 4600bca..3134756 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3198,6 +3198,7 @@ static void __setscheduler_params(struct task_struct *p,
>  	 * getparam()/getattr() don't report silly values for !rt tasks.
>  	 */
>  	p->rt_priority = attr->sched_priority;
> +	p->normal_prio = normal_prio(p);
>  	set_load_weight(p);
>  }
>  
> @@ -3207,6 +3208,12 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
>  {
>  	__setscheduler_params(p, attr);
>  
> +	/*
> +	 * If we get here, there was no pi waiters boosting the
> +	 * task. It is safe to use the normal prio.
> +	 */
> +	p->prio = normal_prio(p);
> +
>  	if (dl_prio(p->prio))
>  		p->sched_class = &dl_sched_class;
>  	else if (rt_prio(p->prio))
> -- 
> 1.8.1.4
> 
> 

Re: [PATCH v2] sched: Fix broken setscheduler()

[Steven Rostedt]

On Thu, 6 Mar 2014 21:43:50 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:
 
> > Reading what the commit is suppose to do, I realize that the p->prio
> > can't be set if the task is boosted with a higher prio, but the
> > p->normal_prio still needs to be set regardless, otherwise, when the
> > task is deboosted, it wont get the new priority.
> 
> Indeed.

Is this an Acked-by?

Peter, can you pull this patch please, it is still broken in tip/master.

Thanks,

-- Steve

Re: [PATCH v2] sched: Fix broken setscheduler()

[Thomas Gleixner]

On Mon, 10 Mar 2014, Steven Rostedt wrote:

> On Thu, 6 Mar 2014 21:43:50 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> wrote:
>  
> > > Reading what the commit is suppose to do, I realize that the p->prio
> > > can't be set if the task is boosted with a higher prio, but the
> > > p->normal_prio still needs to be set regardless, otherwise, when the
> > > task is deboosted, it wont get the new priority.
> > 
> > Indeed.
> 
> Is this an Acked-by?
> 
> Peter, can you pull this patch please, it is still broken in tip/master.

Lemme look at it tomorrow again with an awake brain. This seems to be
some forward porting hickup which needs a closer look. Just look at
the 3.10-rt version of this:

@@ -3825,20 +3826,25 @@ static struct task_struct *find_process_by_pid(pid_t pid)
 	return pid ? find_task_by_vpid(pid) : current;
 }
 
-/* Actually do priority change: must hold rq lock. */
-static void
-__setscheduler(struct rq *rq, struct task_struct *p, int policy, int prio)
+static void __setscheduler_params(struct task_struct *p, int policy, int prio)
 {
 	p->policy = policy;
 	p->rt_priority = prio;
 	p->normal_prio = normal_prio(p);
+	set_load_weight(p);
+}

That code has changed significantly probably due to the EDF merge. We
need to figure out whether there is more damage due to that.

Thanks,

	tglx

Re: [PATCH v2] sched: Fix broken setscheduler()

[Steven Rostedt]

On Mon, 10 Mar 2014 22:18:56 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:


> Lemme look at it tomorrow again with an awake brain. This seems to be
> some forward porting hickup which needs a closer look. Just look at

Yep, I talked with Sebastian on IRC and that seems to be the case.

> the 3.10-rt version of this:
> 
> @@ -3825,20 +3826,25 @@ static struct task_struct *find_process_by_pid(pid_t pid)
>  	return pid ? find_task_by_vpid(pid) : current;
>  }
>  
> -/* Actually do priority change: must hold rq lock. */
> -static void
> -__setscheduler(struct rq *rq, struct task_struct *p, int policy, int prio)
> +static void __setscheduler_params(struct task_struct *p, int policy, int prio)
>  {
>  	p->policy = policy;
>  	p->rt_priority = prio;
>  	p->normal_prio = normal_prio(p);
> +	set_load_weight(p);
> +}
> 
> That code has changed significantly probably due to the EDF merge. We
> need to figure out whether there is more damage due to that.

Yeah, when I looked at the -rt version, it appeared to have my fix
already. But in reality, the forward port broke it. Here's the problem
part of the commit:

+       set_load_weight(p);
+}
 
-       p->normal_prio = normal_prio(p);
-       p->prio = rt_mutex_getprio(p);

Your patch never deleted the above two. And it kept them in the
locations that I placed them in, in my patch.

-- Steve


+/* Actually do priority change: must hold pi & rq lock. */
+static void __setscheduler(struct rq *rq, struct task_struct *p,
+                          const struct sched_attr *attr)
+{

Re: [PATCH v2] sched: Fix broken setscheduler()

[Juri Lelli]

On Mon, 10 Mar 2014 17:37:31 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 10 Mar 2014 22:18:56 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> 
> > Lemme look at it tomorrow again with an awake brain. This seems to be
> > some forward porting hickup which needs a closer look. Just look at
> 
> Yep, I talked with Sebastian on IRC and that seems to be the case.
> 
> > the 3.10-rt version of this:
> > 
> > @@ -3825,20 +3826,25 @@ static struct task_struct *find_process_by_pid(pid_t pid)
> >  	return pid ? find_task_by_vpid(pid) : current;
> >  }
> >  
> > -/* Actually do priority change: must hold rq lock. */
> > -static void
> > -__setscheduler(struct rq *rq, struct task_struct *p, int policy, int prio)
> > +static void __setscheduler_params(struct task_struct *p, int policy, int prio)
> >  {
> >  	p->policy = policy;
> >  	p->rt_priority = prio;
> >  	p->normal_prio = normal_prio(p);
> > +	set_load_weight(p);
> > +}
> > 
> > That code has changed significantly probably due to the EDF merge. We
> > need to figure out whether there is more damage due to that.
> 
> Yeah, when I looked at the -rt version, it appeared to have my fix
> already. But in reality, the forward port broke it. Here's the problem
> part of the commit:
> 
> +       set_load_weight(p);
> +}
>  
> -       p->normal_prio = normal_prio(p);
> -       p->prio = rt_mutex_getprio(p);
> 
> Your patch never deleted the above two. And it kept them in the
> locations that I placed them in, in my patch.
> 

Oh, and you have to have also something like this

@@ -3271,7 +3271,8 @@ static int __sched_setscheduler(struct task_struct *p,
 				const struct sched_attr *attr,
 				bool user)
 {
-	int newprio = MAX_RT_PRIO - 1 - attr->sched_priority;
+	int newprio = dl_policy(attr->sched_policy) ? MAX_DL_PRIO - 1 :
+		      MAX_RT_PRIO - 1 - attr->sched_priority;
 	int retval, oldprio, oldpolicy = -1, on_rq, running;
 	int policy = attr->sched_policy;
 	unsigned long flags;

or you can fail to become DL if you are currently boosted by RT, as
attr->sched_priority == 0 for DL tasks. Then rt_mutex_check_prio ()
returns 1 (just update params) if setting DL params for a task already
boosted by another DL. But this seems to be ok, as we are already
outside enforcement in this situation. (This is going to be trickier
with proxy exec, though :/).

Thanks,

- Juri

> -- Steve
> 
> 
> +/* Actually do priority change: must hold pi & rq lock. */
> +static void __setscheduler(struct rq *rq, struct task_struct *p,
> +                          const struct sched_attr *attr)
> +{
> 
> 

Re: [PATCH v2] sched: Fix broken setscheduler()

[Thomas Gleixner]

On Tue, 11 Mar 2014, Juri Lelli wrote:
> On Mon, 10 Mar 2014 17:37:31 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Mon, 10 Mar 2014 22:18:56 +0100 (CET)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > 
> > > Lemme look at it tomorrow again with an awake brain. This seems to be
> > > some forward porting hickup which needs a closer look. Just look at
> > 
> > Yep, I talked with Sebastian on IRC and that seems to be the case.
> > 
> > > the 3.10-rt version of this:
> > > 
> > > @@ -3825,20 +3826,25 @@ static struct task_struct *find_process_by_pid(pid_t pid)
> > >  	return pid ? find_task_by_vpid(pid) : current;
> > >  }
> > >  
> > > -/* Actually do priority change: must hold rq lock. */
> > > -static void
> > > -__setscheduler(struct rq *rq, struct task_struct *p, int policy, int prio)
> > > +static void __setscheduler_params(struct task_struct *p, int policy, int prio)
> > >  {
> > >  	p->policy = policy;
> > >  	p->rt_priority = prio;
> > >  	p->normal_prio = normal_prio(p);
> > > +	set_load_weight(p);
> > > +}
> > > 
> > > That code has changed significantly probably due to the EDF merge. We
> > > need to figure out whether there is more damage due to that.
> > 
> > Yeah, when I looked at the -rt version, it appeared to have my fix
> > already. But in reality, the forward port broke it. Here's the problem
> > part of the commit:
> > 
> > +       set_load_weight(p);
> > +}
> >  
> > -       p->normal_prio = normal_prio(p);
> > -       p->prio = rt_mutex_getprio(p);
> > 
> > Your patch never deleted the above two. And it kept them in the
> > locations that I placed them in, in my patch.

Correct.
 
> 
> Oh, and you have to have also something like this
> 
> @@ -3271,7 +3271,8 @@ static int __sched_setscheduler(struct task_struct *p,
>  				const struct sched_attr *attr,
>  				bool user)
>  {
> -	int newprio = MAX_RT_PRIO - 1 - attr->sched_priority;
> +	int newprio = dl_policy(attr->sched_policy) ? MAX_DL_PRIO - 1 :
> +		      MAX_RT_PRIO - 1 - attr->sched_priority;

Right, you beat me.

>  	int retval, oldprio, oldpolicy = -1, on_rq, running;
>  	int policy = attr->sched_policy;
>  	unsigned long flags;
> 
> or you can fail to become DL if you are currently boosted by RT, as
> attr->sched_priority == 0 for DL tasks. Then rt_mutex_check_prio ()
> returns 1 (just update params) if setting DL params for a task already
> boosted by another DL. But this seems to be ok, as we are already
> outside enforcement in this situation. (This is going to be trickier
> with proxy exec, though :/).

Steve, can you please send an updated one?

Thanks,

	tglx

[PATCH v3] sched: Fix broken setscheduler()

[Steven Rostedt]

I decided to run my tests on linux-next, and my wakeup_rt tracer was
broken. After running a bisect, I found that the problem commit was:

   linux-next commit c365c292d059
   "sched: Consider pi boosting in setscheduler()"

And the reason the wake_rt tracer test was failing, was because it had
no RT task to trace. I first noticed this when running with
sched_switch event and saw that my RT task still had normal SCHED_OTHER
priority. Looking at the problem commit, I found:

 -       p->normal_prio = normal_prio(p);
 -       p->prio = rt_mutex_getprio(p);

With no

 +       p->normal_prio = normal_prio(p);
 +       p->prio = rt_mutex_getprio(p);

Reading what the commit is suppose to do, I realize that the p->prio
can't be set if the task is boosted with a higher prio, but the
p->normal_prio still needs to be set regardless, otherwise, when the
task is deboosted, it wont get the new priority.

The p->prio has to be set before "check_class_changed()" is called,
otherwise the class wont be changed.

Also added fix to newprio to include a check for deadline policy that
was missing. This change was suggested by Juri Lelli.

Link: http://lkml.kernel.org/r/20140306120438.638bfe94@gandalf.local.home
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ee8004c..f00a032 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3197,6 +3197,7 @@ static void __setscheduler_params(struct task_struct *p,
 	 * getparam()/getattr() don't report silly values for !rt tasks.
 	 */
 	p->rt_priority = attr->sched_priority;
+	p->normal_prio = normal_prio(p);
 	set_load_weight(p);
 }
 
@@ -3206,6 +3207,12 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
 {
 	__setscheduler_params(p, attr);
 
+	/*
+	 * If we get here, there was no pi waiters boosting the
+	 * task. It is safe to use the normal prio.
+	 */
+	p->prio = normal_prio(p);
+
 	if (dl_prio(p->prio))
 		p->sched_class = &dl_sched_class;
 	else if (rt_prio(p->prio))
@@ -3264,7 +3271,8 @@ static int __sched_setscheduler(struct task_struct *p,
 				const struct sched_attr *attr,
 				bool user)
 {
-	int newprio = MAX_RT_PRIO - 1 - attr->sched_priority;
+	int newprio = dl_policy(attr->sched_policy) ? MAX_DL_PRIO - 1 :
+		      MAX_RT_PRIO - 1 - attr->sched_priority;
 	int retval, oldprio, oldpolicy = -1, on_rq, running;
 	int policy = attr->sched_policy;
 	unsigned long flags;
-- 
1.8.1.4

[tip:sched/core] sched: Fix broken setscheduler()

[tip-bot for Steven Rostedt]

Commit-ID:  383afd0971538b3d77532a56404b24cfe967b5dd
Gitweb:     http://git.kernel.org/tip/383afd0971538b3d77532a56404b24cfe967b5dd
Author:     Steven Rostedt <rostedt@goodmis.org>
AuthorDate: Tue, 11 Mar 2014 19:24:20 -0400
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 12 Mar 2014 10:48:59 +0100

sched: Fix broken setscheduler()

I decided to run my tests on linux-next, and my wakeup_rt tracer was
broken. After running a bisect, I found that the problem commit was:

   linux-next commit c365c292d059
   "sched: Consider pi boosting in setscheduler()"

And the reason the wake_rt tracer test was failing, was because it had
no RT task to trace. I first noticed this when running with
sched_switch event and saw that my RT task still had normal SCHED_OTHER
priority. Looking at the problem commit, I found:

 -       p->normal_prio = normal_prio(p);
 -       p->prio = rt_mutex_getprio(p);

With no

 +       p->normal_prio = normal_prio(p);
 +       p->prio = rt_mutex_getprio(p);

Reading what the commit is suppose to do, I realize that the p->prio
can't be set if the task is boosted with a higher prio, but the
p->normal_prio still needs to be set regardless, otherwise, when the
task is deboosted, it wont get the new priority.

The p->prio has to be set before "check_class_changed()" is called,
otherwise the class wont be changed.

Also added fix to newprio to include a check for deadline policy that
was missing. This change was suggested by Juri Lelli.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Cc: SebastianAndrzej Siewior <bigeasy@linutronix.de>
Cc: Juri Lelli <juri.lelli@gmail.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20140306120438.638bfe94@gandalf.local.home
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9e126a2..ae365aa 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3195,6 +3195,7 @@ static void __setscheduler_params(struct task_struct *p,
 	 * getparam()/getattr() don't report silly values for !rt tasks.
 	 */
 	p->rt_priority = attr->sched_priority;
+	p->normal_prio = normal_prio(p);
 	set_load_weight(p);
 }
 
@@ -3204,6 +3205,12 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
 {
 	__setscheduler_params(p, attr);
 
+	/*
+	 * If we get here, there was no pi waiters boosting the
+	 * task. It is safe to use the normal prio.
+	 */
+	p->prio = normal_prio(p);
+
 	if (dl_prio(p->prio))
 		p->sched_class = &dl_sched_class;
 	else if (rt_prio(p->prio))
@@ -3262,7 +3269,8 @@ static int __sched_setscheduler(struct task_struct *p,
 				const struct sched_attr *attr,
 				bool user)
 {
-	int newprio = MAX_RT_PRIO - 1 - attr->sched_priority;
+	int newprio = dl_policy(attr->sched_policy) ? MAX_DL_PRIO - 1 :
+		      MAX_RT_PRIO - 1 - attr->sched_priority;
 	int retval, oldprio, oldpolicy = -1, on_rq, running;
 	int policy = attr->sched_policy;
 	unsigned long flags;

Re: [PATCH v3] sched: Fix broken setscheduler()

[Thomas Gleixner]

On Tue, 11 Mar 2014, Steven Rostedt wrote:

> 
> I decided to run my tests on linux-next, and my wakeup_rt tracer was
> broken. After running a bisect, I found that the problem commit was:
> 
>    linux-next commit c365c292d059
>    "sched: Consider pi boosting in setscheduler()"
> 
> And the reason the wake_rt tracer test was failing, was because it had
> no RT task to trace. I first noticed this when running with
> sched_switch event and saw that my RT task still had normal SCHED_OTHER
> priority. Looking at the problem commit, I found:
> 
>  -       p->normal_prio = normal_prio(p);
>  -       p->prio = rt_mutex_getprio(p);
> 
> With no
> 
>  +       p->normal_prio = normal_prio(p);
>  +       p->prio = rt_mutex_getprio(p);
> 
> Reading what the commit is suppose to do, I realize that the p->prio
> can't be set if the task is boosted with a higher prio, but the
> p->normal_prio still needs to be set regardless, otherwise, when the
> task is deboosted, it wont get the new priority.
> 
> The p->prio has to be set before "check_class_changed()" is called,
> otherwise the class wont be changed.
> 
> Also added fix to newprio to include a check for deadline policy that
> was missing. This change was suggested by Juri Lelli.
> 
> Link: http://lkml.kernel.org/r/20140306120438.638bfe94@gandalf.local.home
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>