[PATCH] workqueue: Handle race between wake up and rebind

[PATCH] workqueue: Handle race between wake up and rebind

[Neeraj Upadhyay]

There is a potential race b/w rebind_workers() and
wakeup of a worker thread, which can result in
workqueue lockup for a bounder worker pool.

Below is the potential race:

- cpu0 is a bounded worker pool, which is unbound
  from its cpu. A new work is queued on this pool,
  which causes its worker (kworker/0:0) to be woken
  up on a cpu different from cpu0, lets say cpu1.

  workqueue_queue_work
    workqueue_activate_work
      <worker 0 is woken up on cpu1>

- cpu0 rebind happens
  rebind_workers()
    Clears POOL_DISASSOCIATED and binds cpumask of all
    workers.

- kworker/0:0 gets chance to run on cpu1; while processing
  a work, it goes to sleep. However, it does not decrement
  pool->nr_running. This is because WORKER_REBOUND (NOT_
  RUNNING) flag was cleared, when worker entered worker_
  thread().

  Worker 0 runs on cpu1
    worker_thread()
      process_one_work()
        wq_worker_sleeping()
          if (worker->flags & WORKER_NOT_RUNNING)
            return NULL;
          if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
            <Does not decrement nr_running>

- After this, when kworker/0:0 wakes up, this time on its
  bounded cpu cpu0, it increments pool->nr_running again.
  So, pool->nr_running becomes 2.

- When kworker/0:0 enters idle, it decrements pool->nr_running
  by 1. This leaves pool->nr_running =1 , with no workers in
  runnable state.

- Now, no new workers will be woken up, as pool->nr_running is
  non-zero. This results in indefinite lockup for this pool.

Fix this by deferring the work to some other idle worker,
if the current worker is not bound to its pool's CPU.

Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
---
 kernel/workqueue.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 43d18cb..71c0023 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2218,6 +2218,17 @@ static int worker_thread(void *__worker)
 	if (unlikely(!may_start_working(pool)) && manage_workers(worker))
 		goto recheck;
 
+	/* handle the case where, while a bounded pool is unbound,
+	 * its worker is woken up on a target CPU, which is different
+	 * from pool->cpu, but pool is rebound before this worker gets
+	 * chance to run on the target CPU.
+	 */
+	if (WARN_ON_ONCE(!(pool->flags & POOL_DISASSOCIATED) &&
+		raw_smp_processor_id() != pool->cpu)) {
+		wake_up_worker(pool);
+		goto sleep;
+	}
+
 	/*
 	 * ->scheduled list can only be filled while a worker is
 	 * preparing to process a work or actually processing it.
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] workqueue: Handle race between wake up and rebind

[Tejun Heo]

Hello, Neeraj.

On Mon, Jan 15, 2018 at 02:08:12PM +0530, Neeraj Upadhyay wrote:
> - kworker/0:0 gets chance to run on cpu1; while processing
>   a work, it goes to sleep. However, it does not decrement
>   pool->nr_running. This is because WORKER_REBOUND (NOT_
>   RUNNING) flag was cleared, when worker entered worker_

Do you mean that because REBOUND was set?

>   thread().
> 
>   Worker 0 runs on cpu1
>     worker_thread()
>       process_one_work()
>         wq_worker_sleeping()
>           if (worker->flags & WORKER_NOT_RUNNING)
>             return NULL;
>           if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
>             <Does not decrement nr_running>
> 
> - After this, when kworker/0:0 wakes up, this time on its
>   bounded cpu cpu0, it increments pool->nr_running again.
>   So, pool->nr_running becomes 2.

Why is it suddenly 2?  Who made it one on the account of the kworker?

Do you see this happening?  Or better, is there a (semi) reliable
repro for this issue?

Thanks.

-- 
tejun

Re: [PATCH] workqueue: Handle race between wake up and rebind

[Neeraj Upadhyay]

On 01/16/2018 11:05 PM, Tejun Heo wrote:
> Hello, Neeraj.
>
> On Mon, Jan 15, 2018 at 02:08:12PM +0530, Neeraj Upadhyay wrote:
>> - kworker/0:0 gets chance to run on cpu1; while processing
>>    a work, it goes to sleep. However, it does not decrement
>>    pool->nr_running. This is because WORKER_REBOUND (NOT_
>>    RUNNING) flag was cleared, when worker entered worker_
> Do you mean that because REBOUND was set?

Actually, I meant REBOUND was not set. Below is the sequence

- cpu0 bounded pool is unbound.

- kworker/0:0 is woken up on cpu1.

- cpu0 pool is rebound
   REBOUND is set for kworker/0:0

- kworker/0:0 starts running on cpu1
   worker_thread()
     // It clears REBOUND and sets nr_running =1 after below call
     worker_clr_flags(worker, WORKER_PREP | WORKER_REBOUND);

- kworker/0:0 goes to sleep
   wq_worker_sleeping()
     // Below condition is not true, as all NOT_RUNNING
     // flags were cleared in worker_thread()
     if (worker->flags & WORKER_NOT_RUNNING)
     // Below is true, as worker is running on cpu1
     if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
       return NULL;
     // Below is not reached and nr_running stays 1
     if (atomic_dec_and_test(&pool->nr_running) &&

- kworker/0:0 wakes up again, this time on cpu0, as worker->task
   cpus_allowed was set to cpu0, in rebind_workers.
   wq_worker_waking_up()
     if (!(worker->flags & WORKER_NOT_RUNNING)) {
         // Increments pool->nr_running to 2
         atomic_inc(&worker->pool->nr_running);

>
>>    thread().
>>
>>    Worker 0 runs on cpu1
>>      worker_thread()
>>        process_one_work()
>>          wq_worker_sleeping()
>>            if (worker->flags & WORKER_NOT_RUNNING)
>>              return NULL;
>>            if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
>>              <Does not decrement nr_running>
>>
>> - After this, when kworker/0:0 wakes up, this time on its
>>    bounded cpu cpu0, it increments pool->nr_running again.
>>    So, pool->nr_running becomes 2.
> Why is it suddenly 2?  Who made it one on the account of the kworker?
As shown in above comment, it became 1 in
worker_clr_flags(worker, WORKER_PREP | WORKER_REBOUND);
>
> Do you see this happening?  Or better, is there a (semi) reliable
> repro for this issue?
Yes, this was reported in our long run testing with random hotplug.
Sorry, don't have a quick reproducer for it. Issue is reported in few
days of testing.
>
> Thanks.
>

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] workqueue: Handle race between wake up and rebind

[Lai Jiangshan]

On Wed, Jan 17, 2018 at 4:08 AM, Neeraj Upadhyay <neeraju@codeaurora.org> wrote:
>
>
> On 01/16/2018 11:05 PM, Tejun Heo wrote:
>>
>> Hello, Neeraj.
>>
>> On Mon, Jan 15, 2018 at 02:08:12PM +0530, Neeraj Upadhyay wrote:
>>>
>>> - kworker/0:0 gets chance to run on cpu1; while processing
>>>    a work, it goes to sleep. However, it does not decrement
>>>    pool->nr_running. This is because WORKER_REBOUND (NOT_
>>>    RUNNING) flag was cleared, when worker entered worker_
>>
>> Do you mean that because REBOUND was set?
>
>
> Actually, I meant REBOUND was not set. Below is the sequence
>
> - cpu0 bounded pool is unbound.
>
> - kworker/0:0 is woken up on cpu1.
>
> - cpu0 pool is rebound
>   REBOUND is set for kworker/0:0
>

Thanks for looking into the detail of workqueue...

"REBOUND is set for kworker/0:0" means set_cpus_allowed_ptr(kworker/0:0)
already successfull returned and kworker/0:0 is already moved to cpu0.

It will not still run on cpu1 as the following steps you described.

If there is something wrong with " set_cpus_allowed_ptr()"
in this situation, could you please elaborate it.

> - kworker/0:0 starts running on cpu1
>   worker_thread()
>     // It clears REBOUND and sets nr_running =1 after below call
>     worker_clr_flags(worker, WORKER_PREP | WORKER_REBOUND);
>
> - kworker/0:0 goes to sleep
>   wq_worker_sleeping()
>     // Below condition is not true, as all NOT_RUNNING
>     // flags were cleared in worker_thread()
>     if (worker->flags & WORKER_NOT_RUNNING)
>     // Below is true, as worker is running on cpu1
>     if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
>       return NULL;
>     // Below is not reached and nr_running stays 1
>     if (atomic_dec_and_test(&pool->nr_running) &&
>
> - kworker/0:0 wakes up again, this time on cpu0, as worker->task
>   cpus_allowed was set to cpu0, in rebind_workers.
>   wq_worker_waking_up()
>     if (!(worker->flags & WORKER_NOT_RUNNING)) {
>         // Increments pool->nr_running to 2
>         atomic_inc(&worker->pool->nr_running);
>
>>
>>>    thread().
>>>
>>>    Worker 0 runs on cpu1
>>>      worker_thread()
>>>        process_one_work()
>>>          wq_worker_sleeping()
>>>            if (worker->flags & WORKER_NOT_RUNNING)
>>>              return NULL;
>>>            if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
>>>              <Does not decrement nr_running>
>>>
>>> - After this, when kworker/0:0 wakes up, this time on its
>>>    bounded cpu cpu0, it increments pool->nr_running again.
>>>    So, pool->nr_running becomes 2.
>>
>> Why is it suddenly 2?  Who made it one on the account of the kworker?
>
> As shown in above comment, it became 1 in
> worker_clr_flags(worker, WORKER_PREP | WORKER_REBOUND);
>>
>>
>> Do you see this happening?  Or better, is there a (semi) reliable
>> repro for this issue?
>
> Yes, this was reported in our long run testing with random hotplug.
> Sorry, don't have a quick reproducer for it. Issue is reported in few
> days of testing.
>>
>>
>> Thanks.
>>
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by The Linux Foundation
>

Re: [PATCH] workqueue: Handle race between wake up and rebind

[Neeraj Upadhyay]

On 01/18/2018 08:32 AM, Lai Jiangshan wrote:
> On Wed, Jan 17, 2018 at 4:08 AM, Neeraj Upadhyay <neeraju@codeaurora.org> wrote:
>>
>> On 01/16/2018 11:05 PM, Tejun Heo wrote:
>>> Hello, Neeraj.
>>>
>>> On Mon, Jan 15, 2018 at 02:08:12PM +0530, Neeraj Upadhyay wrote:
>>>> - kworker/0:0 gets chance to run on cpu1; while processing
>>>>     a work, it goes to sleep. However, it does not decrement
>>>>     pool->nr_running. This is because WORKER_REBOUND (NOT_
>>>>     RUNNING) flag was cleared, when worker entered worker_
>>> Do you mean that because REBOUND was set?
>>
>> Actually, I meant REBOUND was not set. Below is the sequence
>>
>> - cpu0 bounded pool is unbound.
>>
>> - kworker/0:0 is woken up on cpu1.
>>
>> - cpu0 pool is rebound
>>    REBOUND is set for kworker/0:0
>>
> Thanks for looking into the detail of workqueue...
>
> "REBOUND is set for kworker/0:0" means set_cpus_allowed_ptr(kworker/0:0)
> already successfull returned and kworker/0:0 is already moved to cpu0.
>
> It will not still run on cpu1 as the following steps you described.
>
> If there is something wrong with " set_cpus_allowed_ptr()"
> in this situation, could you please elaborate it.

Thanks Lai, I missed that; will debug from that perspective.
>
>> - kworker/0:0 starts running on cpu1
>>    worker_thread()
>>      // It clears REBOUND and sets nr_running =1 after below call
>>      worker_clr_flags(worker, WORKER_PREP | WORKER_REBOUND);
>>
>> - kworker/0:0 goes to sleep
>>    wq_worker_sleeping()
>>      // Below condition is not true, as all NOT_RUNNING
>>      // flags were cleared in worker_thread()
>>      if (worker->flags & WORKER_NOT_RUNNING)
>>      // Below is true, as worker is running on cpu1
>>      if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
>>        return NULL;
>>      // Below is not reached and nr_running stays 1
>>      if (atomic_dec_and_test(&pool->nr_running) &&
>>
>> - kworker/0:0 wakes up again, this time on cpu0, as worker->task
>>    cpus_allowed was set to cpu0, in rebind_workers.
>>    wq_worker_waking_up()
>>      if (!(worker->flags & WORKER_NOT_RUNNING)) {
>>          // Increments pool->nr_running to 2
>>          atomic_inc(&worker->pool->nr_running);
>>
>>>>     thread().
>>>>
>>>>     Worker 0 runs on cpu1
>>>>       worker_thread()
>>>>         process_one_work()
>>>>           wq_worker_sleeping()
>>>>             if (worker->flags & WORKER_NOT_RUNNING)
>>>>               return NULL;
>>>>             if (WARN_ON_ONCE(pool->cpu != raw_smp_processor_id()))
>>>>               <Does not decrement nr_running>
>>>>
>>>> - After this, when kworker/0:0 wakes up, this time on its
>>>>     bounded cpu cpu0, it increments pool->nr_running again.
>>>>     So, pool->nr_running becomes 2.
>>> Why is it suddenly 2?  Who made it one on the account of the kworker?
>> As shown in above comment, it became 1 in
>> worker_clr_flags(worker, WORKER_PREP | WORKER_REBOUND);
>>>
>>> Do you see this happening?  Or better, is there a (semi) reliable
>>> repro for this issue?
>> Yes, this was reported in our long run testing with random hotplug.
>> Sorry, don't have a quick reproducer for it. Issue is reported in few
>> days of testing.
>>>
>>> Thanks.
>>>
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of the Code Aurora Forum, hosted by The Linux Foundation
>>

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation