From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Subject: Re: [RFC bpf-next 0/6] net: xsk: minor improvements around queue handling Date: Mon, 30 Jul 2018 14:49:32 +0200 Message-ID: References: <20180726214148.2087-1-jakub.kicinski@netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , "Karlsson, Magnus" , oss-drivers@netronome.com, Netdev , Ilias Apalodimas , Francois Ozog , MykytaI Iziumtsev To: Jakub Kicinski Return-path: Received: from mail-qk0-f195.google.com ([209.85.220.195]:35059 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728812AbeG3OYc (ORCPT ); Mon, 30 Jul 2018 10:24:32 -0400 Received: by mail-qk0-f195.google.com with SMTP id u21-v6so7662368qku.2 for ; Mon, 30 Jul 2018 05:49:42 -0700 (PDT) In-Reply-To: <20180726214148.2087-1-jakub.kicinski@netronome.com> Sender: netdev-owner@vger.kernel.org List-ID: Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski : > > Hi! > Thanks for spending your time on this, Jakub. I'm (temporarily) back for a week, so you can expect faster replies now... > This set tries to make the core take care of error checking for the > drivers. In particular making sure that the AF_XDP UMEM is not installed > on queues which don't exist (or are disabled) and that changing queue > (AKA ethtool channel) count cannot disable queues with active AF_XDF > zero-copy sockets. > > I'm sending as an RFC because I'm not entirely sure what the desired > behaviour is here. Is it Okay to install AF_XDP on queues which don't > exist? I presume not? Your presumption is correct. The idea with the real_num_rx_queues/real_num_tx_queues check in xsk_bind, is to bail out if the queue doesn't exist at bind call. Note that we *didn't* add any code to avoid the bound queue from being removed via set channel (your patch 6). Our idea was that if you remove a queue, the ingress frames would simply stop flowing, and the queue config change checking was "out-of-band". I think I prefer your approach, i.e. not allowing the channels/queues to change if they're bound to an AF_XDP socket. However, your xdp_umem_query used in ethtool only works for ZC enabled drivers, not for the existing non-ZC/copy case. If we'd like to go the route of disabling ethtool_set_channels for an AF_XDP enabled queue this functionality needs to move the query into netdev core, so we have a consistent behavior. > Are the AF_XDP queue_ids referring to TX queues > as well as RX queues in case of the driver? I presume not? We've had a lot of discussions about this internally. Ideally, we'd like to give driver implementors the most freedom, and not enforcing a certain queue scheme for Tx. OTOH it makes it weird for the userland application *not* to have the same id, e.g. if a userland application would like to get stats or configure the AF_XDP bound Tx queue -- which id is it? Should the Tx queue id for an xsk be exposed in sysfs? If the id is *not* the same, would it be OK to change the number of channels and Tx would continue to operate correctly? A related question; An xsk with *only* Tx, should it be constrained by the number of (enabled) Rx queues? I'd be happy to hear some more opinions/thoughts on this... > Should > we try to prevent disabling queues which have non zero-copy sockets > installed as well? :S > Yes, the ZC/non-ZC case must be consistent IMO. See comment above. > Anyway, if any of those patches seem useful and reasonable, please let > me know I will repost as non-RFC. > I definitely think patch 2 and 3 (and probably 1) should go as non-RFC! Thanks for spotting that we're not holding the rtnl lock when checking the # queues (patch 4)! Bj=C3=B6rn > Jakub Kicinski (6): > net: update real_num_rx_queues even when !CONFIG_SYSFS > xsk: refactor xdp_umem_assign_dev() > xsk: don't allow umem replace at stack level > xsk: don't allow installing UMEM beyond the number of queues > ethtool: rename local variable max -> curr > ethtool: don't allow disabling queues with umem installed > > include/linux/netdevice.h | 16 +++++++-- > net/core/ethtool.c | 19 ++++++---- > net/xdp/xdp_umem.c | 73 ++++++++++++++++++++++++--------------- > 3 files changed, 71 insertions(+), 37 deletions(-) > > -- > 2.17.1 >