Messages in this thread | | | Date | Tue, 26 May 2020 11:45:26 +0200 | From | "Ahmed S. Darwish" <> | Subject | Re: [PATCH v1 07/25] lockdep: Add preemption disabled assertion API |
| |
On Tue, May 26, 2020 at 10:13:50AM +0200, Peter Zijlstra wrote: > On Tue, May 26, 2020 at 02:52:31AM +0200, Ahmed S. Darwish wrote: > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > +#define lockdep_assert_irqs_enabled() \ > > > +do { \ > > > + WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled)); \ > > > +} while (0) > > > > > > > Given that lockdep_off() is defined at lockdep.c as: > > > > void lockdep_off(void) > > { > > current->lockdep_recursion += LOCKDEP_OFF; > > } > > > > This would imply that all of the macros: > > > > - lockdep_assert_irqs_enabled() > > - lockdep_assert_irqs_disabled() > > - lockdep_assert_in_irq() > > - lockdep_assert_preemption_disabled() > > - lockdep_assert_preemption_enabled() > > > > will do the lockdep checks *even if* lockdep_off() was called. > > > > This doesn't sound right. Even if all of the above macros call sites > > didn't care about lockdep_off()/on(), it is semantically incoherent. > > lockdep_off() is an abomination and really should not exist. > > That dm-cache-target.c thing, for example, is atrocious shite that will > explode on -rt. Whoever wrote that needs a 'medal'. > > People using it deserve all the pain they get. > > Also; IRQ state _should_ be tracked irrespective of tracking lock > dependencies -- I see that that currently isn't entirely the case, lemme > go fix that. >
Exactly, currently all the lockdep IRQ checks gets nullified if lockdep_off() is called. That was the source of my confusion.
If you'll have any extra patches on this, I can also queue them in the next iteration of this series, before this patch.
Thanks a lot,
-- Ahmed S. Darwish Linutronix GmbH
| |