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

[Juri Lelli]

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

> On Mon, 10 Mar 2014 22:18:56 +0100 (CET)
> Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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)
> +{
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/