On Fri, Mar 13, 2020 at 10:33:25AM +0100, Peter Zijlstra wrote:
> On Thu, Mar 12, 2020 at 11:12:55PM +0800, Boqun Feng wrote:
>
> Thanks!
Thanks Peter and Boqun, the below patch makes sense to me. Just had some nits
below, otherwise:
Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 32406ef0d6a2..5142a6b11bf5 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1719,9 +1719,11 @@ unsigned long lockdep_count_forward_deps(struct lock_class *class)
> > this.class = class;
> >
> > raw_local_irq_save(flags);
> > + current->lockdep_recursion = 1;
> > arch_spin_lock(&lockdep_lock);
> > ret = __lockdep_count_forward_deps(&this);
> > arch_spin_unlock(&lockdep_lock);
> > + current->lockdep_recursion = 0;
> > raw_local_irq_restore(flags);
> >
> > return ret;
> > @@ -1746,9 +1748,11 @@ unsigned long lockdep_count_backward_deps(struct lock_class *class)
> > this.class = class;
> >
> > raw_local_irq_save(flags);
> > + current->lockdep_recursion = 1;
> > arch_spin_lock(&lockdep_lock);
> > ret = __lockdep_count_backward_deps(&this);
> > arch_spin_unlock(&lockdep_lock);
> > + current->lockdep_recursion = 0;
> > raw_local_irq_restore(flags);
> >
> > return ret;
>
> This copies a bad pattern though; all the sites that do not check
> lockdep_recursion_count first really should be using ++/-- instead. But
> I just found there are indeed already a few sites that violate this.
>
> I've taken this patch and done a general fixup on top.
>
> ---
> Subject: locking/lockdep: Fix bad recursion pattern
> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Date: Fri Mar 13 09:56:38 CET 2020
>
> There were two patterns for lockdep_recursion:
>
> Pattern-A:
> if (current->lockdep_recursion)
> return
>
> current->lockdep_recursion = 1;
> /* do stuff */
> current->lockdep_recursion = 0;
>
> Pattern-B:
> current->lockdep_recursion++;
> /* do stuff */
> current->lockdep_recursion--;
>
> But a third pattern has emerged:
>
> Pattern-C:
> current->lockdep_recursion = 1;
> /* do stuff */
> current->lockdep_recursion = 0;
>
> And while this isn't broken per-se, it is highly dangerous because it
> doesn't nest properly.
>
> Get rid of all Pattern-C instances and shore up Pattern-A with a
> warning.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> kernel/locking/lockdep.c | 74 +++++++++++++++++++++++++----------------------
> 1 file changed, 40 insertions(+), 34 deletions(-)
>
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -389,6 +389,12 @@ void lockdep_on(void)
> }
> EXPORT_SYMBOL(lockdep_on);
>
> +static inline void lockdep_recursion_finish(void)
> +{
> + if (WARN_ON_ONCE(--current->lockdep_recursion))
> + current->lockdep_recursion = 0;
> +}
> +
> void lockdep_set_selftest_task(struct task_struct *task)
> {
> lockdep_selftest_task_struct = task;
> @@ -1719,11 +1725,11 @@ unsigned long lockdep_count_forward_deps
> this.class = class;
>
> raw_local_irq_save(flags);
> - current->lockdep_recursion = 1;
> + current->lockdep_recursion++;
> arch_spin_lock(&lockdep_lock);
> ret = __lockdep_count_forward_deps(&this);
> arch_spin_unlock(&lockdep_lock);
> - current->lockdep_recursion = 0;
> + current->lockdep_recursion--;
This doesn't look like it should recurse. Why not just use the
lockdep_recursion_finish() helper here?
> raw_local_irq_restore(flags);
>
> return ret;
> @@ -1748,11 +1754,11 @@ unsigned long lockdep_count_backward_dep
> this.class = class;
>
> raw_local_irq_save(flags);
> - current->lockdep_recursion = 1;
> + current->lockdep_recursion++;
> arch_spin_lock(&lockdep_lock);
> ret = __lockdep_count_backward_deps(&this);
> arch_spin_unlock(&lockdep_lock);
> - current->lockdep_recursion = 0;
> + current->lockdep_recursion--;
And here.
> @@ -4963,7 +4969,7 @@ static void free_zapped_rcu(struct rcu_h
>
> raw_local_irq_save(flags);
> arch_spin_lock(&lockdep_lock);
> - current->lockdep_recursion = 1;
> + current->lockdep_recursion++;
>
> /* closed head */
> pf = delayed_free.pf + (delayed_free.index ^ 1);
> @@ -4975,7 +4981,7 @@ static void free_zapped_rcu(struct rcu_h
> */
> call_rcu_zapped(delayed_free.pf + delayed_free.index);
>
> - current->lockdep_recursion = 0;
> + current->lockdep_recursion--;
And here also if it applies.
> arch_spin_unlock(&lockdep_lock);
> raw_local_irq_restore(flags);
> }
> @@ -5022,11 +5028,11 @@ static void lockdep_free_key_range_reg(v
>
> raw_local_irq_save(flags);
> arch_spin_lock(&lockdep_lock);
> - current->lockdep_recursion = 1;
> + current->lockdep_recursion++;
> pf = get_pending_free();
> __lockdep_free_key_range(pf, start, size);
> call_rcu_zapped(pf);
> - current->lockdep_recursion = 0;
> + current->lockdep_recursion--;
And here also if it applies.
thanks!
- Joel
