[PATCH] workqueue: doc: Call out the non-reentrance conditions

[PATCH] workqueue: doc: Call out the non-reentrance conditions

[Boqun Feng]

The current doc of workqueue API suggests that work items are
non-reentrant: any work item is guaranteed to be executed by at most one
worker system-wide at any given time. However this is not true, the
following case can cause a work item W executed by two workers at
the same time:

        queue_work_on(0, WQ1, W);
        // after a worker picks up W and clear the pending bit
        queue_work_on(1, WQ2, W);
        // workers on CPU0 and CPU1 will execute W in the same time.

, which means the non-reentrance of a work item is conditional, and
Lai Jiangshan provided a nice summary[1] of the conditions, therefore
use it to improve the doc.

[1]: https://lore.kernel.org/lkml/CAJhGHyDudet_xyNk=8xnuO2==o-u06s0E0GZVP4Q67nmQ84Ceg@mail.gmail.com/

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 Documentation/core-api/workqueue.rst | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/core-api/workqueue.rst b/Documentation/core-api/workqueue.rst
index 541d31de8926..3b22ed137662 100644
--- a/Documentation/core-api/workqueue.rst
+++ b/Documentation/core-api/workqueue.rst
@@ -216,10 +216,6 @@ resources, scheduled and executed.
 
   This flag is meaningless for unbound wq.
 
-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.
-
 
 ``max_active``
 --------------
@@ -391,6 +387,23 @@ the stack trace of the offending worker thread. ::
 The work item's function should be trivially visible in the stack
 trace.
 
+Non-reentrance Conditions
+=========================
+
+Workqueue guarantees that a work item cannot be re-entrant if the following
+conditions hold after a work item gets queued:
+
+        1. The work function hasn't been changed.
+        2. No one queues the work item to another workqueue.
+        3. The work item hasn't been reinitiated.
+
+In other words, if the above conditions hold, the work item is guaranteed to be
+executed by at most one worker system-wide at any given time.
+
+Note that requeuing the work item (to the same queue) in the self function
+doesn't break these conditions, so it's safe to do. Otherwise, caution is
+required when breaking the conditions inside a work function.
+
 
 Kernel Inline Documentations Reference
 ======================================
-- 
2.33.0

Re: [PATCH] workqueue: doc: Call out the non-reentrance conditions

[Matthew Wilcox]

On Mon, Oct 18, 2021 at 09:31:17AM +0800, Boqun Feng wrote:
> @@ -391,6 +387,23 @@ the stack trace of the offending worker thread. ::
>  The work item's function should be trivially visible in the stack
>  trace.
>  
> +Non-reentrance Conditions
> +=========================
> +
> +Workqueue guarantees that a work item cannot be re-entrant if the following
> +conditions hold after a work item gets queued:
> +
> +        1. The work function hasn't been changed.
> +        2. No one queues the work item to another workqueue.
> +        3. The work item hasn't been reinitiated.
> +
> +In other words, if the above conditions hold, the work item is guaranteed to be
> +executed by at most one worker system-wide at any given time.
> +
> +Note that requeuing the work item (to the same queue) in the self function
> +doesn't break these conditions, so it's safe to do. Otherwise, caution is
> +required when breaking the conditions inside a work function.
> +

I'd like to suggest that this be added to the Guidelines section
instead:

* A work item will not normally be processed on multiple CPUs at the
  same time.  It can happen if the work function is changed, the work
  item is queued to multiple queues or the work function is
  reinitialised after being queued.

Re: [PATCH] workqueue: doc: Call out the non-reentrance conditions

[Boqun Feng]

On Mon, Oct 18, 2021 at 03:17:53AM +0100, Matthew Wilcox wrote:
> On Mon, Oct 18, 2021 at 09:31:17AM +0800, Boqun Feng wrote:
> > @@ -391,6 +387,23 @@ the stack trace of the offending worker thread. ::
> >  The work item's function should be trivially visible in the stack
> >  trace.
> >  
> > +Non-reentrance Conditions
> > +=========================
> > +
> > +Workqueue guarantees that a work item cannot be re-entrant if the following
> > +conditions hold after a work item gets queued:
> > +
> > +        1. The work function hasn't been changed.
> > +        2. No one queues the work item to another workqueue.
> > +        3. The work item hasn't been reinitiated.
> > +
> > +In other words, if the above conditions hold, the work item is guaranteed to be
> > +executed by at most one worker system-wide at any given time.
> > +
> > +Note that requeuing the work item (to the same queue) in the self function
> > +doesn't break these conditions, so it's safe to do. Otherwise, caution is
> > +required when breaking the conditions inside a work function.
> > +
> 
> I'd like to suggest that this be added to the Guidelines section

Good idea, Guidelines section is a better place to put these, since it's
for users.

> instead:
> 
> * A work item will not normally be processed on multiple CPUs at the

Precisely speaking, it should be "by mutliple workers" instead of "on
multiple CPUs", because two workers of tw unbound workqueue may process
the same work item on the same CPU, and that's problematic since
processing work is preemptible.

>   same time.  It can happen if the work function is changed, the work
>   item is queued to multiple queues or the work function is
>   reinitialised after being queued.

I end up with something like below, I still want to keep the keyword
"reentrant" for searching, because sometimes one may forget this
particular aspect after reading the whole doc for a while, the keyword
can help locate the lines faster (Ok, the fact is that "one" was me
;-)).

* A work item will not normally be processed by multiple workers at the
  same time, i.e. it's non-reentrant.  However it can happen if the work
  function is changed, the work item is queued to multiple queues or the
  work item is reinitialised after being queued.

Thoughts? Thank for the suggestion!

Regards,
Boqun

Re: [PATCH] workqueue: doc: Call out the non-reentrance conditions

[Matthew Wilcox]

On Mon, Oct 18, 2021 at 11:19:14AM +0800, Boqun Feng wrote:
> On Mon, Oct 18, 2021 at 03:17:53AM +0100, Matthew Wilcox wrote:
> > On Mon, Oct 18, 2021 at 09:31:17AM +0800, Boqun Feng wrote:
> > > @@ -391,6 +387,23 @@ the stack trace of the offending worker thread. ::
> > >  The work item's function should be trivially visible in the stack
> > >  trace.
> > >  
> > > +Non-reentrance Conditions
> > > +=========================
> > > +
> > > +Workqueue guarantees that a work item cannot be re-entrant if the following
> > > +conditions hold after a work item gets queued:
> > > +
> > > +        1. The work function hasn't been changed.
> > > +        2. No one queues the work item to another workqueue.
> > > +        3. The work item hasn't been reinitiated.
> > > +
> > > +In other words, if the above conditions hold, the work item is guaranteed to be
> > > +executed by at most one worker system-wide at any given time.
> > > +
> > > +Note that requeuing the work item (to the same queue) in the self function
> > > +doesn't break these conditions, so it's safe to do. Otherwise, caution is
> > > +required when breaking the conditions inside a work function.
> > > +
> > 
> > I'd like to suggest that this be added to the Guidelines section
> 
> Good idea, Guidelines section is a better place to put these, since it's
> for users.
> 
> > instead:
> > 
> > * A work item will not normally be processed on multiple CPUs at the
> 
> Precisely speaking, it should be "by mutliple workers" instead of "on
> multiple CPUs", because two workers of tw unbound workqueue may process
> the same work item on the same CPU, and that's problematic since
> processing work is preemptible.
> 
> >   same time.  It can happen if the work function is changed, the work
> >   item is queued to multiple queues or the work function is
> >   reinitialised after being queued.
> 
> I end up with something like below, I still want to keep the keyword
> "reentrant" for searching, because sometimes one may forget this
> particular aspect after reading the whole doc for a while, the keyword
> can help locate the lines faster (Ok, the fact is that "one" was me
> ;-)).
> 
> * A work item will not normally be processed by multiple workers at the
>   same time, i.e. it's non-reentrant.  However it can happen if the work
>   function is changed, the work item is queued to multiple queues or the
>   work item is reinitialised after being queued.
> 
> Thoughts? Thank for the suggestion!

Looks good to me!

Re: [PATCH] workqueue: doc: Call out the non-reentrance conditions

[Tejun Heo]

On Mon, Oct 18, 2021 at 09:31:17AM +0800, Boqun Feng wrote:
> +Workqueue guarantees that a work item cannot be re-entrant if the following
> +conditions hold after a work item gets queued:
> +
> +        1. The work function hasn't been changed.
> +        2. No one queues the work item to another workqueue.
> +        3. The work item hasn't been reinitiated.

Maybe phrasing it so that the above are the conditions defining a work item
to be the same instance would be clearer?

Thanks.

-- 
tejun

Re: [PATCH] workqueue: doc: Call out the non-reentrance conditions

[Boqun Feng]

On Tue, Oct 19, 2021 at 07:47:56AM -1000, Tejun Heo wrote:
> On Mon, Oct 18, 2021 at 09:31:17AM +0800, Boqun Feng wrote:
> > +Workqueue guarantees that a work item cannot be re-entrant if the following
> > +conditions hold after a work item gets queued:
> > +
> > +        1. The work function hasn't been changed.
> > +        2. No one queues the work item to another workqueue.
> > +        3. The work item hasn't been reinitiated.
> 
> Maybe phrasing it so that the above are the conditions defining a work item
> to be the same instance would be clearer?
> 

Hmm.. that would mean queue_work_on() may change the work item to
another instance? For example:

	struct work_struct w;

	INIT_WORK_ONSTACK(w, some_f);

	queue_work_on(&w, system_wq, 1); // queue a work and create
					 // an instance

	queue_work_on(&w, system_long_wq, 2); // queue the same work
					      // item but it means
					      // changing it to another
					      // instance.

Looks a little counter-intuitive to me, but let me try (combined with
Matthew's suggestion)

<in Guidelines section>

* A work item instance will not be processed by multiple workers at the
  same time, i.e. it's non-reentrant, so requeuing the same instance of
  a work item is safe and not racy.  Operations considered as changing
  the work item to a different instance are: 1) change the work
  function, 2) queue the work item to a different workqueue, or 3)
  reinitiate the work item.  The non-reentrance guarantee doesn't hold
  for different work item instances.

Regards,
Boqun

> Thanks.
> 
> -- 
> tejun

Re: [PATCH] workqueue: doc: Call out the non-reentrance conditions

[Tejun Heo]

On Wed, Oct 20, 2021 at 01:40:21PM +0800, Boqun Feng wrote:
> * A work item instance will not be processed by multiple workers at the
>   same time, i.e. it's non-reentrant, so requeuing the same instance of
>   a work item is safe and not racy.  Operations considered as changing
>   the work item to a different instance are: 1) change the work
>   function, 2) queue the work item to a different workqueue, or 3)
>   reinitiate the work item.  The non-reentrance guarantee doesn't hold
>   for different work item instances.

Yeah, I prefer it to be described this way but it's not a strong opinion.
Looks good to me either way.

Thanks.

-- 
tejun