From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH bpf-next v2 0/5] xsk: fix bug when trying to use both copy and zero-copy mode Date: Fri, 5 Oct 2018 09:35:38 +0200 Message-ID: References: <1538398297-14862-1-git-send-email-magnus.karlsson@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit To: Magnus Karlsson , bjorn.topel@intel.com, ast@kernel.org, netdev@vger.kernel.org, jakub.kicinski@netronome.com Return-path: Received: from www62.your-server.de ([213.133.104.62]:42896 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728072AbeJEOdL (ORCPT ); Fri, 5 Oct 2018 10:33:11 -0400 In-Reply-To: <1538398297-14862-1-git-send-email-magnus.karlsson@intel.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 10/01/2018 02:51 PM, Magnus Karlsson wrote: > Previously, the xsk code did not record which umem was bound to a > specific queue id. This was not required if all drivers were zero-copy > enabled as this had to be recorded in the driver anyway. So if a user > tried to bind two umems to the same queue, the driver would say > no. But if copy-mode was first enabled and then zero-copy mode (or the > reverse order), we mistakenly enabled both of them on the same umem > leading to buggy behavior. The main culprit for this is that we did > not store the association of umem to queue id in the copy case and > only relied on the driver reporting this. As this relation was not > stored in the driver for copy mode (it does not rely on the AF_XDP > NDOs), this obviously could not work. > > This patch fixes the problem by always recording the umem to queue id > relationship in the netdev_queue and netdev_rx_queue structs. This way > we always know what kind of umem has been bound to a queue id and can > act appropriately at bind time. To make the bind semantics consistent > with ethtool queue manipulations and to facilitate the implementation > of drivers, we also forbid decreasing the number of queues/channels > with ethtool if there is an active AF_XDP socket in the set of queues > that are disabled. > > Jakub, please take a look at your patches. The last one I had to > change slightly to make it fit with the new interface > xdp_get_umem_from_qid(). An added bonus with this function is that we, > in the future, can also use it from the driver to get a umem, thus > simplifying driver implementations (and later remove the umem from the > NDO completely). Björn will mail patches, at a later point in time, > using this in the i40e and ixgbe drivers, that removes a good chunk of > code from the ZC implementations. I also made your code aware of Tx > queues. If we create a socket that only has a Tx queue, then the queue > id will refer to a Tx queue id only and could be larger than the > available amount of Rx queues. Please take a look at it. > > Differences against v1: > * Included patches from Jakub that forbids decreasing the number of active > queues if a queue to be deactivated has an AF_XDP socket. These have > been adapted somewhat to the new interfaces in patch 2. > * Removed redundant check against real_num_[rt]x_queue in xsk_bind > * Only need to test against real_num_[rt]x_queues in > xdp_clear_umem_at_qid. > > Patch 1: Introduces a umem reference in the netdev_rx_queue and > netdev_queue structs. > Patch 2: Records which queue_id is bound to which umem and make sure > that you cannot bind two different umems to the same queue_id. > Patch 3: Pre patch to ethtool_set_channels. > Patch 4: Forbid decreasing the number of active queues if a deactivated > queue has an AF_XDP socket. > Patch 5: Simplify xdp_clear_umem_at_qid now when ethtool cannot deactivate > the queue id we are running on. > > I based this patch set on bpf-next commit 5bf7a60b8e70 ("bpf: permit > CGROUP_DEVICE programs accessing helper bpf_get_current_cgroup_id()") > > Thanks: Magnus > > Jakub Kicinski (2): > ethtool: rename local variable max -> curr > ethtool: don't allow disabling queues with umem installed > > Magnus Karlsson (3): > net: add umem reference in netdev{_rx}_queue > xsk: fix bug when trying to use both copy and zero-copy on one queue > id > xsk: simplify xdp_clear_umem_at_qid implementation > > include/linux/netdevice.h | 6 ++++ > include/net/xdp_sock.h | 7 ++++ > net/core/ethtool.c | 23 +++++++++---- > net/xdp/xdp_umem.c | 87 ++++++++++++++++++++++++++++++++--------------- > net/xdp/xdp_umem.h | 2 +- > net/xdp/xsk.c | 7 ---- > 6 files changed, 91 insertions(+), 41 deletions(-) > > -- > 2.7.4 > Applied to bpf-next, thanks everyone!