Re: [RESEND RFC PATCH v1 1/2] irq/spurious: Reset irqs_unhandled if an irq_thread handles one IRQ request

[Leonardo Bras]

On Wed, Jan 17, 2024 at 07:46:28PM -0300, Leonardo Bras wrote:
> On Wed, Jan 17, 2024 at 11:08:44PM +0100, Thomas Gleixner wrote:
> > On Tue, Jan 16 2024 at 04:36, Leonardo Bras wrote:
> > > This IRQ line disable bug can be easily reproduced with a serial8250
> > > console on a PREEMPT_RT kernel: it only takes the user to print a lot
> > > of text to the console (or to ttyS0): around 300k chars should be
> > > enough.
> > 
> > That has nothing to do with RT, it's a problem of force threaded
> > interrupts in combination with an edge type interrupt line and a
> > hardware which keeps firing interrupts forever.
> 
> Hello Thomas, thanks for your feedback!
> 
> I agreed it has nothing to do with RT.
> I just mentioned PREEMPT_RT as my test case scenario, since it enables 
> force-threaded IRQs.
> 
> > 
> > > To fix this bug, reset irqs_unhandled whenever irq_thread handles at least
> > > one IRQ request.
> > 
> > This papers over the symptom and makes runaway detection way weaker for
> > all interrupts or breaks it completely.
> 
> This change is supposed to only touch threaded interruptions, since it will
> reach the included line only if (action_ret == IRQ_WAKE_THREAD) and if 
> desc->threads_handled changes since the last IRQ request.
> 
> This incrementing also happens only on irq_forced_thread_fn() and 
> irq_thread_fn(), which are called only from irq_thread_fn().
> 
> But I get the overall worry about having this making runaway detection way 
> weaker for all threaded interrupts.
> 
> I have previously worked on a solution that can be more precise and be an 
> opt-in for drivers instead of a general solution:
> 
> It required a change in IRQ interface that let the handlers inform how 
> many IRQs were actually handled (batching). This number would then be 
> added to desc->threads_handle (in irq_*thread_fn(), just changing the 
> atomic_inc() to atomic_add()), and then subtracted from irqs_unhandled
> at note_interrupt().
> 
> In the serial8250 case, the driver would be changed to use that interface, 
> since it's already able to process multiple IRQs, and the bug just 
> vanishes.
> 
> This also solved the serial driver issue, but required a deeper change in 
> the code, which caused me to consider a simpler solution first.
> 
> This solution sure does give better runnaway detection. Do you think it 
> would be better that the one I sent in this patch?

For reference, this is the alternative:
https://gitlab.com/LeoBras/linux/-/commits/serial8250

Please let me know it you think this one is better.

Thanks!
Leo

> 
> > 
> > The problem with edge type interrupts is that we cannot mask them like
> > we do with level type interrupts in the hard interrupt handler and
> > unmask them once the threaded handler finishes.
> > 
> > So yes, we need special rules here when:
> > 
> >    1) The interrupt handler is force threaded
> > 
> >    2) The interrupt line is edge type
> > 
> >    3) The accumulated unhandled interrupts are within a sane margin
> > 
> > Thanks,
> > 
> >         tglx
> > 
> 
> Completelly agree, that's why I am suggesting dealing with threaded 
> interruptions in a different way: reseting the unhandled count when it 
> handles a request. 
> 
> I am not sure how force threaded and just threaded are different in this 
> scenario. Could you help me understand?
> 
> Thanks!
> Leo