RE: [RFC 06/12] genirq: Add per-cpu flow handler with conditional IRQ stats

Michael Kelley
2024-06-05T13:45:29Z

From: Thomas Gleixner <tglx@linutronix.de> Sent: Wednesday, June 5, 2024 6:20 AM
>
> On Tue, Jun 04 2024 at 23:03, Michael Kelley wrote:
> > From: Thomas Gleixner <tglx@linutronix.de> Sent: Tuesday, June 4, 2024 11:14 AM
> >> 1) Move the inner workings of handle_percpu_irq() out into
> >> a static function which returns the 'handled' value and
> >> share it between the two handler functions.
> >
> > The "inner workings" aren't quite the same in the two cases.
> > handle_percpu_irq() uses handle_irq_event_percpu() while
> > handle_percpu_demux_irq() uses __handle_irq_event_percpu().
> > The latter doesn't do add_interrupt_randomness() because the
> > demultiplexed IRQ handler will do it. Doing add_interrupt_randomness()
> > twice doesn't break anything, but it's more overhead in the hard irq
> > path, which I'm trying to avoid. The extra functionality in the
> > non-double-underscore version could be hoisted up to
> > handle_percpu_irq(), but that offsets gains from sharing the
> > inner workings.
>
> That's not rocket science to solve:
>
> static irqreturn_t helper(desc, func)
> {
> boiler_plate..
> ret = func(desc)
> boiler_plate..
> return ret;
> }
>
> No?
>
> TBH, I still hate that conditional accounting :)
>
> >> 2) Allocate a proper interrupt for the management mode and invoke it
> >> via generic_handle_irq() just as any other demultiplex interrupt.
> >> That spares all the special casing in the core code and just
> >> works.
> >
> > Yes, this would work on x86, as the top-level interrupt isn't a Linux IRQ,
> > and the interrupt counting is done in Hyper-V specific code that could be
> > removed. The demux'ed interrupt does the counting.
> >
> > But on arm64 the top-level interrupt *is* a Linux IRQ, so each
> > interrupt will get double-counted, which is a problem.
>
> What is the problem?
>
> You have: toplevel, mgmt, device[], right?
>
> They are all accounted for seperately and each toplevel interrupt might
> result in demultiplexing one or more interrupts (mgmt, device[]), no?
>
> IMO accounting the toplevel interrupt seperately is informative because
> it allows you to figure out whether demultiplexing is clustered or not,
> but I lost that argument long ago. That's why most demultiplex muck
> installs a chained handler, which is a design fail on it's own.

In /proc/interrupts, the double-counting isn't a problem, and is
potentially helpful as you say. But /proc/stat, for example, shows a total
interrupt count, which will be roughly double what it was before. That
/proc/stat value then shows up in user space in vmstat, for example.
That's what I was concerned about, though it's not a huge problem in
the grand scheme of things.

Michael