On 2018-07-30 14:07, Peter Zijlstra wrote:

On Mon, Jul 30, 2018 at 10:12:43AM -0700, Sodagudi Prasad wrote:

quoted

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

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.

-Thanks, Prasad

quoted

--- 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);
+

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.

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