diff mbox series

[iwl-next,v2] igc: Get rid of spurious interrupts

Message ID 20240611-igc_irq-v2-1-c63e413c45c4@linutronix.de
State Accepted
Delegated to: Anthony Nguyen
Headers show
Series [iwl-next,v2] igc: Get rid of spurious interrupts | expand

Commit Message

Kurt Kanzenbach June 21, 2024, 6:56 a.m. UTC 
When running the igc with XDP/ZC in busy polling mode with deferral of hard
interrupts, interrupts still happen from time to time. That is caused by
the igc task watchdog which triggers Rx interrupts periodically.

That mechanism has been introduced to overcome skb/memory allocation
failures [1]. So the Rx clean functions stop processing the Rx ring in case
of such failure. The task watchdog triggers Rx interrupts periodically in
the hope that memory became available in the mean time.

The current behavior is undesirable for real time applications, because the
driver induced Rx interrupts trigger also the softirq processing. However,
all real time packets should be processed by the application which uses the
busy polling method.

Therefore, only trigger the Rx interrupts in case of real allocation
failures. Introduce a new flag for signaling that condition.

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3be507547e6177e5c808544bd6a2efa2c7f1d436

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
Changes in v2:
- Index Rx rings correctly
- Link to v1: https://lore.kernel.org/r/20240611-igc_irq-v1-1-49763284cb57@linutronix.de
---
 drivers/net/ethernet/intel/igc/igc.h      |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 30 ++++++++++++++++++++++++++----
 2 files changed, 27 insertions(+), 4 deletions(-)


---
base-commit: a6ec08beec9ea93f342d6daeac922208709694dc
change-id: 20240611-igc_irq-ccc1c8bc6890

Best regards,

Comments

Simon Horman June 28, 2024, 12:59 p.m. UTC | #1 
On Fri, Jun 21, 2024 at 08:56:30AM +0200, Kurt Kanzenbach wrote:
> When running the igc with XDP/ZC in busy polling mode with deferral of hard
> interrupts, interrupts still happen from time to time. That is caused by
> the igc task watchdog which triggers Rx interrupts periodically.
> 
> That mechanism has been introduced to overcome skb/memory allocation
> failures [1]. So the Rx clean functions stop processing the Rx ring in case
> of such failure. The task watchdog triggers Rx interrupts periodically in
> the hope that memory became available in the mean time.
> 
> The current behavior is undesirable for real time applications, because the
> driver induced Rx interrupts trigger also the softirq processing. However,
> all real time packets should be processed by the application which uses the
> busy polling method.
> 
> Therefore, only trigger the Rx interrupts in case of real allocation
> failures. Introduce a new flag for signaling that condition.
> 
> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3be507547e6177e5c808544bd6a2efa2c7f1d436
> 
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Simon Horman <horms@kernel.org>
Mor Bar-Gabay July 23, 2024, 6:29 a.m. UTC | #2 
On 21/06/2024 9:56, Kurt Kanzenbach wrote:
> When running the igc with XDP/ZC in busy polling mode with deferral of hard
> interrupts, interrupts still happen from time to time. That is caused by
> the igc task watchdog which triggers Rx interrupts periodically.
> 
> That mechanism has been introduced to overcome skb/memory allocation
> failures [1]. So the Rx clean functions stop processing the Rx ring in case
> of such failure. The task watchdog triggers Rx interrupts periodically in
> the hope that memory became available in the mean time.
> 
> The current behavior is undesirable for real time applications, because the
> driver induced Rx interrupts trigger also the softirq processing. However,
> all real time packets should be processed by the application which uses the
> busy polling method.
> 
> Therefore, only trigger the Rx interrupts in case of real allocation
> failures. Introduce a new flag for signaling that condition.
> 
> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3be507547e6177e5c808544bd6a2efa2c7f1d436
> 
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> Reviewed-by: Simon Horman <horms@kernel.org>
> ---
> Changes in v2:
> - Index Rx rings correctly
> - Link to v1: https://lore.kernel.org/r/20240611-igc_irq-v1-1-49763284cb57@linutronix.de
> ---
>   drivers/net/ethernet/intel/igc/igc.h      |  1 +
>   drivers/net/ethernet/intel/igc/igc_main.c | 30 ++++++++++++++++++++++++++----
>   2 files changed, 27 insertions(+), 4 deletions(-)
> 
Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
Brett Creeley July 23, 2024, 3:53 p.m. UTC | #3 
On 6/20/2024 11:56 PM, Kurt Kanzenbach wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> When running the igc with XDP/ZC in busy polling mode with deferral of hard
> interrupts, interrupts still happen from time to time. That is caused by
> the igc task watchdog which triggers Rx interrupts periodically.
> 
> That mechanism has been introduced to overcome skb/memory allocation
> failures [1]. So the Rx clean functions stop processing the Rx ring in case
> of such failure. The task watchdog triggers Rx interrupts periodically in
> the hope that memory became available in the mean time.
> 
> The current behavior is undesirable for real time applications, because the
> driver induced Rx interrupts trigger also the softirq processing. However,
> all real time packets should be processed by the application which uses the
> busy polling method.
> 
> Therefore, only trigger the Rx interrupts in case of real allocation
> failures. Introduce a new flag for signaling that condition.
> 
> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3be507547e6177e5c808544bd6a2efa2c7f1d436
> 
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> Changes in v2:
> - Index Rx rings correctly
> - Link to v1: https://lore.kernel.org/r/20240611-igc_irq-v1-1-49763284cb57@linutronix.de
> ---
>   drivers/net/ethernet/intel/igc/igc.h      |  1 +
>   drivers/net/ethernet/intel/igc/igc_main.c | 30 ++++++++++++++++++++++++++----
>   2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 8b14c029eda1..7bfe5030e2c0 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -682,6 +682,7 @@ enum igc_ring_flags_t {
>          IGC_RING_FLAG_TX_DETECT_HANG,
>          IGC_RING_FLAG_AF_XDP_ZC,
>          IGC_RING_FLAG_TX_HWTSTAMP,
> +       IGC_RING_FLAG_RX_ALLOC_FAILED,
>   };
> 
>   #define ring_uses_large_buffer(ring) \
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 87b655b839c1..850ef6b8b202 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c

<snip>

> @@ -5811,11 +5815,29 @@ static void igc_watchdog_task(struct work_struct *work)
>          if (adapter->flags & IGC_FLAG_HAS_MSIX) {
>                  u32 eics = 0;
> 
> -               for (i = 0; i < adapter->num_q_vectors; i++)
> -                       eics |= adapter->q_vector[i]->eims_value;
> -               wr32(IGC_EICS, eics);
> +               for (i = 0; i < adapter->num_q_vectors; i++) {
> +                       struct igc_q_vector *q_vector = adapter->q_vector[i];
> +                       struct igc_ring *rx_ring;
> +
> +                       if (!q_vector->rx.ring)
> +                               continue;
> +
> +                       rx_ring = adapter->rx_ring[q_vector->rx.ring->queue_index];
> +
> +                       if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
> +                               eics |= q_vector->eims_value;
> +                               clear_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
> +                       }

Tiny nit, but is there a reason to not use test_and_clear_bit() here?

> +               }
> +               if (eics)
> +                       wr32(IGC_EICS, eics);
>          } else {
> -               wr32(IGC_ICS, IGC_ICS_RXDMT0);
> +               struct igc_ring *rx_ring = adapter->rx_ring[0];
> +
> +               if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
> +                       clear_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
> +                       wr32(IGC_ICS, IGC_ICS_RXDMT0);
> +               }

Also here?

Thanks,

Brett

>          }
> 
>          igc_ptp_tx_hang(adapter);
> 
> ---
> base-commit: a6ec08beec9ea93f342d6daeac922208709694dc
> change-id: 20240611-igc_irq-ccc1c8bc6890
> 
> Best regards,
> --
> Kurt Kanzenbach <kurt@linutronix.de>
> 
>
Kurt Kanzenbach July 24, 2024, 7:26 a.m. UTC | #4 
On Tue Jul 23 2024, Brett Creeley wrote:
>> @@ -5811,11 +5815,29 @@ static void igc_watchdog_task(struct work_struct *work)
>>          if (adapter->flags & IGC_FLAG_HAS_MSIX) {
>>                  u32 eics = 0;
>> 
>> -               for (i = 0; i < adapter->num_q_vectors; i++)
>> -                       eics |= adapter->q_vector[i]->eims_value;
>> -               wr32(IGC_EICS, eics);
>> +               for (i = 0; i < adapter->num_q_vectors; i++) {
>> +                       struct igc_q_vector *q_vector = adapter->q_vector[i];
>> +                       struct igc_ring *rx_ring;
>> +
>> +                       if (!q_vector->rx.ring)
>> +                               continue;
>> +
>> +                       rx_ring = adapter->rx_ring[q_vector->rx.ring->queue_index];
>> +
>> +                       if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
>> +                               eics |= q_vector->eims_value;
>> +                               clear_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
>> +                       }
>
> Tiny nit, but is there a reason to not use test_and_clear_bit() here?

I believe that question was answered by Sebastian on v1:

 https://lore.kernel.org/all/20240613062426.Om5bQpR3@linutronix.de/

Other than that no particular reason.

Thanks,
Kurt
Brett Creeley July 24, 2024, 3:30 p.m. UTC | #5 
On 7/24/2024 12:26 AM, Kurt Kanzenbach wrote:
> On Tue Jul 23 2024, Brett Creeley wrote:
>>> @@ -5811,11 +5815,29 @@ static void igc_watchdog_task(struct work_struct *work)
>>>           if (adapter->flags & IGC_FLAG_HAS_MSIX) {
>>>                   u32 eics = 0;
>>>
>>> -               for (i = 0; i < adapter->num_q_vectors; i++)
>>> -                       eics |= adapter->q_vector[i]->eims_value;
>>> -               wr32(IGC_EICS, eics);
>>> +               for (i = 0; i < adapter->num_q_vectors; i++) {
>>> +                       struct igc_q_vector *q_vector = adapter->q_vector[i];
>>> +                       struct igc_ring *rx_ring;
>>> +
>>> +                       if (!q_vector->rx.ring)
>>> +                               continue;
>>> +
>>> +                       rx_ring = adapter->rx_ring[q_vector->rx.ring->queue_index];
>>> +
>>> +                       if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
>>> +                               eics |= q_vector->eims_value;
>>> +                               clear_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
>>> +                       }
>>
>> Tiny nit, but is there a reason to not use test_and_clear_bit() here?
> 
> I believe that question was answered by Sebastian on v1:
> 
>   https://lore.kernel.org/all/20240613062426.Om5bQpR3@linutronix.de/
> 
> Other than that no particular reason.

Okay, that makes sense. I should have looked through the previous 
comments. Thanks for the link though.

Thanks,

Brett
> 
> Thanks,
> Kurt
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 8b14c029eda1..7bfe5030e2c0 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -682,6 +682,7 @@  enum igc_ring_flags_t {
 	IGC_RING_FLAG_TX_DETECT_HANG,
 	IGC_RING_FLAG_AF_XDP_ZC,
 	IGC_RING_FLAG_TX_HWTSTAMP,
+	IGC_RING_FLAG_RX_ALLOC_FAILED,
 };
 
 #define ring_uses_large_buffer(ring) \
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 87b655b839c1..850ef6b8b202 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2192,6 +2192,7 @@  static bool igc_alloc_mapped_page(struct igc_ring *rx_ring,
 	page = dev_alloc_pages(igc_rx_pg_order(rx_ring));
 	if (unlikely(!page)) {
 		rx_ring->rx_stats.alloc_failed++;
+		set_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
 		return false;
 	}
 
@@ -2208,6 +2209,7 @@  static bool igc_alloc_mapped_page(struct igc_ring *rx_ring,
 		__free_page(page);
 
 		rx_ring->rx_stats.alloc_failed++;
+		set_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
 		return false;
 	}
 
@@ -2659,6 +2661,7 @@  static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		if (!skb) {
 			rx_ring->rx_stats.alloc_failed++;
 			rx_buffer->pagecnt_bias++;
+			set_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
 			break;
 		}
 
@@ -2739,6 +2742,7 @@  static void igc_dispatch_skb_zc(struct igc_q_vector *q_vector,
 	skb = igc_construct_skb_zc(ring, xdp);
 	if (!skb) {
 		ring->rx_stats.alloc_failed++;
+		set_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &ring->flags);
 		return;
 	}
 
@@ -5811,11 +5815,29 @@  static void igc_watchdog_task(struct work_struct *work)
 	if (adapter->flags & IGC_FLAG_HAS_MSIX) {
 		u32 eics = 0;
 
-		for (i = 0; i < adapter->num_q_vectors; i++)
-			eics |= adapter->q_vector[i]->eims_value;
-		wr32(IGC_EICS, eics);
+		for (i = 0; i < adapter->num_q_vectors; i++) {
+			struct igc_q_vector *q_vector = adapter->q_vector[i];
+			struct igc_ring *rx_ring;
+
+			if (!q_vector->rx.ring)
+				continue;
+
+			rx_ring = adapter->rx_ring[q_vector->rx.ring->queue_index];
+
+			if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
+				eics |= q_vector->eims_value;
+				clear_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
+			}
+		}
+		if (eics)
+			wr32(IGC_EICS, eics);
 	} else {
-		wr32(IGC_ICS, IGC_ICS_RXDMT0);
+		struct igc_ring *rx_ring = adapter->rx_ring[0];
+
+		if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
+			clear_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags);
+			wr32(IGC_ICS, IGC_ICS_RXDMT0);
+		}
 	}
 
 	igc_ptp_tx_hang(adapter);