commit 14e568e78 (stop_machine: Use smpboot threads) introduced the following regression: Before this commit the stopper enabled bit was set in the online notifier. CPU0 CPU1 cpu_up cpu online hotplug_notifier(ONLINE) stopper(CPU1)->enabled = true; ... stop_machine() The conversion to smpboot threads moved the enablement to the wakeup path of the parked thread. The majority of users seem to have the following working order: CPU0 CPU1 cpu_up cpu online unpark_threads() wakeup(stopper[CPU1]) .... stopper thread runs stopper(CPU1)->enabled = true; stop_machine() But Konrad and Sander have observed: CPU0 CPU1 cpu_up cpu online unpark_threads() wakeup(stopper[CPU1]) .... stop_machine() stopper thread runs stopper(CPU1)->enabled = true; Now the stop machinery kicks CPU0 into the stop loop, where it gets stuck forever because the queue code saw stopper(CPU1)->enabled == false, so CPU0 waits for CPU1 to enter stomp_machine, but the CPU1 stopper work got discarded due to enabled == false. Add a pre_unpark function to the smpboot thread descriptor and call it before waking the thread. This fixes the problem at hand, but the stop_machine code should be more robust. The stopper->enabled flag smells fishy at best. Thanks to Konrad for going through a loop of debug patches and providing the information to decode this issue.
Unified · Split
| @@ -24,6 +24,9 @@ struct smpboot_thread_data; · 23 unmodified lines | |||
| 24 | * parked (cpu offline) | 24 | * parked (cpu offline) |
| 25 | * @unpark: Optional unpark function, called when the thread is | 25 | * @unpark: Optional unpark function, called when the thread is |
| 26 | * unparked (cpu online) | 26 | * unparked (cpu online) |
| 27 | * @pre_unpark: Optional unpark function, called before the thread is | ||
| 28 | * unparked (cpu online). This is not guaranteed to be | ||
| 29 | * called on the target cpu of the thread. Careful! | ||
| 27 | * @selfparking: Thread is not parked by the park function. | 30 | * @selfparking: Thread is not parked by the park function. |
| 28 | * @thread_comm: The base name of the thread | 31 | * @thread_comm: The base name of the thread |
| 29 | */ | 32 | */ |
| @@ -37,6 +40,7 @@ struct smp_hotplug_thread { · 7 unmodified lines | |||
| 37 | void (*cleanup)(unsigned int cpu, bool online); | 40 | void (*cleanup)(unsigned int cpu, bool online); |
| 38 | void (*park)(unsigned int cpu); | 41 | void (*park)(unsigned int cpu); |
| 39 | void (*unpark)(unsigned int cpu); | 42 | void (*unpark)(unsigned int cpu); |
| 43 | void (*pre_unpark)(unsigned int cpu); | ||
| 40 | bool selfparking; | 44 | bool selfparking; |
| 41 | const char *thread_comm; | 45 | const char *thread_comm; |
| 42 | }; | 46 | }; |
| @@ -209,6 +209,8 @@ static void smpboot_unpark_thread(struct smp_hotplug_thread *ht, unsigned int cp · 208 unmodified lines | |||
| 209 | { | 209 | { |
| 210 | struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu); | 210 | struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu); |
| 211 | 211 | ||
| 212 | if (ht->pre_unpark) | ||
| 213 | ht->pre_unpark(cpu); | ||
| 212 | kthread_unpark(tsk); | 214 | kthread_unpark(tsk); |
| 213 | } | 215 | } |
| 214 | 216 | ||
| @@ -336,7 +336,7 @@ static struct smp_hotplug_thread cpu_stop_threads = { · 335 unmodified lines | |||
| 336 | .create = cpu_stop_create, | 336 | .create = cpu_stop_create, |
| 337 | .setup = cpu_stop_unpark, | 337 | .setup = cpu_stop_unpark, |
| 338 | .park = cpu_stop_park, | 338 | .park = cpu_stop_park, |
| 339 | .unpark = cpu_stop_unpark, | 339 | .pre_unpark = cpu_stop_unpark, |
| 340 | .selfparking = true, | 340 | .selfparking = true, |
| 341 | }; | 341 | }; |
| 342 | 342 | ||