[PATCH v2] PCI: iproc-msi: Removed potentially endless loop handling MSI events

[PATCH v2] PCI: iproc-msi: Removed potentially endless loop handling MSI events

[Eugen Hristev]

The iProc MSI chained handler rereads the event queue tail after each
batch and keeps processing until the queue is empty. A continuously fed
queue can therefore keep the CPU in the handler indefinitely, starving
other interrupts and causing RCU stalls under heavy Ethernet traffic.

To fix this scenario, remove the loop that keeps reading the head and
the tail until there are no more pending events. Read the head and tail
only once and process only the events recorded in that snapshot.
Update the head before returning. Events arriving during processing
remain queued for a subsequent parent interrupt, allowing other handlers
to run without imposing an arbitrary event limit.

Discovered while testing it with an Ethernet NIC, when too many MSI
interrupts come in, CPU might get starved:

Connecting to host 192.168.30.1, port 5201
[  5] local 192.168.30.2 port 52586 connected to 192.168.30.1 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   240 MBytes  2.01 Gbits/sec    2    334 KBytes
[  5]   1.00-2.00   sec  63.2 MBytes   531 Mbits/sec    1   1.41 KBytes
[  5]   2.00-3.00   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  5]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  5]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  5]   5.00-6.00   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  5]   6.00-7.00   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  5]   7.00-8.00   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  5]   8.00-9.00   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
[  177.568497] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
[  177.568913] rcu:     0-...!: (1 GPs behind) idle=28ac/1/0x4000000000000002 softirq=28922/28930 fqs=1
[  177.569457] rcu:     (detected by 1, t=5256 jiffies, g=21629, q=819 ncpus=4)
[  177.569870] Sending NMI from CPU 1 to CPUs 0:

With this one loop approach, IRQ handler will complete and continue
to serve MSI events on the next handler invocation.

Signed-off-by: Eugen Hristev <ehristev@kernel.org>
---
Changes in v2:
- Changed as per suggestion from Mani: removed the budget and completely
removed the do/while(true) loop
- Link to v1: https://patch.msgid.link/20261016-iproc-irq-v1-1-ebaaf027f29e@kernel.org

To: Lorenzo Pieralisi <lpieralisi@kernel.org>
To: Krzysztof Wilczyński <kwilczynski@kernel.org>
To: Manivannan Sadhasivam <mani@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Bjorn Helgaas <bhelgaas@google.com>
To: Ray Jui <rjui@broadcom.com>
To: Scott Branden <sbranden@broadcom.com>
To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pci/controller/pcie-iproc-msi.c | 61 ++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc-msi.c b/drivers/pci/controller/pcie-iproc-msi.c
index 9ba242ab9596..86753ae75ed3 100644
--- a/drivers/pci/controller/pcie-iproc-msi.c
+++ b/drivers/pci/controller/pcie-iproc-msi.c
@@ -344,42 +344,39 @@ static void iproc_msi_handler(struct irq_desc *desc)
 	 * data is guaranteed to be in the event queue memory before the tail
 	 * pointer is updated by the iProc MSI core.
 	 */
-	head = iproc_msi_read_reg(msi, IPROC_MSI_EQ_HEAD,
-				  eq) & IPROC_MSI_EQ_MASK;
-	do {
-		tail = iproc_msi_read_reg(msi, IPROC_MSI_EQ_TAIL,
-					  eq) & IPROC_MSI_EQ_MASK;
+	head = iproc_msi_read_reg(msi, IPROC_MSI_EQ_HEAD, eq) &
+				  IPROC_MSI_EQ_MASK;
+	tail = iproc_msi_read_reg(msi, IPROC_MSI_EQ_TAIL, eq) &
+				  IPROC_MSI_EQ_MASK;
 
-		/*
-		 * Figure out total number of events (MSI data) to be
-		 * processed.
-		 */
-		nr_events = (tail < head) ?
-			(EQ_LEN - (head - tail)) : (tail - head);
-		if (!nr_events)
-			break;
-
-		/* process all outstanding events */
-		while (nr_events--) {
-			hwirq = decode_msi_hwirq(msi, eq, head);
-			generic_handle_domain_irq(msi->inner_domain, hwirq);
-
-			head++;
-			head %= EQ_LEN;
-		}
+	/*
+	 * Figure out total number of events (MSI data) to be
+	 * processed.
+	 */
+	nr_events = (tail < head) ? (EQ_LEN - (head - tail)) : (tail - head);
+	if (!nr_events)
+		goto iproc_msi_handler_out;
 
-		/*
-		 * Now all outstanding events have been processed.  Update the
-		 * head pointer.
-		 */
-		iproc_msi_write_reg(msi, IPROC_MSI_EQ_HEAD, eq, head);
+	/* process all outstanding events */
+	while (nr_events--) {
+		hwirq = decode_msi_hwirq(msi, eq, head);
+		generic_handle_domain_irq(msi->inner_domain, hwirq);
 
-		/*
-		 * Now go read the tail pointer again to see if there are new
-		 * outstanding events that came in during the above window.
-		 */
-	} while (true);
+		head++;
+		head %= EQ_LEN;
+	}
 
+	/*
+	 * Now all outstanding events have been processed.  Update the
+	 * head pointer.
+	 */
+	iproc_msi_write_reg(msi, IPROC_MSI_EQ_HEAD, eq, head);
+
+	/*
+	 * The tail pointer will be reread on the next interrupt to process
+	 * any new events that arrived while handling this batch.
+	 */
+iproc_msi_handler_out:
 	chained_irq_exit(chip, desc);
 }
 

---
base-commit: b810335a0699d1810683796b0e3ffe8827ddfe6c
change-id: 20261016-iproc-irq-9c58cfe39158

Best regards,
--  
Eugen Hristev <ehristev@kernel.org>

Re: [PATCH v2] PCI: iproc-msi: Removed potentially endless loop handling MSI events

[sashiko-bot]

> The iProc MSI chained handler rereads the event queue tail after each
> batch and keeps processing until the queue is empty. A continuously fed
> queue can therefore keep the CPU in the handler indefinitely, starving
> other interrupts and causing RCU stalls under heavy Ethernet traffic.
> 
> To fix this scenario, remove the loop that keeps reading the head and
> the tail until there are no more pending events. Read the head and tail
> only once and process only the events recorded in that snapshot.
> Update the head before returning. Events arriving during processing
> remain queued for a subsequent parent interrupt, allowing other handlers
> to run without imposing an arbitrary event limit.
> 
> Discovered while testing it with an Ethernet NIC, when too many MSI
> interrupts come in, CPU might get starved:
> 
> [ ... ]
> 
> With this one loop approach, IRQ handler will complete and continue
> to serve MSI events on the next handler invocation.
> 
> Signed-off-by: Eugen Hristev <ehristev@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-iproc-irq-v2-1-dc7223df35fc@kernel.org?part=1