lkml.org 
[lkml]   [2021]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 2/2] workqueue: Fix work re-entrance when requeue to a different workqueue
On Sat, Oct 09, 2021 at 10:06:23AM +0800, Lai Jiangshan wrote:
> On Fri, Oct 8, 2021 at 6:06 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > When requeuing a work to a different workqueue while it's still getting
> > processed, re-entrace as the follow can happen:
> >
> > { both WQ1 and WQ2 are bounded workqueue, and a work W has been
> > queued on CPU0 for WQ1}
> >
> > CPU 0 CPU 1
> > ===== ====
> > <In worker on CPU 0>
> > process_one_work():
> > ...
> > // pick up W
> > worker->current_work = W;
> > worker->current_func = W->func;
> > ...
> > set_work_pool_and_clear_pending(...);
> > // W can be requeued afterwards
> > queue_work_on(1, WQ2, W):
> > if (!test_and_set_bit(...)) {
> > // this branch is taken, as CPU 0
> > // just clears pending bit.
> > __queue_work(...):
> > pwq = <pool for CPU1 of WQ2>;
> > last_pool = <pool for CPU 0 of WQ1>;
> > if (last_pool != pwq->pool) { // true
> > if (.. && worker->current_pwq->wq == wq) {
> > // false, since @worker is a
> > // a worker of @last_pool (for
> > // WQ1), and @wq is WQ2.
> > }
> > ...
> > insert_work(pwq, W, ...);
> > }
> > // W queued.
> > <schedule to worker on CPU 1>
> > process_one_work():
> > collision = find_worker_executing_work(..);
> > // NULL, because we're searching the
> > // worker pool of CPU 1, while W is
> > // the current work on worker pool of
> > // CPU 0.
> > worker->current_work = W;
> > worker->current_func = W->func;
> > worker->current_func(...);
> > ...
> > worker->current_func(...); // Re-entrance
>
> Concurrent or parallel executions on the same work item aren't
> considered as "Re-entrance" if the workqueue is changed.
>

Well, then Documentation/core-api/workqueue.rst can use some help:

"Note that the flag ``WQ_NON_REENTRANT`` no longer exists as all
workqueues are now non-reentrant - any work item is guaranteed to be
executed by at most one worker system-wide at any given time."

Clearly in the above case that a work item is executed by two worker at
the same time.

> It allows the work function to free itself(the item) and another
> subsystem allocates the same item and reuses it.
>

So you're saying in process_one_work(), ->current_work can point to a
work which gets freed and reallocated before the worker actually
execute it? And users should guarantee it's safe to do so? I mean this
is something that workqueue subsystem allows/expects users to do?

> "Re-entrance" is defined as:
> work function has not been changed
> wq has not been changed
> the item has not been reinitiated.
> (To reduce the check complication, the workqueue subsystem often
> considers it "Re-entrance" if the condition is changed and has changed
> back. But the wq users should not depend on this behavior and should avoid
> it)
>

Thanks for clarifiction, could you also update the documentation to
avoid future confusion? Thanks!

Regards,
Boqun

>
> >
> > This issue is already partially fixed because in queue_work_on(),
> > last_pool can be used to queue the work, as a result the requeued work
> > processing will find the collision and wait for the existing one to
> > finish. However, currently the last_pool is only used when two
> > workqueues are the same one, which causes the issue. Therefore extend
> > the behavior to allow last_pool to requeue the work W even if the
> > workqueues are different. It's safe to do this since the work W has been
> > proved safe to queue and run on the last_pool.
> >
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> > kernel/workqueue.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 1418710bffcd..410141cc5f88 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -1465,7 +1465,7 @@ static void __queue_work(int cpu, struct workqueue_struct *wq,
> >
> > worker = find_worker_executing_work(last_pool, work);
> >
> > - if (worker && worker->current_pwq->wq == wq) {
> > + if (worker) {
> > pwq = worker->current_pwq;
> > } else {
> > /* meh... not running there, queue here */
> > --
> > 2.32.0
> >

\
 
 \ /
  Last update: 2021-10-09 05:23    [W:0.790 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site