Re: [PATCH] stop_machine: Disable preemption after queueing stopper threads

[Sodagudi Prasad]

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@xxxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project