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

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

[Kurt Kanzenbach]

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

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc.h      |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 24 ++++++++++++++++++++----
 2 files changed, 21 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 305e05294a26..e666739dfac7 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,23 @@ 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_ring *rx_ring = adapter->rx_ring[i];
+
+			if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
+				eics |= adapter->q_vector[i]->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);

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

Best regards,
-- 
Kurt Kanzenbach <kurt@linutronix.de>

Re: [PATCH iwl-next] igc: Get rid of spurious interrupts

[Sebastian Andrzej Siewior]

On 2024-06-12 11:24:53 [+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
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

Re: [PATCH iwl-next] igc: Get rid of spurious interrupts

[Maciej Fijalkowski]

On Wed, Jun 12, 2024 at 11:24:53AM +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
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  1 +
>  drivers/net/ethernet/intel/igc/igc_main.c | 24 ++++++++++++++++++++----
>  2 files changed, 21 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 305e05294a26..e666739dfac7 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,23 @@ 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_ring *rx_ring = adapter->rx_ring[i];
> +
> +			if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
> +				eics |= adapter->q_vector[i]->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);
> 
> ---
> base-commit: bb678f01804ccaa861b012b2b9426d69673d8a84
> change-id: 20240611-igc_irq-ccc1c8bc6890
> 
> Best regards,
> -- 
> Kurt Kanzenbach <kurt@linutronix.de>
> 

Re: [PATCH iwl-next] igc: Get rid of spurious interrupts

[Vinicius Costa Gomes]

Kurt Kanzenbach <kurt@linutronix.de> writes:

> 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
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

>  drivers/net/ethernet/intel/igc/igc.h      |  1 +
>  drivers/net/ethernet/intel/igc/igc_main.c | 24 ++++++++++++++++++++----
>  2 files changed, 21 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 305e05294a26..e666739dfac7 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,23 @@ 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_ring *rx_ring = adapter->rx_ring[i];
> +
> +			if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {

Minor and optional: I guess you can replace test_bit() -> clear_bit()
with __test_and_clear_bit() here and below.

In any case:

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

> +				eics |= adapter->q_vector[i]->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);
>
> ---
> base-commit: bb678f01804ccaa861b012b2b9426d69673d8a84
> change-id: 20240611-igc_irq-ccc1c8bc6890
>
> Best regards,
> -- 
> Kurt Kanzenbach <kurt@linutronix.de>
>

-- 
Vinicius

Re: [PATCH iwl-next] igc: Get rid of spurious interrupts

[Sebastian Andrzej Siewior]

On 2024-06-12 12:49:21 [-0700], Vinicius Costa Gomes wrote:
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 305e05294a26..e666739dfac7 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -5811,11 +5815,23 @@ 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_ring *rx_ring = adapter->rx_ring[i];
> > +
> > +			if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
> 
> Minor and optional: I guess you can replace test_bit() -> clear_bit()
> with __test_and_clear_bit() here and below.

That are two steps, first test+clear is merged into one and then __ is
added. The former is doable but it will always lead to a write operation
while in the common case the flag isn't set so it will be skipped.
Adding the __ leads to an unlocked operation and I don't see how this is
synchronized against the other writes. In fact, nobody else is doing it.

Sebastian

Re: [PATCH iwl-next] igc: Get rid of spurious interrupts

[Vinicius Costa Gomes]

Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes:

> On 2024-06-12 12:49:21 [-0700], Vinicius Costa Gomes wrote:
>> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> > index 305e05294a26..e666739dfac7 100644
>> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> > @@ -5811,11 +5815,23 @@ 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_ring *rx_ring = adapter->rx_ring[i];
>> > +
>> > +			if (test_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &rx_ring->flags)) {
>> 
>> Minor and optional: I guess you can replace test_bit() -> clear_bit()
>> with __test_and_clear_bit() here and below.
>
> That are two steps, first test+clear is merged into one and then __ is
> added. The former is doable but it will always lead to a write operation
> while in the common case the flag isn't set so it will be skipped.
> Adding the __ leads to an unlocked operation and I don't see how this is
> synchronized against the other writes. In fact, nobody else is doing it.
>

I just took a look at the available operations, and thought that this
one could save a few lines of code. But didn't think too deeply about
that. Thanks.


Cheers,
-- 
Vinicius

Re: [PATCH iwl-next] igc: Get rid of spurious interrupts

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

Hi Tony,

On Wed Jun 12 2024, 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
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Can you drop this patch from your queue, please? I've found one issue
with this if the number of queues is reconfigured e.g., with 'ethtool -L
combined 1'. The number of vectors is not necessarily the number of rx
rings. I'll update this patch and sent v2.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH iwl-next] igc: Get rid of spurious interrupts

[Tony Nguyen]

On 6/20/2024 7:34 AM, Kurt Kanzenbach wrote:
> 
> Can you drop this patch from your queue, please? I've found one issue
> with this if the number of queues is reconfigured e.g., with 'ethtool -L
> combined 1'. The number of vectors is not necessarily the number of rx
> rings. I'll update this patch and sent v2.

Sure thing. Patch dropped.

Thanks,
Tony