Message-ID: <fa737740-7cd0-4109-8712-09f2cb8dbef0@engleder-embedded.com>
Date: Tue, 14 Jan 2025 21:58:24 +0100
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Jakub Kicinski <kuba@...nel.org>, Joe Damato <jdamato@...tly.com>
Cc: magnus.karlsson@...el.com, andrew@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] tsnep: Link queues to NAPIs

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

Powered by blists - more mailing lists