On 2018-07-30 05:41, Thomas Gleixner wrote:
On Mon, 30 Jul 2018, Peter Zijlstra wrote:
On Mon, Jul 30, 2018 at 12:20:57PM +0200, Thomas Gleixner wrote:
On Tue, 24 Jul 2018, Sebastian Andrzej Siewior wrote:
On 2018-07-23 18:13:48 [-0700], isaacm@codeaurora.org wrote:
Hi all,
Hi,
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 tglx@linutronix.de Signed-off-by: Peter Zijlstra (Intel) peterz@infradead.org
Hi Peter/Thomas,
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.
--- a/kernel/stop_machine.c +++ b/kernel/stop_machine.c @@ -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; + 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;
- wait_for_completion(&done.completion); + ret = wait_for_completion_timeout(&done.completion, msecs_to_jiffies(1000)); + if (!ret) + BUG_ON(1); +
Thanks for cleaning that up!
Reviewed-by: Thomas Gleixner tglx@linutronix.de