On Thu, Apr 27, 2023 at 01:28:15PM +0200, Kevin Wolf wrote: > Am 25.04.2023 um 22:36 hat Eric Blake geschrieben: > > On Tue, Apr 25, 2023 at 07:31:45PM +0200, Kevin Wolf wrote: > > > GRAPH_RDLOCK_GUARD() and GRAPH_RDLOCK_GUARD_MAINLOOP() only take a > > > reader lock for the graph, so the correct annotation for them to use is > > > TSA_ASSERT_SHARED rather than TSA_ASSERT. > > > > The comments at the start of graph-lock.h state that there is only 1 > > writer (main loop, under BQL), and all others are readers (coroutines > > in varous AioContext) - but that's regarding the main BdrvGraphRWlock. > > I think much of that comment is actually unrelated to BdrvGraphRWlock > (which tracks lock/unlock operations of a single thread), but to graph > locking in general.
Yeah, I ended up at the same point - writing the graph vs. grabbing a reader/writer lock as a writer are not the same thing, so the comments at the start of the file are unrelated to the TSA annotations. > > > I guess my confusion is over the act of writing the graph (only in the > > main loop) and using TSA annotations to check for safety. Am I > > correct that the reason graph_lockable_auto_lock() only needs a > > TSA_ASSERT_SHARED locking is that it is only reachable from the other > > threads (and not the main loop thread itself) to check that we are > > indeed in a point where we aren't contending with the main loop's > > writable lock? > > TSA_ASSERT_SHARED is not a precondition requirement, but a postcondition > assertion. That is, callers of the function can assume that they hold > the lock after this function returns. > > This should really be TSA_ACQUIRE_SHARED for graph_lockable_auto_lock(), > but as the comment above it states, this is impossible because TSA > doesn't understand unlocking via the cleanup attribute. clang has really dropped the ball on their cleanup attribute handling; it's been a known issue for years now, and could make their static checking so much more powerful if we didn't have to keep working around it. > > "shared" and "exclusive" in TSA map to "reader" and "writer" lock of a > RWLock. So since we're only taking a reader lock in this function, we > can only assert that the caller now holds a shared lock (which allows > you to call GRAPH_RDLOCK functions), but not an exclusive one like the > code previously suggested (this would allow you to call GRAPH_WRLOCK > functions). > > I'm not sure if this fully addresses your confusion yet. Feel free to > ask if there are more unclear parts. At this point, I don't have any wording suggestions, and appreciate your responses confirming what I arrived at on my own, so my R-b still stands. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
