From: Johannes Berg <email@example.com> To: Bart Van Assche <firstname.lastname@example.org>, Tejun Heo <email@example.com> Cc: "firstname.lastname@example.org" <email@example.com>, Christoph Hellwig <firstname.lastname@example.org>, Sagi Grimberg <email@example.com>, "firstname.lastname@example.org" <email@example.com> Subject: Re: [PATCH 2/3] kernel/workqueue: Surround work execution with shared lock annotations Date: Thu, 25 Oct 2018 21:17:29 +0200 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> (sfid-20181025_192247_142973_0CC4C4B6) On Thu, 2018-10-25 at 10:22 -0700, Bart Van Assche wrote: > > 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. Again, I'm not sure what you'd want to achieve by that? If you look at lockdep, read==1 really doesn't do all that much. Note read is the 4th arg to lock_acquire() via lock_acquire_shared(). Hold time statistics are accounted differently, but that's meaningless here. check_deadlock() will behave differently - if read==2 (recursive read) it will not complain about recursing into a previous read==1 acquire. Not relevant here, because you can't recurse into the workqueue while you're running on it. Some in-IRQ things are different, but we can't be in-IRQ here. Note that lockdep doesn't care about multiple threads "holding" the same lock. Everything it does in terms of this tracking is per-thread, afaict (uses "current" everywhere, and that's how the reports look like etc.). So ultimately the only places where this read==1 would matter cannot be hit (recursion within the same thread). Importantly, it doesn't try to track across the whole system if you're holding the same lock twice. That's actually impossible with most locks anyway (one would wait), but when it does happen like it can here, it doesn't really do anything bad. I don't think it actually *breaks* anything to make this a read/write lock, because even with a read/write lock you still get a deadlock if you do read(A) lock(B) write(A) lock(B) or write(A) lock(B) read(A) lock(B) since read/write are mutually exclusive, and you're not changing the flush-side to also be a readlock. If you were, I'd argue that'd be wrong since then you *don't* detect such deadlocks: read(A) lock(B) read(A) lock(B) would not result in a complaint (I think), but in the workqueue case it *should* because you can't flush the workqueue while holding a lock that you also hold in a work that's executing on it. So in the end, I just simply don't see a point in this, and I don't think you've sufficiently explained why this change should be made. johannes
next prev parent reply other threads:[~2018-10-25 19:17 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-25 15:05 [PATCH 0/3] Suppress false positives triggered by workqueue lockdep annotations Bart Van Assche 2018-10-25 15:05 ` [PATCH 1/3] kernel/workqueue: Remove lockdep annotation from __flush_work() Bart Van Assche 2018-10-25 15:31 ` Johannes Berg 2018-10-25 15:57 ` Johannes Berg 2018-10-25 16:01 ` Bart Van Assche 2018-10-25 15:05 ` [PATCH 2/3] kernel/workqueue: Surround work execution with shared lock annotations Bart Van Assche 2018-10-25 16:53 ` Johannes Berg 2018-10-25 17:22 ` Bart Van Assche 2018-10-25 19:17 ` Johannes Berg [this message] 2018-10-25 15:05 ` [PATCH 3/3] kernel/workqueue: Suppress a false positive lockdep complaint Bart Van Assche 2018-10-25 15:34 ` Johannes Berg 2018-10-25 15:55 ` Bart Van Assche 2018-10-25 19:59 ` Johannes Berg 2018-10-25 20:21 ` Theodore Y. Ts'o 2018-10-25 20:26 ` Johannes Berg 2018-10-25 15:36 ` Tejun Heo 2018-10-25 15:37 ` Tejun Heo 2018-10-25 20:13 ` Johannes Berg 2018-10-25 15:40 ` Theodore Y. Ts'o 2018-10-25 17:02 ` Johannes Berg 2018-10-25 17:11 ` Bart Van Assche 2018-10-25 19:51 ` Johannes Berg 2018-10-25 20:39 ` Bart Van Assche 2018-10-25 20:47 ` Johannes Berg 2018-10-25 15:27 ` [PATCH 0/3] Suppress false positives triggered by workqueue lockdep annotations Johannes Berg 2018-10-25 15:47 ` Bart Van Assche
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 2/3] kernel/workqueue: Surround work execution with shared lock annotations' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).