* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-10 22:39 [PATCH net-next] tsnep: Link queues to NAPIs Gerhard Engleder
@ 2025-01-13 12:24 ` Alexander Lobakin
2025-01-13 19:59 ` Joe Damato
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Alexander Lobakin @ 2025-01-13 12:24 UTC (permalink / raw)
To: Gerhard Engleder; +Cc: andrew, davem, edumazet, kuba, pabeni, netdev
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-10 22:39 [PATCH net-next] tsnep: Link queues to NAPIs Gerhard Engleder
2025-01-13 12:24 ` Alexander Lobakin
@ 2025-01-13 19:59 ` Joe Damato
2025-01-13 20:32 ` Gerhard Engleder
2025-01-14 23:00 ` patchwork-bot+netdevbpf
2025-01-14 23:05 ` Joe Damato
3 siblings, 1 reply; 13+ messages in thread
From: Joe Damato @ 2025-01-13 19:59 UTC (permalink / raw)
To: Gerhard Engleder; +Cc: andrew, davem, edumazet, kuba, pabeni, netdev
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?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-13 19:59 ` Joe Damato
@ 2025-01-13 20:32 ` Gerhard Engleder
2025-01-13 21:48 ` Joe Damato
0 siblings, 1 reply; 13+ messages in thread
From: Gerhard Engleder @ 2025-01-13 20:32 UTC (permalink / raw)
To: Joe Damato, magnus.karlsson; +Cc: andrew, davem, edumazet, kuba, pabeni, netdev
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-13 20:32 ` Gerhard Engleder
@ 2025-01-13 21:48 ` Joe Damato
2025-01-13 21:56 ` Jakub Kicinski
0 siblings, 1 reply; 13+ messages in thread
From: Joe Damato @ 2025-01-13 21:48 UTC (permalink / raw)
To: Gerhard Engleder
Cc: magnus.karlsson, andrew, davem, edumazet, kuba, pabeni, netdev
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 :|
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-13 21:48 ` Joe Damato
@ 2025-01-13 21:56 ` Jakub Kicinski
2025-01-13 22:20 ` Joe Damato
2025-01-14 20:58 ` Gerhard Engleder
0 siblings, 2 replies; 13+ messages in thread
From: Jakub Kicinski @ 2025-01-13 21:56 UTC (permalink / raw)
To: Joe Damato
Cc: Gerhard Engleder, magnus.karlsson, andrew, davem, edumazet,
pabeni, netdev
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-13 21:56 ` Jakub Kicinski
@ 2025-01-13 22:20 ` Joe Damato
2025-01-13 22:31 ` Jakub Kicinski
2025-01-14 20:58 ` Gerhard Engleder
1 sibling, 1 reply; 13+ messages in thread
From: Joe Damato @ 2025-01-13 22:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Gerhard Engleder, magnus.karlsson, andrew, davem, edumazet,
pabeni, netdev
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?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-13 22:20 ` Joe Damato
@ 2025-01-13 22:31 ` Jakub Kicinski
2025-01-13 22:36 ` Joe Damato
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2025-01-13 22:31 UTC (permalink / raw)
To: Joe Damato
Cc: Gerhard Engleder, magnus.karlsson, andrew, davem, edumazet,
pabeni, netdev
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-13 22:31 ` Jakub Kicinski
@ 2025-01-13 22:36 ` Joe Damato
0 siblings, 0 replies; 13+ messages in thread
From: Joe Damato @ 2025-01-13 22:36 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Gerhard Engleder, magnus.karlsson, andrew, davem, edumazet,
pabeni, netdev
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...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-13 21:56 ` Jakub Kicinski
2025-01-13 22:20 ` Joe Damato
@ 2025-01-14 20:58 ` Gerhard Engleder
2025-01-14 21:22 ` Jakub Kicinski
1 sibling, 1 reply; 13+ messages in thread
From: Gerhard Engleder @ 2025-01-14 20:58 UTC (permalink / raw)
To: Jakub Kicinski, Joe Damato
Cc: magnus.karlsson, andrew, davem, edumazet, pabeni, netdev
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-14 20:58 ` Gerhard Engleder
@ 2025-01-14 21:22 ` Jakub Kicinski
0 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2025-01-14 21:22 UTC (permalink / raw)
To: Gerhard Engleder
Cc: Joe Damato, magnus.karlsson, andrew, davem, edumazet, pabeni,
netdev
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-10 22:39 [PATCH net-next] tsnep: Link queues to NAPIs Gerhard Engleder
2025-01-13 12:24 ` Alexander Lobakin
2025-01-13 19:59 ` Joe Damato
@ 2025-01-14 23:00 ` patchwork-bot+netdevbpf
2025-01-14 23:05 ` Joe Damato
3 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-01-14 23:00 UTC (permalink / raw)
To: Gerhard Engleder; +Cc: andrew, davem, edumazet, kuba, pabeni, netdev
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next] tsnep: Link queues to NAPIs
2025-01-10 22:39 [PATCH net-next] tsnep: Link queues to NAPIs Gerhard Engleder
` (2 preceding siblings ...)
2025-01-14 23:00 ` patchwork-bot+netdevbpf
@ 2025-01-14 23:05 ` Joe Damato
3 siblings, 0 replies; 13+ messages in thread
From: Joe Damato @ 2025-01-14 23:05 UTC (permalink / raw)
To: Gerhard Engleder; +Cc: andrew, davem, edumazet, kuba, pabeni, netdev
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>
^ permalink raw reply [flat|nested] 13+ messages in thread