sched: Fix wakeup preemption regression

Since I want to make ->task_woken() conditional on the task getting migrated, we cannot use it to call record_wakee().

Quoted 2 lines · Peter Zijlstra

Since I want to make ->task_woken() conditional on the task getting migrated, we cannot use it to call record_wakee().

You mean ->task_waking(), right?

Mike reported that the recent commit 3a47d5124a95 ("sched/fair: Fix fairness issue on migration") broke interactivity and the signal starve test.

Quoted 3 lines · Peter Zijlstra

Mike reported that the recent commit 3a47d5124a95 ("sched/fair: Fix fairness issue on migration") broke interactivity and the signal starve test.

This looks pretty dangerous for v4.6 - how about simply reverting in 3a47d5124a95 and re-trying in v4.7?

Mike reported that the recent commit 3a47d5124a95 ("sched/fair: Fix fairness issue on migration") broke interactivity and the signal starve test.

The problem is that I assumed ENQUEUE_WAKING was only set when we do a cross-cpu wakeup (migration), which isn't true. This means we now destroy the vruntime history of tasks and wakeup-preemption suffers.

Cure this by making my assumption true, only call sched_class::task_waking() when we do a cross-cpu wakeup. This avoids the indirect call in the case we do a local wakeup.

Cc Pavan Kondeti <[email protected]>

Cc Ben Segall <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Morten Rasmussen <[email protected]>

Cc Paul Turner <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [email protected]

Cc Andrew Hunter <[email protected]>

Fixes 3a47d5124a95 ("sched/fair: Fix fairness issue on migration")

Reported-by Mike Galbraith <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/sched/core.c |   29 +++++++++++++++++++++--------
 kernel/sched/fair.c |   41 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 9 deletions(-)

Wakees that were not migrated/normalized eat an unwanted min_vruntime, and likely take a size XXL latency hit. Big box running master bled profusely under heavy load until I turned TTWU_QUEUE off.

Quoted 3 lines · Mike Galbraith

Wakees that were not migrated/normalized eat an unwanted min_vruntime, and likely take a size XXL latency hit. Big box running master bled profusely under heavy load until I turned TTWU_QUEUE off.

Quoted 5 lines · Mike Galbraith

Quoted 3 lines · Mike Galbraith

Wakees that were not migrated/normalized eat an unwanted min_vruntime, and likely take a size XXL latency hit. Big box running master bled profusely under heavy load until I turned TTWU_QUEUE off.

May as well make it official and against master.today. Fly or die little patchlet.

sched/fair: Move se->vruntime normalization state into struct sched_entity

Quoted 10 lines · Mike Galbraith

Quoted 5 lines · Mike Galbraith

Quoted 3 lines · Mike Galbraith

Wakees that were not migrated/normalized eat an unwanted min_vruntime, and likely take a size XXL latency hit. Big box running master bled profusely under heavy load until I turned TTWU_QUEUE off.

May as well make it official and against master.today. Fly or die little patchlet.

Quoted 1 line

sched/fair: Move se->vruntime normalization state into struct sched_entity

Yeah, I used to have a patch like this; but for debugging. I don't particularly like carrying this information other than for verification because it means we either do too much or too little normalization.

I'll try and have a look on Monday, but I got some real-life things to sort out first..

Quoted 2 lines

sched/fair: Move se->vruntime normalization state into struct sched_entity

Yeah, I used to have a patch like this; but for debugging. I don't particularly like carrying this information other than for verification because it means we either do too much or too little normalization.

I'll try and have a look on Monday, but I got some real-life things to sort out first..

Quoted 5 lines · Mike Galbraith

Quoted 3 lines · Mike Galbraith

Wakees that were not migrated/normalized eat an unwanted min_vruntime, and likely take a size XXL latency hit. Big box running master bled profusely under heavy load until I turned TTWU_QUEUE off.

May as well make it official and against master.today. Fly or die little patchlet.

sched/fair: Move se->vruntime normalization state into struct sched_entity

Quoted 7 lines · Mike Galbraith

Quoted 5 lines · Mike Galbraith

Quoted 3 lines · Mike Galbraith

Wakees that were not migrated/normalized eat an unwanted min_vruntime, and likely take a size XXL latency hit. Big box running master bled profusely under heavy load until I turned TTWU_QUEUE off.

Quoted 4 lines

May as well make it official and against master.today. Fly or die little patchlet.

sched/fair: Move se->vruntime normalization state into struct sched_entity

Does this work?

---
 include/linux/sched.h |  1 +
 kernel/sched/core.c   | 18 +++++++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

include/linux/sched.h +1 -0

index 1b43b45a22b9..a2001e01b3df 100644
@@ -1534,6 +1534,7 @@ struct task_struct {
 	unsigned sched_reset_on_fork:1;
 	unsigned sched_contributes_to_load:1;
 	unsigned sched_migrated:1;
+	unsigned sched_remote_wakeup:1;
 	unsigned :0; /* force alignment to the next boundary */
 
 	/* unserialized, strictly 'current' */

kernel/sched/core.c +2 -2

index 404c0784b1fc..7f2cae4620c7 100644
@@ -1768,13 +1768,15 @@ void sched_ttwu_pending(void)
 	cookie = lockdep_pin_lock(&rq->lock);
 
 	while (llist) {
+		int wake_flags = 0;
+
 		p = llist_entry(llist, struct task_struct,
 wake_entry);
 		llist = llist_next(llist);
-		/*
-		 * See ttwu_queue(); we only call

ttwu_queue_remote() when

kernel/sched/core.c +8 -4

-		 * its a x-cpu wakeup.
-		 */
-		ttwu_do_activate(rq, p, WF_MIGRATED, cookie);
+
+		if (p->sched_remote_wakeup)
+			wake_flags = WF_MIGRATED;
+
+		ttwu_do_activate(rq, p, wake_flags, cookie);
 	}
 
 	lockdep_unpin_lock(&rq->lock, cookie);

@@ -1819,10 +1821,12 @@ void scheduler_ipi(void)
 	irq_exit();
 }
 
-static void ttwu_queue_remote(struct task_struct *p, int cpu)
+static void ttwu_queue_remote(struct task_struct *p, int cpu, int
 wake_flags)
 {
 	struct rq *rq = cpu_rq(cpu);
 
+	p->sched_remote_wakeup = !!(wake_flags & WF_MIGRATED);
+
 	if (llist_add(&p->wake_entry, &cpu_rq(cpu)->wake_list)) {
 		if (!set_nr_if_polling(rq->idle))

kernel/sched/core.c +1 -1

 			smp_send_reschedule(cpu);

@@ -1869,7 +1873,7 @@ static void ttwu_queue(struct task_struct *p,
 int cpu, int wake_flags)
 #if defined(CONFIG_SMP)
 	if (sched_feat(TTWU_QUEUE) &&
 !cpus_share_cache(smp_processor_id(), cpu)) {
 		sched_clock_cpu(cpu); /* sync clocks x-cpu */
-		ttwu_queue_remote(p, cpu);
+		ttwu_queue_remote(p, cpu, wake_flags);
 		return;

}
#endif

Quoted 11 lines · Mike Galbraith

Quoted 10 lines · Mike Galbraith

Quoted 5 lines · Mike Galbraith

Quoted 4 lines

Wakees that were not migrated/normalized eat an unwanted min_vruntime, and likely take a size XXL latency hit. Big box running master bled profusely under heavy load until I turned TTWU_QUEUE off.

May as well make it official and against master.today. Fly or die little patchlet.

Quoted 3 lines

Quoted 1 line

sched/fair: Move se->vruntime normalization state into struct sched_entity

Does this work?

Yup, bugs--. Kinda funny, I considered ~this way first, but thought you'd not that approach.. dang, got it back-assward ;-)

Quoted 70 lines

---
 include/linux/sched.h |  1 +
 kernel/sched/core.c   | 18 +++++++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

include/linux/sched.h +1 -0

index 1b43b45a22b9..a2001e01b3df 100644
@@ -1534,6 +1534,7 @@ struct task_struct {
      unsigned sched_reset_on_fork:1;
      unsigned sched_contributes_to_load:1;
      unsigned sched_migrated:1;
+     unsigned sched_remote_wakeup:1;
      unsigned :0; /* force alignment to the next boundary */
 
      /* unserialized, strictly 'current' */

kernel/sched/core.c +2 -2

index 404c0784b1fc..7f2cae4620c7 100644
@@ -1768,13 +1768,15 @@ void sched_ttwu_pending(void)
      cookie = lockdep_pin_lock(&rq->lock);
 
      while (llist) {
+             int wake_flags = 0;
+
              p = llist_entry(llist, struct task_struct,
 wake_entry);
              llist = llist_next(llist);
-             /*
-              * See ttwu_queue(); we only call

ttwu_queue_remote() when

kernel/sched/core.c +8 -4

-              * its a x-cpu wakeup.
-              */
-             ttwu_do_activate(rq, p, WF_MIGRATED, cookie);
+
+             if (p->sched_remote_wakeup)
+                     wake_flags = WF_MIGRATED;
+
+             ttwu_do_activate(rq, p, wake_flags, cookie);
      }
 
      lockdep_unpin_lock(&rq->lock, cookie);

@@ -1819,10 +1821,12 @@ void scheduler_ipi(void)
      irq_exit();
 }
 
-static void ttwu_queue_remote(struct task_struct *p, int cpu)
+static void ttwu_queue_remote(struct task_struct *p, int cpu, int
 wake_flags)
 {
      struct rq *rq = cpu_rq(cpu);
 
+     p->sched_remote_wakeup = !!(wake_flags & WF_MIGRATED);
+
      if (llist_add(&p->wake_entry, &cpu_rq(cpu)->wake_list)) {
              if (!set_nr_if_polling(rq->idle))

                      smp_send_reschedule(cpu);

kernel/sched/core.c +1 -1

@@ -1869,7 +1873,7 @@ static void ttwu_queue(struct task_struct *p,
 int cpu, int wake_flags)
 #if defined(CONFIG_SMP)
      if (sched_feat(TTWU_QUEUE) &&
 !cpus_share_cache(smp_processor_id(), cpu)) {
              sched_clock_cpu(cpu); /* sync clocks x-cpu */
-             ttwu_queue_remote(p, cpu);
+             ttwu_queue_remote(p, cpu, wake_flags);
              return;

      }

#endif

2016-05-23 17:40 GMT+08:00 Mike Galbraith <[email protected]>:

Quoted 18 lines · Peter Zijlstra

Quoted 17 lines · Mike Galbraith

Quoted 15 lines · Mike Galbraith

Quoted 7 lines · Mike Galbraith

Quoted 6 lines

Wakees that were not migrated/normalized eat an unwanted min_vruntime,
and likely take a size XXL latency hit. Big box running master bled
profusely under heavy load until I turned TTWU_QUEUE off.

May as well make it official and against master.today. Fly or die
little patchlet.

sched/fair: Move se->vruntime normalization state into struct sched_entity

Quoted 6 lines

Quoted 1 line

Does this work?

Yup, bugs--. Kinda funny, I considered ~this way first, but thought
you'd not that approach.. dang, got it back-assward ;-)

Nicer this one.

Yup, bugs--. Kinda funny, I considered ~this way first, but thought you'd not that approach.. dang, got it back-assward ;-)

(Evolution authors must either not do patch review, or use some other mailer. Squint hard, this crud really is your patch;)

Quoted 3 lines

kernel/sched/core.c +0 -0

kernel/sched/core.c void sched_ttwu_pending(void) · line 1777

Quoted 10 lines

kernel/sched/core.c +5 -1

@@ -1762,7 +1770,11 @@ void sched_ttwu_pending(void)
 >    > while (llist) {
 >    >       > p = llist_entry(llist, struct task_struct, wake_entry);
 >    >       > llist = llist_next(llist);
->    >       > ttwu_do_activate(rq, p, 0, cookie);
+>    >       > /*
+>    >       >  * See ttwu_queue(); we only call ttwu_queue_remote() when
+>    >       >  * its a x-cpu wakeup.
+>    >       >  */
+>    >       > ttwu_do_activate(rq, p, WF_MIGRATED, cookie);

Wakees that were not migrated/normalized eat an unwanted min_vruntime,

2016-05-21 22:04 GMT+08:00 Mike Galbraith <[email protected]>:

Quoted 8 lines · Peter Zijlstra

(Evolution authors must either not do patch review, or use some other mailer. Squint hard, this crud really is your patch;)

Quoted 3 lines

kernel/sched/core.c +0 -0

Quoted 12 lines

kernel/sched/core.c void sched_ttwu_pending(void) · line 1777

Quoted 10 lines

kernel/sched/core.c +5 -1

@@ -1762,7 +1770,11 @@ void sched_ttwu_pending(void)
 >    > while (llist) {
 >    >       > p = llist_entry(llist, struct task_struct, wake_entry);
 >    >       > llist = llist_next(llist);
->    >       > ttwu_do_activate(rq, p, 0, cookie);
+>    >       > /*
+>    >       >  * See ttwu_queue(); we only call ttwu_queue_remote() when
+>    >       >  * its a x-cpu wakeup.
+>    >       >  */
+>    >       > ttwu_do_activate(rq, p, WF_MIGRATED, cookie);

Wakees that were not migrated/normalized eat an unwanted min_vruntime,

Why there were wakees queued by twu_queue_remote() not migrated?

Quoted 22 lines

2016-05-21 22:04 GMT+08:00 Mike Galbraith <[email protected]>:

Quoted 20 lines · Peter Zijlstra

(Evolution authors must either not do patch review, or use some other mailer. Squint hard, this crud really is your patch;)

kernel/sched/core.c void sched_ttwu_pending(void) · line 1777

Quoted 13 lines

kernel/sched/core.c +5 -1

@@ -1762,7 +1770,11 @@ void sched_ttwu_pending(void)
 >    > while (llist) {
 >    >       > p = llist_entry(llist, struct task_struct, wake_entry);
 >    >       > llist = llist_next(llist);
->    >       > ttwu_do_activate(rq, p, 0, cookie);
+>    >       > /*
+>    >       >  * See ttwu_queue(); we only call ttwu_queue_remote() when
+>    >       >  * its a x-cpu wakeup.
+>    >       >  */
+>    >       > ttwu_do_activate(rq, p, WF_MIGRATED, cookie);

Wakees that were not migrated/normalized eat an unwanted min_vruntime,

Quoted 1 line

Why there were wakees queued by twu_queue_remote() not migrated?

Queuing to a remote cache domain implies x-cpu wakeup, but does not imply migration.

What's the meaning of 'x-cpu wakeup'? ;-)

Quoted 2 lines

What's the meaning of 'x-cpu wakeup'? ;-)

Generally, cross CPU, as in waker/wakee reside on different CPUs, but in this case, it's cross socket wakeup.

2016-05-22 15:32 GMT+08:00 Mike Galbraith <[email protected]>:

Quoted 7 lines · Wanpeng Li

Quoted 2 lines

What's the meaning of 'x-cpu wakeup'? ;-)

Generally, cross CPU, as in waker/wakee reside on different CPUs, but
in this case, it's cross socket wakeup.

Do you mean wakeup wakees on remote socket don't imply migration/normalized, why?

Quoted 11 lines

2016-05-22 15:32 GMT+08:00 Mike Galbraith <[email protected]>:

Quoted 7 lines · Wanpeng Li

Quoted 2 lines

What's the meaning of 'x-cpu wakeup'? ;-)

Generally, cross CPU, as in waker/wakee reside on different CPUs, but
in this case, it's cross socket wakeup.

Do you mean wakeup wakees on remote socket don't imply migration/normalized, why?

Because the wakee is NOT necessarily migrated simply because it lives in some remote cache domain. It was a simple but nasty booboo.

ttwu():

        cpu = select_task_rq(p, p->wake_cpu, SD_BALANCE_WAKE, wake_flags);
        if (task_cpu(p) != cpu) {
                wake_flags |= WF_MIGRATED;
                set_task_cpu(p, cpu);

set_task_cpu():

                if (task_cpu(p) != new_cpu) {
                if (p->sched_class->migrate_task_rq)
                        p->sched_class->migrate_task_rq(p);
                                        ^^^^^^^^^^^^^^^^^^

migrate_task_rq_fair() normalizes wakee, those wakees that did not migrate have NOT been normalized, leaving two flavors of wakee on the wake_list, with no discriminator. Store class information internally, and the x-socket information disconnect evaporates.

Do all wakees live on some remote cache domain will not migrate or just the ones which can't find an suitable cpu in current local socket(failed in select_task_rq())?

Quoted 4 lines

Do all wakees live on some remote cache domain will not migrate or just the ones which can't find an suitable cpu in current local socket(failed in select_task_rq())?

?? The picture I drew already answered that.

Mike, Pavan, could you guys please confirm?

A recent commit caused an interactivity/starvation issue because we wrecked rq local wakeup preemption.

These patches rectify this while also (hopefully) keeping the problem that led to the fault patch fixed.

Mike, Pavan, could you guys please confirm?

A recent commit caused an interactivity/starvation issue because we wrecked rq local wakeup preemption.

These patches rectify this while also (hopefully) keeping the problem that led to the fault patch fixed.

Mike, Pavan, could you guys please confirm?