From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once Date: Wed, 10 May 2017 15:46:30 -0700 Message-ID: <20170510154630.316010f9@cakuba.netronome.com> References: <20170509201842.2ed5e330@cakuba.netronome.com> <5912DF16.7050603@iogearbox.net> <20170510140754.739e4123@cakuba.netronome.com> <59139338.5020405@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, alexei.starovoitov@gmail.com, john.fastabend@gmail.com, netdev@vger.kernel.org To: Daniel Borkmann Return-path: Received: from mx3.wp.pl ([212.77.101.9]:6090 "EHLO mx3.wp.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751755AbdEJWqy (ORCPT ); Wed, 10 May 2017 18:46:54 -0400 In-Reply-To: <59139338.5020405@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 11 May 2017 00:24:56 +0200, Daniel Borkmann wrote: > > I understand the counter argument that from user space perspective it > > would make things slightly more complicated because there would be two > > conditions in which driver hook is used: > > 1) DRV_MODE set on dump; > > 2) flags attribute not present (old kernel). > > > > I'm concerned about number 2). We can't simply depend on SKB_MODE > > not being set because we may add more *_MODE flags in the future. So > > doing: > > > > if (flags & SKB_MODE) > > printf("skb-mode"); > > else > > printf("drv-mode"); > > > > is not correct. The flags attribute must not be present at all (think > > HW_MODE flag). But going further there can also be non-MODE flags, > > like, say.. NEVER_TX, and then there may be flags present in dump, > > and if SKB_MODE isn't be set, the mode could be some new MODE user space > > doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no > > way to tell :S > > Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for > dumping we're wasting bit space for flags we would never dump back > such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future > flags that are only relevant for loading, but never for dumping). > Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE, > we can still change this, since it's not too late. > > How about the following proposal: IFLA_XDP_ATTACHED we have as-is > (need to keep that anyway), if that is true, it means "something > is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE > (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1). > I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS > back, separating both attrs would avoid this hassle of what current > or future load flag fits for dump as well and which not. Wdyt? I really like the idea of not reusing IFLA_XDP_FLAGS for dumps! New enum sounds good, but perhaps reusing IFLA_XDP_ATTACHED isn't 100% off-limits either? 4.11 would report (0) - driver supports XDP but nothing is attached, (1) - something attached at the driver, could we make (2) mean - something attached at generic XDP? Would that be considered breaking the ABI, are we bound to boolean semantics?