Re: [PATCH 3/3] kernel/workqueue: Suppress a false positive lockdep complaint

[Bart Van Assche]

On Thu, 2018-10-25 at 21:51 +-0200, Johannes Berg wrote:
+AD4 +AFs ... +AF0
+AD4 diff --git a/fs/direct-io.c b/fs/direct-io.c
+AD4 index 093fb54cd316..9ef33d6cba56 100644
+AD4 --- a/fs/direct-io.c
+AD4 +-+-+- b/fs/direct-io.c
+AD4 +AEAAQA -629,9 +-629,16 +AEAAQA int sb+AF8-init+AF8-dio+AF8-done+AF8-wq(struct super+AF8-block +ACo-sb)
+AD4  	 +ACo This has to be atomic as more DIOs can race to create the workqueue
+AD4  	 +ACo-/
+AD4  	old +AD0 cmpxchg(+ACY-sb-+AD4-s+AF8-dio+AF8-done+AF8-wq, NULL, wq)+ADs
+AD4 -	/+ACo Someone created workqueue before us? Free ours... +ACo-/
+AD4 +-	/+ACo
+AD4 +-	 +ACo Someone created workqueue before us? Free ours...
+AD4 +-	 +ACo Note the +AF8-nested(), that pushes down to the (in this case actually
+AD4 +-	 +ACo pointless) flush+AF8-workqueue() happening inside, since this function
+AD4 +-	 +ACo might be called in contexts that hold the same locks that an fs may
+AD4 +-	 +ACo take while being called from dio+AF8-aio+AF8-complete+AF8-work() from another
+AD4 +-	 +ACo instance of the workqueue we allocate here.
+AD4 +-	 +ACo-/
+AD4  	if (old)
+AD4 -		destroy+AF8-workqueue(wq)+ADs
+AD4 +-		destroy+AF8-workqueue+AF8-nested(wq, SINGLE+AF8-DEPTH+AF8-NESTING)+ADs
+AD4  	return 0+ADs
+AD4  +AH0
+AD4 +AFs ... +AF0
+AD4  /+ACoAKg
+AD4 - +ACo flush+AF8-workqueue - ensure that any scheduled work has run to completion.
+AD4 +- +ACo flush+AF8-workqueue+AF8-nested - ensure that any scheduled work has run to completion.
+AD4   +ACo +AEA-wq: workqueue to flush
+AD4 +- +ACo +AEA-subclass: subclass for lockdep
+AD4   +ACo
+AD4   +ACo This function sleeps until all work items which were queued on entry
+AD4   +ACo have finished execution, but it is not livelocked by new incoming ones.
+AD4   +ACo-/
+AD4 -void flush+AF8-workqueue(struct workqueue+AF8-struct +ACo-wq)
+AD4 +-void flush+AF8-workqueue+AF8-nested(struct workqueue+AF8-struct +ACo-wq, int subclass)
+AD4  +AHs
+AD4  	struct wq+AF8-flusher this+AF8-flusher +AD0 +AHs
+AD4  		.list +AD0 LIST+AF8-HEAD+AF8-INIT(this+AF8-flusher.list),
+AD4 +AEAAQA -2652,7 +-2653,7 +AEAAQA void flush+AF8-workqueue(struct workqueue+AF8-struct +ACo-wq)
+AD4  	if (WARN+AF8-ON(+ACE-wq+AF8-online))
+AD4  		return+ADs
+AD4  
+AD4 -	lock+AF8-map+AF8-acquire(+ACY-wq-+AD4-lockdep+AF8-map)+ADs
+AD4 +-	lock+AF8-acquire+AF8-exclusive(+ACY-wq-+AD4-lockdep+AF8-map, subclass, 0, NULL, +AF8-THIS+AF8-IP+AF8)+ADs
+AD4  	lock+AF8-map+AF8-release(+ACY-wq-+AD4-lockdep+AF8-map)+ADs
+AD4  
+AD4  	mutex+AF8-lock(+ACY-wq-+AD4-mutex)+ADs
+AD4 +AFs ... +AF0

I don't like this approach because it doesn't match how other kernel code uses
lockdep annotations. All other kernel code I know of only annotates lock depmaps
as nested if the same kernel thread calls lock+AF8-acquire() twice for the same depmap
without intervening lock+AF8-release(). My understanding is that with your patch
applied flush+AF8-workqueue+AF8-nested(wq, 1) calls lock+AF8-acquire() only once and with the
subclass argument set to one. I think this will confuse other people who will read
the workqueue implementation and who have not followed this conversation.

I like Tejuns proposal much better than the above proposal.

Bart.