| Message ID | 20240611-igc_irq-v2-1-c63e413c45c4@linutronix.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-next,v2] igc: Get rid of spurious interrupts | expand |
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>
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>
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> > >
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
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 --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);