* [net-next 0/2] ena: Link IRQs, queues, and NAPI instances @ 2024-09-30 19:56 Joe Damato 2024-09-30 19:56 ` [net-next 1/2] ena: Link IRQs to " Joe Damato 2024-09-30 19:56 ` [net-next 2/2] ena: Link queues to NAPIs Joe Damato 0 siblings, 2 replies; 9+ messages in thread From: Joe Damato @ 2024-09-30 19:56 UTC (permalink / raw) To: netdev Cc: Joe Damato, Arthur Kiyanovski, David Arinzon, David S. Miller, Eric Dumazet, Jakub Kicinski, Kamal Heib, open list, Noam Dagan, Paolo Abeni, Saeed Bishara, Shay Agroskin Greetings: This series uses the netdev-genl API to link IRQs and queues to NAPI IDs so that this information is queryable by user apps. This is particularly useful for epoll-based busy polling apps which rely on having access to the NAPI ID. I've tested these commits on an EC2 instance with an ENA NIC configured and have included test output in the commit messages for each patch showing how to query the information. I noted in the implementation that the driver requests an IRQ for management purposes which does not have an associated NAPI. I tried to take this into account in patch 1, but would appreciate if ENA maintainers can verify I did this correctly. Thanks, Joe Joe Damato (2): ena: Link IRQs to NAPI instances ena: Link queues to NAPIs drivers/net/ethernet/amazon/ena/ena_netdev.c | 38 +++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [net-next 1/2] ena: Link IRQs to NAPI instances 2024-09-30 19:56 [net-next 0/2] ena: Link IRQs, queues, and NAPI instances Joe Damato @ 2024-09-30 19:56 ` Joe Damato 2024-10-01 8:57 ` Arinzon, David 2024-09-30 19:56 ` [net-next 2/2] ena: Link queues to NAPIs Joe Damato 1 sibling, 1 reply; 9+ messages in thread From: Joe Damato @ 2024-09-30 19:56 UTC (permalink / raw) To: netdev Cc: Joe Damato, Shay Agroskin, Arthur Kiyanovski, David Arinzon, Noam Dagan, Saeed Bishara, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list Link IRQs to NAPI instances with netif_napi_set_irq. This information can be queried with the netdev-genl API. Note that the ENA device appears to allocate an IRQ for management purposes which does not have a NAPI associated with it; this commit takes this into consideration to accurately construct a map between IRQs and NAPI instances. Compare the output of /proc/interrupts for my ena device with the output of netdev-genl after applying this patch: $ cat /proc/interrupts | grep enp55s0 | cut -f1 --delimiter=':' 94 95 96 97 98 99 100 101 $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump napi-get --json='{"ifindex": 2}' [{'id': 8208, 'ifindex': 2, 'irq': 101}, {'id': 8207, 'ifindex': 2, 'irq': 100}, {'id': 8206, 'ifindex': 2, 'irq': 99}, {'id': 8205, 'ifindex': 2, 'irq': 98}, {'id': 8204, 'ifindex': 2, 'irq': 97}, {'id': 8203, 'ifindex': 2, 'irq': 96}, {'id': 8202, 'ifindex': 2, 'irq': 95}, {'id': 8201, 'ifindex': 2, 'irq': 94}] Signed-off-by: Joe Damato <jdamato@fastly.com> --- drivers/net/ethernet/amazon/ena/ena_netdev.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c index c5b50cfa935a..e88de5e426ef 100644 --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c @@ -1679,7 +1679,7 @@ static int ena_request_io_irq(struct ena_adapter *adapter) u32 io_queue_count = adapter->num_io_queues + adapter->xdp_num_queues; unsigned long flags = 0; struct ena_irq *irq; - int rc = 0, i, k; + int rc = 0, i, k, irq_idx; if (!test_bit(ENA_FLAG_MSIX_ENABLED, &adapter->flags)) { netif_err(adapter, ifup, adapter->netdev, @@ -1705,6 +1705,16 @@ static int ena_request_io_irq(struct ena_adapter *adapter) irq_set_affinity_hint(irq->vector, &irq->affinity_hint_mask); } + /* Now that IO IRQs have been successfully allocated map them to the + * corresponding IO NAPI instance. Note that the mgmnt IRQ does not + * have a NAPI, so care must be taken to correctly map IRQs to NAPIs. + */ + for (i = 0; i < io_queue_count; i++) { + irq_idx = ENA_IO_IRQ_IDX(i); + irq = &adapter->irq_tbl[irq_idx]; + netif_napi_set_irq(&adapter->ena_napi[i].napi, irq->vector); + } + return rc; err: -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [net-next 1/2] ena: Link IRQs to NAPI instances 2024-09-30 19:56 ` [net-next 1/2] ena: Link IRQs to " Joe Damato @ 2024-10-01 8:57 ` Arinzon, David 2024-10-01 16:18 ` Joe Damato 0 siblings, 1 reply; 9+ messages in thread From: Arinzon, David @ 2024-10-01 8:57 UTC (permalink / raw) To: Joe Damato, netdev@vger.kernel.org Cc: Agroskin, Shay, Kiyanovski, Arthur, Dagan, Noam, Bshara, Saeed, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list > Link IRQs to NAPI instances with netif_napi_set_irq. This information can be > queried with the netdev-genl API. Note that the ENA device appears to > allocate an IRQ for management purposes which does not have a NAPI > associated with it; this commit takes this into consideration to accurately > construct a map between IRQs and NAPI instances. > > Compare the output of /proc/interrupts for my ena device with the output > of netdev-genl after applying this patch: > > $ cat /proc/interrupts | grep enp55s0 | cut -f1 --delimiter=':' > 94 > 95 > 96 > 97 > 98 > 99 > 100 > 101 > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump napi-get --json='{"ifindex": 2}' > > [{'id': 8208, 'ifindex': 2, 'irq': 101}, > {'id': 8207, 'ifindex': 2, 'irq': 100}, > {'id': 8206, 'ifindex': 2, 'irq': 99}, > {'id': 8205, 'ifindex': 2, 'irq': 98}, > {'id': 8204, 'ifindex': 2, 'irq': 97}, > {'id': 8203, 'ifindex': 2, 'irq': 96}, > {'id': 8202, 'ifindex': 2, 'irq': 95}, > {'id': 8201, 'ifindex': 2, 'irq': 94}] > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > drivers/net/ethernet/amazon/ena/ena_netdev.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c > b/drivers/net/ethernet/amazon/ena/ena_netdev.c > index c5b50cfa935a..e88de5e426ef 100644 > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c > @@ -1679,7 +1679,7 @@ static int ena_request_io_irq(struct ena_adapter > *adapter) > u32 io_queue_count = adapter->num_io_queues + adapter- > >xdp_num_queues; > unsigned long flags = 0; > struct ena_irq *irq; > - int rc = 0, i, k; > + int rc = 0, i, k, irq_idx; nit: This breaks RCT guidelines, can you please move it to be below io_queue_count? > > if (!test_bit(ENA_FLAG_MSIX_ENABLED, &adapter->flags)) { > netif_err(adapter, ifup, adapter->netdev, @@ -1705,6 +1705,16 @@ > static int ena_request_io_irq(struct ena_adapter *adapter) > irq_set_affinity_hint(irq->vector, &irq->affinity_hint_mask); > } > > + /* Now that IO IRQs have been successfully allocated map them to the > + * corresponding IO NAPI instance. Note that the mgmnt IRQ does not > + * have a NAPI, so care must be taken to correctly map IRQs to NAPIs. > + */ > + for (i = 0; i < io_queue_count; i++) { > + irq_idx = ENA_IO_IRQ_IDX(i); > + irq = &adapter->irq_tbl[irq_idx]; > + netif_napi_set_irq(&adapter->ena_napi[i].napi, irq->vector); > + } > + > return rc; > > err: > -- > 2.43.0 Thank you for uploading this patch. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next 1/2] ena: Link IRQs to NAPI instances 2024-10-01 8:57 ` Arinzon, David @ 2024-10-01 16:18 ` Joe Damato 0 siblings, 0 replies; 9+ messages in thread From: Joe Damato @ 2024-10-01 16:18 UTC (permalink / raw) To: Arinzon, David Cc: netdev@vger.kernel.org, Agroskin, Shay, Kiyanovski, Arthur, Dagan, Noam, Bshara, Saeed, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list On Tue, Oct 01, 2024 at 08:57:47AM +0000, Arinzon, David wrote: > > Link IRQs to NAPI instances with netif_napi_set_irq. This information can be > > queried with the netdev-genl API. Note that the ENA device appears to > > allocate an IRQ for management purposes which does not have a NAPI > > associated with it; this commit takes this into consideration to accurately > > construct a map between IRQs and NAPI instances. > > > > Compare the output of /proc/interrupts for my ena device with the output > > of netdev-genl after applying this patch: > > > > $ cat /proc/interrupts | grep enp55s0 | cut -f1 --delimiter=':' > > 94 > > 95 > > 96 > > 97 > > 98 > > 99 > > 100 > > 101 > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > --dump napi-get --json='{"ifindex": 2}' > > > > [{'id': 8208, 'ifindex': 2, 'irq': 101}, > > {'id': 8207, 'ifindex': 2, 'irq': 100}, > > {'id': 8206, 'ifindex': 2, 'irq': 99}, > > {'id': 8205, 'ifindex': 2, 'irq': 98}, > > {'id': 8204, 'ifindex': 2, 'irq': 97}, > > {'id': 8203, 'ifindex': 2, 'irq': 96}, > > {'id': 8202, 'ifindex': 2, 'irq': 95}, > > {'id': 8201, 'ifindex': 2, 'irq': 94}] > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > drivers/net/ethernet/amazon/ena/ena_netdev.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c > > b/drivers/net/ethernet/amazon/ena/ena_netdev.c > > index c5b50cfa935a..e88de5e426ef 100644 > > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c > > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c > > @@ -1679,7 +1679,7 @@ static int ena_request_io_irq(struct ena_adapter > > *adapter) > > u32 io_queue_count = adapter->num_io_queues + adapter- > > >xdp_num_queues; > > unsigned long flags = 0; > > struct ena_irq *irq; > > - int rc = 0, i, k; > > + int rc = 0, i, k, irq_idx; > > nit: This breaks RCT guidelines, can you please move it to be below io_queue_count? You are right; sorry I missed that. Will fix in the next revision. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [net-next 2/2] ena: Link queues to NAPIs 2024-09-30 19:56 [net-next 0/2] ena: Link IRQs, queues, and NAPI instances Joe Damato 2024-09-30 19:56 ` [net-next 1/2] ena: Link IRQs to " Joe Damato @ 2024-09-30 19:56 ` Joe Damato 2024-10-01 9:06 ` Arinzon, David 1 sibling, 1 reply; 9+ messages in thread From: Joe Damato @ 2024-09-30 19:56 UTC (permalink / raw) To: netdev Cc: Joe Damato, Shay Agroskin, Arthur Kiyanovski, David Arinzon, Noam Dagan, Saeed Bishara, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list Link queues to NAPIs using the netdev-genl API so this information is queryable. $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ --dump queue-get --json='{"ifindex": 2}' [{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'}, {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'}, {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'}, {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'rx'}, {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'rx'}, {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'rx'}, {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'rx'}, {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'}, {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'}, {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'}, {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}, {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'tx'}, {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'tx'}, {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'tx'}, {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'tx'}] Signed-off-by: Joe Damato <jdamato@fastly.com> --- drivers/net/ethernet/amazon/ena/ena_netdev.c | 26 +++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c index e88de5e426ef..1c59aedaa5d5 100644 --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c @@ -1821,20 +1821,38 @@ static void ena_napi_disable_in_range(struct ena_adapter *adapter, int first_index, int count) { + struct napi_struct *napi; int i; - for (i = first_index; i < first_index + count; i++) - napi_disable(&adapter->ena_napi[i].napi); + for (i = first_index; i < first_index + count; i++) { + napi = &adapter->ena_napi[i].napi; + if (!ENA_IS_XDP_INDEX(adapter, i)) { + netif_queue_set_napi(adapter->netdev, i, + NETDEV_QUEUE_TYPE_TX, NULL); + netif_queue_set_napi(adapter->netdev, i, + NETDEV_QUEUE_TYPE_RX, NULL); + } + napi_disable(napi); + } } static void ena_napi_enable_in_range(struct ena_adapter *adapter, int first_index, int count) { + struct napi_struct *napi; int i; - for (i = first_index; i < first_index + count; i++) - napi_enable(&adapter->ena_napi[i].napi); + for (i = first_index; i < first_index + count; i++) { + napi = &adapter->ena_napi[i].napi; + napi_enable(napi); + if (!ENA_IS_XDP_INDEX(adapter, i)) { + netif_queue_set_napi(adapter->netdev, i, + NETDEV_QUEUE_TYPE_RX, napi); + netif_queue_set_napi(adapter->netdev, i, + NETDEV_QUEUE_TYPE_TX, napi); + } + } } /* Configure the Rx forwarding */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [net-next 2/2] ena: Link queues to NAPIs 2024-09-30 19:56 ` [net-next 2/2] ena: Link queues to NAPIs Joe Damato @ 2024-10-01 9:06 ` Arinzon, David 2024-10-01 14:28 ` Joe Damato 0 siblings, 1 reply; 9+ messages in thread From: Arinzon, David @ 2024-10-01 9:06 UTC (permalink / raw) To: Joe Damato, netdev@vger.kernel.org Cc: Agroskin, Shay, Kiyanovski, Arthur, Dagan, Noam, Bshara, Saeed, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list, Bernstein, Amit > Link queues to NAPIs using the netdev-genl API so this information is > queryable. > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > --dump queue-get --json='{"ifindex": 2}' > > [{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'}, > {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'rx'}, > {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'rx'}, > {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'rx'}, > {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'rx'}, > {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'}, > {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'}, > {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'}, > {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}, > {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'tx'}, > {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'tx'}, > {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'tx'}, > {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'tx'}] > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > drivers/net/ethernet/amazon/ena/ena_netdev.c | 26 > +++++++++++++++++--- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c > b/drivers/net/ethernet/amazon/ena/ena_netdev.c > index e88de5e426ef..1c59aedaa5d5 100644 > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c > @@ -1821,20 +1821,38 @@ static void ena_napi_disable_in_range(struct > ena_adapter *adapter, > int first_index, > int count) { > + struct napi_struct *napi; Is this variable necessary? It has been defined in the enable function because it is needed in netif_queue_set_napi() API as well as in napi_enable(), and it makes sense in order to avoid long lines In here, the variable is only used in a call to napi_disable(), can the code be kept as it is? don't see a need to shorten the napi_disable() call line. > int i; > > - for (i = first_index; i < first_index + count; i++) > - napi_disable(&adapter->ena_napi[i].napi); > + for (i = first_index; i < first_index + count; i++) { > + napi = &adapter->ena_napi[i].napi; > + if (!ENA_IS_XDP_INDEX(adapter, i)) { > + netif_queue_set_napi(adapter->netdev, i, > + NETDEV_QUEUE_TYPE_TX, NULL); > + netif_queue_set_napi(adapter->netdev, i, > + NETDEV_QUEUE_TYPE_RX, NULL); > + } > + napi_disable(napi); > + } > } > > static void ena_napi_enable_in_range(struct ena_adapter *adapter, > int first_index, > int count) { > + struct napi_struct *napi; > int i; > > - for (i = first_index; i < first_index + count; i++) > - napi_enable(&adapter->ena_napi[i].napi); > + for (i = first_index; i < first_index + count; i++) { > + napi = &adapter->ena_napi[i].napi; > + napi_enable(napi); > + if (!ENA_IS_XDP_INDEX(adapter, i)) { Can you share some info on why you decided to set the queue to napi association only in non-xdp case? In XDP, there's a napi poll function that's executed and it runs on the TX queue. I am assuming that it's because XDP is not yet supported in the framework? If so, there's a need to add an explicit comment above if (!ENA_IS_XDP_INDEX(adapter, i)) { explaining this in order to avoid confusion with the rest of the code. > + netif_queue_set_napi(adapter->netdev, i, > + NETDEV_QUEUE_TYPE_RX, napi); > + netif_queue_set_napi(adapter->netdev, i, > + NETDEV_QUEUE_TYPE_TX, napi); > + } > + } > } > > /* Configure the Rx forwarding */ > -- > 2.43.0 Thank you for uploading this patch. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next 2/2] ena: Link queues to NAPIs 2024-10-01 9:06 ` Arinzon, David @ 2024-10-01 14:28 ` Joe Damato 2024-10-01 15:50 ` Arinzon, David 0 siblings, 1 reply; 9+ messages in thread From: Joe Damato @ 2024-10-01 14:28 UTC (permalink / raw) To: Arinzon, David Cc: netdev@vger.kernel.org, Agroskin, Shay, Kiyanovski, Arthur, Dagan, Noam, Bshara, Saeed, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list, Bernstein, Amit On Tue, Oct 01, 2024 at 09:06:16AM +0000, Arinzon, David wrote: > > Link queues to NAPIs using the netdev-genl API so this information is > > queryable. > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ > > --dump queue-get --json='{"ifindex": 2}' > > > > [{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'}, > > {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'}, > > {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'}, > > {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'}, > > {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'rx'}, > > {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'rx'}, > > {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'rx'}, > > {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'rx'}, > > {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'}, > > {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'}, > > {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'}, > > {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}, > > {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'tx'}, > > {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'tx'}, > > {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'tx'}, > > {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'tx'}] > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > drivers/net/ethernet/amazon/ena/ena_netdev.c | 26 > > +++++++++++++++++--- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c > > b/drivers/net/ethernet/amazon/ena/ena_netdev.c > > index e88de5e426ef..1c59aedaa5d5 100644 > > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c > > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c > > @@ -1821,20 +1821,38 @@ static void ena_napi_disable_in_range(struct > > ena_adapter *adapter, > > int first_index, > > int count) { > > + struct napi_struct *napi; > > Is this variable necessary? It has been defined in the enable function because > it is needed in netif_queue_set_napi() API as well as in napi_enable(), > and it makes sense in order to avoid long lines > In here, the variable is only used in a call to napi_disable(), can the code > be kept as it is? don't see a need to shorten the napi_disable() call line. It is true that its only used to call napi_disable so if you prefer to have it removed let me know? I think it looks nicer with the variable, but it's your driver. > > int i; > > > > - for (i = first_index; i < first_index + count; i++) > > - napi_disable(&adapter->ena_napi[i].napi); > > + for (i = first_index; i < first_index + count; i++) { > > + napi = &adapter->ena_napi[i].napi; > > + if (!ENA_IS_XDP_INDEX(adapter, i)) { > > + netif_queue_set_napi(adapter->netdev, i, > > + NETDEV_QUEUE_TYPE_TX, NULL); > > + netif_queue_set_napi(adapter->netdev, i, > > + NETDEV_QUEUE_TYPE_RX, NULL); > > + } > > + napi_disable(napi); > > + } > > } > > > > static void ena_napi_enable_in_range(struct ena_adapter *adapter, > > int first_index, > > int count) { > > + struct napi_struct *napi; > > int i; > > > > - for (i = first_index; i < first_index + count; i++) > > - napi_enable(&adapter->ena_napi[i].napi); > > + for (i = first_index; i < first_index + count; i++) { > > + napi = &adapter->ena_napi[i].napi; > > + napi_enable(napi); > > + if (!ENA_IS_XDP_INDEX(adapter, i)) { > > Can you share some info on why you decided to set the queue to napi association > only in non-xdp case? > In XDP, there's a napi poll function that's executed and it runs on the TX queue. > I am assuming that it's because XDP is not yet supported in the framework? If so, > there's a need to add an explicit comment above if (!ENA_IS_XDP_INDEX(adapter, i)) { > explaining this in order to avoid confusion with the rest of the code. Yes; it is skipped for XDP queues, but they could be supported in the future. Other drivers that support this API work similarly (see also: bnxt, ice, mlx4, etc). > > + netif_queue_set_napi(adapter->netdev, i, > > + NETDEV_QUEUE_TYPE_RX, napi); > > + netif_queue_set_napi(adapter->netdev, i, > > + NETDEV_QUEUE_TYPE_TX, napi); > > + } > > + } > > } > > > > /* Configure the Rx forwarding */ > > -- > > 2.43.0 > > Thank you for uploading this patch. Can you please let me know (explicitly) if you want me to send a second revision (when net-next allows for it) to remove the 'struct napi_struct' and add a comment as described above? ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [net-next 2/2] ena: Link queues to NAPIs 2024-10-01 14:28 ` Joe Damato @ 2024-10-01 15:50 ` Arinzon, David 2024-10-01 16:21 ` Joe Damato 0 siblings, 1 reply; 9+ messages in thread From: Arinzon, David @ 2024-10-01 15:50 UTC (permalink / raw) To: Joe Damato Cc: netdev@vger.kernel.org, Agroskin, Shay, Kiyanovski, Arthur, Dagan, Noam, Bshara, Saeed, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list, Bernstein, Amit > > > Link queues to NAPIs using the netdev-genl API so this information > > > is queryable. > > > > > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml > \ > > > --dump queue-get --json='{"ifindex": 2}' > > > > > > [{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'}, > > > {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'}, > > > {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'}, > > > {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'}, > > > {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'rx'}, > > > {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'rx'}, > > > {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'rx'}, > > > {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'rx'}, > > > {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'}, > > > {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'}, > > > {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'}, > > > {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}, > > > {'id': 4, 'ifindex': 2, 'napi-id': 8205, 'type': 'tx'}, > > > {'id': 5, 'ifindex': 2, 'napi-id': 8206, 'type': 'tx'}, > > > {'id': 6, 'ifindex': 2, 'napi-id': 8207, 'type': 'tx'}, > > > {'id': 7, 'ifindex': 2, 'napi-id': 8208, 'type': 'tx'}] > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > drivers/net/ethernet/amazon/ena/ena_netdev.c | 26 > > > +++++++++++++++++--- > > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c > > > b/drivers/net/ethernet/amazon/ena/ena_netdev.c > > > index e88de5e426ef..1c59aedaa5d5 100644 > > > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c > > > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c > > > @@ -1821,20 +1821,38 @@ static void ena_napi_disable_in_range(struct > > > ena_adapter *adapter, > > > int first_index, > > > int count) { > > > + struct napi_struct *napi; > > > > Is this variable necessary? It has been defined in the enable function > > because it is needed in netif_queue_set_napi() API as well as in > > napi_enable(), and it makes sense in order to avoid long lines In > > here, the variable is only used in a call to napi_disable(), can the > > code be kept as it is? don't see a need to shorten the napi_disable() call > line. > > It is true that its only used to call napi_disable so if you prefer to have it > removed let me know? > > I think it looks nicer with the variable, but it's your driver. > > > > int i; > > > > > > - for (i = first_index; i < first_index + count; i++) > > > - napi_disable(&adapter->ena_napi[i].napi); > > > + for (i = first_index; i < first_index + count; i++) { > > > + napi = &adapter->ena_napi[i].napi; > > > + if (!ENA_IS_XDP_INDEX(adapter, i)) { > > > + netif_queue_set_napi(adapter->netdev, i, > > > + NETDEV_QUEUE_TYPE_TX, NULL); > > > + netif_queue_set_napi(adapter->netdev, i, > > > + NETDEV_QUEUE_TYPE_RX, NULL); > > > + } > > > + napi_disable(napi); > > > + } > > > } > > > > > > static void ena_napi_enable_in_range(struct ena_adapter *adapter, > > > int first_index, > > > int count) { > > > + struct napi_struct *napi; > > > int i; > > > > > > - for (i = first_index; i < first_index + count; i++) > > > - napi_enable(&adapter->ena_napi[i].napi); > > > + for (i = first_index; i < first_index + count; i++) { > > > + napi = &adapter->ena_napi[i].napi; > > > + napi_enable(napi); > > > + if (!ENA_IS_XDP_INDEX(adapter, i)) { > > > > Can you share some info on why you decided to set the queue to napi > > association only in non-xdp case? > > In XDP, there's a napi poll function that's executed and it runs on the TX > queue. > > I am assuming that it's because XDP is not yet supported in the > > framework? If so, there's a need to add an explicit comment above if > > (!ENA_IS_XDP_INDEX(adapter, i)) { explaining this in order to avoid > confusion with the rest of the code. > > Yes; it is skipped for XDP queues, but they could be supported in the future. > > Other drivers that support this API work similarly (see also: bnxt, ice, mlx4, > etc). > > > > + netif_queue_set_napi(adapter->netdev, i, > > > + NETDEV_QUEUE_TYPE_RX, napi); > > > + netif_queue_set_napi(adapter->netdev, i, > > > + NETDEV_QUEUE_TYPE_TX, napi); > > > + } > > > + } > > > } > > > > > > /* Configure the Rx forwarding */ > > > -- > > > 2.43.0 > > > > Thank you for uploading this patch. > > Can you please let me know (explicitly) if you want me to send a second > revision (when net-next allows for it) to remove the 'struct napi_struct' and > add a comment as described above? Yes, I would appreciate that. I guess the `struct napi_struct` is OK, this way both functions will look the same. Regarding the comment, yes please, something like /* This API is supported for non-XDP queues only */ in both places. I also added a small request to preserve reverse christmas tree order on patch 1/2 in the series. Thank you again for the patches in the driver. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next 2/2] ena: Link queues to NAPIs 2024-10-01 15:50 ` Arinzon, David @ 2024-10-01 16:21 ` Joe Damato 0 siblings, 0 replies; 9+ messages in thread From: Joe Damato @ 2024-10-01 16:21 UTC (permalink / raw) To: Arinzon, David Cc: netdev@vger.kernel.org, Agroskin, Shay, Kiyanovski, Arthur, Dagan, Noam, Bshara, Saeed, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kamal Heib, open list, Bernstein, Amit On Tue, Oct 01, 2024 at 03:50:24PM +0000, Arinzon, David wrote: [...] > > > > > > Thank you for uploading this patch. > > > > Can you please let me know (explicitly) if you want me to send a second > > revision (when net-next allows for it) to remove the 'struct napi_struct' and > > add a comment as described above? > > Yes, I would appreciate that. > I guess the `struct napi_struct` is OK, this way both functions will look the same. > Regarding the comment, yes please, something like /* This API is supported for non-XDP queues only */ in both places. > I also added a small request to preserve reverse christmas tree order on patch 1/2 in the series. Thanks for mentioning the nit about reverse christmas tree order; I missed that. I will: - Fix the ordering of the variables in 1/2 - Add 2 comments in 2/2 I'll have to wait the ~48hr timeout before I can post the v2, but I'll be sure to CC you on the next revision. > Thank you again for the patches in the driver. No worries, thanks for the review. BTW: Since neither of the changes you've asked me to make are functional changes, would you mind testing the driver changes on your side just to make sure? I tested them myself on an ec2 instance with an ENA driver, but I am not an expert on ENA :) - Joe ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-01 16:22 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-30 19:56 [net-next 0/2] ena: Link IRQs, queues, and NAPI instances Joe Damato 2024-09-30 19:56 ` [net-next 1/2] ena: Link IRQs to " Joe Damato 2024-10-01 8:57 ` Arinzon, David 2024-10-01 16:18 ` Joe Damato 2024-09-30 19:56 ` [net-next 2/2] ena: Link queues to NAPIs Joe Damato 2024-10-01 9:06 ` Arinzon, David 2024-10-01 14:28 ` Joe Damato 2024-10-01 15:50 ` Arinzon, David 2024-10-01 16:21 ` Joe Damato
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).