Re: [PATCH] sched/deadline: fix switching to -deadline

[Luca Abeni <luca.abeni@santannapisa.it>]

On Mon, 24 Apr 2017 11:16:24 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> On 21/04/17 21:08, Luca Abeni wrote:
> > On Fri, 21 Apr 2017 11:26:59 +0100
> > Juri Lelli <juri.lelli@arm.com> wrote:  
> > > On 21/04/17 11:59, Luca Abeni wrote:  
> > > > On Fri, 21 Apr 2017 10:47:29 +0100
> > > > Juri Lelli <juri.lelli@arm.com> wrote:
> > > > [...]    
> > > > > > > > *dl_se, update_dl_entity(dl_se, pi_se);
> > > > > > > >  	else if (flags & ENQUEUE_REPLENISH)
> > > > > > > >  		replenish_dl_entity(dl_se, pi_se);
> > > > > > > > +	else if ((flags & ENQUEUE_RESTORE) &&        
> > > > > > > 
> > > > > > > Not sure I understand how this works. AFAICT we are doing
> > > > > > > __sched_setscheduler() when we want to catch the case of
> > > > > > > a new dl_entity (SCHED_{OTHER,FIFO} -> SCHED_DEADLINE},
> > > > > > > but queue_flags (which are passed to enqueue_task())
> > > > > > > don't seem to have ENQUEUE_RESTORE set?      
> > > > > > 
> > > > > > I was under the impression sched_setscheduler() sets
> > > > > > ENQUEUE_RESTORE...     
> > > > > 
> > > > > Oh, I think it works "by coincidence", as ENQUEUE_RESTORE ==
> > > > > DEQUEUE_SAVE == 0x02 ? :)    
> > > > 
> > > > Not sure if this is a conincidence... By looking at the
> > > > comments in sched/sched.h I got the impression the two values
> > > > match by design (and __sched_setscheduler() is using this
> > > > property to simplify the code :)   
> > > 
> > > Yep, right.
> > > 
> > > Do you think we might get into trouble with do_set_cpus_allowed()?
> > > Can it happen that we change a task affinity while its deadline
> > > is in the past?  
> > 
> > Well, double thinking about it, this is an interesting problem...
> > What do we want to do with do_set_cpus_allowed()? (I mean: what is
> > the expected behaviour?)
> > 
> > With this patch, if a task is moved to a different runqueue when its
> > deadline is in the past (because we are doing gEDF, or because of
> > timer granularity issues) its scheduling deadline is reinitialized
> > to current time + relative deadline... I think this makes perfect
> > sense, doesn't it?
> >   
> 
> Mmm, I don't think we will (with this patch) actually reinitialize the
> deadline when a "normal" gEDF migration happen (push/pull), as
> (de)activate_task() have no flag set. Which brings the question,
> should we actually take care of this corner case (as what you say
> makes sense to me too)?

I might be misunderstanding the problem, here... Are you talking about
do_set_cpus_allowed()? Or about push/pull migrations happening because
of the gEDF algorithm?

If you are referring to do_set_cpus_allowed, this is my understanding:
1) If do_set_cpus_allowed() is called on a queued task, then
   dequeue_task() with DEQUEUE_SAVE is called, followed by
   enqueue_task() with ENQUEUE_RESTORE... So, if the deadline is in the
   past it is correctly reinitialized
2) If do_set_cpus_allowed() is called on a non-queued task, this means
   the task is blocked, no? So, when it will wake up enqueue_dl_entity()
   will invoke update_dl_entity() that will check if the deadline is in
   the past.

If you are referring to push/pull migrations due to gEDF, then
enqueue_dl_entity() will be invoked with "flags" = 0, so the deadline
will not be changed (and this is correct: we do not want to
initialize / change tasks' deadlines during gEDF migrations).

In my previous email, with "a task is moved to a different runqueue" I
wanted to say that the taks is forced to moved to a different runqueue
because its affinity is changed; I did not want to talk about "regular
migrations" due to the push/pull (gEDF) mechanism.


				Luca