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

From: Bart Van Assche
Date: Thu Oct 25 2018 - 13:22:48 EST


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

How about modifying the wq-+AD4-lockdep+AF8-map annotations only and not touching the
work-+AD4-lockdep+AF8-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.