cpu stopper threads and load balancing leads to deadlock

Hi guys,

We've seen a bug in one of our SLE kernels where the cpu stopper thread ("migration/15") is entering idle balance. This then triggers active load balance.

At the same time, a task on another CPU triggers a page fault and NUMA balancing kicks in to try and migrate the task closer to the NUMA node for that page (we're inside stop_two_cpus()). This faulting task is spinning in try_to_wake_up() (inside smp_cond_load_acquire(&p->on_cpu, !VAL)), waiting for "migration/15" to context switch.

Unfortunately, because "migration/15" is doing active load balance it's spinning waiting for the NUMA-page-faulting CPU's stopper lock, which is already held (since it's inside stop_two_cpus()).

Deadlock ensues.

This seems like a situation that should be prohibited, but I cannot find any code to prevent it. Is it OK for stopper threads to load balance? Is there something that should prevent this situation from happening?

Quoted 18 lines · Matt Fleming

Hi guys,

We've seen a bug in one of our SLE kernels where the cpu stopper thread ("migration/15") is entering idle balance. This then triggers active load balance.

At the same time, a task on another CPU triggers a page fault and NUMA balancing kicks in to try and migrate the task closer to the NUMA node for that page (we're inside stop_two_cpus()). This faulting task is spinning in try_to_wake_up() (inside smp_cond_load_acquire(&p->on_cpu, !VAL)), waiting for "migration/15" to context switch.

Unfortunately, because "migration/15" is doing active load balance it's spinning waiting for the NUMA-page-faulting CPU's stopper lock, which is already held (since it's inside stop_two_cpus()).

Deadlock ensues.

Quoted 4 lines

This seems like a situation that should be prohibited, but I cannot find any code to prevent it. Is it OK for stopper threads to load balance? Is there something that should prevent this situation from happening?

I don't see anything to stop the deadlock either, would exclude stop class from playing idle balancer entirely...

Hi guys,

We've seen a bug in one of our SLE kernels where the cpu stopper thread ("migration/15") is entering idle balance. This then triggers active load balance.

At the same time, a task on another CPU triggers a page fault and NUMA balancing kicks in to try and migrate the task closer to the NUMA node for that page (we're inside stop_two_cpus()). This faulting task is spinning in try_to_wake_up() (inside smp_cond_load_acquire(&p->on_cpu, !VAL)), waiting for "migration/15" to context switch.

Unfortunately, because "migration/15" is doing active load balance it's spinning waiting for the NUMA-page-faulting CPU's stopper lock, which is already held (since it's inside stop_two_cpus()).

Deadlock ensues.

Quoted 12 lines · Matt Fleming

Hi guys,

We've seen a bug in one of our SLE kernels where the cpu stopper thread ("migration/15") is entering idle balance. This then triggers active load balance.

At the same time, a task on another CPU triggers a page fault and NUMA balancing kicks in to try and migrate the task closer to the NUMA node for that page (we're inside stop_two_cpus()). This faulting task is spinning in try_to_wake_up() (inside smp_cond_load_acquire(&p->on_cpu, !VAL)), waiting for "migration/15" to context switch.

Quoted 5 lines

Unfortunately, because "migration/15" is doing active load balance it's spinning waiting for the NUMA-page-faulting CPU's stopper lock, which is already held (since it's inside stop_two_cpus()).

Deadlock ensues.

So if I read that right, something like the following happens:

CPU0 CPU1

schedule(.prev=migrate/0) <fault>
pick_next_task ...

    idle_balance			    migrate_swap()
      active_balance			      stop_two_cpus()
						spin_lock(stopper0->lock)
						spin_lock(stopper1->lock)
						ttwu(migrate/0)
						  smp_cond_load_acquire() -- waits for schedule()
        stop_one_cpu(1)
	  spin_lock(stopper1->lock) -- waits for stopper lock

Fix _this_ deadlock by taking out the wakeups from under stopper->lock. I'm not entirely sure there isn't more dragons here, but this particular one seems fixable by doing that.

Is there any way you can reproduce/test this?

Quoted 26 lines · Peter Zijlstra

Quoted 12 lines · Matt Fleming

Hi guys,

We've seen a bug in one of our SLE kernels where the cpu stopper thread ("migration/15") is entering idle balance. This then triggers active load balance.

At the same time, a task on another CPU triggers a page fault and NUMA balancing kicks in to try and migrate the task closer to the NUMA node for that page (we're inside stop_two_cpus()). This faulting task is spinning in try_to_wake_up() (inside smp_cond_load_acquire(&p->on_cpu, !VAL)), waiting for "migration/15" to context switch.

Quoted 5 lines

Unfortunately, because "migration/15" is doing active load balance it's spinning waiting for the NUMA-page-faulting CPU's stopper lock, which is already held (since it's inside stop_two_cpus()).

Deadlock ensues.

So if I read that right, something like the following happens:

CPU0 CPU1

schedule(.prev=migrate/0) <fault>
pick_next_task ...

Quoted 8 lines

    idle_balance			    migrate_swap()
      active_balance			      stop_two_cpus()
						spin_lock(stopper0->lock)
						spin_lock(stopper1->lock)
						ttwu(migrate/0)
						  smp_cond_load_acquire() -- waits for schedule()
        stop_one_cpu(1)
	  spin_lock(stopper1->lock) -- waits for stopper lock

Yep, that's exactly right.

Quoted 4 lines

Fix _this_ deadlock by taking out the wakeups from under stopper->lock. I'm not entirely sure there isn't more dragons here, but this particular one seems fixable by doing that.

Quoted 1 line

Is there any way you can reproduce/test this?

I'm afraid I don't have any way to test this, but I can ask the customer that reported it if they can.

Either way, this fix looks good to me.

[  124.216939] =============================
[  124.216939] WARNING: suspicious RCU usage
[  124.216941] 4.17.0.g66d489e-tip-default #82 Tainted: G            E    
[  124.216941] -----------------------------
[  124.216943] kernel/sched/core.c:1614 suspicious rcu_dereference_check() usage!
[  124.216944] 
               other info that might help us debug this:

[  124.216945] 
               RCU used illegally from offline CPU!
               rcu_scheduler_active = 2, debug_locks = 0
[  124.216946] 4 locks held by swapper/2/0:
[  124.216947]  #0: 000000001f9fa447 (stop_cpus_mutex){+.+.}, at: stop_machine_from_inactive_cpu+0x86/0x130
[  124.216953]  #1: 000000004cb07b3b (&stopper->lock){..-.}, at: cpu_stop_queue_work+0x2d/0x80
[  124.216958]  #2: 00000000d3a46b90 (&p->pi_lock){-.-.}, at: try_to_wake_up+0x2d/0x5f0
[  124.216964]  #3: 00000000f360767b (rcu_read_lock){....}, at: rcu_read_lock+0x0/0x80
[  124.216969] 
               stack backtrace:
[  124.216971] CPU: 2 PID: 0 Comm: swapper/2 Kdump: loaded Tainted: G            E     4.17.0.g66d489e-tip-default #82
[  124.216972] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013
[  124.216973] Call Trace:
[  124.216977]  dump_stack+0x78/0xb3
[  124.216979]  ttwu_stat+0x121/0x130
[  124.216983]  try_to_wake_up+0x2c2/0x5f0
[  124.216988]  ? cpu_stop_park+0x30/0x30
[  124.216990]  cpu_stop_queue_work+0x7c/0x80

[  124.216993]  queue_stop_cpus_work+0x61/0xb0
[  124.216997]  stop_machine_from_inactive_cpu+0xd3/0x130
[  124.216999]  ? mtrr_restore+0x80/0x80
[  124.217005]  mtrr_ap_init+0x62/0x70
[  124.217008]  identify_secondary_cpu+0x18/0x80
[  124.217011]  smp_store_cpu_info+0x44/0x50
[  124.217014]  start_secondary+0x9a/0x1e0
[  124.217017]  secondary_startup_64+0xa5/0xb0

Hurm.. I don't see how this is 'new'. We moved the wakeup out from under stopper lock, but that should not affect the RCU state.

Quoted 3 lines · Peter Zijlstra

Hurm.. I don't see how this is 'new'. We moved the wakeup out from under stopper lock, but that should not affect the RCU state.

No, not new, just an additional woes from same spot.

Quoted 6 lines · Mike Galbraith

Quoted 3 lines · Peter Zijlstra

Hurm.. I don't see how this is 'new'. We moved the wakeup out from under stopper lock, but that should not affect the RCU state.

No, not new, just an additional woes from same spot.

Ah, ok. Does somsething like this make it go away?

kernel/stop_machine.c +4 -2

index f89014a2c238..a32518c2ba4a 100644
@@ -650,8 +650,10 @@ int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 	/* Schedule work on other CPUs and execute directly for local CPU */
 	set_state(&msdata, MULTI_STOP_PREPARE);
 	cpu_stop_init_done(&done, num_active_cpus());
-	queue_stop_cpus_work(cpu_active_mask, multi_cpu_stop, &msdata,
-			     &done);
+
+	RCU_NONIDLE(queue_stop_cpus_work(cpu_active_mask, multi_cpu_stop,
+				         &msdata, &done));
+
 	ret = multi_cpu_stop(&msdata);
 
 	/* Busy wait for completion. */

Dang. With $subject fix applied as well..

[  151.103732] smpboot: Booting Node 0 Processor 2 APIC 0x4
[  151.104908] =============================
[  151.104909] WARNING: suspicious RCU usage
[  151.104910] 4.17.0.g66d489e-tip-default #84 Tainted: G            E    
[  151.104911] -----------------------------
[  151.104912] kernel/sched/core.c:1625 suspicious rcu_dereference_check() usage!
[  151.104913] 
               other info that might help us debug this:

[  151.104914] 
               RCU used illegally from offline CPU!
               rcu_scheduler_active = 2, debug_locks = 0
[  151.104916] 3 locks held by swapper/2/0:
[  151.104916]  #0: 00000000560adb60 (stop_cpus_mutex){+.+.}, at: stop_machine_from_inactive_cpu+0x86/0x140
[  151.104923]  #1: 00000000e4fb0238 (&p->pi_lock){-.-.}, at: try_to_wake_up+0x2d/0x5f0
[  151.104929]  #2: 000000003341403b (rcu_read_lock){....}, at: rcu_read_lock+0x0/0x80
[  151.104934] 
               stack backtrace:
[  151.104937] CPU: 2 PID: 0 Comm: swapper/2 Kdump: loaded Tainted: G            E     4.17.0.g66d489e-tip-default #84
[  151.104938] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013
[  151.104938] Call Trace:
[  151.104942]  dump_stack+0x78/0xb3
[  151.104945]  ttwu_stat+0x121/0x130
[  151.104949]  try_to_wake_up+0x2c2/0x5f0
[  151.104953]  ? cpu_stop_park+0x30/0x30
[  151.104956]  wake_up_q+0x4a/0x70
[  151.104959]  cpu_stop_queue_work+0x6b/0xa0
[  151.104963]  queue_stop_cpus_work+0x61/0xb0
[  151.104968]  stop_machine_from_inactive_cpu+0xd8/0x140

Quoted 5 lines

kernel/stop_machine.c +0 -0

index f89014a2c238..a32518c2ba4a 100644

kernel/stop_machine.c int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data, · line 657

Quoted 10 lines

kernel/stop_machine.c +4 -2

@@ -650,8 +650,10 @@ int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 	/* Schedule work on other CPUs and execute directly for local CPU */
 	set_state(&msdata, MULTI_STOP_PREPARE);
 	cpu_stop_init_done(&done, num_active_cpus());
-	queue_stop_cpus_work(cpu_active_mask, multi_cpu_stop, &msdata,
-			     &done);
+
+	RCU_NONIDLE(queue_stop_cpus_work(cpu_active_mask, multi_cpu_stop,
+				         &msdata, &done));
+
 	ret = multi_cpu_stop(&msdata);

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Could say who cares about oddball offline wakeup stat. <cringe>

Quoted 8 lines · Mike Galbraith

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Could say who cares about oddball offline wakeup stat. <cringe>

Yeah, nobody.. but I don't want to have to change the wakeup code to deal with this if at all possible. That'd just add conditions that are 'always' false, except in this exceedingly rare circumstance.

So ideally we manage to tell RCU that it needs to pay attention while we're doing this here thing, which is what I thought RCU_NONIDLE() was about.

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Mike Galbraith

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Could say who cares about oddball offline wakeup stat. <cringe>

Quoted 7 lines

Yeah, nobody.. but I don't want to have to change the wakeup code to deal with this if at all possible. That'd just add conditions that are 'always' false, except in this exceedingly rare circumstance.

So ideally we manage to tell RCU that it needs to pay attention while we're doing this here thing, which is what I thought RCU_NONIDLE() was about.

One straightforward approach would be to provide a arch-specific Kconfig option that tells notify_cpu_starting() not to bother invoking rcu_cpu_starting(). Then x86 selects this Kconfig option and invokes rcu_cpu_starting() itself early enough to avoid splats.

See the (untested, probably does not even build) patch below.

I have no idea where to insert either the "select" or the call to rcu_cpu_starting(), so I left those out. I know that putting the call too early will cause trouble, but I have no idea what constitutes "too early". :-/

Quoted 24 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Mike Galbraith

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Could say who cares about oddball offline wakeup stat. <cringe>

Quoted 7 lines

Yeah, nobody.. but I don't want to have to change the wakeup code to deal with this if at all possible. That'd just add conditions that are 'always' false, except in this exceedingly rare circumstance.

So ideally we manage to tell RCU that it needs to pay attention while we're doing this here thing, which is what I thought RCU_NONIDLE() was about.

One straightforward approach would be to provide a arch-specific Kconfig option that tells notify_cpu_starting() not to bother invoking rcu_cpu_starting(). Then x86 selects this Kconfig option and invokes rcu_cpu_starting() itself early enough to avoid splats.

Quoted 6 lines

See the (untested, probably does not even build) patch below.

I have no idea where to insert either the "select" or the call to rcu_cpu_starting(), so I left those out. I know that putting the call too early will cause trouble, but I have no idea what constitutes "too early". :-/

Something like so perhaps? Mike, can you play around with that? Could burn your granny and eat your cookies.

arch/x86/kernel/cpu/mtrr/main.c +3 -0

index 7468de429087..07360523c3ce 100644
@@ -793,6 +793,9 @@ void mtrr_ap_init(void)
 
 	if (!use_intel() || mtrr_aps_delayed_init)
 		return;
+
+	rcu_cpu_starting(smp_processor_id());
+
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
 	 * changed, but this routine will be called in cpu boot time,

kernel/rcu/tree.c +2 -0

index 2a734692a581..4dab46950fdb 100644
@@ -3775,6 +3775,8 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+static DEFINE_PER_CPU(int, rcu_cpu_started);
+
 /*
  * Mark the specified CPU as being online so that subsequent grace periods
  * (both expedited and normal) will wait on it.  Note that this means that

kernel/rcu/tree.c +6 -0

+		return;
+
+	per_cpu(rcu_cpu_started, cpu) = 1;
+
 	for_each_rcu_flavor(rsp) {
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		rnp = rdp->mynode;

@@ -3852,6 +3859,8 @@ void rcu_report_dead(unsigned int cpu)
 	preempt_enable();
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
+
+	per_cpu(rcu_cpu_started, cpu) = 0;
 }
 
 /* Migrate the dead CPU's callbacks to the current CPU. */

kernel/rcu/tree.c void rcu_cpu_starting(unsigned int cpu) · line 3801

Quoted 1 line

kernel/rcu/tree.c +1 -0

+	if (per_cpu(rcu_cpu_started, cpu))

I would log a non-splat dmesg the first time this happened, just for my future sanity, but otherwise looks fine. I am a bit concerned about calls to rcu_cpu_starting() getting sprinkled all through the code. Or am I being excessively paranoid?

Quoted 6 lines · Paul E. McKenney

kernel/rcu/tree.c void rcu_cpu_starting(unsigned int cpu) · line 3801

Quoted 1 line

kernel/rcu/tree.c +1 -0

+	if (per_cpu(rcu_cpu_started, cpu))

I would log a non-splat dmesg the first time this happened, just for my future sanity, but otherwise looks fine. I am a bit concerned about calls to rcu_cpu_starting() getting sprinkled all through the code. Or am I being excessively paranoid?

Don't think so, but lets first see if this at all works. If not we'll need to figure out something else anyway.

Something like so perhaps? Mike, can you play around with that? Could burn your granny and eat your cookies.

arch/x86/kernel/cpu/mtrr/main.c +3 -0

index 7468de429087..07360523c3ce 100644
@@ -793,6 +793,9 @@ void mtrr_ap_init(void)
 
 	if (!use_intel() || mtrr_aps_delayed_init)
 		return;
+
+	rcu_cpu_starting(smp_processor_id());
+
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
 	 * changed, but this routine will be called in cpu boot time,

kernel/rcu/tree.c +9 -0

index 2a734692a581..4dab46950fdb 100644
@@ -3775,6 +3775,8 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+static DEFINE_PER_CPU(int, rcu_cpu_started);
+
 /*
  * Mark the specified CPU as being online so that subsequent grace periods
  * (both expedited and normal) will wait on it.  Note that this means that

@@ -3796,6 +3798,11 @@ void rcu_cpu_starting(unsigned int cpu)
 	struct rcu_node *rnp;
 	struct rcu_state *rsp;
 
+	if (per_cpu(rcu_cpu_started, cpu))
+		return;
+
+	per_cpu(rcu_cpu_started, cpu) = 1;
+
 	for_each_rcu_flavor(rsp) {
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		rnp = rdp->mynode;

@@ -3852,6 +3859,8 @@ void rcu_report_dead(unsigned int cpu)
 	preempt_enable();
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
+
+	per_cpu(rcu_cpu_started, cpu) = 0;
 }
 
 /* Migrate the dead CPU's callbacks to the current CPU. */

Quoted 26 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Mike Galbraith

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Could say who cares about oddball offline wakeup stat. <cringe>

Quoted 7 lines

Yeah, nobody.. but I don't want to have to change the wakeup code to deal with this if at all possible. That'd just add conditions that are 'always' false, except in this exceedingly rare circumstance.

So ideally we manage to tell RCU that it needs to pay attention while we're doing this here thing, which is what I thought RCU_NONIDLE() was about.

One straightforward approach would be to provide a arch-specific Kconfig option that tells notify_cpu_starting() not to bother invoking rcu_cpu_starting(). Then x86 selects this Kconfig option and invokes rcu_cpu_starting() itself early enough to avoid splats.

See the (untested, probably does not even build) patch below.

Quoted 4 lines

I have no idea where to insert either the "select" or the call to rcu_cpu_starting(), so I left those out. I know that putting the call too early will cause trouble, but I have no idea what constitutes "too early". :-/

Something like so perhaps? Mike, can you play around with that? Could burn your granny and eat your cookies.

arch/x86/kernel/cpu/mtrr/main.c +3 -0

index 7468de429087..07360523c3ce 100644
@@ -793,6 +793,9 @@ void mtrr_ap_init(void)
 
 	if (!use_intel() || mtrr_aps_delayed_init)
 		return;
+
+	rcu_cpu_starting(smp_processor_id());
+
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
 	 * changed, but this routine will be called in cpu boot time,

kernel/rcu/tree.c +9 -0

index 2a734692a581..4dab46950fdb 100644
@@ -3775,6 +3775,8 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+static DEFINE_PER_CPU(int, rcu_cpu_started);
+
 /*
  * Mark the specified CPU as being online so that subsequent grace periods
  * (both expedited and normal) will wait on it.  Note that this means that

@@ -3796,6 +3798,11 @@ void rcu_cpu_starting(unsigned int cpu)
 	struct rcu_node *rnp;
 	struct rcu_state *rsp;
 
+	if (per_cpu(rcu_cpu_started, cpu))
+		return;
+
+	per_cpu(rcu_cpu_started, cpu) = 1;
+
 	for_each_rcu_flavor(rsp) {
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		rnp = rdp->mynode;

@@ -3852,6 +3859,8 @@ void rcu_report_dead(unsigned int cpu)
 	preempt_enable();
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
+
+	per_cpu(rcu_cpu_started, cpu) = 0;
 }
 
 /* Migrate the dead CPU's callbacks to the current CPU. */

Quoted 32 lines · Peter Zijlstra

Quoted 30 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Mike Galbraith

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Could say who cares about oddball offline wakeup stat. <cringe>

Quoted 7 lines

Yeah, nobody.. but I don't want to have to change the wakeup code to deal with this if at all possible. That'd just add conditions that are 'always' false, except in this exceedingly rare circumstance.

So ideally we manage to tell RCU that it needs to pay attention while we're doing this here thing, which is what I thought RCU_NONIDLE() was about.

One straightforward approach would be to provide a arch-specific Kconfig option that tells notify_cpu_starting() not to bother invoking rcu_cpu_starting(). Then x86 selects this Kconfig option and invokes rcu_cpu_starting() itself early enough to avoid splats.

See the (untested, probably does not even build) patch below.

I have no idea where to insert either the "select" or the call to rcu_cpu_starting(), so I left those out. I know that putting the call too early will cause trouble, but I have no idea what constitutes "too early". :-/

Quoted 2 lines

Something like so perhaps? Mike, can you play around with that? Could burn your granny and eat your cookies.

Did this get queued anywhere?

Quoted 48 lines

arch/x86/kernel/cpu/mtrr/main.c +3 -0

index 7468de429087..07360523c3ce 100644
@@ -793,6 +793,9 @@ void mtrr_ap_init(void)
 
 	if (!use_intel() || mtrr_aps_delayed_init)
 		return;
+
+	rcu_cpu_starting(smp_processor_id());
+
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
 	 * changed, but this routine will be called in cpu boot time,

kernel/rcu/tree.c +9 -0

index 2a734692a581..4dab46950fdb 100644
@@ -3775,6 +3775,8 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+static DEFINE_PER_CPU(int, rcu_cpu_started);
+
 /*
  * Mark the specified CPU as being online so that subsequent grace periods
  * (both expedited and normal) will wait on it.  Note that this means that

@@ -3796,6 +3798,11 @@ void rcu_cpu_starting(unsigned int cpu)
 	struct rcu_node *rnp;
 	struct rcu_state *rsp;
 
+	if (per_cpu(rcu_cpu_started, cpu))
+		return;
+
+	per_cpu(rcu_cpu_started, cpu) = 1;
+
 	for_each_rcu_flavor(rsp) {
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		rnp = rdp->mynode;

@@ -3852,6 +3859,8 @@ void rcu_report_dead(unsigned int cpu)
 	preempt_enable();
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
+
+	per_cpu(rcu_cpu_started, cpu) = 0;
 }
 
 /* Migrate the dead CPU's callbacks to the current CPU. */

Quoted 4 lines · Mike Galbraith

Quoted 2 lines

Something like so perhaps? Mike, can you play around with that? Could burn your granny and eat your cookies.

Did this get queued anywhere?

I have not queued it, but given Peter's Signed-off-by and your Tested-by I would be happy to do so.

Quoted 49 lines

Quoted 48 lines

arch/x86/kernel/cpu/mtrr/main.c +3 -0

index 7468de429087..07360523c3ce 100644
@@ -793,6 +793,9 @@ void mtrr_ap_init(void)
 
 	if (!use_intel() || mtrr_aps_delayed_init)
 		return;
+
+	rcu_cpu_starting(smp_processor_id());
+
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
 	 * changed, but this routine will be called in cpu boot time,

kernel/rcu/tree.c +9 -0

index 2a734692a581..4dab46950fdb 100644
@@ -3775,6 +3775,8 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+static DEFINE_PER_CPU(int, rcu_cpu_started);
+
 /*
  * Mark the specified CPU as being online so that subsequent grace periods
  * (both expedited and normal) will wait on it.  Note that this means that

@@ -3796,6 +3798,11 @@ void rcu_cpu_starting(unsigned int cpu)
 	struct rcu_node *rnp;
 	struct rcu_state *rsp;
 
+	if (per_cpu(rcu_cpu_started, cpu))
+		return;
+
+	per_cpu(rcu_cpu_started, cpu) = 1;
+
 	for_each_rcu_flavor(rsp) {
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		rnp = rdp->mynode;

@@ -3852,6 +3859,8 @@ void rcu_report_dead(unsigned int cpu)
 	preempt_enable();
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
+
+	per_cpu(rcu_cpu_started, cpu) = 0;
 }
 
 /* Migrate the dead CPU's callbacks to the current CPU. */

On Tue, May 15, 2018 at 06:30:26AM +0200, Mike Galbraith wrote:
I have not queued it, but given Peter's Signed-off-by and your Tested-by I would be happy to do so.

Quoted 49 lines

Quoted 48 lines

arch/x86/kernel/cpu/mtrr/main.c +3 -0

index 7468de429087..07360523c3ce 100644
@@ -793,6 +793,9 @@ void mtrr_ap_init(void)
 
 	if (!use_intel() || mtrr_aps_delayed_init)
 		return;
+
+	rcu_cpu_starting(smp_processor_id());
+
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
 	 * changed, but this routine will be called in cpu boot time,

kernel/rcu/tree.c +9 -0

index 2a734692a581..4dab46950fdb 100644
@@ -3775,6 +3775,8 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+static DEFINE_PER_CPU(int, rcu_cpu_started);
+
 /*
  * Mark the specified CPU as being online so that subsequent grace periods
  * (both expedited and normal) will wait on it.  Note that this means that

@@ -3796,6 +3798,11 @@ void rcu_cpu_starting(unsigned int cpu)
 	struct rcu_node *rnp;
 	struct rcu_state *rsp;
 
+	if (per_cpu(rcu_cpu_started, cpu))
+		return;
+
+	per_cpu(rcu_cpu_started, cpu) = 1;
+
 	for_each_rcu_flavor(rsp) {
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		rnp = rdp->mynode;

@@ -3852,6 +3859,8 @@ void rcu_report_dead(unsigned int cpu)
 	preempt_enable();
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
+
+	per_cpu(rcu_cpu_started, cpu) = 0;
 }
 
 /* Migrate the dead CPU's callbacks to the current CPU. */

Quoted 3 lines · Paul E. McKenney

On Tue, May 15, 2018 at 06:30:26AM +0200, Mike Galbraith wrote:
I have not queued it, but given Peter's Signed-off-by and your Tested-by I would be happy to do so.

And a Changelog of course :-)

---
From: Peter Zijlstra <[email protected]>
Subject: rcu/x86: Provide early rcu_cpu_starting() callback

The x86/mtrr code does horrific things because hardware. It uses stop_machine_from_inactive_cpu(), which does a wakeup (of the stopper thread on another CPU), which uses RCU, all before the CPU is onlined.

RCU complains about this, because wakeups use RCU and RCU does (rightfully) not consider offline CPUs for grace-periods.

Fix this by initializing RCU way early in the MTRR case.

Tested-by Mike Galbraith <[email protected]>

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

---

Quoted 50 lines

Quoted 49 lines

Quoted 48 lines

arch/x86/kernel/cpu/mtrr/main.c +3 -0

index 7468de429087..07360523c3ce 100644
@@ -793,6 +793,9 @@ void mtrr_ap_init(void)
 
 	if (!use_intel() || mtrr_aps_delayed_init)
 		return;
+
+	rcu_cpu_starting(smp_processor_id());
+
 	/*
 	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
 	 * changed, but this routine will be called in cpu boot time,

kernel/rcu/tree.c +9 -0

index 2a734692a581..4dab46950fdb 100644
@@ -3775,6 +3775,8 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+static DEFINE_PER_CPU(int, rcu_cpu_started);
+
 /*
  * Mark the specified CPU as being online so that subsequent grace periods
  * (both expedited and normal) will wait on it.  Note that this means that

@@ -3796,6 +3798,11 @@ void rcu_cpu_starting(unsigned int cpu)
 	struct rcu_node *rnp;
 	struct rcu_state *rsp;
 
+	if (per_cpu(rcu_cpu_started, cpu))
+		return;
+
+	per_cpu(rcu_cpu_started, cpu) = 1;
+
 	for_each_rcu_flavor(rsp) {
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		rnp = rdp->mynode;

@@ -3852,6 +3859,8 @@ void rcu_report_dead(unsigned int cpu)
 	preempt_enable();
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
+
+	per_cpu(rcu_cpu_started, cpu) = 0;
 }
 
 /* Migrate the dead CPU's callbacks to the current CPU. */

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

On Tue, May 15, 2018 at 06:30:26AM +0200, Mike Galbraith wrote:
I have not queued it, but given Peter's Signed-off-by and your Tested-by I would be happy to do so.

Quoted 1 line

And a Changelog of course :-)

Thank you both!

Is this OK for 4.19, or do we need it for 4.18? I am guessing 4.18, but figured I should ask before wildly rebasing the 82 commits destined for 4.19. ;-)

Quoted 1 line · Mike Galbraith

Dang. With $subject fix applied as well..

That's a NO then... :-(

Quoted 30 lines

[  151.103732] smpboot: Booting Node 0 Processor 2 APIC 0x4
[  151.104908] =============================
[  151.104909] WARNING: suspicious RCU usage
[  151.104910] 4.17.0.g66d489e-tip-default #84 Tainted: G            E    
[  151.104911] -----------------------------
[  151.104912] kernel/sched/core.c:1625 suspicious rcu_dereference_check() usage!
[  151.104913] 
               other info that might help us debug this:

[  151.104914] 
               RCU used illegally from offline CPU!
               rcu_scheduler_active = 2, debug_locks = 0
[  151.104916] 3 locks held by swapper/2/0:
[  151.104916]  #0: 00000000560adb60 (stop_cpus_mutex){+.+.}, at: stop_machine_from_inactive_cpu+0x86/0x140
[  151.104923]  #1: 00000000e4fb0238 (&p->pi_lock){-.-.}, at: try_to_wake_up+0x2d/0x5f0
[  151.104929]  #2: 000000003341403b (rcu_read_lock){....}, at: rcu_read_lock+0x0/0x80
[  151.104934] 
               stack backtrace:
[  151.104937] CPU: 2 PID: 0 Comm: swapper/2 Kdump: loaded Tainted: G            E     4.17.0.g66d489e-tip-default #84
[  151.104938] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013
[  151.104938] Call Trace:
[  151.104942]  dump_stack+0x78/0xb3
[  151.104945]  ttwu_stat+0x121/0x130
[  151.104949]  try_to_wake_up+0x2c2/0x5f0
[  151.104953]  ? cpu_stop_park+0x30/0x30
[  151.104956]  wake_up_q+0x4a/0x70
[  151.104959]  cpu_stop_queue_work+0x6b/0xa0
[  151.104963]  queue_stop_cpus_work+0x61/0xb0
[  151.104968]  stop_machine_from_inactive_cpu+0xd8/0x140

Quoted 15 lines

Quoted 5 lines

kernel/stop_machine.c +0 -0

index f89014a2c238..a32518c2ba4a 100644

kernel/stop_machine.c int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data, · line 657

Quoted 10 lines

kernel/stop_machine.c +4 -2

@@ -650,8 +650,10 @@ int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 	/* Schedule work on other CPUs and execute directly for local CPU */
 	set_state(&msdata, MULTI_STOP_PREPARE);
 	cpu_stop_init_done(&done, num_active_cpus());
-	queue_stop_cpus_work(cpu_active_mask, multi_cpu_stop, &msdata,
-			     &done);
+
+	RCU_NONIDLE(queue_stop_cpus_work(cpu_active_mask, multi_cpu_stop,
+				         &msdata, &done));
+
 	ret = multi_cpu_stop(&msdata);

Paul, any clue on what else to try here? The whole MTRR setup is radically crazy but it's something we're stuck with (yay hardware) :/

So the issue is that we're doing wakeups from an offline CPU (very early during bringup) and RCU (rightfully) complains about that. I thought RCU_NONIDLE() was the magic incantation that makes RCU 'watch', but clearly it's not enough here.

Huh.

No, RCU_NONIDLE() only works for idle, not for offline.

Maybe... Let me take a look. There must be some way to mark a specific lock acquisition and release as being lockdep-invisible...