* [RFC net-next v2 0/2] igc: Link IRQs and queues to NAPIs
@ 2024-10-14 21:30 Joe Damato
2024-10-14 21:30 ` [RFC net-next v2 1/2] igc: Link IRQs to NAPI instances Joe Damato
2024-10-14 21:30 ` [RFC net-next v2 2/2] igc: Link queues " Joe Damato
0 siblings, 2 replies; 9+ messages in thread
From: Joe Damato @ 2024-10-14 21:30 UTC (permalink / raw)
To: netdev
Cc: kurt, vinicius.gomes, Joe Damato, Alexei Starovoitov,
open list:XDP (eXpress Data Path), Daniel Borkmann,
David S. Miller, Eric Dumazet,
moderated list:INTEL ETHERNET DRIVERS, Jakub Kicinski,
Jesper Dangaard Brouer, John Fastabend, open list, Paolo Abeni,
Przemek Kitszel, Tony Nguyen
Greetings:
Welcome to v2, still an RFC. See changelog below and in each patch for
changes from v1 [1].
This series adds support for netdev-genl to igc so that userland apps
can query IRQ, queue, and NAPI instance relationships. This is useful
because developers who have igc NICs (for example, in their Intel NUCs)
who are working on epoll-based busy polling apps and using
SO_INCOMING_NAPI_ID, need access to this API to map NAPI IDs back to
queues.
See the commit messages of each patch for example output I got on my igc
hardware.
I've taken the feedback from both Kurt Kanzenbach and Vinicius Costa
Gomes to handle the IGC_FLAG_QUEUE_PAIRS bug and the XDP case
(respectively) from v1.
If this implementation looks OK, I will follow up in a few days with an
official (non-RFC) submission.
Thanks to reviewers and maintainers for their comments/feedback!
Thanks,
Joe
[1]: https://lore.kernel.org/lkml/20241003233850.199495-1-jdamato@fastly.com/
v2:
- Patch 1: update line wrapping to 80 chars
- Patch 2:
- Update commit message to include output for IGC_FLAG_QUEUE_PAIRS
enabled and disabled
- Significant refactor to move queue mapping code to helpers to be
called from multiple locations
- Adjusted code to handle IGC_FLAG_QUEUE_PAIRS disabled as suggested
by Kurt Kanzenbach
- Map / unmap queues in igc_xdp_disable_pool and
igc_xdp_enable_pool, respectively, as suggested by Vinicius Costa
Gomes to handle the XDP case
Joe Damato (2):
igc: Link IRQs to NAPI instances
igc: Link queues to NAPI instances
drivers/net/ethernet/intel/igc/igc.h | 3 ++
drivers/net/ethernet/intel/igc/igc_main.c | 61 +++++++++++++++++++++--
drivers/net/ethernet/intel/igc/igc_xdp.c | 2 +
3 files changed, 62 insertions(+), 4 deletions(-)
base-commit: 01b6b9315f15f199a206c8b3bd3e051584237d7e
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [RFC net-next v2 1/2] igc: Link IRQs to NAPI instances 2024-10-14 21:30 [RFC net-next v2 0/2] igc: Link IRQs and queues to NAPIs Joe Damato @ 2024-10-14 21:30 ` Joe Damato 2024-10-14 21:30 ` [RFC net-next v2 2/2] igc: Link queues " Joe Damato 1 sibling, 0 replies; 9+ messages in thread From: Joe Damato @ 2024-10-14 21:30 UTC (permalink / raw) To: netdev Cc: kurt, vinicius.gomes, Joe Damato, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, moderated list:INTEL ETHERNET DRIVERS, open list Link IRQs to NAPI instances via netdev-genl API so that users can query this information with netlink. Compare the output of /proc/interrupts (noting that IRQ 144 is the "other" IRQ which does not appear to have a NAPI instance): $ cat /proc/interrupts | grep enp86s0 | cut --delimiter=":" -f1 144 145 146 147 148 The output from netlink shows the mapping of NAPI IDs to IRQs (again noting that 144 is absent as it is the "other" IRQ): $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump napi-get --json='{"ifindex": 2}' [{'id': 8196, 'ifindex': 2, 'irq': 148}, {'id': 8195, 'ifindex': 2, 'irq': 147}, {'id': 8194, 'ifindex': 2, 'irq': 146}, {'id': 8193, 'ifindex': 2, 'irq': 145}] Signed-off-by: Joe Damato <jdamato@fastly.com> --- v2: - Line wrap at 80 characters drivers/net/ethernet/intel/igc/igc_main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 6e70bca15db1..7964bbedb16c 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -5576,6 +5576,9 @@ static int igc_request_msix(struct igc_adapter *adapter) q_vector); if (err) goto err_free; + + netif_napi_set_irq(&q_vector->napi, + adapter->msix_entries[vector].vector); } igc_configure_msix(adapter); -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC net-next v2 2/2] igc: Link queues to NAPI instances 2024-10-14 21:30 [RFC net-next v2 0/2] igc: Link IRQs and queues to NAPIs Joe Damato 2024-10-14 21:30 ` [RFC net-next v2 1/2] igc: Link IRQs to NAPI instances Joe Damato @ 2024-10-14 21:30 ` Joe Damato 2024-10-15 1:51 ` Vinicius Costa Gomes 2024-10-15 10:27 ` Kurt Kanzenbach 1 sibling, 2 replies; 9+ messages in thread From: Joe Damato @ 2024-10-14 21:30 UTC (permalink / raw) To: netdev Cc: kurt, vinicius.gomes, Joe Damato, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, moderated list:INTEL ETHERNET DRIVERS, open list, open list:XDP (eXpress Data Path) Link queues to NAPI instances via netdev-genl API so that users can query this information with netlink. Handle a few cases in the driver: 1. Link/unlink the NAPIs when XDP is enabled/disabled 2. Handle IGC_FLAG_QUEUE_PAIRS enabled and disabled Example output when IGC_FLAG_QUEUE_PAIRS is enabled: $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump queue-get --json='{"ifindex": 2}' [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'}, {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'}, {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] Since IGC_FLAG_QUEUE_PAIRS is enabled, you'll note that the same NAPI ID is present for both rx and tx queues at the same index, for example index 0: {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, To test IGC_FLAG_QUEUE_PAIRS disabled, a test system was booted using the grub command line option "maxcpus=2" to force igc_set_interrupt_capability to disable IGC_FLAG_QUEUE_PAIRS. Example output when IGC_FLAG_QUEUE_PAIRS is disabled: $ lscpu | grep "On-line CPU" On-line CPU(s) list: 0,2 $ ethtool -l enp86s0 | tail -5 Current hardware settings: RX: n/a TX: n/a Other: 1 Combined: 2 $ cat /proc/interrupts | grep enp 144: [...] enp86s0 145: [...] enp86s0-rx-0 146: [...] enp86s0-rx-1 147: [...] enp86s0-tx-0 148: [...] enp86s0-tx-1 1 "other" IRQ, and 2 IRQs for each of RX and Tx, so we expect netlink to report 4 IRQs with unique NAPI IDs: $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump napi-get --json='{"ifindex": 2}' [{'id': 8196, 'ifindex': 2, 'irq': 148}, {'id': 8195, 'ifindex': 2, 'irq': 147}, {'id': 8194, 'ifindex': 2, 'irq': 146}, {'id': 8193, 'ifindex': 2, 'irq': 145}] Now we examine which queues these NAPIs are associated with, expecting that since IGC_FLAG_QUEUE_PAIRS is disabled each RX and TX queue will have its own NAPI instance: $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump queue-get --json='{"ifindex": 2}' [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] Signed-off-by: Joe Damato <jdamato@fastly.com> --- v2: - Update commit message to include tests for IGC_FLAG_QUEUE_PAIRS disabled - Refactored code to move napi queue mapping and unmapping to helper functions igc_set_queue_napi and igc_unset_queue_napi - Adjust the code to handle IGC_FLAG_QUEUE_PAIRS disabled - Call helpers to map/unmap queues to NAPIs in igc_up, __igc_open, igc_xdp_enable_pool, and igc_xdp_disable_pool drivers/net/ethernet/intel/igc/igc.h | 3 ++ drivers/net/ethernet/intel/igc/igc_main.c | 58 +++++++++++++++++++++-- drivers/net/ethernet/intel/igc/igc_xdp.c | 2 + 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index eac0f966e0e4..7b1c9ea60056 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -337,6 +337,9 @@ struct igc_adapter { struct igc_led_classdev *leds; }; +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, + struct napi_struct *napi); +void igc_unset_queue_napi(struct igc_adapter *adapter, int q_idx); void igc_up(struct igc_adapter *adapter); void igc_down(struct igc_adapter *adapter); int igc_open(struct net_device *netdev); diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 7964bbedb16c..59c00acfa0ed 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -4948,6 +4948,47 @@ static int igc_sw_init(struct igc_adapter *adapter) return 0; } +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, + struct napi_struct *napi) +{ + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { + netif_queue_set_napi(adapter->netdev, q_idx, + NETDEV_QUEUE_TYPE_RX, napi); + netif_queue_set_napi(adapter->netdev, q_idx, + NETDEV_QUEUE_TYPE_TX, napi); + } else { + if (q_idx < adapter->num_rx_queues) { + netif_queue_set_napi(adapter->netdev, q_idx, + NETDEV_QUEUE_TYPE_RX, napi); + } else { + q_idx -= adapter->num_rx_queues; + netif_queue_set_napi(adapter->netdev, q_idx, + NETDEV_QUEUE_TYPE_TX, napi); + } + } +} + +void igc_unset_queue_napi(struct igc_adapter *adapter, int q_idx) +{ + struct net_device *netdev = adapter->netdev; + + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { + netif_queue_set_napi(netdev, q_idx, NETDEV_QUEUE_TYPE_RX, + NULL); + netif_queue_set_napi(netdev, q_idx, NETDEV_QUEUE_TYPE_TX, + NULL); + } else { + if (q_idx < adapter->num_rx_queues) { + netif_queue_set_napi(adapter->netdev, q_idx, + NETDEV_QUEUE_TYPE_RX, NULL); + } else { + q_idx -= adapter->num_rx_queues; + netif_queue_set_napi(adapter->netdev, q_idx, + NETDEV_QUEUE_TYPE_TX, NULL); + } + } +} + /** * igc_up - Open the interface and prepare it to handle traffic * @adapter: board private structure @@ -4955,6 +4996,7 @@ static int igc_sw_init(struct igc_adapter *adapter) void igc_up(struct igc_adapter *adapter) { struct igc_hw *hw = &adapter->hw; + struct napi_struct *napi; int i = 0; /* hardware has been reset, we need to reload some things */ @@ -4962,8 +5004,11 @@ void igc_up(struct igc_adapter *adapter) clear_bit(__IGC_DOWN, &adapter->state); - for (i = 0; i < adapter->num_q_vectors; i++) - napi_enable(&adapter->q_vector[i]->napi); + for (i = 0; i < adapter->num_q_vectors; i++) { + napi = &adapter->q_vector[i]->napi; + napi_enable(napi); + igc_set_queue_napi(adapter, i, napi); + } if (adapter->msix_entries) igc_configure_msix(adapter); @@ -5192,6 +5237,7 @@ void igc_down(struct igc_adapter *adapter) for (i = 0; i < adapter->num_q_vectors; i++) { if (adapter->q_vector[i]) { napi_synchronize(&adapter->q_vector[i]->napi); + igc_unset_queue_napi(adapter, i); napi_disable(&adapter->q_vector[i]->napi); } } @@ -6021,6 +6067,7 @@ static int __igc_open(struct net_device *netdev, bool resuming) struct igc_adapter *adapter = netdev_priv(netdev); struct pci_dev *pdev = adapter->pdev; struct igc_hw *hw = &adapter->hw; + struct napi_struct *napi; int err = 0; int i = 0; @@ -6056,8 +6103,11 @@ static int __igc_open(struct net_device *netdev, bool resuming) clear_bit(__IGC_DOWN, &adapter->state); - for (i = 0; i < adapter->num_q_vectors; i++) - napi_enable(&adapter->q_vector[i]->napi); + for (i = 0; i < adapter->num_q_vectors; i++) { + napi = &adapter->q_vector[i]->napi; + napi_enable(napi); + igc_set_queue_napi(adapter, i, napi); + } /* Clear any pending interrupts. */ rd32(IGC_ICR); diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c index e27af72aada8..886f04b8c394 100644 --- a/drivers/net/ethernet/intel/igc/igc_xdp.c +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c @@ -84,6 +84,7 @@ static int igc_xdp_enable_pool(struct igc_adapter *adapter, napi_disable(napi); } + igc_unset_queue_napi(adapter, queue_id); set_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags); set_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags); @@ -133,6 +134,7 @@ static int igc_xdp_disable_pool(struct igc_adapter *adapter, u16 queue_id) xsk_pool_dma_unmap(pool, IGC_RX_DMA_ATTR); clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags); clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags); + igc_set_queue_napi(adapter, queue_id, napi); if (needs_reset) { napi_enable(napi); -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC net-next v2 2/2] igc: Link queues to NAPI instances 2024-10-14 21:30 ` [RFC net-next v2 2/2] igc: Link queues " Joe Damato @ 2024-10-15 1:51 ` Vinicius Costa Gomes 2024-10-15 3:44 ` Joe Damato 2024-10-15 10:27 ` Kurt Kanzenbach 1 sibling, 1 reply; 9+ messages in thread From: Vinicius Costa Gomes @ 2024-10-15 1:51 UTC (permalink / raw) To: Joe Damato, netdev Cc: kurt, Joe Damato, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, moderated list:INTEL ETHERNET DRIVERS, open list, open list:XDP (eXpress Data Path) Joe Damato <jdamato@fastly.com> writes: > Link queues to NAPI instances via netdev-genl API so that users can > query this information with netlink. Handle a few cases in the driver: > 1. Link/unlink the NAPIs when XDP is enabled/disabled > 2. Handle IGC_FLAG_QUEUE_PAIRS enabled and disabled > > Example output when IGC_FLAG_QUEUE_PAIRS is enabled: > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump queue-get --json='{"ifindex": 2}' > > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] > > Since IGC_FLAG_QUEUE_PAIRS is enabled, you'll note that the same NAPI ID > is present for both rx and tx queues at the same index, for example > index 0: > > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > > To test IGC_FLAG_QUEUE_PAIRS disabled, a test system was booted using > the grub command line option "maxcpus=2" to force > igc_set_interrupt_capability to disable IGC_FLAG_QUEUE_PAIRS. > > Example output when IGC_FLAG_QUEUE_PAIRS is disabled: > > $ lscpu | grep "On-line CPU" > On-line CPU(s) list: 0,2 > > $ ethtool -l enp86s0 | tail -5 > Current hardware settings: > RX: n/a > TX: n/a > Other: 1 > Combined: 2 > > $ cat /proc/interrupts | grep enp > 144: [...] enp86s0 > 145: [...] enp86s0-rx-0 > 146: [...] enp86s0-rx-1 > 147: [...] enp86s0-tx-0 > 148: [...] enp86s0-tx-1 > > 1 "other" IRQ, and 2 IRQs for each of RX and Tx, so we expect netlink to > report 4 IRQs with unique NAPI IDs: > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump napi-get --json='{"ifindex": 2}' > [{'id': 8196, 'ifindex': 2, 'irq': 148}, > {'id': 8195, 'ifindex': 2, 'irq': 147}, > {'id': 8194, 'ifindex': 2, 'irq': 146}, > {'id': 8193, 'ifindex': 2, 'irq': 145}] > > Now we examine which queues these NAPIs are associated with, expecting > that since IGC_FLAG_QUEUE_PAIRS is disabled each RX and TX queue will > have its own NAPI instance: > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump queue-get --json='{"ifindex": 2}' > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > v2: > - Update commit message to include tests for IGC_FLAG_QUEUE_PAIRS > disabled > - Refactored code to move napi queue mapping and unmapping to helper > functions igc_set_queue_napi and igc_unset_queue_napi > - Adjust the code to handle IGC_FLAG_QUEUE_PAIRS disabled > - Call helpers to map/unmap queues to NAPIs in igc_up, __igc_open, > igc_xdp_enable_pool, and igc_xdp_disable_pool > > drivers/net/ethernet/intel/igc/igc.h | 3 ++ > drivers/net/ethernet/intel/igc/igc_main.c | 58 +++++++++++++++++++++-- > drivers/net/ethernet/intel/igc/igc_xdp.c | 2 + > 3 files changed, 59 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index eac0f966e0e4..7b1c9ea60056 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -337,6 +337,9 @@ struct igc_adapter { > struct igc_led_classdev *leds; > }; > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > + struct napi_struct *napi); > +void igc_unset_queue_napi(struct igc_adapter *adapter, int q_idx); > void igc_up(struct igc_adapter *adapter); > void igc_down(struct igc_adapter *adapter); > int igc_open(struct net_device *netdev); > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 7964bbedb16c..59c00acfa0ed 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -4948,6 +4948,47 @@ static int igc_sw_init(struct igc_adapter *adapter) > return 0; > } > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > + struct napi_struct *napi) > +{ > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_RX, napi); > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_TX, napi); > + } else { > + if (q_idx < adapter->num_rx_queues) { > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_RX, napi); > + } else { > + q_idx -= adapter->num_rx_queues; > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_TX, napi); > + } > + } > +} > + > +void igc_unset_queue_napi(struct igc_adapter *adapter, int q_idx) > +{ > + struct net_device *netdev = adapter->netdev; > + > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > + netif_queue_set_napi(netdev, q_idx, NETDEV_QUEUE_TYPE_RX, > + NULL); > + netif_queue_set_napi(netdev, q_idx, NETDEV_QUEUE_TYPE_TX, > + NULL); > + } else { > + if (q_idx < adapter->num_rx_queues) { > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_RX, NULL); > + } else { > + q_idx -= adapter->num_rx_queues; > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_TX, NULL); > + } > + } > +} It seems that igc_unset_queue_napi() is igc_set_queue_napi(x, y, NULL), so I would suggest either implementing "unset" in terms of "set", or using igc_set_queue_napi(x, y, NULL) directly in the "unlink" case (I have a slight preference for the second option). > + > /** > * igc_up - Open the interface and prepare it to handle traffic > * @adapter: board private structure > @@ -4955,6 +4996,7 @@ static int igc_sw_init(struct igc_adapter *adapter) > void igc_up(struct igc_adapter *adapter) > { > struct igc_hw *hw = &adapter->hw; > + struct napi_struct *napi; > int i = 0; > > /* hardware has been reset, we need to reload some things */ > @@ -4962,8 +5004,11 @@ void igc_up(struct igc_adapter *adapter) > > clear_bit(__IGC_DOWN, &adapter->state); > > - for (i = 0; i < adapter->num_q_vectors; i++) > - napi_enable(&adapter->q_vector[i]->napi); > + for (i = 0; i < adapter->num_q_vectors; i++) { > + napi = &adapter->q_vector[i]->napi; > + napi_enable(napi); > + igc_set_queue_napi(adapter, i, napi); > + } > > if (adapter->msix_entries) > igc_configure_msix(adapter); > @@ -5192,6 +5237,7 @@ void igc_down(struct igc_adapter *adapter) > for (i = 0; i < adapter->num_q_vectors; i++) { > if (adapter->q_vector[i]) { > napi_synchronize(&adapter->q_vector[i]->napi); > + igc_unset_queue_napi(adapter, i); > napi_disable(&adapter->q_vector[i]->napi); > } > } > @@ -6021,6 +6067,7 @@ static int __igc_open(struct net_device *netdev, bool resuming) > struct igc_adapter *adapter = netdev_priv(netdev); > struct pci_dev *pdev = adapter->pdev; > struct igc_hw *hw = &adapter->hw; > + struct napi_struct *napi; > int err = 0; > int i = 0; > > @@ -6056,8 +6103,11 @@ static int __igc_open(struct net_device *netdev, bool resuming) > > clear_bit(__IGC_DOWN, &adapter->state); > > - for (i = 0; i < adapter->num_q_vectors; i++) > - napi_enable(&adapter->q_vector[i]->napi); > + for (i = 0; i < adapter->num_q_vectors; i++) { > + napi = &adapter->q_vector[i]->napi; > + napi_enable(napi); > + igc_set_queue_napi(adapter, i, napi); > + } > > /* Clear any pending interrupts. */ > rd32(IGC_ICR); > diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c > index e27af72aada8..886f04b8c394 100644 > --- a/drivers/net/ethernet/intel/igc/igc_xdp.c > +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c > @@ -84,6 +84,7 @@ static int igc_xdp_enable_pool(struct igc_adapter *adapter, > napi_disable(napi); > } > > + igc_unset_queue_napi(adapter, queue_id); > set_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags); > set_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags); > > @@ -133,6 +134,7 @@ static int igc_xdp_disable_pool(struct igc_adapter *adapter, u16 queue_id) > xsk_pool_dma_unmap(pool, IGC_RX_DMA_ATTR); > clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags); > clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags); > + igc_set_queue_napi(adapter, queue_id, napi); > > if (needs_reset) { > napi_enable(napi); > -- > 2.25.1 > Cheers, -- Vinicius ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC net-next v2 2/2] igc: Link queues to NAPI instances 2024-10-15 1:51 ` Vinicius Costa Gomes @ 2024-10-15 3:44 ` Joe Damato 0 siblings, 0 replies; 9+ messages in thread From: Joe Damato @ 2024-10-15 3:44 UTC (permalink / raw) To: Vinicius Costa Gomes Cc: netdev, kurt, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, moderated list:INTEL ETHERNET DRIVERS, open list, open list:XDP (eXpress Data Path) On Mon, Oct 14, 2024 at 06:51:57PM -0700, Vinicius Costa Gomes wrote: > Joe Damato <jdamato@fastly.com> writes: [...] > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > > + struct napi_struct *napi) > > +{ > > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_RX, napi); > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_TX, napi); > > + } else { > > + if (q_idx < adapter->num_rx_queues) { > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_RX, napi); > > + } else { > > + q_idx -= adapter->num_rx_queues; > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_TX, napi); > > + } > > + } > > +} > > + > > +void igc_unset_queue_napi(struct igc_adapter *adapter, int q_idx) > > +{ > > + struct net_device *netdev = adapter->netdev; > > + > > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > > + netif_queue_set_napi(netdev, q_idx, NETDEV_QUEUE_TYPE_RX, > > + NULL); > > + netif_queue_set_napi(netdev, q_idx, NETDEV_QUEUE_TYPE_TX, > > + NULL); > > + } else { > > + if (q_idx < adapter->num_rx_queues) { > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_RX, NULL); > > + } else { > > + q_idx -= adapter->num_rx_queues; > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_TX, NULL); > > + } > > + } > > +} > > It seems that igc_unset_queue_napi() is igc_set_queue_napi(x, y, NULL), > so I would suggest either implementing "unset" in terms of "set", or > using igc_set_queue_napi(x, y, NULL) directly in the "unlink" case (I > have a slight preference for the second option). Ah, yes, of course. That is much simpler; I'll go with the second option for the v3. Thank you for catching that in your review. Unless any other significant feedback comes in, I'll likely send the v3 as a PATCH instead of an RFC later this week. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC net-next v2 2/2] igc: Link queues to NAPI instances 2024-10-14 21:30 ` [RFC net-next v2 2/2] igc: Link queues " Joe Damato 2024-10-15 1:51 ` Vinicius Costa Gomes @ 2024-10-15 10:27 ` Kurt Kanzenbach 2024-10-16 1:01 ` Joe Damato 2024-10-18 17:04 ` Joe Damato 1 sibling, 2 replies; 9+ messages in thread From: Kurt Kanzenbach @ 2024-10-15 10:27 UTC (permalink / raw) To: Joe Damato, netdev Cc: vinicius.gomes, Joe Damato, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, moderated list:INTEL ETHERNET DRIVERS, open list, open list:XDP (eXpress Data Path) [-- Attachment #1: Type: text/plain, Size: 5767 bytes --] On Mon Oct 14 2024, Joe Damato wrote: > Link queues to NAPI instances via netdev-genl API so that users can > query this information with netlink. Handle a few cases in the driver: > 1. Link/unlink the NAPIs when XDP is enabled/disabled > 2. Handle IGC_FLAG_QUEUE_PAIRS enabled and disabled > > Example output when IGC_FLAG_QUEUE_PAIRS is enabled: > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump queue-get --json='{"ifindex": 2}' > > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] > > Since IGC_FLAG_QUEUE_PAIRS is enabled, you'll note that the same NAPI ID > is present for both rx and tx queues at the same index, for example > index 0: > > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > > To test IGC_FLAG_QUEUE_PAIRS disabled, a test system was booted using > the grub command line option "maxcpus=2" to force > igc_set_interrupt_capability to disable IGC_FLAG_QUEUE_PAIRS. > > Example output when IGC_FLAG_QUEUE_PAIRS is disabled: > > $ lscpu | grep "On-line CPU" > On-line CPU(s) list: 0,2 > > $ ethtool -l enp86s0 | tail -5 > Current hardware settings: > RX: n/a > TX: n/a > Other: 1 > Combined: 2 > > $ cat /proc/interrupts | grep enp > 144: [...] enp86s0 > 145: [...] enp86s0-rx-0 > 146: [...] enp86s0-rx-1 > 147: [...] enp86s0-tx-0 > 148: [...] enp86s0-tx-1 > > 1 "other" IRQ, and 2 IRQs for each of RX and Tx, so we expect netlink to > report 4 IRQs with unique NAPI IDs: > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump napi-get --json='{"ifindex": 2}' > [{'id': 8196, 'ifindex': 2, 'irq': 148}, > {'id': 8195, 'ifindex': 2, 'irq': 147}, > {'id': 8194, 'ifindex': 2, 'irq': 146}, > {'id': 8193, 'ifindex': 2, 'irq': 145}] > > Now we examine which queues these NAPIs are associated with, expecting > that since IGC_FLAG_QUEUE_PAIRS is disabled each RX and TX queue will > have its own NAPI instance: > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump queue-get --json='{"ifindex": 2}' > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > v2: > - Update commit message to include tests for IGC_FLAG_QUEUE_PAIRS > disabled > - Refactored code to move napi queue mapping and unmapping to helper > functions igc_set_queue_napi and igc_unset_queue_napi > - Adjust the code to handle IGC_FLAG_QUEUE_PAIRS disabled > - Call helpers to map/unmap queues to NAPIs in igc_up, __igc_open, > igc_xdp_enable_pool, and igc_xdp_disable_pool > > drivers/net/ethernet/intel/igc/igc.h | 3 ++ > drivers/net/ethernet/intel/igc/igc_main.c | 58 +++++++++++++++++++++-- > drivers/net/ethernet/intel/igc/igc_xdp.c | 2 + > 3 files changed, 59 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index eac0f966e0e4..7b1c9ea60056 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -337,6 +337,9 @@ struct igc_adapter { > struct igc_led_classdev *leds; > }; > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > + struct napi_struct *napi); > +void igc_unset_queue_napi(struct igc_adapter *adapter, int q_idx); > void igc_up(struct igc_adapter *adapter); > void igc_down(struct igc_adapter *adapter); > int igc_open(struct net_device *netdev); > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 7964bbedb16c..59c00acfa0ed 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -4948,6 +4948,47 @@ static int igc_sw_init(struct igc_adapter *adapter) > return 0; > } > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > + struct napi_struct *napi) > +{ > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_RX, napi); > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_TX, napi); > + } else { > + if (q_idx < adapter->num_rx_queues) { > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_RX, napi); > + } else { > + q_idx -= adapter->num_rx_queues; > + netif_queue_set_napi(adapter->netdev, q_idx, > + NETDEV_QUEUE_TYPE_TX, napi); > + } > + } > +} In addition, to what Vinicius said. I think this can be done simpler. Something like this? void igc_set_queue_napi(struct igc_adapter *adapter, int vector, struct napi_struct *napi) { struct igc_q_vector *q_vector = adapter->q_vector[vector]; if (q_vector->rx.ring) netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_RX, napi); if (q_vector->tx.ring) netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_TX, napi); } Thanks, Kurt [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 861 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC net-next v2 2/2] igc: Link queues to NAPI instances 2024-10-15 10:27 ` Kurt Kanzenbach @ 2024-10-16 1:01 ` Joe Damato 2024-10-18 17:04 ` Joe Damato 1 sibling, 0 replies; 9+ messages in thread From: Joe Damato @ 2024-10-16 1:01 UTC (permalink / raw) To: Kurt Kanzenbach Cc: netdev, vinicius.gomes, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, moderated list:INTEL ETHERNET DRIVERS, open list, open list:XDP (eXpress Data Path) On Tue, Oct 15, 2024 at 12:27:01PM +0200, Kurt Kanzenbach wrote: > On Mon Oct 14 2024, Joe Damato wrote: > > Link queues to NAPI instances via netdev-genl API so that users can > > query this information with netlink. Handle a few cases in the driver: > > 1. Link/unlink the NAPIs when XDP is enabled/disabled > > 2. Handle IGC_FLAG_QUEUE_PAIRS enabled and disabled > > > > Example output when IGC_FLAG_QUEUE_PAIRS is enabled: > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > --dump queue-get --json='{"ifindex": 2}' > > > > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'}, > > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'}, > > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'}, > > {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > > {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] > > > > Since IGC_FLAG_QUEUE_PAIRS is enabled, you'll note that the same NAPI ID > > is present for both rx and tx queues at the same index, for example > > index 0: > > > > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}, > > > > To test IGC_FLAG_QUEUE_PAIRS disabled, a test system was booted using > > the grub command line option "maxcpus=2" to force > > igc_set_interrupt_capability to disable IGC_FLAG_QUEUE_PAIRS. > > > > Example output when IGC_FLAG_QUEUE_PAIRS is disabled: > > > > $ lscpu | grep "On-line CPU" > > On-line CPU(s) list: 0,2 > > > > $ ethtool -l enp86s0 | tail -5 > > Current hardware settings: > > RX: n/a > > TX: n/a > > Other: 1 > > Combined: 2 > > > > $ cat /proc/interrupts | grep enp > > 144: [...] enp86s0 > > 145: [...] enp86s0-rx-0 > > 146: [...] enp86s0-rx-1 > > 147: [...] enp86s0-tx-0 > > 148: [...] enp86s0-tx-1 > > > > 1 "other" IRQ, and 2 IRQs for each of RX and Tx, so we expect netlink to > > report 4 IRQs with unique NAPI IDs: > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > --dump napi-get --json='{"ifindex": 2}' > > [{'id': 8196, 'ifindex': 2, 'irq': 148}, > > {'id': 8195, 'ifindex': 2, 'irq': 147}, > > {'id': 8194, 'ifindex': 2, 'irq': 146}, > > {'id': 8193, 'ifindex': 2, 'irq': 145}] > > > > Now we examine which queues these NAPIs are associated with, expecting > > that since IGC_FLAG_QUEUE_PAIRS is disabled each RX and TX queue will > > have its own NAPI instance: > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > --dump queue-get --json='{"ifindex": 2}' > > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > > {'id': 0, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, > > {'id': 1, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > v2: > > - Update commit message to include tests for IGC_FLAG_QUEUE_PAIRS > > disabled > > - Refactored code to move napi queue mapping and unmapping to helper > > functions igc_set_queue_napi and igc_unset_queue_napi > > - Adjust the code to handle IGC_FLAG_QUEUE_PAIRS disabled > > - Call helpers to map/unmap queues to NAPIs in igc_up, __igc_open, > > igc_xdp_enable_pool, and igc_xdp_disable_pool > > > > drivers/net/ethernet/intel/igc/igc.h | 3 ++ > > drivers/net/ethernet/intel/igc/igc_main.c | 58 +++++++++++++++++++++-- > > drivers/net/ethernet/intel/igc/igc_xdp.c | 2 + > > 3 files changed, 59 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > > index eac0f966e0e4..7b1c9ea60056 100644 > > --- a/drivers/net/ethernet/intel/igc/igc.h > > +++ b/drivers/net/ethernet/intel/igc/igc.h > > @@ -337,6 +337,9 @@ struct igc_adapter { > > struct igc_led_classdev *leds; > > }; > > > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > > + struct napi_struct *napi); > > +void igc_unset_queue_napi(struct igc_adapter *adapter, int q_idx); > > void igc_up(struct igc_adapter *adapter); > > void igc_down(struct igc_adapter *adapter); > > int igc_open(struct net_device *netdev); > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > > index 7964bbedb16c..59c00acfa0ed 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_main.c > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > > @@ -4948,6 +4948,47 @@ static int igc_sw_init(struct igc_adapter *adapter) > > return 0; > > } > > > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > > + struct napi_struct *napi) > > +{ > > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_RX, napi); > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_TX, napi); > > + } else { > > + if (q_idx < adapter->num_rx_queues) { > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_RX, napi); > > + } else { > > + q_idx -= adapter->num_rx_queues; > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_TX, napi); > > + } > > + } > > +} > > In addition, to what Vinicius said. I think this can be done > simpler. Something like this? > > void igc_set_queue_napi(struct igc_adapter *adapter, int vector, > struct napi_struct *napi) > { > struct igc_q_vector *q_vector = adapter->q_vector[vector]; > > if (q_vector->rx.ring) > netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_RX, napi); > > if (q_vector->tx.ring) > netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_TX, napi); > } Ah, yes, that is much simpler. Thanks for the suggestion. I'll do that for the v3. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC net-next v2 2/2] igc: Link queues to NAPI instances 2024-10-15 10:27 ` Kurt Kanzenbach 2024-10-16 1:01 ` Joe Damato @ 2024-10-18 17:04 ` Joe Damato 2024-10-21 5:42 ` Kurt Kanzenbach 1 sibling, 1 reply; 9+ messages in thread From: Joe Damato @ 2024-10-18 17:04 UTC (permalink / raw) To: Kurt Kanzenbach Cc: netdev, vinicius.gomes, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, moderated list:INTEL ETHERNET DRIVERS, open list, open list:XDP (eXpress Data Path) On Tue, Oct 15, 2024 at 12:27:01PM +0200, Kurt Kanzenbach wrote: > On Mon Oct 14 2024, Joe Damato wrote: [...] > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > > index 7964bbedb16c..59c00acfa0ed 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_main.c > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > > @@ -4948,6 +4948,47 @@ static int igc_sw_init(struct igc_adapter *adapter) > > return 0; > > } > > > > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, > > + struct napi_struct *napi) > > +{ > > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_RX, napi); > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_TX, napi); > > + } else { > > + if (q_idx < adapter->num_rx_queues) { > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_RX, napi); > > + } else { > > + q_idx -= adapter->num_rx_queues; > > + netif_queue_set_napi(adapter->netdev, q_idx, > > + NETDEV_QUEUE_TYPE_TX, napi); > > + } > > + } > > +} > > In addition, to what Vinicius said. I think this can be done > simpler. Something like this? > > void igc_set_queue_napi(struct igc_adapter *adapter, int vector, > struct napi_struct *napi) > { > struct igc_q_vector *q_vector = adapter->q_vector[vector]; > > if (q_vector->rx.ring) > netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_RX, napi); > > if (q_vector->tx.ring) > netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_TX, napi); > } I tried this suggestion but this does not result in correct output in the case where IGC_FLAG_QUEUE_PAIRS is disabled. The output from netlink: $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump queue-get --json='{"ifindex": 2}' [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'type': 'tx'}, {'id': 1, 'ifindex': 2, 'type': 'tx'}] Note the lack of a napi-id for the TX queues. This typically happens when the linking is not done correctly; netif_queue_set_napi should take a queue id as the second parameter. I believe the suggested code above should be modified to be as follows to use ring->queue_index: if (q_vector->rx.ring) netif_queue_set_napi(adapter->netdev, q_vector->rx.ring->queue_index, NETDEV_QUEUE_TYPE_RX, napi); if (q_vector->tx.ring) netif_queue_set_napi(adapter->netdev, q_vector->tx.ring->queue_index, NETDEV_QUEUE_TYPE_TX, napi); Which produces correct output: $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump queue-get --json='{"ifindex": 2}' [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}] I wanted to send you a note about this before I post the v3 so that if/when you review it you'll have the context as to why the v3 code is slightly different than what was suggested. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC net-next v2 2/2] igc: Link queues to NAPI instances 2024-10-18 17:04 ` Joe Damato @ 2024-10-21 5:42 ` Kurt Kanzenbach 0 siblings, 0 replies; 9+ messages in thread From: Kurt Kanzenbach @ 2024-10-21 5:42 UTC (permalink / raw) To: Joe Damato Cc: netdev, vinicius.gomes, Tony Nguyen, Przemek Kitszel, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend, moderated list:INTEL ETHERNET DRIVERS, open list, open list:XDP (eXpress Data Path) [-- Attachment #1: Type: text/plain, Size: 3001 bytes --] On Fri Oct 18 2024, Joe Damato wrote: > On Tue, Oct 15, 2024 at 12:27:01PM +0200, Kurt Kanzenbach wrote: >> On Mon Oct 14 2024, Joe Damato wrote: > > [...] > >> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> > index 7964bbedb16c..59c00acfa0ed 100644 >> > --- a/drivers/net/ethernet/intel/igc/igc_main.c >> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> > @@ -4948,6 +4948,47 @@ static int igc_sw_init(struct igc_adapter *adapter) >> > return 0; >> > } >> > >> > +void igc_set_queue_napi(struct igc_adapter *adapter, int q_idx, >> > + struct napi_struct *napi) >> > +{ >> > + if (adapter->flags & IGC_FLAG_QUEUE_PAIRS) { >> > + netif_queue_set_napi(adapter->netdev, q_idx, >> > + NETDEV_QUEUE_TYPE_RX, napi); >> > + netif_queue_set_napi(adapter->netdev, q_idx, >> > + NETDEV_QUEUE_TYPE_TX, napi); >> > + } else { >> > + if (q_idx < adapter->num_rx_queues) { >> > + netif_queue_set_napi(adapter->netdev, q_idx, >> > + NETDEV_QUEUE_TYPE_RX, napi); >> > + } else { >> > + q_idx -= adapter->num_rx_queues; >> > + netif_queue_set_napi(adapter->netdev, q_idx, >> > + NETDEV_QUEUE_TYPE_TX, napi); >> > + } >> > + } >> > +} >> >> In addition, to what Vinicius said. I think this can be done >> simpler. Something like this? >> >> void igc_set_queue_napi(struct igc_adapter *adapter, int vector, >> struct napi_struct *napi) >> { >> struct igc_q_vector *q_vector = adapter->q_vector[vector]; >> >> if (q_vector->rx.ring) >> netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_RX, napi); >> >> if (q_vector->tx.ring) >> netif_queue_set_napi(adapter->netdev, vector, NETDEV_QUEUE_TYPE_TX, napi); >> } > > I tried this suggestion but this does not result in correct output > in the case where IGC_FLAG_QUEUE_PAIRS is disabled. > > The output from netlink: > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump queue-get --json='{"ifindex": 2}' > > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'type': 'tx'}] > > Note the lack of a napi-id for the TX queues. This typically happens > when the linking is not done correctly; netif_queue_set_napi should > take a queue id as the second parameter. > > I believe the suggested code above should be modified to be as > follows to use ring->queue_index: > > if (q_vector->rx.ring) > netif_queue_set_napi(adapter->netdev, > q_vector->rx.ring->queue_index, > NETDEV_QUEUE_TYPE_RX, napi); > > if (q_vector->tx.ring) > netif_queue_set_napi(adapter->netdev, > q_vector->tx.ring->queue_index, > NETDEV_QUEUE_TYPE_TX, napi); LGTM. Thanks. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 861 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-21 5:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-14 21:30 [RFC net-next v2 0/2] igc: Link IRQs and queues to NAPIs Joe Damato 2024-10-14 21:30 ` [RFC net-next v2 1/2] igc: Link IRQs to NAPI instances Joe Damato 2024-10-14 21:30 ` [RFC net-next v2 2/2] igc: Link queues " Joe Damato 2024-10-15 1:51 ` Vinicius Costa Gomes 2024-10-15 3:44 ` Joe Damato 2024-10-15 10:27 ` Kurt Kanzenbach 2024-10-16 1:01 ` Joe Damato 2024-10-18 17:04 ` Joe Damato 2024-10-21 5:42 ` Kurt Kanzenbach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).