lkml.org 
[lkml]   [2020]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] workqueue: Don't double assign worker->sleeping
On Wed, Apr 1, 2020 at 11:22 AM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> Hello
>
> If I don't miss all the issues you have listed, it is a good and straightforward
> fix, but I have concern that cmpxchg_local() might have performance impact
> on some non-x86 arch.
>
> The two issues as you have listed:
> 1) WARN_ON_ONCE() on valid condition when interrupted(async-page-faulted)
> 2) wq_worker_running() can be interrupted(async-page-faulted in virtual machine)
> and nr_running would be decreased twice.

would be *increased* twice

I just saw the V2 patch, this issue is not listed, but need to be fixed too.

>
> For fixing issue one, we can just remove WARN_ON_ONCE() as this patch.
> For fixing issue two, ->sleeping in wq_worker_running() can be checked&modified
> under irq-disabled. (we can't use preempt-disabled context here)
>
> thanks,
> Lai
>
> On Sat, Mar 28, 2020 at 1:53 AM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > The kernel test robot triggered a warning with the following race:
> > task-ctx interrupt-ctx
> > worker
> > -> process_one_work()
> > -> work_item()
> > -> schedule();
> > -> sched_submit_work()
> > -> wq_worker_sleeping()
> > -> ->sleeping = 1
> > atomic_dec_and_test(nr_running)
> > __schedule(); *interrupt*
> > async_page_fault()
> > -> local_irq_enable();
> > -> schedule();
> > -> sched_submit_work()
> > -> wq_worker_sleeping()
> > -> if (WARN_ON(->sleeping)) return
> > -> __schedule()
> > -> sched_update_worker()
> > -> wq_worker_running()
> > -> atomic_inc(nr_running);
> > -> ->sleeping = 0;
> >
> > -> sched_update_worker()
> > -> wq_worker_running()
> > if (!->sleeping) return
> >
> > In this context the warning is pointless everything is fine.
> >
> > However, if the interrupt occurs in wq_worker_sleeping() between reading and
> > setting `sleeping' i.e.
> >
> > | if (WARN_ON_ONCE(worker->sleeping))
> > | return;
> > *interrupt*
> > | worker->sleeping = 1;
> >
> > then pool->nr_running will be decremented twice in wq_worker_sleeping()
> > but it will be incremented only once in wq_worker_running().
> >
> > Replace the assignment of `sleeping' with a cmpxchg_local() to ensure
> > that there is no double assignment of the variable. The variable is only
> > accessed from the local CPU. Remove the WARN statement because this
> > condition can be valid.
> >
> > An alternative would be to move `->sleeping' to `->flags' as a new bit
> > but this would require to acquire the pool->lock in wq_worker_running().
> >
> > Fixes: 6d25be5782e48 ("sched/core, workqueues: Distangle worker accounting from rq lock")
> > Link: https://lkml.kernel.org/r/20200327074308.GY11705@shao2-debian
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > kernel/workqueue.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 4e01c448b4b48..dc477a2a3ce30 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -846,11 +846,10 @@ void wq_worker_running(struct task_struct *task)
> > {
> > struct worker *worker = kthread_data(task);
> >
> > - if (!worker->sleeping)
> > + if (cmpxchg_local(&worker->sleeping, 1, 0) == 0)
> > return;
> > if (!(worker->flags & WORKER_NOT_RUNNING))
> > atomic_inc(&worker->pool->nr_running);
> > - worker->sleeping = 0;
> > }
> >
> > /**
> > @@ -875,10 +874,9 @@ void wq_worker_sleeping(struct task_struct *task)
> >
> > pool = worker->pool;
> >
> > - if (WARN_ON_ONCE(worker->sleeping))
> > + if (cmpxchg_local(&worker->sleeping, 0, 1) == 1)
> > return;
> >
> > - worker->sleeping = 1;
> > spin_lock_irq(&pool->lock);
> >
> > /*
> > --
> > 2.26.0
> >

\
 
 \ /
  Last update: 2020-04-01 05:45    [W:0.125 / U:0.764 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site