| Message ID | 20230425173158.574203-8-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Graph locking, part 3 (more block drivers) | expand |
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 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? > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/graph-lock.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h > index 7ef391fab3..adaa3ed089 100644 > --- a/include/block/graph-lock.h > +++ b/include/block/graph-lock.h > @@ -210,7 +210,7 @@ typedef struct GraphLockable { } GraphLockable; > * unlocked. TSA_ASSERT() makes sure that the following calls know that we > * hold the lock while unlocking is left unchecked. > */ > -static inline GraphLockable * TSA_ASSERT(graph_lock) TSA_NO_TSA > +static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA > graph_lockable_auto_lock(GraphLockable *x) > { > bdrv_graph_co_rdlock(); But the act of grabbing a rdlock is definitely a SHARED not EXCLUSIVE action, so I think I agree with your changing of the annotation, even if the commit message could be slightly improved. Assuming I've sorted out my own confusion on the various lock terminoligies, Reviewed-by: Eric Blake <eblake@redhat.com>
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. > 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. "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. Kevin
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.
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. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/graph-lock.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h > index 7ef391fab3..adaa3ed089 100644 > --- a/include/block/graph-lock.h > +++ b/include/block/graph-lock.h > @@ -210,7 +210,7 @@ typedef struct GraphLockable { } GraphLockable; > * unlocked. TSA_ASSERT() makes sure that the following calls know that we Does this comment need to be updated to TSA_ASSERT_SHARED()? > * hold the lock while unlocking is left unchecked. > */ > -static inline GraphLockable * TSA_ASSERT(graph_lock) TSA_NO_TSA > +static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA > graph_lockable_auto_lock(GraphLockable *x) > { > bdrv_graph_co_rdlock(); > @@ -254,7 +254,7 @@ typedef struct GraphLockableMainloop { } GraphLockableMainloop; > * unlocked. TSA_ASSERT() makes sure that the following calls know that we Same.
diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h index 7ef391fab3..adaa3ed089 100644 --- a/include/block/graph-lock.h +++ b/include/block/graph-lock.h @@ -210,7 +210,7 @@ typedef struct GraphLockable { } GraphLockable; * unlocked. TSA_ASSERT() makes sure that the following calls know that we * hold the lock while unlocking is left unchecked. */ -static inline GraphLockable * TSA_ASSERT(graph_lock) TSA_NO_TSA +static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA graph_lockable_auto_lock(GraphLockable *x) { bdrv_graph_co_rdlock(); @@ -254,7 +254,7 @@ typedef struct GraphLockableMainloop { } GraphLockableMainloop; * unlocked. TSA_ASSERT() makes sure that the following calls know that we * hold the lock while unlocking is left unchecked. */ -static inline GraphLockableMainloop * TSA_ASSERT(graph_lock) TSA_NO_TSA +static inline GraphLockableMainloop * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA graph_lockable_auto_lock_mainloop(GraphLockableMainloop *x) { bdrv_graph_rdlock_main_loop();
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. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/graph-lock.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)