[Question] srcu: is it making sense to recursively invoke srcu_read_lock?

[Question] srcu: is it making sense to recursively invoke srcu_read_lock?

[Hillf Danton]

Given rcu_lock_acquire() in srcu_read_lock(),

	iA = srcu_read_lock(foo);
	iB = srcu_read_lock(foo); // not bar
	...
	srcu_read_unlock(foo, iB);
	srcu_read_unlock(foo, iA);

can the call sequence above trigger warning with CONFIG_DEBUG_LOCK_ALLOC enabled?

Does it make sense to add srcu_lock_acquire() in line with rwsem_acquire_read() if
warning is expected but not triggered?

Thanks
Hillf

static inline void rcu_lock_acquire(struct lockdep_map *map)
{
	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
}

static inline void srcu_lock_acquire(struct lockdep_map *map)
{
	lock_acquire(map, 0, 0, 1, 0, NULL, _THIS_IP_);
}

Re: [Question] srcu: is it making sense to recursively invoke srcu_read_lock?

[Paul E. McKenney]

On Thu, Apr 21, 2022 at 12:22:11PM +0800, Hillf Danton wrote:
> Given rcu_lock_acquire() in srcu_read_lock(),
> 
> 	iA = srcu_read_lock(foo);
> 	iB = srcu_read_lock(foo); // not bar
> 	...
> 	srcu_read_unlock(foo, iB);
> 	srcu_read_unlock(foo, iA);
> 
> can the call sequence above trigger warning with CONFIG_DEBUG_LOCK_ALLOC enabled?

I hope not!  After all, nesting SRCU read-side critical sections is
perfectly legal.  But why not just try it and see?

> Does it make sense to add srcu_lock_acquire() in line with rwsem_acquire_read() if
> warning is expected but not triggered?

Please understand that while SRCU can often be used where an rwsem
might otherwise be used, SRCU is not an rwsem.  For one thing, rwsem
readers can deadlock in ways that SRCU reader cannot.

Now, I don't yet know of a non-destructive use case for partially
overlapping SRCU read-side critical sections, for example, if you
switched the two srcu_read_unlock() calls above.  But at the same
time, I cannot prove that there is no valid use case, not yet,
anyway.

						Thanx, Paul

> Thanks
> Hillf
> 
> static inline void rcu_lock_acquire(struct lockdep_map *map)
> {
> 	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
> }
> 
> static inline void srcu_lock_acquire(struct lockdep_map *map)
> {
> 	lock_acquire(map, 0, 0, 1, 0, NULL, _THIS_IP_);
> }

Re: [Question] srcu: is it making sense to recursively invoke srcu_read_lock?

[Paul E. McKenney]

On Thu, Apr 21, 2022 at 06:34:14AM -0700, Paul E. McKenney wrote:
> On Thu, Apr 21, 2022 at 12:22:11PM +0800, Hillf Danton wrote:
> > Given rcu_lock_acquire() in srcu_read_lock(),
> > 
> > 	iA = srcu_read_lock(foo);
> > 	iB = srcu_read_lock(foo); // not bar
> > 	...
> > 	srcu_read_unlock(foo, iB);
> > 	srcu_read_unlock(foo, iA);
> > 
> > can the call sequence above trigger warning with CONFIG_DEBUG_LOCK_ALLOC enabled?
> 
> I hope not!  After all, nesting SRCU read-side critical sections is
> perfectly legal.  But why not just try it and see?
> 
> > Does it make sense to add srcu_lock_acquire() in line with rwsem_acquire_read() if
> > warning is expected but not triggered?
> 
> Please understand that while SRCU can often be used where an rwsem
> might otherwise be used, SRCU is not an rwsem.  For one thing, rwsem
> readers can deadlock in ways that SRCU reader cannot.
> 
> Now, I don't yet know of a non-destructive use case for partially
> overlapping SRCU read-side critical sections, for example, if you
> switched the two srcu_read_unlock() calls above.  But at the same
> time, I cannot prove that there is no valid use case, not yet,
> anyway.

But I do see one now.

Imagine someone passing a callback to another function, with an SRCU
read-side critical section starting before this other function is invoked
and ending within the callback.  Suppose that this other function also
uses SRCU to protect the invocation of the callback.  The two partially
overlapping SRCU read-side critical sections are independent, so they
are OK.

When the critical sections are not independent, trouble ensues:
https://paulmck.livejournal.com/40593.html

						Thanx, Paul
> 
> > Thanks
> > Hillf
> > 
> > static inline void rcu_lock_acquire(struct lockdep_map *map)
> > {
> > 	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
> > }
> > 
> > static inline void srcu_lock_acquire(struct lockdep_map *map)
> > {
> > 	lock_acquire(map, 0, 0, 1, 0, NULL, _THIS_IP_);
> > }

Re: [Question] srcu: is it making sense to recursively invoke srcu_read_lock?

[Hillf Danton]

On Thu, 21 Apr 2022 06:34:14 -0700 Paul E. McKenney wrote:
> On Thu, Apr 21, 2022 at 12:22:11PM +0800, Hillf Danton wrote:
> > Given rcu_lock_acquire() in srcu_read_lock(),
> > 
> > 	iA = srcu_read_lock(foo);
> > 	iB = srcu_read_lock(foo); // not bar
> > 	...
> > 	srcu_read_unlock(foo, iB);
> > 	srcu_read_unlock(foo, iA);
> > 
> > can the call sequence above trigger warning with CONFIG_DEBUG_LOCK_ALLOC enabled?
> 
> I hope not!  After all, nesting SRCU read-side critical sections is
> perfectly legal.  But why not just try it and see?

Thanks for shedding light on nested SRCUs - it cures my pain working out
the reason for how detecting nested SRCUs is added [1]. Now I see why it
is out of kernel/rcu.

Hillf

[1] https://lore.kernel.org/lkml/20220415004343.2203171-4-seanjc@google.com/

> > Does it make sense to add srcu_lock_acquire() in line with rwsem_acquire_read() if
> > warning is expected but not triggered?
> 
> Please understand that while SRCU can often be used where an rwsem
> might otherwise be used, SRCU is not an rwsem.  For one thing, rwsem
> readers can deadlock in ways that SRCU reader cannot.
> 
> Now, I don't yet know of a non-destructive use case for partially
> overlapping SRCU read-side critical sections, for example, if you
> switched the two srcu_read_unlock() calls above.  But at the same
> time, I cannot prove that there is no valid use case, not yet,
> anyway.
> 
> 						Thanx, Paul
> 
> > Thanks
> > Hillf
> > 
> > static inline void rcu_lock_acquire(struct lockdep_map *map)
> > {
> > 	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
> > }
> > 
> > static inline void srcu_lock_acquire(struct lockdep_map *map)
> > {
> > 	lock_acquire(map, 0, 0, 1, 0, NULL, _THIS_IP_);
> > }
> 

Re: [Question] srcu: is it making sense to recursively invoke srcu_read_lock?

[Paul E. McKenney]

On Fri, Apr 22, 2022 at 08:52:12AM +0800, Hillf Danton wrote:
> On Thu, 21 Apr 2022 06:34:14 -0700 Paul E. McKenney wrote:
> > On Thu, Apr 21, 2022 at 12:22:11PM +0800, Hillf Danton wrote:
> > > Given rcu_lock_acquire() in srcu_read_lock(),
> > > 
> > > 	iA = srcu_read_lock(foo);
> > > 	iB = srcu_read_lock(foo); // not bar
> > > 	...
> > > 	srcu_read_unlock(foo, iB);
> > > 	srcu_read_unlock(foo, iA);
> > > 
> > > can the call sequence above trigger warning with CONFIG_DEBUG_LOCK_ALLOC enabled?
> > 
> > I hope not!  After all, nesting SRCU read-side critical sections is
> > perfectly legal.  But why not just try it and see?
> 
> Thanks for shedding light on nested SRCUs - it cures my pain working out
> the reason for how detecting nested SRCUs is added [1]. Now I see why it
> is out of kernel/rcu.

Just to be clear...  If the KVM guys want to impose a design rule that
SRCU read-side critical sections never be nested within their code,
that is a perfectly reasonable thing for them to do.

							Thanx, Paul

> Hillf
> 
> [1] https://lore.kernel.org/lkml/20220415004343.2203171-4-seanjc@google.com/
> 
> > > Does it make sense to add srcu_lock_acquire() in line with rwsem_acquire_read() if
> > > warning is expected but not triggered?
> > 
> > Please understand that while SRCU can often be used where an rwsem
> > might otherwise be used, SRCU is not an rwsem.  For one thing, rwsem
> > readers can deadlock in ways that SRCU reader cannot.
> > 
> > Now, I don't yet know of a non-destructive use case for partially
> > overlapping SRCU read-side critical sections, for example, if you
> > switched the two srcu_read_unlock() calls above.  But at the same
> > time, I cannot prove that there is no valid use case, not yet,
> > anyway.
> > 
> > 						Thanx, Paul
> > 
> > > Thanks
> > > Hillf
> > > 
> > > static inline void rcu_lock_acquire(struct lockdep_map *map)
> > > {
> > > 	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
> > > }
> > > 
> > > static inline void srcu_lock_acquire(struct lockdep_map *map)
> > > {
> > > 	lock_acquire(map, 0, 0, 1, 0, NULL, _THIS_IP_);
> > > }
> >