stop_machine: Disable preemption after queueing stopper threads

This commit:

9fb8d5dc4b64 ("stop_machine, Disable preemption when waking two stopper threads")

does not fully address the race condition that can occur as follows:

On one CPU, call it CPU 3, thread 1 invokes cpu_stop_queue_two_works(2, 3,...), and the execution is such that thread 1 queues the works for migration/2 and migration/3, and is preempted after releasing the locks for migration/2 and migration/3, but before waking the threads.

Then, On CPU 2, a kworker, call it thread 2, is running, and it invokes cpu_stop_queue_two_works(1, 2,...), such that thread 2 queues the works for migration/1 and migration/2. Meanwhile, on CPU 3, thread 1 resumes execution, and wakes migration/2 and migration/3. This means that when CPU 2 releases the locks for migration/1 and migration/2, but before it wakes those threads, it can be preempted by migration/2.

If thread 2 is preempted by migration/2, then migration/2 will execute the first work item successfully, since migration/3 was woken up by CPU 3, but when it goes to execute the second work item, it disables preemption, calls multi_cpu_stop(), and thus, CPU 2 will wait forever for migration/1, which should have been woken up by thread 2. However migration/1 cannot be woken up by thread 2, since it is a kworker, so it is affine to CPU 2, but CPU 2 is running migration/2 with preemption disabled, so thread 2 will never run.

Disable preemption after queueing works for stopper threads to ensure that the operation of queueing the works and waking the stopper threads is atomic.

Fixes 9fb8d5dc4b64 ("stop_machine, Disable preemption when waking two

stopper threads")

Co-Developed-by Prasad Sodagudi <[email protected]>

Co-Developed-by Pavankumar Kondeti <[email protected]>

Signed-off-by Isaac J. Manjarres <[email protected]>

Signed-off-by Prasad Sodagudi <[email protected]>

Signed-off-by Pavankumar Kondeti <[email protected]>

Cc [email protected]

---
 kernel/stop_machine.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

kernel/stop_machine.c +9 -0

index 1ff523d..e190d1e 100644
@@ -260,6 +260,15 @@ static int cpu_stop_queue_two_works(int cpu1,
 struct cpu_stop_work *work1,
 	err = 0;
 	__cpu_stop_queue_work(stopper1, work1, &wakeq);
 	__cpu_stop_queue_work(stopper2, work2, &wakeq);
+	/*
+	 * The waking up of stopper threads has to happen
+	 * in the same scheduling context as the queueing.
+	 * Otherwise, there is a possibility of one of the
+	 * above stoppers being woken up by another CPU,
+	 * and preempting us. This will cause us to n ot
+	 * wake up the other stopper forever.
+	 */
+	preempt_disable();
 unlock:
 	raw_spin_unlock(&stopper2->lock);

kernel/stop_machine.c +0 -1

 	raw_spin_unlock_irq(&stopper1->lock);

@@ -271,7 +280,6 @@ static int cpu_stop_queue_two_works(int cpu1,
 struct cpu_stop_work *work1,
 	}
 
 	if (!err) {
-		preempt_disable();
 		wake_up_q(&wakeq);
 		preempt_enable();

}

Hi all,

Are there any comments about this patch?

Thanks,
Isaac Manjarres

Quoted 1 line · On 2018-07-23 18:13:48 [-0700], [email protected] wrote:

Hi all,

Hi,

Quoted 1 line

Are there any comments about this patch?

I haven't look in detail at this but your new preempt_disable() makes things unbalanced for the err != 0 case.

Quoted 2 lines

Thanks,
Isaac Manjarres

Sebastian

Quoted 1 line · On 2018-07-23 18:13:48 [-0700], [email protected] wrote:

Hi all,

Hi,

Quoted 1 line

Are there any comments about this patch?

I haven't look in detail at this but your new preempt_disable() makes things unbalanced for the err != 0 case.

Quoted 8 lines · Sebastian Andrzej Siewior

Quoted 1 line · On 2018-07-23 18:13:48 [-0700], [email protected] wrote:

Hi all,

Hi,

Quoted 1 line

Are there any comments about this patch?

I haven't look in detail at this but your new preempt_disable() makes things unbalanced for the err != 0 case.

It doesn't but that code is really an unreadable pile of ...

Quoted 11 lines · Thomas Gleixner

Quoted 8 lines · Sebastian Andrzej Siewior

Quoted 1 line · On 2018-07-23 18:13:48 [-0700], [email protected] wrote:

Hi all,

Hi,

Quoted 1 line

Are there any comments about this patch?

I haven't look in detail at this but your new preempt_disable() makes things unbalanced for the err != 0 case.

It doesn't but that code is really an unreadable pile of ...

---
Subject: stop_machine: Reflow cpu_stop_queue_two_works()

The code flow in cpu_stop_queue_two_works() is a little arcane; fix this by lifting the preempt_disable() to the top to create more natural nesting wrt the spinlocks and make the wake_up_q() and preempt_enable() unconditional at the end.

Furthermore, enable preemption in the -EDEADLK case, such that we spin-wait with preemption enabled.

Suggested-by Thomas Gleixner <[email protected]>

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

Quoted 21 lines · Peter Zijlstra

Quoted 11 lines · Thomas Gleixner

Quoted 8 lines · Sebastian Andrzej Siewior

Quoted 1 line · On 2018-07-23 18:13:48 [-0700], [email protected] wrote:

Hi all,

Hi,

Quoted 1 line

Are there any comments about this patch?

I haven't look in detail at this but your new preempt_disable() makes things unbalanced for the err != 0 case.

It doesn't but that code is really an unreadable pile of ...

---
Subject: stop_machine: Reflow cpu_stop_queue_two_works()

The code flow in cpu_stop_queue_two_works() is a little arcane; fix this by lifting the preempt_disable() to the top to create more natural nesting wrt the spinlocks and make the wake_up_q() and preempt_enable() unconditional at the end.

Quoted 5 lines

Furthermore, enable preemption in the -EDEADLK case, such that we spin-wait with preemption enabled.

Suggested-by Thomas Gleixner <[email protected]>

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

Thanks for cleaning that up!

Reviewed-by Thomas Gleixner <[email protected]>

How about including below change as well? Currently, there is no way to

identify thread migrations completed or not. When we observe this issue, the symptom was work queue lock up. It is better to have some timeout here and induce the bug_on.

How about including below change as well? Currently, there is no way to identify thread migrations completed or not. When we observe this issue, the symptom was work queue lock up. It is better to have some timeout here and induce the bug_on.

There is no way to identify the migration threads stuck or not.

kernel/stop_machine.c +0 -0

@@ -290,6 +290,7 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2,

cpu_stop_fn_t fn, void *

        struct cpu_stop_done done;
        struct cpu_stop_work work1, work2;
        struct multi_stop_data msdata;

kernel/stop_machine.c +1 -0

+       int ret;
 
        msdata = (struct multi_stop_data){
                .fn = fn,

@@ -312,7 +313,10 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2,

cpu_stop_fn_t fn, void *

        if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
                return -ENOENT;

kernel/stop_machine.c +1 -1

-       wait_for_completion(&done.completion);
+       ret = wait_for_completion_timeout(&done.completion,

msecs_to_jiffies(1000));

Quoted 4 lines · Sodagudi Prasad

How about including below change as well? Currently, there is no way to identify thread migrations completed or not. When we observe this issue, the symptom was work queue lock up. It is better to have some timeout here and induce the bug_on.

You'd trigger the soft-lockup or hung-task detector I think. And if not, we ought to look at making it trigger at least one of those.

Quoted 1 line

There is no way to identify the migration threads stuck or not.

Should be pretty obvious from the splat generated by the above, no?

Quoted 22 lines

kernel/stop_machine.c +0 -0

@@ -290,6 +290,7 @@ int stop_two_cpus(unsigned int cpu1, unsigned int 
 cpu2,

cpu_stop_fn_t fn, void *

        struct cpu_stop_done done;
        struct cpu_stop_work work1, work2;
        struct multi_stop_data msdata;

kernel/stop_machine.c +1 -0

+       int ret;
 
        msdata = (struct multi_stop_data){
                .fn = fn,

@@ -312,7 +313,10 @@ int stop_two_cpus(unsigned int cpu1, unsigned int 
 cpu2,

cpu_stop_fn_t fn, void *

        if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
                return -ENOENT;

kernel/stop_machine.c +1 -1

-       wait_for_completion(&done.completion);
+       ret = wait_for_completion_timeout(&done.completion,

msecs_to_jiffies(1000));

kernel/stop_machine.c int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void * · line 319

Quoted 3 lines

kernel/stop_machine.c +3 -0

+       if (!ret)
+               BUG_ON(1);
+

That's a random timeout, which if you spuriously trigger it, will take down your machine. That seems like a cure worse than the disease.

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Sodagudi Prasad

How about including below change as well? Currently, there is no way to
identify thread migrations completed or not. When we observe this issue,
the symptom was work queue lock up. It is better to have some timeout here
and induce the bug_on.

Quoted 7 lines

You'd trigger the soft-lockup or hung-task detector I think. And if not,
we ought to look at making it trigger at least one of those.

Quoted 1 line

There is no way to identify the migration threads stuck or not.

Should be pretty obvious from the splat generated by the above, no?

Hi Peter and Thomas,

Thanks for your support.
I have another question on this flow and retry mechanism used in this cpu_stop_queue_two_works() function using the global variable stop_cpus_in_progress.

This variable is getting used in various paths, such as task migration, set task affinity, and CPU hotplug.

For example cpu hotplug path, stop_cpus_in_progress variable getting set with true with out checking.
takedown_cpu()
--stop_machine_cpuslocked()
---stop_cpus()
---__stop_cpus()
----queue_stop_cpus_work()
setting stop_cpus_in_progress to true directly.

But in the task migration path only, the stop_cpus_in_progress variable is used for retry.

I am thinking that stop_cpus_in_progress variable lead race conditions, where CPU hotplug and task migration happening simultaneously. Please correct me If my understanding wrong.