[RFC,v2,3/4] irq: Introduce IRQ_HANDLED_MANY

  In threaded IRQs, some irq handlers are able to handle many requests at a
single run, but this is only accounted as a single IRQ_HANDLED when
increasing threads_handled.

In order to fix this, introduce IRQ_HANDLED_MANY, so the returned value of
those IRQ handlers are able to signal that many IRQs got handled at that
run.

Is scenarios where there is no need to keep track of IRQ handled, convert
it back to IRQ_HANDLED.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 include/linux/irqreturn.h | 23 ++++++++++++++++++++++-
 kernel/irq/chip.c         | 10 ++++++++--
 kernel/irq/handle.c       |  3 +++
 kernel/irq/manage.c       |  4 ++++
 4 files changed, 37 insertions(+), 3 deletions(-)
  

On Fri, Feb 16 2024 at 04:59, Leonardo Bras wrote:

> In threaded IRQs, some irq handlers are able to handle many requests at a
> single run, but this is only accounted as a single IRQ_HANDLED when
> increasing threads_handled.
>
> In order to fix this, introduce IRQ_HANDLED_MANY, so the returned value of
> those IRQ handlers are able to signal that many IRQs got handled at that
> run.
>
> Is scenarios where there is no need to keep track of IRQ handled, convert
> it back to IRQ_HANDLED.

That's not really workable as you'd have to update tons of drivers just
to deal with that corner case. That's error prone and just extra
complexity all over the place.

This really needs to be solved in the core code.

Thanks,

        tglx
  

On Mon, Feb 19 2024 at 10:59, Thomas Gleixner wrote:
> On Fri, Feb 16 2024 at 04:59, Leonardo Bras wrote:
>
>> In threaded IRQs, some irq handlers are able to handle many requests at a
>> single run, but this is only accounted as a single IRQ_HANDLED when
>> increasing threads_handled.
>>
>> In order to fix this, introduce IRQ_HANDLED_MANY, so the returned value of
>> those IRQ handlers are able to signal that many IRQs got handled at that
>> run.
>>
>> Is scenarios where there is no need to keep track of IRQ handled, convert
>> it back to IRQ_HANDLED.
>
> That's not really workable as you'd have to update tons of drivers just
> to deal with that corner case. That's error prone and just extra
> complexity all over the place.
>
> This really needs to be solved in the core code.

Something like the uncompiled below should do the trick.

Thanks,

        tglx
---
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -38,7 +38,8 @@ struct pt_regs;
  * @affinity_notify:	context for notification of affinity changes
  * @pending_mask:	pending rebalanced interrupts
  * @threads_oneshot:	bitfield to handle shared oneshot threads
- * @threads_active:	number of irqaction threads currently running
+ * @threads_active:	number of irqaction threads currently activated
+ * @threads_running:	number of irqaction threads currently running
  * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
  * @nr_actions:		number of installed actions on this descriptor
  * @no_suspend_depth:	number of irqactions on a irq descriptor with
@@ -80,6 +81,7 @@ struct irq_desc {
 #endif
 	unsigned long		threads_oneshot;
 	atomic_t		threads_active;
+	atomic_t		threads_running;
 	wait_queue_head_t       wait_for_threads;
 #ifdef CONFIG_PM_SLEEP
 	unsigned int		nr_actions;
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1194,9 +1194,11 @@ irq_forced_thread_fn(struct irq_desc *de
 	local_bh_disable();
 	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
 		local_irq_disable();
+	atomic_inc(&desc->threads_running);
 	ret = action->thread_fn(action->irq, action->dev_id);
 	if (ret == IRQ_HANDLED)
 		atomic_inc(&desc->threads_handled);
+	atomic_dec(&desc->threads_running);
 
 	irq_finalize_oneshot(desc, action);
 	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -350,6 +350,12 @@ void note_interrupt(struct irq_desc *des
 				desc->threads_handled_last = handled;
 			} else {
 				/*
+				 * Avoid false positives when there is
+				 * actually a thread running.
+				 */
+				if (atomic_read(&desc->threads_running))
+					return;
+				/*
 				 * None of the threaded handlers felt
 				 * responsible for the last interrupt
 				 *
  

On Mon, Feb 19, 2024 at 12:03:07PM +0100, Thomas Gleixner wrote:
> On Mon, Feb 19 2024 at 10:59, Thomas Gleixner wrote:

Hi Thomas, thanks for reviewing!

> > On Fri, Feb 16 2024 at 04:59, Leonardo Bras wrote:
> >
> >> In threaded IRQs, some irq handlers are able to handle many requests at a
> >> single run, but this is only accounted as a single IRQ_HANDLED when
> >> increasing threads_handled.
> >>
> >> In order to fix this, introduce IRQ_HANDLED_MANY, so the returned value of
> >> those IRQ handlers are able to signal that many IRQs got handled at that
> >> run.
> >>
> >> Is scenarios where there is no need to keep track of IRQ handled, convert
> >> it back to IRQ_HANDLED.
> >
> > That's not really workable as you'd have to update tons of drivers just
> > to deal with that corner case. That's error prone and just extra
> > complexity all over the place.

I agree, that's a downside of this implementation. 


> >
> > This really needs to be solved in the core code.
> 
> Something like the uncompiled below should do the trick.
> 
> Thanks,
> 
>         tglx
> ---
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -38,7 +38,8 @@ struct pt_regs;
>   * @affinity_notify:	context for notification of affinity changes
>   * @pending_mask:	pending rebalanced interrupts
>   * @threads_oneshot:	bitfield to handle shared oneshot threads
> - * @threads_active:	number of irqaction threads currently running
> + * @threads_active:	number of irqaction threads currently activated
> + * @threads_running:	number of irqaction threads currently running
>   * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
>   * @nr_actions:		number of installed actions on this descriptor
>   * @no_suspend_depth:	number of irqactions on a irq descriptor with
> @@ -80,6 +81,7 @@ struct irq_desc {
>  #endif
>  	unsigned long		threads_oneshot;
>  	atomic_t		threads_active;
> +	atomic_t		threads_running;
>  	wait_queue_head_t       wait_for_threads;
>  #ifdef CONFIG_PM_SLEEP
>  	unsigned int		nr_actions;
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1194,9 +1194,11 @@ irq_forced_thread_fn(struct irq_desc *de
>  	local_bh_disable();
>  	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
>  		local_irq_disable();
> +	atomic_inc(&desc->threads_running);
>  	ret = action->thread_fn(action->irq, action->dev_id);
>  	if (ret == IRQ_HANDLED)
>  		atomic_inc(&desc->threads_handled);
> +	atomic_dec(&desc->threads_running);
>  
>  	irq_finalize_oneshot(desc, action);
>  	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> --- a/kernel/irq/spurious.c
> +++ b/kernel/irq/spurious.c
> @@ -350,6 +350,12 @@ void note_interrupt(struct irq_desc *des
>  				desc->threads_handled_last = handled;
>  			} else {
>  				/*
> +				 * Avoid false positives when there is
> +				 * actually a thread running.
> +				 */
> +				if (atomic_read(&desc->threads_running))
> +					return;
> +				/*
>  				 * None of the threaded handlers felt
>  				 * responsible for the last interrupt
>  				 *
> 

I agree the above may be able to solve the issue, but it would make 2 extra 
atomic ops necessary in the thread handling the IRQ, as well as one extra 
atomic operation in note_interrupt(), which could increase latency on this 
IRQ deferring the handler to a thread.

I mean, yes, the cpu running note_interrupt() would probably already have 
exclusiveness for this cacheline, but it further increases cacheline 
bouncing and also adds the mem barriers that incur on atomic operations, 
even if we use an extra bit from threads_handled instead of allocate a new 
field for threads_running. 


On top of that, let's think on a scenario where the threaded handler will 
solve a lot of requests, but not necessarily spend a lot of time doing so.
This allows the thread to run for little time while solving a lot of 
requests.

In this scenario, note_interrupt() could return without incrementing 
irqs_unhandled for those IRQ that happen while the brief thread is running, 
but every other IRQ would cause note_interrupt() to increase 
irqs_unhandled, which would cause the bug to still reproduce.


I understand my suggested change increases irq_return complexity, but it
should not increase much of the overhead in both IRQ deferring and IRQ 
thread handling. Also, since it accounts for every IRQ actually handled, it 
does not matter how long the handler thread runs, it still avoids the bug.

As you previously noted, the main issue in my suggestion is about changing 
drivers' code. But while this change is optional, I wonder how many 
drivers handle IRQs that:
- use edge type interrupts, and
- can trigger really fast, many many times, and
- can run in force-thread mode, and
- have handlers that are able to deal with multiple IRQs per call.

If there aren't many that met this requirement, I could try to tackle them 
as they become a problem.


About solving it directly in generic code, I agree it would be the ideal 
scenario. That's why I suggested that in RFCv1: if the thread handles a 
single IRQ, we reset irqs_unhandled to zero. That's a good pointer on the 
thread being stuck, and TBH I don't think this is too far away from the 
current 100/100k if we consider some of those handlers can handle multiple 
IRQs at once.


What are your thoughts on that?

Thanks!
Leo
  

On Wed, Feb 21 2024 at 02:39, Leonardo Bras wrote:
> On Mon, Feb 19, 2024 at 12:03:07PM +0100, Thomas Gleixner wrote:
>> >> Is scenarios where there is no need to keep track of IRQ handled, convert
>> >> it back to IRQ_HANDLED.
>> >
>> > That's not really workable as you'd have to update tons of drivers just
>> > to deal with that corner case. That's error prone and just extra
>> > complexity all over the place.
>
> I agree, that's a downside of this implementation. 

A serious one which is not really workable. See below.

> I agree the above may be able to solve the issue, but it would make 2 extra 
> atomic ops necessary in the thread handling the IRQ, as well as one extra 
> atomic operation in note_interrupt(), which could increase latency on this 
> IRQ deferring the handler to a thread.
>
> I mean, yes, the cpu running note_interrupt() would probably already have 
> exclusiveness for this cacheline, but it further increases cacheline 
> bouncing and also adds the mem barriers that incur on atomic operations, 
> even if we use an extra bit from threads_handled instead of allocate a new 
> field for threads_running.

I think that's a strawman. Atomic operations can of course be more
expensive than non-atomic ones, but they only start to make a difference
when the cache line is contended. That's not the case here for the
normal operations.

Interrupts and their threads are strictly targeted to a single CPU and
the cache line is already hot and had to be made exclusive because of
other write operations to it.

There is usually no concurrency at all, except for administrative
operations like enable/disable or affinity changes. Those administrative
operations are not high frequency and the resulting cache line bouncing
is unavoidable even without that change. But does it matter in the
larger picture? I don't think so.

> On top of that, let's think on a scenario where the threaded handler will 
> solve a lot of requests, but not necessarily spend a lot of time doing so.
> This allows the thread to run for little time while solving a lot of 
> requests.
>
> In this scenario, note_interrupt() could return without incrementing 
> irqs_unhandled for those IRQ that happen while the brief thread is running, 
> but every other IRQ would cause note_interrupt() to increase 
> irqs_unhandled, which would cause the bug to still reproduce.

In theory yes. Does it happen in practice?

But that exposes a flaw in the actual detection code. The code is
unconditionally accumulating if there is an unhandled interrupt within
100ms after the last unhandled one. IOW, if there is a periodic
unhandled one every 50ms, the interrupt will be shut down after 100000 *
50ms = 5000s ~= 83.3m ~= 1.4h. And it neither cares about the number of
actually handled interrupts.

The spurious detector is really about runaway interrupts which hog a CPU
completely, but the above is not what we want to protect against.

> I understand my suggested change increases irq_return complexity, but it
> should not increase much of the overhead in both IRQ deferring and IRQ 
> thread handling. Also, since it accounts for every IRQ actually handled, it 
> does not matter how long the handler thread runs, it still avoids the
> bug.

I'm not worried about the extra complexity in note_interrupt(). That's
fine and I don't have a strong opinion whether using your patch 2/4 or
the simpler one I suggested.

> As you previously noted, the main issue in my suggestion is about changing 
> drivers' code. But while this change is optional, I wonder how many 
> drivers handle IRQs that:
> - use edge type interrupts, and

It's not up to the driver to decide that. That's a platform property and
the driver does not even know and they should not care because all what
matters for the driver is that it gets the interrupts and does not lose
anything.

> - can trigger really fast, many many times, and

That's a hardware or emulation property and I don't know how many
devices would expose this behaviour.

> - can run in force-thread mode, and

Almost all drivers.

> - have handlers that are able to deal with multiple IRQs per call.

Pretty much every driver which has to deal with queues, FIFOs
etc. That's a lot of them.

> About solving it directly in generic code, I agree it would be the ideal 
> scenario. That's why I suggested that in RFCv1: if the thread handles a 
> single IRQ, we reset irqs_unhandled to zero. That's a good pointer on the 
> thread being stuck, and TBH I don't think this is too far away from the 
> current 100/100k if we consider some of those handlers can handle multiple 
> IRQs at once.

It has the downside that a scenario where there is a spurious interrupt
flood with an occasional one inbetween which is handled by the thread
will not be detected anymore. The accumulation and time period tracking
are there for a reason.

But as I pointed out above the detection logic is flawed due to the
unconditional accumulation. Can you give the uncompiled below a test
ride with your scenario?

Thanks,

        tglx
---
 include/linux/irqdesc.h |    2 ++
 kernel/irq/spurious.c   |   33 +++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -29,6 +29,7 @@ struct pt_regs;
  * @wake_depth:		enable depth, for multiple irq_set_irq_wake() callers
  * @tot_count:		stats field for non-percpu irqs
  * @irq_count:		stats field to detect stalled irqs
+ * @first_unhandled:	Timestamp of the first unhandled interrupt
  * @last_unhandled:	aging timer for unhandled count
  * @irqs_unhandled:	stats field for spurious unhandled interrupts
  * @threads_handled:	stats field for deferred spurious detection of threaded handlers
@@ -64,6 +65,7 @@ struct irq_desc {
 	unsigned int		wake_depth;	/* nested wake enables */
 	unsigned int		tot_count;
 	unsigned int		irq_count;	/* For detecting broken IRQs */
+	unsigned long		first_unhandled;
 	unsigned long		last_unhandled;	/* Aging timer for unhandled count */
 	unsigned int		irqs_unhandled;
 	atomic_t		threads_handled;
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -390,9 +390,14 @@ void note_interrupt(struct irq_desc *des
 		 * working systems
 		 */
 		if (time_after(jiffies, desc->last_unhandled + HZ/10))
-			desc->irqs_unhandled = 1;
-		else
-			desc->irqs_unhandled++;
+			desc->irqs_unhandled = 0;
+
+		if (!desc->irqs_unhandled) {
+			desc->irq_count = 0;
+			desc->first_unhandled = jiffies;
+		}
+
+		desc->irqs_unhandled++;
 		desc->last_unhandled = jiffies;
 	}
 
@@ -411,9 +416,27 @@ void note_interrupt(struct irq_desc *des
 	if (likely(desc->irq_count < 100000))
 		return;
 
-	desc->irq_count = 0;
 	if (unlikely(desc->irqs_unhandled > 99900)) {
 		/*
+		 * Validate that this is actually an interrupt storm, which
+		 * happens to:
+		 *
+		 *  - increment the unhandled count to ~100k within 10 seconds
+		 *    which means there is an spurious interrupt every 50us
+		 *  - have an unhandled to handled ratio > 2
+		 *
+		 * Otherwise reset the detector and start over. This also
+		 * covers the case where a threaded handler consumes
+		 * several hard interrupts in one go which would otherwise
+		 * accumulate to 99900 over time and trigger a false positive.
+		 */
+		unsigned long td = desc->last_unhandled - desc->first_unhandled;
+		unsigned int handled = desc->irq_count - desc->irqs_unhandled;
+
+		if (td > 5 * HZ || desc->irqs_unhandled / handled < 3)
+			goto out;
+
+		/*
 		 * The interrupt is stuck
 		 */
 		__report_bad_irq(desc, action_ret);
@@ -428,7 +451,9 @@ void note_interrupt(struct irq_desc *des
 		mod_timer(&poll_spurious_irq_timer,
 			  jiffies + POLL_SPURIOUS_IRQ_INTERVAL);
 	}
+out:
 	desc->irqs_unhandled = 0;
+	desc->irq_count = 0;
 }
 
 bool noirqdebug __read_mostly;
  

On Wed, Feb 21 2024 at 16:41, Thomas Gleixner wrote:
> On Wed, Feb 21 2024 at 02:39, Leonardo Bras wrote:
> But as I pointed out above the detection logic is flawed due to the
> unconditional accumulation. Can you give the uncompiled below a test
> ride with your scenario?

Bah. Ignore this. I misread the code completely. No idea where my brain
was.

This thing triggers only when there are 100K interrupts and 99.9k of
them unhandled. The 100k total resets the unhandled counts.

Though one thing which strikes me odd is that this actually triggers at
all because it needs 99.9k unhandled out of 100k total. That means on
average every thread handler invocation handles 1000 hardware interrupts
in one go. Is that even realistic?

Thanks,

        tglx
  

On Wed, Feb 21, 2024 at 04:41:20PM +0100, Thomas Gleixner wrote:
> On Wed, Feb 21 2024 at 02:39, Leonardo Bras wrote:
> > On Mon, Feb 19, 2024 at 12:03:07PM +0100, Thomas Gleixner wrote:
> >> >> Is scenarios where there is no need to keep track of IRQ handled, convert
> >> >> it back to IRQ_HANDLED.
> >> >
> >> > That's not really workable as you'd have to update tons of drivers just
> >> > to deal with that corner case. That's error prone and just extra
> >> > complexity all over the place.
> >
> > I agree, that's a downside of this implementation. 
> 
> A serious one which is not really workable. See below.
> 
> > I agree the above may be able to solve the issue, but it would make 2 extra 
> > atomic ops necessary in the thread handling the IRQ, as well as one extra 
> > atomic operation in note_interrupt(), which could increase latency on this 
> > IRQ deferring the handler to a thread.
> >
> > I mean, yes, the cpu running note_interrupt() would probably already have 
> > exclusiveness for this cacheline, but it further increases cacheline 
> > bouncing and also adds the mem barriers that incur on atomic operations, 
> > even if we use an extra bit from threads_handled instead of allocate a new 
> > field for threads_running.
> 
> I think that's a strawman. Atomic operations can of course be more
> expensive than non-atomic ones, but they only start to make a difference
> when the cache line is contended. That's not the case here for the
> normal operations.
> 
> Interrupts and their threads are strictly targeted to a single CPU and
> the cache line is already hot and had to be made exclusive because of
> other write operations to it.
> 
> There is usually no concurrency at all, except for administrative
> operations like enable/disable or affinity changes. Those administrative
> operations are not high frequency and the resulting cache line bouncing
> is unavoidable even without that change. But does it matter in the
> larger picture? I don't think so.

That's a fair point, but there are some use cases that use CPU Isolation on 
top of PREEMPT_RT in order to reduce interference on a CPU running an RT 
workload.

For those cases, IIRC the handler will run on a different (housekeeping) 
CPU when those IRQs originate on an Isolated CPU, meaning the above 
described cacheline bouncing is expected.


> 
> > On top of that, let's think on a scenario where the threaded handler will 
> > solve a lot of requests, but not necessarily spend a lot of time doing so.
> > This allows the thread to run for little time while solving a lot of 
> > requests.
> >
> > In this scenario, note_interrupt() could return without incrementing 
> > irqs_unhandled for those IRQ that happen while the brief thread is running, 
> > but every other IRQ would cause note_interrupt() to increase 
> > irqs_unhandled, which would cause the bug to still reproduce.
> 
> In theory yes. Does it happen in practice?
> 
> But that exposes a flaw in the actual detection code. The code is
> unconditionally accumulating if there is an unhandled interrupt within
> 100ms after the last unhandled one. IOW, if there is a periodic
> unhandled one every 50ms, the interrupt will be shut down after 100000 *
> 50ms = 5000s ~= 83.3m ~= 1.4h. And it neither cares about the number of
> actually handled interrupts.
> 
> The spurious detector is really about runaway interrupts which hog a CPU
> completely, but the above is not what we want to protect against.

Now it makes a lot more sense to me.
Thanks!

> 
> > I understand my suggested change increases irq_return complexity, but it
> > should not increase much of the overhead in both IRQ deferring and IRQ 
> > thread handling. Also, since it accounts for every IRQ actually handled, it 
> > does not matter how long the handler thread runs, it still avoids the
> > bug.
> 
> I'm not worried about the extra complexity in note_interrupt(). That's
> fine and I don't have a strong opinion whether using your patch 2/4 or
> the simpler one I suggested.
> 
> > As you previously noted, the main issue in my suggestion is about changing 
> > drivers' code. But while this change is optional, I wonder how many 
> > drivers handle IRQs that:
> > - use edge type interrupts, and
> 
> It's not up to the driver to decide that. That's a platform property and
> the driver does not even know and they should not care because all what
> matters for the driver is that it gets the interrupts and does not lose
> anything.
> 
> > - can trigger really fast, many many times, and
> 
> That's a hardware or emulation property and I don't know how many
> devices would expose this behaviour.
> 
> > - can run in force-thread mode, and
> 
> Almost all drivers.
> 
> > - have handlers that are able to deal with multiple IRQs per call.
> 
> Pretty much every driver which has to deal with queues, FIFOs
> etc. That's a lot of them.
> 
> > About solving it directly in generic code, I agree it would be the ideal 
> > scenario. That's why I suggested that in RFCv1: if the thread handles a 
> > single IRQ, we reset irqs_unhandled to zero. That's a good pointer on the 
> > thread being stuck, and TBH I don't think this is too far away from the 
> > current 100/100k if we consider some of those handlers can handle multiple 
> > IRQs at once.
> 
> It has the downside that a scenario where there is a spurious interrupt
> flood with an occasional one inbetween which is handled by the thread
> will not be detected anymore. The accumulation and time period tracking
> are there for a reason.

Ok, fair point.
But if we disable it, we end up not having even those few successes.

But now that I understand the point is preventing the CPU from hogging, it 
makes sense: we prefer disabling the interrupt than compomising the system.
It also makes a warning that may help to track down the driver/device 
issue.

> 
> But as I pointed out above the detection logic is flawed due to the
> unconditional accumulation. Can you give the uncompiled below a test
> ride with your scenario?
> 
> Thanks,
> 
>         tglx
> ---
>  include/linux/irqdesc.h |    2 ++
>  kernel/irq/spurious.c   |   33 +++++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -29,6 +29,7 @@ struct pt_regs;
>   * @wake_depth:		enable depth, for multiple irq_set_irq_wake() callers
>   * @tot_count:		stats field for non-percpu irqs
>   * @irq_count:		stats field to detect stalled irqs
> + * @first_unhandled:	Timestamp of the first unhandled interrupt
>   * @last_unhandled:	aging timer for unhandled count
>   * @irqs_unhandled:	stats field for spurious unhandled interrupts
>   * @threads_handled:	stats field for deferred spurious detection of threaded handlers
> @@ -64,6 +65,7 @@ struct irq_desc {
>  	unsigned int		wake_depth;	/* nested wake enables */
>  	unsigned int		tot_count;
>  	unsigned int		irq_count;	/* For detecting broken IRQs */
> +	unsigned long		first_unhandled;
>  	unsigned long		last_unhandled;	/* Aging timer for unhandled count */
>  	unsigned int		irqs_unhandled;
>  	atomic_t		threads_handled;
> --- a/kernel/irq/spurious.c
> +++ b/kernel/irq/spurious.c
> @@ -390,9 +390,14 @@ void note_interrupt(struct irq_desc *des
>  		 * working systems
>  		 */
>  		if (time_after(jiffies, desc->last_unhandled + HZ/10))
> -			desc->irqs_unhandled = 1;
> -		else
> -			desc->irqs_unhandled++;
> +			desc->irqs_unhandled = 0;
> +
> +		if (!desc->irqs_unhandled) {
> +			desc->irq_count = 0;
> +			desc->first_unhandled = jiffies;
> +		}
> +
> +		desc->irqs_unhandled++;
>  		desc->last_unhandled = jiffies;
>  	}
>  
> @@ -411,9 +416,27 @@ void note_interrupt(struct irq_desc *des
>  	if (likely(desc->irq_count < 100000))
>  		return;
>  
> -	desc->irq_count = 0;
>  	if (unlikely(desc->irqs_unhandled > 99900)) {
>  		/*
> +		 * Validate that this is actually an interrupt storm, which
> +		 * happens to:
> +		 *
> +		 *  - increment the unhandled count to ~100k within 10 seconds
> +		 *    which means there is an spurious interrupt every 50us
> +		 *  - have an unhandled to handled ratio > 2
> +		 *
> +		 * Otherwise reset the detector and start over. This also
> +		 * covers the case where a threaded handler consumes
> +		 * several hard interrupts in one go which would otherwise
> +		 * accumulate to 99900 over time and trigger a false positive.
> +		 */
> +		unsigned long td = desc->last_unhandled - desc->first_unhandled;
> +		unsigned int handled = desc->irq_count - desc->irqs_unhandled;
> +
> +		if (td > 5 * HZ || desc->irqs_unhandled / handled < 3)
> +			goto out;
> +
> +		/*
>  		 * The interrupt is stuck
>  		 */
>  		__report_bad_irq(desc, action_ret);
> @@ -428,7 +451,9 @@ void note_interrupt(struct irq_desc *des
>  		mod_timer(&poll_spurious_irq_timer,
>  			  jiffies + POLL_SPURIOUS_IRQ_INTERVAL);
>  	}
> +out:
>  	desc->irqs_unhandled = 0;
> +	desc->irq_count = 0;
>  }
>  
>  bool noirqdebug __read_mostly;
> 

Sure, I will give it a try.

Now having a better understanding of what is expected here, I will also try 
to experiment a little here.

Thank you!
Leo
  

On Wed, Feb 21, 2024 at 06:04:21PM +0100, Thomas Gleixner wrote:
> On Wed, Feb 21 2024 at 16:41, Thomas Gleixner wrote:
> > On Wed, Feb 21 2024 at 02:39, Leonardo Bras wrote:
> > But as I pointed out above the detection logic is flawed due to the
> > unconditional accumulation. Can you give the uncompiled below a test
> > ride with your scenario?
> 
> Bah. Ignore this. I misread the code completely. No idea where my brain
> was.
> 
> This thing triggers only when there are 100K interrupts and 99.9k of
> them unhandled. The 100k total resets the unhandled counts.
> 
> Though one thing which strikes me odd is that this actually triggers at
> all because it needs 99.9k unhandled out of 100k total. That means on
> average every thread handler invocation handles 1000 hardware interrupts
> in one go. Is that even realistic?

Yeap, it triggers pretty easily if you bring a vm with a serial console, 
and try to use it to work with something very verbose.

It was detected by someone trying to unpack a kernel source tarball.

Maybe this is an issue that only becomes reproducible for this and maybe a 
couple extra drivers, so the solution will only need to be implemented in 
those drivers when (if) this bug reproduces.

This being said, thank you for helping me improve my understandig of this 
piece of code. I will put some effort in trying to find a solution that 
works by changing generic-code only, but would like to understand if the 
current proposal is valid if I am unable to find any.

Thanks!
Leo


> 
> Thanks,
> 
>         tglx
> 
>
  

On Fri, Feb 23 2024 at 01:37, Leonardo Bras wrote:
> On Wed, Feb 21, 2024 at 04:41:20PM +0100, Thomas Gleixner wrote:
>> There is usually no concurrency at all, except for administrative
>> operations like enable/disable or affinity changes. Those administrative
>> operations are not high frequency and the resulting cache line bouncing
>> is unavoidable even without that change. But does it matter in the
>> larger picture? I don't think so.
>
> That's a fair point, but there are some use cases that use CPU Isolation on 
> top of PREEMPT_RT in order to reduce interference on a CPU running an RT 
> workload.
>
> For those cases, IIRC the handler will run on a different (housekeeping) 
> CPU when those IRQs originate on an Isolated CPU, meaning the above 
> described cacheline bouncing is expected.

No. The affinity of the interrupt and the thread are strictly coupled
and always on the same CPU except for one instance during migration, but
that's irrelevant.

Thanks,

        tglx