Re: [patch 23/39] PCI/MSI: Move pci_alloc_irq_vectors_affinity() to api.c
On Wed, Nov 16, 2022 at 10:23:22AM -0600, Bjorn Helgaas wrote: > On Fri, Nov 11, 2022 at 02:54:51PM +0100, Thomas Gleixner wrote: ... > > + > > +/** > > + * pci_alloc_irq_vectors_affinity() - Allocate multiple device interrupt > > + *vectors with affinity requirements > > + * @dev: the PCI device to operate on > > + * @min_vecs: minimum required number of vectors (must be >= 1) > > + * @max_vecs: maximum desired number of vectors > > + * @flags:allocation flags, as in pci_alloc_irq_vectors() > > + * @affd: affinity requirements (can be %NULL). > > + * > > + * Same as pci_alloc_irq_vectors(), but with the extra @affd parameter. > > + * Check that function docs, and irq_affinity, for more details. > > Is " irq_affinity" some kernel-doc syntax, or is the "&" > superfluous? > Hmmm, I stole it from Documentation/doc-guide/kernel-doc.rst. htmldoc parses it and generates a link to the referenced structure's kernel-doc. But, yeah, this was literally the first usage of such a doc pattern in the entire kernel's C code :) Thanks, -- Ahmed S. Darwish Linutronix GmbH
Re: [PATCH v4 7/8] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables
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
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
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
