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

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

[luca abeni]

From: Luca Abeni <luca.abeni@santannapisa.it>

When switching to -deadline, if the scheduling deadline of a task is
in the past then switched_to_dl() calls setup_new_entity() to properly
initialize the scheduling deadline and runtime.

The problem is that the task is enqueued _before_ having its parameters
initialized by setup_new_entity(), and this can cause problems.
For example, a task with its out-of-date deadline in the past will
potentially be enqueued as the highest priority one; however, its
adjusted deadline may not be the earliest one.

This patch fixes the problem by initializing the task's parameters before
enqueuing it.

Signed-off-by: Luca Abeni <luca.abeni@santannapisa.it>
Reviewed-by: Daniel Bristot de Oliveira <bristot@redhat.com>
---
 kernel/sched/deadline.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index a2ce590..ec53d24 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -950,6 +950,10 @@ enqueue_dl_entity(struct sched_dl_entity *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) &&
+		  dl_time_before(dl_se->deadline,
+				 rq_clock(rq_of_dl_rq(dl_rq_of_se(dl_se)))))
+		setup_new_dl_entity(dl_se);
 
 	__enqueue_dl_entity(dl_se);
 }
@@ -1767,14 +1771,6 @@ static void switched_to_dl(struct rq *rq, struct task_struct *p)
 	if (!task_on_rq_queued(p))
 		return;
 
-	/*
-	 * If p is boosted we already updated its params in
-	 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
-	 * p's deadline being now already after rq_clock(rq).
-	 */
-	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
-		setup_new_dl_entity(&p->dl);
-
 	if (rq->curr != p) {
 #ifdef CONFIG_SMP
 		if (p->nr_cpus_allowed > 1 && rq->dl.overloaded)
-- 
2.7.4

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

[Juri Lelli]

Hi Luca,

On 20/04/17 21:30, Luca Abeni wrote:
> From: Luca Abeni <luca.abeni@santannapisa.it>
> 
> When switching to -deadline, if the scheduling deadline of a task is
> in the past then switched_to_dl() calls setup_new_entity() to properly
> initialize the scheduling deadline and runtime.
> 
> The problem is that the task is enqueued _before_ having its parameters
> initialized by setup_new_entity(), and this can cause problems.
> For example, a task with its out-of-date deadline in the past will
> potentially be enqueued as the highest priority one; however, its
> adjusted deadline may not be the earliest one.
> 
> This patch fixes the problem by initializing the task's parameters before
> enqueuing it.
> 
> Signed-off-by: Luca Abeni <luca.abeni@santannapisa.it>
> Reviewed-by: Daniel Bristot de Oliveira <bristot@redhat.com>
> ---
>  kernel/sched/deadline.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index a2ce590..ec53d24 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -950,6 +950,10 @@ enqueue_dl_entity(struct sched_dl_entity *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?

Thanks,

- Juri

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

[luca abeni]

On Fri, 21 Apr 2017 10:39:26 +0100
Juri Lelli <juri.lelli@arm.com> wrote:

> Hi Luca,
> 
> On 20/04/17 21:30, Luca Abeni wrote:
> > From: Luca Abeni <luca.abeni@santannapisa.it>
> > 
> > When switching to -deadline, if the scheduling deadline of a task is
> > in the past then switched_to_dl() calls setup_new_entity() to
> > properly initialize the scheduling deadline and runtime.
> > 
> > The problem is that the task is enqueued _before_ having its
> > parameters initialized by setup_new_entity(), and this can cause
> > problems. For example, a task with its out-of-date deadline in the
> > past will potentially be enqueued as the highest priority one;
> > however, its adjusted deadline may not be the earliest one.
> > 
> > This patch fixes the problem by initializing the task's parameters
> > before enqueuing it.
> > 
> > Signed-off-by: Luca Abeni <luca.abeni@santannapisa.it>
> > Reviewed-by: Daniel Bristot de Oliveira <bristot@redhat.com>
> > ---
> >  kernel/sched/deadline.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index a2ce590..ec53d24 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -950,6 +950,10 @@ enqueue_dl_entity(struct sched_dl_entity
> > *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...


				Luca

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

[Juri Lelli]

On 21/04/17 11:42, Luca Abeni wrote:
> On Fri, 21 Apr 2017 10:39:26 +0100
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > Hi Luca,
> > 
> > On 20/04/17 21:30, Luca Abeni wrote:
> > > From: Luca Abeni <luca.abeni@santannapisa.it>
> > > 
> > > When switching to -deadline, if the scheduling deadline of a task is
> > > in the past then switched_to_dl() calls setup_new_entity() to
> > > properly initialize the scheduling deadline and runtime.
> > > 
> > > The problem is that the task is enqueued _before_ having its
> > > parameters initialized by setup_new_entity(), and this can cause
> > > problems. For example, a task with its out-of-date deadline in the
> > > past will potentially be enqueued as the highest priority one;
> > > however, its adjusted deadline may not be the earliest one.
> > > 
> > > This patch fixes the problem by initializing the task's parameters
> > > before enqueuing it.
> > > 
> > > Signed-off-by: Luca Abeni <luca.abeni@santannapisa.it>
> > > Reviewed-by: Daniel Bristot de Oliveira <bristot@redhat.com>
> > > ---
> > >  kernel/sched/deadline.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > index a2ce590..ec53d24 100644
> > > --- a/kernel/sched/deadline.c
> > > +++ b/kernel/sched/deadline.c
> > > @@ -950,6 +950,10 @@ enqueue_dl_entity(struct sched_dl_entity
> > > *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 ? :)

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

[luca abeni]

On Fri, 21 Apr 2017 11:42:40 +0200
luca abeni <luca.abeni@santannapisa.it> wrote:
[...]
> > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > index a2ce590..ec53d24 100644
> > > --- a/kernel/sched/deadline.c
> > > +++ b/kernel/sched/deadline.c
> > > @@ -950,6 +950,10 @@ enqueue_dl_entity(struct sched_dl_entity
> > > *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...

__sched_setscheduler() sets queue_flags to DEQUEUE_SAVE, which matches
ENQUEUE_RESTORE (see comments in sched/sched.h), so things should work
correctly, right?



				Luca

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

[luca abeni]

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 :)



			Luca

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

[Peter Zijlstra]

On Fri, Apr 21, 2017 at 10:47:29AM +0100, Juri Lelli wrote:

> Oh, I think it works "by coincidence", as ENQUEUE_RESTORE == DEQUEUE_SAVE
> == 0x02 ? :)

That's very much on purpose, also see:

#define DEQUEUE_SAVE            0x02 /* matches ENQUEUE_RESTORE */

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

[Peter Zijlstra]

On Fri, Apr 21, 2017 at 11:59:07AM +0200, luca abeni wrote:

> 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 :)

Exactly. Makes things simpler if they line up properly, because then you
can use the same flags for dequeue and enqueue.

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

[Juri Lelli]

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?

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

[Steven Rostedt]

On Fri, 21 Apr 2017 11:54:21 +0200
luca abeni <luca.abeni@santannapisa.it> wrote:

> On Fri, 21 Apr 2017 11:42:40 +0200
> luca abeni <luca.abeni@santannapisa.it> wrote:
> [...]
> > > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > > index a2ce590..ec53d24 100644
> > > > --- a/kernel/sched/deadline.c
> > > > +++ b/kernel/sched/deadline.c
> > > > @@ -950,6 +950,10 @@ enqueue_dl_entity(struct sched_dl_entity
> > > > *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...  
> 
> __sched_setscheduler() sets queue_flags to DEQUEUE_SAVE, which matches
> ENQUEUE_RESTORE (see comments in sched/sched.h), so things should work
> correctly, right?

I was tripping over this too, but missed the comments in sched/sched.h.

Probably want to stick a comment about this in here as well.

-- Steve

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

[luca abeni]

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?


				Luca

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

[Juri Lelli]

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)?

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

[Luca Abeni]

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

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

[Juri Lelli]

On 24/04/17 12:36, Luca Abeni wrote:
> 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:

[...]

> > > 
> > > 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?
> 

My concern was about do_set_cpus_allowed(), but then you mentioned
"because we are doing gEDF" and that made me think of what happens when
we do push/pull. :)

> 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.
> 

OK. I think it makes sense, and your patch should cure the problem.
Maybe add a comment to note this down.

> 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).
> 

Ok, but I was wondering about the (admittedly) corner case in which we
migrate (via push/pull) a task on a rq, the rq_clock of which is after
the task's deadline (because clocks on src_rq and dst_rq are not in
sync). Anyway, maybe it's so corner case that we don't really want to
deal with it right now? I guess bigger things to fix first. :)

> 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.
> 

Thanks for claryfing. As said, I just got distracted by what you
mentioned as examples between parenthesis.

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

[Luca Abeni]

Hi all,

I have this patch in a local tree, together with other fixes / cleanups
I plan to submit in the next weeks...

Since I see that the patch has not been applied, I wonder if some
changes are needed before submitting it another time.
If anyone has requests for changes in the patch, let me know.



			Thanks,
				Luca

On Thu, 20 Apr 2017 21:30:56 +0200
luca abeni <luca.abeni@santannapisa.it> wrote:

> From: Luca Abeni <luca.abeni@santannapisa.it>
> 
> When switching to -deadline, if the scheduling deadline of a task is
> in the past then switched_to_dl() calls setup_new_entity() to properly
> initialize the scheduling deadline and runtime.
> 
> The problem is that the task is enqueued _before_ having its parameters
> initialized by setup_new_entity(), and this can cause problems.
> For example, a task with its out-of-date deadline in the past will
> potentially be enqueued as the highest priority one; however, its
> adjusted deadline may not be the earliest one.
> 
> This patch fixes the problem by initializing the task's parameters before
> enqueuing it.
> 
> Signed-off-by: Luca Abeni <luca.abeni@santannapisa.it>
> Reviewed-by: Daniel Bristot de Oliveira <bristot@redhat.com>
> ---
>  kernel/sched/deadline.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index a2ce590..ec53d24 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -950,6 +950,10 @@ enqueue_dl_entity(struct sched_dl_entity *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) &&
> +		  dl_time_before(dl_se->deadline,
> +				 rq_clock(rq_of_dl_rq(dl_rq_of_se(dl_se)))))
> +		setup_new_dl_entity(dl_se);
>  
>  	__enqueue_dl_entity(dl_se);
>  }
> @@ -1767,14 +1771,6 @@ static void switched_to_dl(struct rq *rq, struct task_struct *p)
>  	if (!task_on_rq_queued(p))
>  		return;
>  
> -	/*
> -	 * If p is boosted we already updated its params in
> -	 * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
> -	 * p's deadline being now already after rq_clock(rq).
> -	 */
> -	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> -		setup_new_dl_entity(&p->dl);
> -
>  	if (rq->curr != p) {
>  #ifdef CONFIG_SMP
>  		if (p->nr_cpus_allowed > 1 && rq->dl.overloaded)