Message-ID: <ee1b4e93-eab1-9010-df17-ee093aac6e01@mellanox.com>
Date:   Tue, 30 Apr 2019 18:11:41 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Björn Töpel <bjorn.topel@...el.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Jonathan Lemon <bsd@...com>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [PATCH net-next 4/6] xsk: Extend channels to support combined
 XSK/non-XSK traffic

On 2019-04-26 22:11, Jakub Kicinski wrote:
> On Fri, 26 Apr 2019 11:42:37 +0000, Maxim Mikityanskiy wrote:
>> Currently, the drivers that implement AF_XDP zero-copy support (e.g.,
>> i40e) switch the channel into a different mode when an XSK is opened. It
>> causes some issues that have to be taken into account. For example, RSS
>> needs to be reconfigured to skip the XSK-enabled channels, or the XDP
>> program should filter out traffic not intended for that socket and
>> XDP_PASS it with an additional copy. As nothing validates or forces the
>> proper configuration, it's easy to have packets drops, when they get
>> into an XSK by mistake, and, in fact, it's the default configuration.
>> There has to be some tool to have RSS reconfigured on each socket open
>> and close event, but such a tool is problematic to implement, because no
>> one reports these events, and it's race-prone.
>>
>> This commit extends XSK to support both kinds of traffic (XSK and
>> non-XSK) in the same channel. It implies having two RX queues in
>> XSK-enabled channels: one for the regular traffic, and the other for
>> XSK. It solves the problem with RSS: the default configuration just
>> works without the need to manually reconfigure RSS or to perform some
>> possibly complicated filtering in the XDP layer. It makes it easy to run
>> both AF_XDP and regular sockets on the same machine. In the XDP program,
>> the QID's most significant bit will serve as a flag to indicate whether
>> it's the XSK queue or not. The extension is compatible with the legacy
>> configuration, so if one wants to run the legacy mode, they can
>> reconfigure RSS and ignore the flag in the XDP program (implemented in
>> the reference XDP program in libbpf). mlx5e will support this extension.
>>
>> A single XDP program can run both with drivers supporting or not
>> supporting this extension. The xdpsock sample and libbpf are updated
>> accordingly.
>>
>> Signed-off-by: Maxim Mikityanskiy <maximmi@...lanox.com>
> 
> It's been a while but this patch makes very little sense to me.
> 
> User installs a UMEM on a queue, all packets are delivered to that
> UMEM, if XDP program doesn't redirect the packet to a XSK socket the
> driver should copy it to a normal kernel page and proceed.  No drops
> should happen.

The thing is that the reference XDP program (included in libbpf) 
redirects everything to the XSK. It doesn't do any filtering. So, the 
packets steered to the queue by RSS also get into the XSK when they 
shouldn't - here is where drops appear. Moreover, if the XDP program did 
the filtering, you would have to setup the same rules both via ethtool 
and in the XDP program, and this has a lot of disadvantages comparing to 
just not having stray packets in the queue.

> Having two queues which pretend to share an ID but really one of them
> has a magic encoding does not help IMO.
> 
> I definitely agree with you that the way queues are taken out of the
> stack's ID space and the fact they aren't automatically excluded from
> RSS etc is quite suboptimal..
> 

Powered by blists - more mailing lists