netdev - Re: [PATCH net-next 4/6] xsk: Extend channels to support combined XSK/non-XSK traffic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 Apr 2019 18:11:52 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Björn Töpel <bjorn.topel@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Magnus Karlsson <magnus.karlsson@...el.com>
CC:     "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>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH net-next 4/6] xsk: Extend channels to support combined
 XSK/non-XSK traffic

On 2019-04-26 23:26, Björn Töpel wrote:
> On 2019-04-26 13:42, 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>
> 
> 
> I acknowledge the problem you're addressing, but I'm not a fan of this
> design.
> 
> Netdevs, queues and now channels. This is too complicated. The setup is
> complex as it is. Adding more cognitive load is not good.

IMO, using "queues" for AF_XDP is a misnomer. You don't really attach an 
AF_XDP socket to RX queue number X and TX queue number Y. Instead, you 
have a single number, and that number actually selects a channel (a ring 
in i40e) number X, which has an RX queue and TX queue. Even libbpf 
queries the number of channels to determine the maximum X allowed.

So, I'm not adding anything particularly new here. Everything that used 
to work still works. I'm not using netdevs+queues+channels, I'm still 
using netdevs+channels, just as it was before. There is no incompatible 
change here, only an improvement for the drivers that support it.

I'm going to respin this series, adding the mlx5 patches and addressing 
the other comments. If you feel like there are more things to discuss 
regarding this patch, please move on to the v2.

Thanks for looking into it!

> There's already an assumption that a user attaching to a certain netdev
> queue has to take care of the flow steering.
> 
> What about adding a mode where the AF_XDP user just get a *new* queue,
> by binding to (say) netdev;qid==-1? This new queue wouldn't be included
> in the existing queue set. Create a bunch of AF_XDP sockets and steer
> traffic to that set. And yes, we need a mechanism to steer to that
> queue, in-app/socket or out-of-band. Please see this [1] discussion.
> 
> I don't like this "let's split a queue into channels".
> 
> 
> Björn
> 
> 
> [1] https://lore.kernel.org/netdev/20190415183258.36dcee9a@carbon/