Re: [PATCH] irq: fasteoi handler re-runs on concurrent invoke

[Gowans, James]

Hi Marc and Thomas,

On Tue, 2023-04-18 at 12:56 +0200, James Gowans wrote:
> >   static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index 49e7bc871fec..73546ba8bc43 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -692,8 +692,11 @@ void handle_fasteoi_irq(struct irq_desc *desc)
> >          raw_spin_lock(&desc->lock);
> > -       if (!irq_may_run(desc))
> > +       if (!irq_may_run(desc)) {
> > +               if (irqd_needs_resend_when_in_progress(&desc->irq_data))
> > +                       check_irq_resend(desc, true);
> >                  goto out;
> > +       }
> 
> 
> This will run check_irq_resend() on the *newly affined* CPU, while the old
> one is still running the original handler. AFAICT what will happen is:
> check_irq_resend
>   try_retrigger
>     irq_chip_retrigger_hierarchy
>       its_irq_retrigger
> ... which will cause the ITS to *immediately* re-trigger the IRQ. The
> original CPU can still be running the handler in that case.
> 
> If that happens, consider what will happen in check_irq_resend:
> - first IRQ comes in, successflly runs try_retrigger and sets IRQS_REPLAY.
> - it is *immediately* retriggered by ITS, and because the original handler
> on the other CPU is still running, comes into check_irq_resend again.
> - check_irq_resend now observes that IRQS_REPLAY is set and early outs.
> - No more resends, the IRQ is still lost. :-(
> 
> Now I admit the failure mode is getting a bit pathological: two re-
> triggers while the original handler is still running, but I was able to
> hit this on my test machine by intentionally slowing
> the handler down by a few dozen micros. Should we cater for this?
> 
> I can see two possibilities:
> - tweak check_irq_resend() to not early-out in this case but to keep re-
> triggering until it eventually runs.
> - move the check_irq_resend to only happen later, *after* the original
> handler has finished running. This would be very similar to what I
> suggested in my original patch, except instead of running a do/while loop,
> the code would observe that the pending flag was set again and run
> check_irq_resend.
> 
> I'm also wondering what will happen for users who don't have the
> chip->irq_retrigger callback set and fall back to the tasklet
> via irq_sw_resend()... Looks like it will work fine. However if we do my
> suggestion and move check_irq_resend to the end of handle_fasteoi_irq then
> the tasklet will be scheduled on the old CPU again, which may be sub-
> optimal.

Just checking to see if you've had a chance to consider these
issues/thoughts, and if/how they should be handled?
I'm still tending towards saying that the check_irq_resend() should run
after handle_irq_event() and the IRQS_PENDING flag should be wrangled to
decide whether or not to resend.

I just don't know if having the tasklet scheduled and run on the original
CPU via irq_sw_resend() would be problematic or not. In general it
probably won't but in the CPU offlining case.... maybe? I realise that for
GIC-v3 the tasklet won't be used because GIC has chip->irq_retrigger
callback defined - I'm just thinking in general here, especially so
assuming we drop the new IRQD_RESEND_WHEN_IN_PROGRESS flag).

Thoughts?

I can put together a PoC and test it along with Yipeng from Huawei if you
think it sounds reasonable.

JG