On 2018-07-30 14:07, Peter Zijlstra wrote:
On Mon, Jul 30, 2018 at 10:12:43AM -0700, Sodagudi Prasad wrote:
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.
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
--- 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.