[PATCH net-next] tsnep: Link queues to NAPIs

[PATCH net-next] tsnep: Link queues to NAPIs

[Gerhard Engleder]

Use netif_queue_set_napi() to link queues to NAPI instances so that they
can be queried with netlink.

$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
                         --dump queue-get --json='{"ifindex": 11}'
[{'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'rx'},
 {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'rx'},
 {'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'tx'},
 {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'tx'}]

Additionally use netif_napi_set_irq() to also provide NAPI interrupt
number to userspace.

$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
                         --do napi-get --json='{"id": 9}'
{'defer-hard-irqs': 0,
 'gro-flush-timeout': 0,
 'id': 9,
 'ifindex': 11,
 'irq': 42,
 'irq-suspend-timeout': 0}

Providing information about queues to userspace makes sense as APIs like
XSK provide queue specific access. Also XSK busy polling relies on
queues linked to NAPIs.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 28 ++++++++++++++++++----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 45b9f5780902..71e950e023dc 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -1984,23 +1984,41 @@ static int tsnep_queue_open(struct tsnep_adapter *adapter,
 
 static void tsnep_queue_enable(struct tsnep_queue *queue)
 {
+	struct tsnep_adapter *adapter = queue->adapter;
+
+	netif_napi_set_irq(&queue->napi, queue->irq);
 	napi_enable(&queue->napi);
-	tsnep_enable_irq(queue->adapter, queue->irq_mask);
+	tsnep_enable_irq(adapter, queue->irq_mask);
 
-	if (queue->tx)
+	if (queue->tx) {
+		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
+				     NETDEV_QUEUE_TYPE_TX, &queue->napi);
 		tsnep_tx_enable(queue->tx);
+	}
 
-	if (queue->rx)
+	if (queue->rx) {
+		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
+				     NETDEV_QUEUE_TYPE_RX, &queue->napi);
 		tsnep_rx_enable(queue->rx);
+	}
 }
 
 static void tsnep_queue_disable(struct tsnep_queue *queue)
 {
-	if (queue->tx)
+	struct tsnep_adapter *adapter = queue->adapter;
+
+	if (queue->rx)
+		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
+				     NETDEV_QUEUE_TYPE_RX, NULL);
+
+	if (queue->tx) {
 		tsnep_tx_disable(queue->tx, &queue->napi);
+		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
+				     NETDEV_QUEUE_TYPE_TX, NULL);
+	}
 
 	napi_disable(&queue->napi);
-	tsnep_disable_irq(queue->adapter, queue->irq_mask);
+	tsnep_disable_irq(adapter, queue->irq_mask);
 
 	/* disable RX after NAPI polling has been disabled, because RX can be
 	 * enabled during NAPI polling
-- 
2.39.5

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Alexander Lobakin]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Fri, 10 Jan 2025 23:39:39 +0100

> Use netif_queue_set_napi() to link queues to NAPI instances so that they
> can be queried with netlink.
> 
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --dump queue-get --json='{"ifindex": 11}'
> [{'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'rx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'rx'},
>  {'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'tx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'tx'}]
> 
> Additionally use netif_napi_set_irq() to also provide NAPI interrupt
> number to userspace.
> 
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --do napi-get --json='{"id": 9}'
> {'defer-hard-irqs': 0,
>  'gro-flush-timeout': 0,
>  'id': 9,
>  'ifindex': 11,
>  'irq': 42,
>  'irq-suspend-timeout': 0}
> 
> Providing information about queues to userspace makes sense as APIs like
> XSK provide queue specific access. Also XSK busy polling relies on
> queues linked to NAPIs.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>

Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Thanks,
Olek

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Joe Damato]

On Fri, Jan 10, 2025 at 11:39:39PM +0100, Gerhard Engleder wrote:
> Use netif_queue_set_napi() to link queues to NAPI instances so that they
> can be queried with netlink.
> 
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --dump queue-get --json='{"ifindex": 11}'
> [{'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'rx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'rx'},
>  {'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'tx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'tx'}]
> 
> Additionally use netif_napi_set_irq() to also provide NAPI interrupt
> number to userspace.
> 
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --do napi-get --json='{"id": 9}'
> {'defer-hard-irqs': 0,
>  'gro-flush-timeout': 0,
>  'id': 9,
>  'ifindex': 11,
>  'irq': 42,
>  'irq-suspend-timeout': 0}
> 
> Providing information about queues to userspace makes sense as APIs like
> XSK provide queue specific access. Also XSK busy polling relies on
> queues linked to NAPIs.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep_main.c | 28 ++++++++++++++++++----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index 45b9f5780902..71e950e023dc 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -1984,23 +1984,41 @@ static int tsnep_queue_open(struct tsnep_adapter *adapter,
>  
>  static void tsnep_queue_enable(struct tsnep_queue *queue)
>  {
> +	struct tsnep_adapter *adapter = queue->adapter;
> +
> +	netif_napi_set_irq(&queue->napi, queue->irq);
>  	napi_enable(&queue->napi);
> -	tsnep_enable_irq(queue->adapter, queue->irq_mask);
> +	tsnep_enable_irq(adapter, queue->irq_mask);
>  
> -	if (queue->tx)
> +	if (queue->tx) {
> +		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
> +				     NETDEV_QUEUE_TYPE_TX, &queue->napi);
>  		tsnep_tx_enable(queue->tx);
> +	}
>  
> -	if (queue->rx)
> +	if (queue->rx) {
> +		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
> +				     NETDEV_QUEUE_TYPE_RX, &queue->napi);
>  		tsnep_rx_enable(queue->rx);
> +	}
>  }
>  
>  static void tsnep_queue_disable(struct tsnep_queue *queue)
A>  {
> -	if (queue->tx)
> +	struct tsnep_adapter *adapter = queue->adapter;
> +
> +	if (queue->rx)
> +		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
> +				     NETDEV_QUEUE_TYPE_RX, NULL);
> +
> +	if (queue->tx) {
>  		tsnep_tx_disable(queue->tx, &queue->napi);
> +		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
> +				     NETDEV_QUEUE_TYPE_TX, NULL);
> +	}
>  
>  	napi_disable(&queue->napi);
> -	tsnep_disable_irq(queue->adapter, queue->irq_mask);
> +	tsnep_disable_irq(adapter, queue->irq_mask);
>  
>  	/* disable RX after NAPI polling has been disabled, because RX can be
>  	 * enabled during NAPI polling

The changes generally look OK to me (it seems RTNL is held on all
paths where this code can be called from as far as I can tell), but
there was one thing that stood out to me.

AFAIU, drivers avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
or NETDEV_QUEUE_TYPE_TX. I could be wrong, but that was my
understanding and I submit patches to several drivers with this
assumption.

For example, in commit b65969856d4f ("igc: Link queues to NAPI
instances"), I unlinked/linked the NAPIs and queue IDs when XDP was
enabled/disabled. Likewise, in commit 64b62146ba9e ("net/mlx4: link
NAPI instances to queues and IRQs"), I avoided the XDP queues.

If drivers are to avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
or NETDEV_QUEUE_TYPE_TX, perhaps tsnep needs to be modified
similarly?

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Gerhard Engleder]

On 13.01.25 20:59, Joe Damato wrote:
> On Fri, Jan 10, 2025 at 11:39:39PM +0100, Gerhard Engleder wrote:
>> Use netif_queue_set_napi() to link queues to NAPI instances so that they
>> can be queried with netlink.
>>
>> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>>                           --dump queue-get --json='{"ifindex": 11}'
>> [{'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'rx'},
>>   {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'rx'},
>>   {'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'tx'},
>>   {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'tx'}]
>>
>> Additionally use netif_napi_set_irq() to also provide NAPI interrupt
>> number to userspace.
>>
>> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>>                           --do napi-get --json='{"id": 9}'
>> {'defer-hard-irqs': 0,
>>   'gro-flush-timeout': 0,
>>   'id': 9,
>>   'ifindex': 11,
>>   'irq': 42,
>>   'irq-suspend-timeout': 0}
>>
>> Providing information about queues to userspace makes sense as APIs like
>> XSK provide queue specific access. Also XSK busy polling relies on
>> queues linked to NAPIs.
>>
>> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
>> ---
>>   drivers/net/ethernet/engleder/tsnep_main.c | 28 ++++++++++++++++++----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
>> index 45b9f5780902..71e950e023dc 100644
>> --- a/drivers/net/ethernet/engleder/tsnep_main.c
>> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
>> @@ -1984,23 +1984,41 @@ static int tsnep_queue_open(struct tsnep_adapter *adapter,
>>   
>>   static void tsnep_queue_enable(struct tsnep_queue *queue)
>>   {
>> +	struct tsnep_adapter *adapter = queue->adapter;
>> +
>> +	netif_napi_set_irq(&queue->napi, queue->irq);
>>   	napi_enable(&queue->napi);
>> -	tsnep_enable_irq(queue->adapter, queue->irq_mask);
>> +	tsnep_enable_irq(adapter, queue->irq_mask);
>>   
>> -	if (queue->tx)
>> +	if (queue->tx) {
>> +		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
>> +				     NETDEV_QUEUE_TYPE_TX, &queue->napi);
>>   		tsnep_tx_enable(queue->tx);
>> +	}
>>   
>> -	if (queue->rx)
>> +	if (queue->rx) {
>> +		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
>> +				     NETDEV_QUEUE_TYPE_RX, &queue->napi);
>>   		tsnep_rx_enable(queue->rx);
>> +	}
>>   }
>>   
>>   static void tsnep_queue_disable(struct tsnep_queue *queue)
> A>  {
>> -	if (queue->tx)
>> +	struct tsnep_adapter *adapter = queue->adapter;
>> +
>> +	if (queue->rx)
>> +		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
>> +				     NETDEV_QUEUE_TYPE_RX, NULL);
>> +
>> +	if (queue->tx) {
>>   		tsnep_tx_disable(queue->tx, &queue->napi);
>> +		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
>> +				     NETDEV_QUEUE_TYPE_TX, NULL);
>> +	}
>>   
>>   	napi_disable(&queue->napi);
>> -	tsnep_disable_irq(queue->adapter, queue->irq_mask);
>> +	tsnep_disable_irq(adapter, queue->irq_mask);
>>   
>>   	/* disable RX after NAPI polling has been disabled, because RX can be
>>   	 * enabled during NAPI polling
> 
> The changes generally look OK to me (it seems RTNL is held on all
> paths where this code can be called from as far as I can tell), but
> there was one thing that stood out to me.
> 
> AFAIU, drivers avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> or NETDEV_QUEUE_TYPE_TX. I could be wrong, but that was my
> understanding and I submit patches to several drivers with this
> assumption.
> 
> For example, in commit b65969856d4f ("igc: Link queues to NAPI
> instances"), I unlinked/linked the NAPIs and queue IDs when XDP was
> enabled/disabled. Likewise, in commit 64b62146ba9e ("net/mlx4: link
> NAPI instances to queues and IRQs"), I avoided the XDP queues.
> 
> If drivers are to avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> or NETDEV_QUEUE_TYPE_TX, perhaps tsnep needs to be modified
> similarly?

With 5ef44b3cb4 ("xsk: Bring back busy polling support") the linking of
the NAPIs is required for XDP/XSK. So it is strange to me if for XDP/XSK
the NAPIs should be unlinked. But I'm not an expert, so maybe there is
a reason why.

I added Magnus, maybe he knows if XSK queues shall still be linked to
NAPIs.

Gerhard

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Joe Damato]

On Mon, Jan 13, 2025 at 09:32:23PM +0100, Gerhard Engleder wrote:
> On 13.01.25 20:59, Joe Damato wrote:
> > On Fri, Jan 10, 2025 at 11:39:39PM +0100, Gerhard Engleder wrote:
> > > Use netif_queue_set_napi() to link queues to NAPI instances so that they
> > > can be queried with netlink.
> > > 
> > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> > >                           --dump queue-get --json='{"ifindex": 11}'
> > > [{'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'rx'},
> > >   {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'rx'},
> > >   {'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'tx'},
> > >   {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'tx'}]
> > > 
> > > Additionally use netif_napi_set_irq() to also provide NAPI interrupt
> > > number to userspace.
> > > 
> > > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> > >                           --do napi-get --json='{"id": 9}'
> > > {'defer-hard-irqs': 0,
> > >   'gro-flush-timeout': 0,
> > >   'id': 9,
> > >   'ifindex': 11,
> > >   'irq': 42,
> > >   'irq-suspend-timeout': 0}
> > > 
> > > Providing information about queues to userspace makes sense as APIs like
> > > XSK provide queue specific access. Also XSK busy polling relies on
> > > queues linked to NAPIs.
> > > 
> > > Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> > > ---
> > >   drivers/net/ethernet/engleder/tsnep_main.c | 28 ++++++++++++++++++----
> > >   1 file changed, 23 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> > > index 45b9f5780902..71e950e023dc 100644
> > > --- a/drivers/net/ethernet/engleder/tsnep_main.c
> > > +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> > > @@ -1984,23 +1984,41 @@ static int tsnep_queue_open(struct tsnep_adapter *adapter,
> > >   static void tsnep_queue_enable(struct tsnep_queue *queue)
> > >   {
> > > +	struct tsnep_adapter *adapter = queue->adapter;
> > > +
> > > +	netif_napi_set_irq(&queue->napi, queue->irq);
> > >   	napi_enable(&queue->napi);
> > > -	tsnep_enable_irq(queue->adapter, queue->irq_mask);
> > > +	tsnep_enable_irq(adapter, queue->irq_mask);
> > > -	if (queue->tx)
> > > +	if (queue->tx) {
> > > +		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
> > > +				     NETDEV_QUEUE_TYPE_TX, &queue->napi);
> > >   		tsnep_tx_enable(queue->tx);
> > > +	}
> > > -	if (queue->rx)
> > > +	if (queue->rx) {
> > > +		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
> > > +				     NETDEV_QUEUE_TYPE_RX, &queue->napi);
> > >   		tsnep_rx_enable(queue->rx);
> > > +	}
> > >   }
> > >   static void tsnep_queue_disable(struct tsnep_queue *queue)
> > A>  {
> > > -	if (queue->tx)
> > > +	struct tsnep_adapter *adapter = queue->adapter;
> > > +
> > > +	if (queue->rx)
> > > +		netif_queue_set_napi(adapter->netdev, queue->rx->queue_index,
> > > +				     NETDEV_QUEUE_TYPE_RX, NULL);
> > > +
> > > +	if (queue->tx) {
> > >   		tsnep_tx_disable(queue->tx, &queue->napi);
> > > +		netif_queue_set_napi(adapter->netdev, queue->tx->queue_index,
> > > +				     NETDEV_QUEUE_TYPE_TX, NULL);
> > > +	}
> > >   	napi_disable(&queue->napi);
> > > -	tsnep_disable_irq(queue->adapter, queue->irq_mask);
> > > +	tsnep_disable_irq(adapter, queue->irq_mask);
> > >   	/* disable RX after NAPI polling has been disabled, because RX can be
> > >   	 * enabled during NAPI polling
> > 
> > The changes generally look OK to me (it seems RTNL is held on all
> > paths where this code can be called from as far as I can tell), but
> > there was one thing that stood out to me.
> > 
> > AFAIU, drivers avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> > or NETDEV_QUEUE_TYPE_TX. I could be wrong, but that was my
> > understanding and I submit patches to several drivers with this
> > assumption.
> > 
> > For example, in commit b65969856d4f ("igc: Link queues to NAPI
> > instances"), I unlinked/linked the NAPIs and queue IDs when XDP was
> > enabled/disabled. Likewise, in commit 64b62146ba9e ("net/mlx4: link
> > NAPI instances to queues and IRQs"), I avoided the XDP queues.
> > 
> > If drivers are to avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> > or NETDEV_QUEUE_TYPE_TX, perhaps tsnep needs to be modified
> > similarly?
> 
> With 5ef44b3cb4 ("xsk: Bring back busy polling support") the linking of
> the NAPIs is required for XDP/XSK. So it is strange to me if for XDP/XSK
> the NAPIs should be unlinked. But I'm not an expert, so maybe there is
> a reason why.
> 
> I added Magnus, maybe he knows if XSK queues shall still be linked to
> NAPIs.

OK, so I think I was probably just wrong?

I looked at bnxt and it seems to mark XDP queues, which means
probably my patches for igc, ena, and mlx4 need to be fixed and the
proposed patch I have for virtio_net needs to be adjusted.

I can't remember now why I thought XDP queues should be avoided. I
feel like I read that or got that as feedback at some point, but I
can't remember now. Maybe it was just one driver or something I was
working on and I accidentally thought it should be avoided
everywhere? Not sure.

Hopefully some one can give a definitive answer on this one before I
go through and try to fix all the drivers I modified :|

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Jakub Kicinski]

On Mon, 13 Jan 2025 13:48:14 -0800 Joe Damato wrote:
> > > The changes generally look OK to me (it seems RTNL is held on all
> > > paths where this code can be called from as far as I can tell), but
> > > there was one thing that stood out to me.
> > > 
> > > AFAIU, drivers avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> > > or NETDEV_QUEUE_TYPE_TX. I could be wrong, but that was my
> > > understanding and I submit patches to several drivers with this
> > > assumption.
> > > 
> > > For example, in commit b65969856d4f ("igc: Link queues to NAPI
> > > instances"), I unlinked/linked the NAPIs and queue IDs when XDP was
> > > enabled/disabled. Likewise, in commit 64b62146ba9e ("net/mlx4: link
> > > NAPI instances to queues and IRQs"), I avoided the XDP queues.
> > > 
> > > If drivers are to avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> > > or NETDEV_QUEUE_TYPE_TX, perhaps tsnep needs to be modified
> > > similarly?  
> > 
> > With 5ef44b3cb4 ("xsk: Bring back busy polling support") the linking of
> > the NAPIs is required for XDP/XSK. So it is strange to me if for XDP/XSK
> > the NAPIs should be unlinked. But I'm not an expert, so maybe there is
> > a reason why.
> > 
> > I added Magnus, maybe he knows if XSK queues shall still be linked to
> > NAPIs.  
> 
> OK, so I think I was probably just wrong?
> 
> I looked at bnxt and it seems to mark XDP queues, which means
> probably my patches for igc, ena, and mlx4 need to be fixed and the
> proposed patch I have for virtio_net needs to be adjusted.
> 
> I can't remember now why I thought XDP queues should be avoided. I
> feel like I read that or got that as feedback at some point, but I
> can't remember now. Maybe it was just one driver or something I was
> working on and I accidentally thought it should be avoided
> everywhere? Not sure.
> 
> Hopefully some one can give a definitive answer on this one before I
> go through and try to fix all the drivers I modified :|

XDP and AF_XDP are different things. The XDP part of AF_XDP is to some
extent for advertising purposes :) If memory serves me well:

XDP Tx -> these are additional queues automatically allocated for
          in-kernel XDP, allocated when XDP is attached on Rx.
          These should _not_ be listed in netlink queue, or NAPI;
          IOW should not be linked to NAPI instances.
XDP Rx -> is not a thing, XDP attaches to stack queues, there are no
          dedicated XDP Rx queues
AF_XDP -> AF_XDP "takes over" stack queues. It's a bit of a gray area.
          I don't recall if we made a call on these being linked, but
          they could probably be listed like devmem as a queue with
          an extra attribute, not a completely separate queue type.

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Joe Damato]

On Mon, Jan 13, 2025 at 01:56:09PM -0800, Jakub Kicinski wrote:
> On Mon, 13 Jan 2025 13:48:14 -0800 Joe Damato wrote:
> > > > The changes generally look OK to me (it seems RTNL is held on all
> > > > paths where this code can be called from as far as I can tell), but
> > > > there was one thing that stood out to me.
> > > > 
> > > > AFAIU, drivers avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> > > > or NETDEV_QUEUE_TYPE_TX. I could be wrong, but that was my
> > > > understanding and I submit patches to several drivers with this
> > > > assumption.
> > > > 
> > > > For example, in commit b65969856d4f ("igc: Link queues to NAPI
> > > > instances"), I unlinked/linked the NAPIs and queue IDs when XDP was
> > > > enabled/disabled. Likewise, in commit 64b62146ba9e ("net/mlx4: link
> > > > NAPI instances to queues and IRQs"), I avoided the XDP queues.
> > > > 
> > > > If drivers are to avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
> > > > or NETDEV_QUEUE_TYPE_TX, perhaps tsnep needs to be modified
> > > > similarly?  
> > > 
> > > With 5ef44b3cb4 ("xsk: Bring back busy polling support") the linking of
> > > the NAPIs is required for XDP/XSK. So it is strange to me if for XDP/XSK
> > > the NAPIs should be unlinked. But I'm not an expert, so maybe there is
> > > a reason why.
> > > 
> > > I added Magnus, maybe he knows if XSK queues shall still be linked to
> > > NAPIs.  
> > 
> > OK, so I think I was probably just wrong?
> > 
> > I looked at bnxt and it seems to mark XDP queues, which means
> > probably my patches for igc, ena, and mlx4 need to be fixed and the
> > proposed patch I have for virtio_net needs to be adjusted.
> > 
> > I can't remember now why I thought XDP queues should be avoided. I
> > feel like I read that or got that as feedback at some point, but I
> > can't remember now. Maybe it was just one driver or something I was
> > working on and I accidentally thought it should be avoided
> > everywhere? Not sure.
> > 
> > Hopefully some one can give a definitive answer on this one before I
> > go through and try to fix all the drivers I modified :|
> 
> XDP and AF_XDP are different things. The XDP part of AF_XDP is to some
> extent for advertising purposes :) If memory serves me well:
> 
> XDP Tx -> these are additional queues automatically allocated for
>           in-kernel XDP, allocated when XDP is attached on Rx.
>           These should _not_ be listed in netlink queue, or NAPI;
>           IOW should not be linked to NAPI instances.
> XDP Rx -> is not a thing, XDP attaches to stack queues, there are no
>           dedicated XDP Rx queues
> AF_XDP -> AF_XDP "takes over" stack queues. It's a bit of a gray area.
>           I don't recall if we made a call on these being linked, but
>           they could probably be listed like devmem as a queue with
>           an extra attribute, not a completely separate queue type.

Sorry to be an annoyance, but could this be added to docs somewhere?

I think I did the AF_XDP case I did two different ways; exported for
mlx5, but (iiuc) not exporter for igc.

I don't want to hijack Gerhard's thread; maybe I should start a new
thread to double check that the drivers I modified are right?

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Jakub Kicinski]

On Mon, 13 Jan 2025 14:20:56 -0800 Joe Damato wrote:
> > XDP and AF_XDP are different things. The XDP part of AF_XDP is to some
> > extent for advertising purposes :) If memory serves me well:
> > 
> > XDP Tx -> these are additional queues automatically allocated for
> >           in-kernel XDP, allocated when XDP is attached on Rx.
> >           These should _not_ be listed in netlink queue, or NAPI;
> >           IOW should not be linked to NAPI instances.
> > XDP Rx -> is not a thing, XDP attaches to stack queues, there are no
> >           dedicated XDP Rx queues
> > AF_XDP -> AF_XDP "takes over" stack queues. It's a bit of a gray area.
> >           I don't recall if we made a call on these being linked, but
> >           they could probably be listed like devmem as a queue with
> >           an extra attribute, not a completely separate queue type.  
> 
> Sorry to be an annoyance, but could this be added to docs somewhere?
> 
> I think I did the AF_XDP case I did two different ways; exported for
> mlx5, but (iiuc) not exporter for igc.

Yes, I think netdev.yaml is the best place to document the meaning of
rx and tx queue type. Are you going to take a stab at it?

> I don't want to hijack Gerhard's thread; maybe I should start a new
> thread to double check that the drivers I modified are right?

Ideally we'd have a test for this. How is your Python?
tools/testing/selftests/drivers/net/queues.py

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Joe Damato]

On Mon, Jan 13, 2025 at 02:31:09PM -0800, Jakub Kicinski wrote:
> On Mon, 13 Jan 2025 14:20:56 -0800 Joe Damato wrote:
> > > XDP and AF_XDP are different things. The XDP part of AF_XDP is to some
> > > extent for advertising purposes :) If memory serves me well:
> > > 
> > > XDP Tx -> these are additional queues automatically allocated for
> > >           in-kernel XDP, allocated when XDP is attached on Rx.
> > >           These should _not_ be listed in netlink queue, or NAPI;
> > >           IOW should not be linked to NAPI instances.
> > > XDP Rx -> is not a thing, XDP attaches to stack queues, there are no
> > >           dedicated XDP Rx queues
> > > AF_XDP -> AF_XDP "takes over" stack queues. It's a bit of a gray area.
> > >           I don't recall if we made a call on these being linked, but
> > >           they could probably be listed like devmem as a queue with
> > >           an extra attribute, not a completely separate queue type.  
> > 
> > Sorry to be an annoyance, but could this be added to docs somewhere?
> > 
> > I think I did the AF_XDP case I did two different ways; exported for
> > mlx5, but (iiuc) not exporter for igc.
> 
> Yes, I think netdev.yaml is the best place to document the meaning of
> rx and tx queue type. Are you going to take a stab at it?

I'll give it a try, why not.

> > I don't want to hijack Gerhard's thread; maybe I should start a new
> > thread to double check that the drivers I modified are right?
> 
> Ideally we'd have a test for this. How is your Python?
> tools/testing/selftests/drivers/net/queues.py

Good idea. My python is non-existent, but since I'm the one who
submit all the driver patches...

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Gerhard Engleder]

On 13.01.25 22:56, Jakub Kicinski wrote:
> On Mon, 13 Jan 2025 13:48:14 -0800 Joe Damato wrote:
>>>> The changes generally look OK to me (it seems RTNL is held on all
>>>> paths where this code can be called from as far as I can tell), but
>>>> there was one thing that stood out to me.
>>>>
>>>> AFAIU, drivers avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
>>>> or NETDEV_QUEUE_TYPE_TX. I could be wrong, but that was my
>>>> understanding and I submit patches to several drivers with this
>>>> assumption.
>>>>
>>>> For example, in commit b65969856d4f ("igc: Link queues to NAPI
>>>> instances"), I unlinked/linked the NAPIs and queue IDs when XDP was
>>>> enabled/disabled. Likewise, in commit 64b62146ba9e ("net/mlx4: link
>>>> NAPI instances to queues and IRQs"), I avoided the XDP queues.
>>>>
>>>> If drivers are to avoid marking XDP queues as NETDEV_QUEUE_TYPE_RX
>>>> or NETDEV_QUEUE_TYPE_TX, perhaps tsnep needs to be modified
>>>> similarly?
>>>
>>> With 5ef44b3cb4 ("xsk: Bring back busy polling support") the linking of
>>> the NAPIs is required for XDP/XSK. So it is strange to me if for XDP/XSK
>>> the NAPIs should be unlinked. But I'm not an expert, so maybe there is
>>> a reason why.
>>>
>>> I added Magnus, maybe he knows if XSK queues shall still be linked to
>>> NAPIs.
>>
>> OK, so I think I was probably just wrong?
>>
>> I looked at bnxt and it seems to mark XDP queues, which means
>> probably my patches for igc, ena, and mlx4 need to be fixed and the
>> proposed patch I have for virtio_net needs to be adjusted.
>>
>> I can't remember now why I thought XDP queues should be avoided. I
>> feel like I read that or got that as feedback at some point, but I
>> can't remember now. Maybe it was just one driver or something I was
>> working on and I accidentally thought it should be avoided
>> everywhere? Not sure.
>>
>> Hopefully some one can give a definitive answer on this one before I
>> go through and try to fix all the drivers I modified :|
> 
> XDP and AF_XDP are different things. The XDP part of AF_XDP is to some
> extent for advertising purposes :) If memory serves me well:
> 
> XDP Tx -> these are additional queues automatically allocated for
>            in-kernel XDP, allocated when XDP is attached on Rx.
>            These should _not_ be listed in netlink queue, or NAPI;
>            IOW should not be linked to NAPI instances.
> XDP Rx -> is not a thing, XDP attaches to stack queues, there are no
>            dedicated XDP Rx queues
> AF_XDP -> AF_XDP "takes over" stack queues. It's a bit of a gray area.
>            I don't recall if we made a call on these being linked, but
>            they could probably be listed like devmem as a queue with
>            an extra attribute, not a completely separate queue type.

For tsnep if have no additional XDP Tx queues, only the netdev queues
are used. For AF_XDP/XSK I would keep the linking, as the stack queues
still exist and are operated still with NAPI. Maybe queues taken over
by AF_XDP/XSK get an extra attribute in the future. So I can keep the
permanent linking to NAPI while interface is up no matter if XDP or
AF_XDP/XSK is used or not. Did I understand it right?

Gerhard

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Jakub Kicinski]

On Tue, 14 Jan 2025 21:58:24 +0100 Gerhard Engleder wrote:
> > XDP and AF_XDP are different things. The XDP part of AF_XDP is to some
> > extent for advertising purposes :) If memory serves me well:
> > 
> > XDP Tx -> these are additional queues automatically allocated for
> >            in-kernel XDP, allocated when XDP is attached on Rx.
> >            These should _not_ be listed in netlink queue, or NAPI;
> >            IOW should not be linked to NAPI instances.
> > XDP Rx -> is not a thing, XDP attaches to stack queues, there are no
> >            dedicated XDP Rx queues
> > AF_XDP -> AF_XDP "takes over" stack queues. It's a bit of a gray area.
> >            I don't recall if we made a call on these being linked, but
> >            they could probably be listed like devmem as a queue with
> >            an extra attribute, not a completely separate queue type.  
> 
> For tsnep if have no additional XDP Tx queues, only the netdev queues
> are used. For AF_XDP/XSK I would keep the linking, as the stack queues
> still exist and are operated still with NAPI. Maybe queues taken over
> by AF_XDP/XSK get an extra attribute in the future. So I can keep the
> permanent linking to NAPI while interface is up no matter if XDP or
> AF_XDP/XSK is used or not. Did I understand it right?

I think so.

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 10 Jan 2025 23:39:39 +0100 you wrote:
> Use netif_queue_set_napi() to link queues to NAPI instances so that they
> can be queried with netlink.
> 
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --dump queue-get --json='{"ifindex": 11}'
> [{'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'rx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'rx'},
>  {'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'tx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'tx'}]
> 
> [...]

Here is the summary with links:
  - [net-next] tsnep: Link queues to NAPIs
    https://git.kernel.org/netdev/net-next/c/b1b5cff6002a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] tsnep: Link queues to NAPIs

[Joe Damato]

On Fri, Jan 10, 2025 at 11:39:39PM +0100, Gerhard Engleder wrote:
> Use netif_queue_set_napi() to link queues to NAPI instances so that they
> can be queried with netlink.
> 
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --dump queue-get --json='{"ifindex": 11}'
> [{'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'rx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'rx'},
>  {'id': 0, 'ifindex': 11, 'napi-id': 9, 'type': 'tx'},
>  {'id': 1, 'ifindex': 11, 'napi-id': 10, 'type': 'tx'}]
> 
> Additionally use netif_napi_set_irq() to also provide NAPI interrupt
> number to userspace.
> 
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --do napi-get --json='{"id": 9}'
> {'defer-hard-irqs': 0,
>  'gro-flush-timeout': 0,
>  'id': 9,
>  'ifindex': 11,
>  'irq': 42,
>  'irq-suspend-timeout': 0}
> 
> Providing information about queues to userspace makes sense as APIs like
> XSK provide queue specific access. Also XSK busy polling relies on
> queues linked to NAPIs.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep_main.c | 28 ++++++++++++++++++----
>  1 file changed, 23 insertions(+), 5 deletions(-)

Sorry for diverting your thread with the questions around XDP.

Based on the conversation, this patch looks good to me.

Reviewed-by: Joe Damato <jdamato@fastly.com>