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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Oct 2021 11:19:14 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        Tejun Heo <tj@...nel.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH] workqueue: doc: Call out the non-reentrance conditions

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