Re: [PATCH v4 7/8] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables

2020-06-30 Thread Ahmed S. Darwish
Peter Zijlstra wrote:

...

> -#define lockdep_assert_irqs_disabled()   do {\
> - WARN_ONCE(debug_locks && !current->lockdep_recursion && \
> -   current->hardirqs_enabled,\
> -   "IRQs not disabled as expected\n");   \
> - } while (0)

...

> +#define lockdep_assert_irqs_disabled()   \
> +do { \
> + WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled));   \
> +} while (0)

I think it would be nice to keep the "IRQs not disabled as expected"
message. It makes the lockdep splat much more readable.

This is similarly the case for the v3 lockdep preemption macros:

  https://lkml.kernel.org/r/20200630054452.3675847-5-a.darw...@linutronix.de

I did not add a message though to get in-sync with the IRQ macros above.

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH


Re: [PATCH v4 7/8] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables

2020-06-23 Thread Ahmed S. Darwish
On Tue, Jun 23, 2020 at 05:24:50PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 23, 2020 at 05:00:31PM +0200, Ahmed S. Darwish wrote:
> > On Tue, Jun 23, 2020 at 10:36:52AM +0200, Peter Zijlstra wrote:
> > ...
> > > -#define lockdep_assert_irqs_disabled()   do {
> > > \
> > > - WARN_ONCE(debug_locks && !current->lockdep_recursion && \
> > > -   current->hardirqs_enabled,\
> > > -   "IRQs not disabled as expected\n");   \
> > > - } while (0)
> > > +#define lockdep_assert_irqs_enabled()
> > > \
> > > +do { 
> > > \
> > > + WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled));  \
> > > +} while (0)
> > >
> >
> > Can we add a small comment on top of lockdep_off(), stating that lockdep
> > IRQ tracking will still be kept after a lockdep_off call?
>
> That would only legitimize lockdep_off(). The only comment I want to put
> on that is: "if you use this, you're doing it wrong'.
>

Well, freshly merged code is using it. For example, KCSAN:

=> f1bc96210c6a ("kcsan: Make KCSAN compatible with lockdep")
=> kernel/kcsan/report.c:

void kcsan_report(...)
{
...
/*
 * With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
 * we do not turn off lockdep here; this could happen due to recursion
 * into lockdep via KCSAN if we detect a race in utilities used by
 * lockdep.
 */
lockdep_off();
...
}

thanks,

--
Ahmed S. Darwish
Linutronix GmbH


Re: [PATCH v4 7/8] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables

2020-06-23 Thread Ahmed S. Darwish
On Tue, Jun 23, 2020 at 10:36:52AM +0200, Peter Zijlstra wrote:
...
> -#define lockdep_assert_irqs_disabled()   do {
> \
> - WARN_ONCE(debug_locks && !current->lockdep_recursion && \
> -   current->hardirqs_enabled,\
> -   "IRQs not disabled as expected\n");   \
> - } while (0)
> +#define lockdep_assert_irqs_enabled()
> \
> +do { \
> + WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled));  \
> +} while (0)
>

Can we add a small comment on top of lockdep_off(), stating that lockdep
IRQ tracking will still be kept after a lockdep_off call?

thanks,

--
Ahmed S. Darwish
Linutronix GmbH