[PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu

[PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

The dl task will be replenished after dl task timer fire and start a 
new period. It will be enqueued and to re-evaluate its dependency on 
the tick in order to restart it. However, if cpu is hot-unplug, 
irq_work_queue will splash since the target cpu is offline.

As a result:

    WARNING: CPU: 2 PID: 0 at kernel/irq_work.c:69 irq_work_queue_on+0xad/0xe0
    Call Trace:
     dump_stack+0x99/0xd0
     __warn+0xd1/0xf0
     warn_slowpath_null+0x1d/0x20
     irq_work_queue_on+0xad/0xe0
     tick_nohz_full_kick_cpu+0x44/0x50
     tick_nohz_dep_set_cpu+0x74/0xb0
     enqueue_task_dl+0x226/0x480
     activate_task+0x5c/0xa0
     dl_task_timer+0x19b/0x2c0
     ? push_dl_task.part.31+0x190/0x190
  
This can be triggered by hot-unplug the full dynticks cpu which dl 
task is running on. 

We enqueue the dl task on the offline CPU, because we need to do 
replenish for start_dl_timer(). So, as Juri pointed out, we would 
need to do is calling replenish_dl_entity() directly, instead of 
enqueue_task_dl(). pi_se shouldn't be a problem as the task shouldn't 
be boosted if it was throttled.

This patch fix it by just replenish dl entity to avoid the intention 
to re-evaluate tick dependency if the cpu is offline.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@arm.com>
Cc: Luca Abeni <luca.abeni@unitn.it>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v2 -> v3:
 * move rq->online check under CONFIG_SMP
v1 -> v2:
 * replenish dl entity

 kernel/sched/deadline.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index d091f4a..ce0fb00 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -641,6 +641,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 		goto unlock;
 	}
 
+#ifdef CONFIG_SMP
+	if (unlikely(!rq->online))
+		goto offline;
+#endif
+
 	enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
 	if (dl_task(rq->curr))
 		check_preempt_curr_dl(rq, p, 0);
@@ -648,6 +653,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 		resched_curr(rq);
 
 #ifdef CONFIG_SMP
+offline:
 	/*
 	 * Perform balancing operations here; after the replenishments.  We
 	 * cannot drop rq->lock before this, otherwise the assertion in
@@ -659,6 +665,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	 * XXX figure out if select_task_rq_dl() deals with offline cpus.
 	 */
 	if (unlikely(!rq->online)) {
+		replenish_dl_entity(dl_se, dl_se);
 		lockdep_unpin_lock(&rq->lock, rf.cookie);
 		rq = dl_task_offline_migration(rq, p);
 		rf.cookie = lockdep_pin_lock(&rq->lock);
-- 
1.9.1

Re: [PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu

[Wanpeng Li]

Ping Juri, Frederic, could I get your Acked?
2016-08-12 17:24 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> The dl task will be replenished after dl task timer fire and start a
> new period. It will be enqueued and to re-evaluate its dependency on
> the tick in order to restart it. However, if cpu is hot-unplug,
> irq_work_queue will splash since the target cpu is offline.
>
> As a result:
>
>     WARNING: CPU: 2 PID: 0 at kernel/irq_work.c:69 irq_work_queue_on+0xad/0xe0
>     Call Trace:
>      dump_stack+0x99/0xd0
>      __warn+0xd1/0xf0
>      warn_slowpath_null+0x1d/0x20
>      irq_work_queue_on+0xad/0xe0
>      tick_nohz_full_kick_cpu+0x44/0x50
>      tick_nohz_dep_set_cpu+0x74/0xb0
>      enqueue_task_dl+0x226/0x480
>      activate_task+0x5c/0xa0
>      dl_task_timer+0x19b/0x2c0
>      ? push_dl_task.part.31+0x190/0x190
>
> This can be triggered by hot-unplug the full dynticks cpu which dl
> task is running on.
>
> We enqueue the dl task on the offline CPU, because we need to do
> replenish for start_dl_timer(). So, as Juri pointed out, we would
> need to do is calling replenish_dl_entity() directly, instead of
> enqueue_task_dl(). pi_se shouldn't be a problem as the task shouldn't
> be boosted if it was throttled.
>
> This patch fix it by just replenish dl entity to avoid the intention
> to re-evaluate tick dependency if the cpu is offline.
>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Cc: Luca Abeni <luca.abeni@unitn.it>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v2 -> v3:
>  * move rq->online check under CONFIG_SMP
> v1 -> v2:
>  * replenish dl entity
>
>  kernel/sched/deadline.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index d091f4a..ce0fb00 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -641,6 +641,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>                 goto unlock;
>         }
>
> +#ifdef CONFIG_SMP
> +       if (unlikely(!rq->online))
> +               goto offline;
> +#endif
> +
>         enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
>         if (dl_task(rq->curr))
>                 check_preempt_curr_dl(rq, p, 0);
> @@ -648,6 +653,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>                 resched_curr(rq);
>
>  #ifdef CONFIG_SMP
> +offline:
>         /*
>          * Perform balancing operations here; after the replenishments.  We
>          * cannot drop rq->lock before this, otherwise the assertion in
> @@ -659,6 +665,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>          * XXX figure out if select_task_rq_dl() deals with offline cpus.
>          */
>         if (unlikely(!rq->online)) {
> +               replenish_dl_entity(dl_se, dl_se);
>                 lockdep_unpin_lock(&rq->lock, rf.cookie);
>                 rq = dl_task_offline_migration(rq, p);
>                 rf.cookie = lockdep_pin_lock(&rq->lock);
> --
> 1.9.1
>



-- 
Regards,
Wanpeng Li

Re: [PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu

[Peter Zijlstra]

On Fri, Aug 12, 2016 at 05:24:03PM +0800, Wanpeng Li wrote:

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index d091f4a..ce0fb00 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -641,6 +641,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>  		goto unlock;
>  	}
>  
> +#ifdef CONFIG_SMP
> +	if (unlikely(!rq->online))
> +		goto offline;
> +#endif
> +
>  	enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
>  	if (dl_task(rq->curr))
>  		check_preempt_curr_dl(rq, p, 0);
> @@ -648,6 +653,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>  		resched_curr(rq);
>  
>  #ifdef CONFIG_SMP
> +offline:
>  	/*
>  	 * Perform balancing operations here; after the replenishments.  We
>  	 * cannot drop rq->lock before this, otherwise the assertion in
> @@ -659,6 +665,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>  	 * XXX figure out if select_task_rq_dl() deals with offline cpus.
>  	 */
>  	if (unlikely(!rq->online)) {
> +		replenish_dl_entity(dl_se, dl_se);
>  		lockdep_unpin_lock(&rq->lock, rf.cookie);
>  		rq = dl_task_offline_migration(rq, p);

So I don't like this, even if it magically works. With this we end up
calling dl_task_offline_migration() -> deactivate_task() while the task
isn't on the runqueue at all.

>  		rf.cookie = lockdep_pin_lock(&rq->lock);
> -- 
> 1.9.1
> 

Re: [PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu

[Wanpeng Li]

2016-08-19 21:25 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Fri, Aug 12, 2016 at 05:24:03PM +0800, Wanpeng Li wrote:
>
>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> index d091f4a..ce0fb00 100644
>> --- a/kernel/sched/deadline.c
>> +++ b/kernel/sched/deadline.c
>> @@ -641,6 +641,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>>               goto unlock;
>>       }
>>
>> +#ifdef CONFIG_SMP
>> +     if (unlikely(!rq->online))
>> +             goto offline;
>> +#endif
>> +
>>       enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
>>       if (dl_task(rq->curr))
>>               check_preempt_curr_dl(rq, p, 0);
>> @@ -648,6 +653,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>>               resched_curr(rq);
>>
>>  #ifdef CONFIG_SMP
>> +offline:
>>       /*
>>        * Perform balancing operations here; after the replenishments.  We
>>        * cannot drop rq->lock before this, otherwise the assertion in
>> @@ -659,6 +665,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>>        * XXX figure out if select_task_rq_dl() deals with offline cpus.
>>        */
>>       if (unlikely(!rq->online)) {
>> +             replenish_dl_entity(dl_se, dl_se);
>>               lockdep_unpin_lock(&rq->lock, rf.cookie);
>>               rq = dl_task_offline_migration(rq, p);
>
> So I don't like this, even if it magically works. With this we end up
> calling dl_task_offline_migration() -> deactivate_task() while the task
> isn't on the runqueue at all.

So how about v1, it also works :), https://lkml.org/lkml/2016/8/10/898

Regards,
Wanpeng Li

Re: [PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu

[Peter Zijlstra]

On Fri, Aug 19, 2016 at 09:56:59PM +0800, Wanpeng Li wrote:
> 2016-08-19 21:25 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> > On Fri, Aug 12, 2016 at 05:24:03PM +0800, Wanpeng Li wrote:
> >
> >> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> >> index d091f4a..ce0fb00 100644
> >> --- a/kernel/sched/deadline.c
> >> +++ b/kernel/sched/deadline.c
> >> @@ -641,6 +641,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> >>               goto unlock;
> >>       }
> >>
> >> +#ifdef CONFIG_SMP
> >> +     if (unlikely(!rq->online))
> >> +             goto offline;
> >> +#endif
> >> +
> >>       enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
> >>       if (dl_task(rq->curr))
> >>               check_preempt_curr_dl(rq, p, 0);
> >> @@ -648,6 +653,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> >>               resched_curr(rq);
> >>
> >>  #ifdef CONFIG_SMP
> >> +offline:
> >>       /*
> >>        * Perform balancing operations here; after the replenishments.  We
> >>        * cannot drop rq->lock before this, otherwise the assertion in
> >> @@ -659,6 +665,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> >>        * XXX figure out if select_task_rq_dl() deals with offline cpus.
> >>        */
> >>       if (unlikely(!rq->online)) {
> >> +             replenish_dl_entity(dl_se, dl_se);
> >>               lockdep_unpin_lock(&rq->lock, rf.cookie);
> >>               rq = dl_task_offline_migration(rq, p);
> >
> > So I don't like this, even if it magically works. With this we end up
> > calling dl_task_offline_migration() -> deactivate_task() while the task
> > isn't on the runqueue at all.
> 
> So how about v1, it also works :), https://lkml.org/lkml/2016/8/10/898

Does something like so work? Notice what we have preemption disabled (by
virtue of having pi_lock and rq->lock taken) and thus cannot hotplug.
Therefore if we notice a rq not being online, it must stay that way,
equally any online rq must also stay that way.

This means we can fold the two online tests you had and simply do the rq
switch beforehand.

Completely untested...

---
 kernel/sched/deadline.c | 46 ++++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index d091f4a95416..bcade08772a8 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -243,10 +243,8 @@ static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq);
 static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p)
 {
 	struct rq *later_rq = NULL;
-	bool fallback = false;
 
 	later_rq = find_lock_later_rq(p, rq);
-
 	if (!later_rq) {
 		int cpu;
 
@@ -254,7 +252,6 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
 		 * If we cannot preempt any rq, fall back to pick any
 		 * online cpu.
 		 */
-		fallback = true;
 		cpu = cpumask_any_and(cpu_active_mask, tsk_cpus_allowed(p));
 		if (cpu >= nr_cpu_ids) {
 			/*
@@ -274,16 +271,7 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
 		double_lock_balance(rq, later_rq);
 	}
 
-	/*
-	 * By now the task is replenished and enqueued; migrate it.
-	 */
-	deactivate_task(rq, p, 0);
 	set_task_cpu(p, later_rq->cpu);
-	activate_task(later_rq, p, 0);
-
-	if (!fallback)
-		resched_curr(later_rq);
-
 	double_unlock_balance(later_rq, rq);
 
 	return later_rq;
@@ -641,29 +629,31 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 		goto unlock;
 	}
 
-	enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
-	if (dl_task(rq->curr))
-		check_preempt_curr_dl(rq, p, 0);
-	else
-		resched_curr(rq);
-
 #ifdef CONFIG_SMP
-	/*
-	 * Perform balancing operations here; after the replenishments.  We
-	 * cannot drop rq->lock before this, otherwise the assertion in
-	 * start_dl_timer() about not missing updates is not true.
-	 *
-	 * If we find that the rq the task was on is no longer available, we
-	 * need to select a new rq.
-	 *
-	 * XXX figure out if select_task_rq_dl() deals with offline cpus.
-	 */
 	if (unlikely(!rq->online)) {
+		/*
+		 * If the runqueue is no longer available, migrate the
+		 * task elsewhere. This necessarily changes rq.
+		 */
 		lockdep_unpin_lock(&rq->lock, rf.cookie);
 		rq = dl_task_offline_migration(rq, p);
 		rf.cookie = lockdep_pin_lock(&rq->lock);
+
+		/*
+		 * Now that the task has been migrated to the new RQ and we
+		 * have that locked, proceed as normal and enqueue the task
+		 * there.
+		 */
 	}
+#endif
 
+	enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
+	if (dl_task(rq->curr))
+		check_preempt_curr_dl(rq, p, 0);
+	else
+		resched_curr(rq);
+
+#ifdef CONFIG_SMP
 	/*
 	 * Queueing this task back might have overloaded rq, check if we need
 	 * to kick someone away.

Re: [PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu

[Wanpeng Li]

2016-08-31 17:21 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Fri, Aug 19, 2016 at 09:56:59PM +0800, Wanpeng Li wrote:
>> 2016-08-19 21:25 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>> > On Fri, Aug 12, 2016 at 05:24:03PM +0800, Wanpeng Li wrote:
>> >
>> >> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> >> index d091f4a..ce0fb00 100644
>> >> --- a/kernel/sched/deadline.c
>> >> +++ b/kernel/sched/deadline.c
>> >> @@ -641,6 +641,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>> >>               goto unlock;
>> >>       }
>> >>
>> >> +#ifdef CONFIG_SMP
>> >> +     if (unlikely(!rq->online))
>> >> +             goto offline;
>> >> +#endif
>> >> +
>> >>       enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
>> >>       if (dl_task(rq->curr))
>> >>               check_preempt_curr_dl(rq, p, 0);
>> >> @@ -648,6 +653,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>> >>               resched_curr(rq);
>> >>
>> >>  #ifdef CONFIG_SMP
>> >> +offline:
>> >>       /*
>> >>        * Perform balancing operations here; after the replenishments.  We
>> >>        * cannot drop rq->lock before this, otherwise the assertion in
>> >> @@ -659,6 +665,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>> >>        * XXX figure out if select_task_rq_dl() deals with offline cpus.
>> >>        */
>> >>       if (unlikely(!rq->online)) {
>> >> +             replenish_dl_entity(dl_se, dl_se);
>> >>               lockdep_unpin_lock(&rq->lock, rf.cookie);
>> >>               rq = dl_task_offline_migration(rq, p);
>> >
>> > So I don't like this, even if it magically works. With this we end up
>> > calling dl_task_offline_migration() -> deactivate_task() while the task
>> > isn't on the runqueue at all.
>>
>> So how about v1, it also works :), https://lkml.org/lkml/2016/8/10/898
>
> Does something like so work? Notice what we have preemption disabled (by
> virtue of having pi_lock and rq->lock taken) and thus cannot hotplug.
> Therefore if we notice a rq not being online, it must stay that way,
> equally any online rq must also stay that way.
>
> This means we can fold the two online tests you had and simply do the rq
> switch beforehand.

Thanks for the proposal Peterz! It works and I just sent out v4. :)

Regards,
Wanpeng Li

>
> Completely untested...
>
> ---
>  kernel/sched/deadline.c | 46 ++++++++++++++++++----------------------------
>  1 file changed, 18 insertions(+), 28 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index d091f4a95416..bcade08772a8 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -243,10 +243,8 @@ static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq);
>  static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p)
>  {
>         struct rq *later_rq = NULL;
> -       bool fallback = false;
>
>         later_rq = find_lock_later_rq(p, rq);
> -
>         if (!later_rq) {
>                 int cpu;
>
> @@ -254,7 +252,6 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
>                  * If we cannot preempt any rq, fall back to pick any
>                  * online cpu.
>                  */
> -               fallback = true;
>                 cpu = cpumask_any_and(cpu_active_mask, tsk_cpus_allowed(p));
>                 if (cpu >= nr_cpu_ids) {
>                         /*
> @@ -274,16 +271,7 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
>                 double_lock_balance(rq, later_rq);
>         }
>
> -       /*
> -        * By now the task is replenished and enqueued; migrate it.
> -        */
> -       deactivate_task(rq, p, 0);
>         set_task_cpu(p, later_rq->cpu);
> -       activate_task(later_rq, p, 0);
> -
> -       if (!fallback)
> -               resched_curr(later_rq);
> -
>         double_unlock_balance(later_rq, rq);
>
>         return later_rq;
> @@ -641,29 +629,31 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>                 goto unlock;
>         }
>
> -       enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
> -       if (dl_task(rq->curr))
> -               check_preempt_curr_dl(rq, p, 0);
> -       else
> -               resched_curr(rq);
> -
>  #ifdef CONFIG_SMP
> -       /*
> -        * Perform balancing operations here; after the replenishments.  We
> -        * cannot drop rq->lock before this, otherwise the assertion in
> -        * start_dl_timer() about not missing updates is not true.
> -        *
> -        * If we find that the rq the task was on is no longer available, we
> -        * need to select a new rq.
> -        *
> -        * XXX figure out if select_task_rq_dl() deals with offline cpus.
> -        */
>         if (unlikely(!rq->online)) {
> +               /*
> +                * If the runqueue is no longer available, migrate the
> +                * task elsewhere. This necessarily changes rq.
> +                */
>                 lockdep_unpin_lock(&rq->lock, rf.cookie);
>                 rq = dl_task_offline_migration(rq, p);
>                 rf.cookie = lockdep_pin_lock(&rq->lock);
> +
> +               /*
> +                * Now that the task has been migrated to the new RQ and we
> +                * have that locked, proceed as normal and enqueue the task
> +                * there.
> +                */
>         }
> +#endif
>
> +       enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
> +       if (dl_task(rq->curr))
> +               check_preempt_curr_dl(rq, p, 0);
> +       else
> +               resched_curr(rq);
> +
> +#ifdef CONFIG_SMP
>         /*
>          * Queueing this task back might have overloaded rq, check if we need
>          * to kick someone away.



-- 
Regards,
Wanpeng Li