Re: [PATCH 2/3] kernel/workqueue: Surround work execution with shared lock annotations

[Bart Van Assche <bvanassche@acm.org>]

On Thu, 2018-10-25 at 18:53 +0200, Johannes Berg wrote:
> On Thu, 2018-10-25 at 15:05 +0000, Bart Van Assche wrote:
> > Surround execution of work with a shared lockdep annotation because multiple
> > work items associated with a work queue may execute concurrently.
> 
> Hmm. So, I'm not really entirely sure of the semantics here, but I fail
> to see how "may execute concurrently" means "can be taken recursively"?
> 
> After all, if they execute concurrently, that's in a different thread,
> right? So each thread is really just doing something with this work. It
> may not match mutex semantics in how mutexes would lock each other out
> and prevent concurrency, but I don't think that matters to lockdep at
> all.
> 
> In fact, I'm not sure this actually changes anything, since you can't
> really execute a work struct while executing one already?
> 
> What's this intended to change? I currently don't see how lockdep's
> behaviour would differ with read==1, unless you actually tried to do
> recursive locking, which isn't really possible?
> 
> Or perhaps this is actually the right change for the issue described in
> patch 1, where a work struct flushes another work on the same wq, and
> that causes recursion of sorts? But that recursion should only happen if
> the workqueues is actually marked as ordered, in which case it *is* in
> fact wrong?

How about modifying the wq->lockdep_map annotations only and not touching the
work->lockdep_map annotations? My comment about concurrency in the patch
description refers to a multithreaded workqueue executing multiple different
work items concurrently. I am aware that great care has been taken in the
workqueue implementation to ensure that each work item is executed by exactly
one worker.

Bart.