On Fri, Dec 30, 2022 at 09:58:23PM +0000, Matthew Wilcox wrote:
> On Fri, Dec 30, 2022 at 12:30:12PM -0800, Paul E. McKenney wrote:
> > On Fri, Dec 30, 2022 at 11:38:21AM +0000, Matthew Wilcox wrote:
> > >
> > > <dwmw2> why doesn't lockdep catch us calling synchronize_srcu() with
> > > a lock held, and elsewhere obtaining that lock within an srcu critical
> > > region ("read lock") ?
> > >
> > > Because synchronize_srcu() doesn't acquire the lock, merely checks that
> > > it isn't held.
> > >
> > > Would this work? Not even compile tested.
> > >
> > > You can put my SoB on this if it works.
> > >
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index ca4b5dcec675..e9c2ab8369c0 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -1267,11 +1267,11 @@ static void __synchronize_srcu(struct srcu_struct *ssp, bool do_norm)
> > > {
> > > struct rcu_synchronize rcu;
> > >
> > > - RCU_LOCKDEP_WARN(lockdep_is_held(ssp) ||
> > > - lock_is_held(&rcu_bh_lock_map) ||
> > > + rcu_lock_acquire(&ssp->dep_map);
> >
> > If this does do anything, why doesn't lockdep complain about things
> > like this?
> >
> > i1 = srcu_read_lock(&my_srcu);
> > i2 = srcu_read_lock(&my_srcu);
> > srcu_read_unlock(&my_srcu, i2);
> > srcu_read_unlock(&my_srcu, i1);
> >
> > Or does it in fact complain and I have somehow managed to confuse things
> > such that rcutorture does not detect such complaints? I would expect
> > rcutorture_extend_mask_max() to make this happen, but then again, RCU
> > has confused me in the past.
>
> static inline void rcu_lock_acquire(struct lockdep_map *map)
> {
> lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
> }
>
> That's clear as mud, of course. The fourth argument, which has value
> '2' is documented as 'int read', and that has the meaning:
>
> * Values for "read":
> *
> * 0: exclusive (write) acquire
> * 1: read-acquire (no recursion allowed)
> * 2: read-acquire with same-instance recursion allowed
>
> But that's enough to establish the locking hierarchy. ie if you do:
>
> read-lock-A
> write-lock-B
>
> in one thread and in the other thread do:
>
> write-lock-B
> read-lock-A
>
> lockdep is still going to detect that as a deadly embrace. Isn't it?
Maybe?
> In any case, I was just trying to point out the general outlines of
> something I saw as a potential solution. If we need to turn that into
> lock_acquire(map, 0, 0, 0, 0, NULL, _THIS_IP), that's fine. And Joel
> is right, there needs to be a release in that other path.
>
> But I am not an expert in RCU, lockdep, or working while travelling,
> so I was rather hoping someone like dwmw2 would pick up the sketch,
> make it compile, and then test it.
Sounds like a plan to me!
Thanx, Paul
