RE: [net-next 2/2] ena: Link queues to NAPIs

RE: [net-next 2/2] ena: Link queues to NAPIs

[Arinzon, David]

> > > >
> > > > 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.

It's not at least a 24hr timeout?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-netdev.rst#n394

> 
> > 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

I picked up the patch and got to the same results that you did when running on an EC2 instance.
Thank you for sharing the commands in the commit messages, it was really helpful.
Correct me if I'm wrong, but there's no functional impact to these changes except the ability to
view the mappings through netlink.

Re: [net-next 2/2] ena: Link queues to NAPIs

[Joe Damato]

On Tue, Oct 01, 2024 at 04:40:32PM +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.
> 
> It's not at least a 24hr timeout?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-netdev.rst#n394

Ah, looks like you are right. For some reason I had 48hr in my head;
I think I usually wait a bit longer for larger / more complicated
stuff, but in this case 24hr seems OK.
 
> > 
> > > 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
> 
> I picked up the patch and got to the same results that you did when running on an EC2 instance.
> Thank you for sharing the commands in the commit messages, it was really helpful.
> Correct me if I'm wrong, but there's no functional impact to these changes except the ability to
> view the mappings through netlink.

This doesn't change anything about how the driver processes packets
or handles data or anything. It's a "control plane" sort of change;
it allows the mapping of IRQs queues and NAPI IDs to be exposed via
netlink from core networking code (see also: net/core/netdev-genl.c
and net/core/netdev-genl-gen.c).

This can be useful if an app uses, for example,
SO_INCOMING_NAPI_ID (typically used for busy polling, but not
required to busy poll).

A user app might design some logic like (just making this up as an
example):

  New fd from accept() has NAPI_ID 123 which corresponds to ifindex
  3 and so thread number X should handle the connection, because it
  is pinned to a CPU that is most optimal for ifindex 3 (e.g. NUMA
  local or softIRQ local or whatever the user app wants).

Without this change, user apps can get the NAPI id of an FD but have
no way to know which ifindex or queue it is associated with.

It also allows user apps to more easily determine which IRQ maps to
which queue (without having to parse, say /proc/interrupts).

This can be used for monitoring/alerting/observability purposes by
user apps trying to track down the source of hardware IRQ
generation. Say you are using RSS to direct specific types of flows
to specific queues, knowing which queues are associated with which
IRQs (via their NAPI ID) can help narrow down where IRQ generation
is coming from.

Overall: there's a lot of use cases where exposing this mapping to
userland can be very helpful.

[net-next 0/2] ena: Link IRQs, queues, and NAPI instances

[Joe Damato]

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

[net-next 2/2] ena: Link queues to NAPIs

[Joe Damato]

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

RE: [net-next 2/2] ena: Link queues to NAPIs

[Arinzon, David]

> 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.

Re: [net-next 2/2] ena: Link queues to NAPIs

[Joe Damato]

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?

RE: [net-next 2/2] ena: Link queues to NAPIs

[Arinzon, David]

> > > 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.

Re: [net-next 2/2] ena: Link queues to NAPIs

[Joe Damato]

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