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?
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.
