Re: [PATCH RFC v2 01/33] xdp: add frame size to xdp_buff

From: Jakub Kicinski <kuba@kernel.org>
To: Saeed Mahameed <saeedm@mellanox.com>
Cc: "akiyano@amazon.com" <akiyano@amazon.com>,
	"willemdebruijn.kernel@gmail.com"
	<willemdebruijn.kernel@gmail.com>,
	"borkmann@iogearbox.net" <borkmann@iogearbox.net>,
	"jeffrey.t.kirsher@intel.com" <jeffrey.t.kirsher@intel.com>,
	"john.fastabend@gmail.com" <john.fastabend@gmail.com>,
	"toke@redhat.com" <toke@redhat.com>,
	"alexei.starovoitov@gmail.com" <alexei.starovoitov@gmail.com>,
	"gtzalik@amazon.com" <gtzalik@amazon.com>,
	"dsahern@gmail.com" <dsahern@gmail.com>,
	"brouer@redhat.com" <brouer@redhat.com>,
	"sameehj@amazon.com" <sameehj@amazon.com>,
	"zorik@amazon.com" <zorik@amazon.com>,
	"alexander.duyck@gmail.com" <alexander.duyck@gmail.com>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"ilias.apalodimas@linaro.org" <ilias.apalodimas@linaro.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"lorenzo@kernel.org" <lorenzo@kernel.org>
Subject: Re: [PATCH RFC v2 01/33] xdp: add frame size to xdp_buff
Date: Thu, 9 Apr 2020 16:27:27 -0700	[thread overview]
Message-ID: <20200409162727.5f17d305@kicinski-fedora-PC1C0HJN> (raw)
In-Reply-To: <7a92118a956a29bbc62373af43832e30a39225f5.camel@mellanox.com>

On Thu, 9 Apr 2020 23:07:42 +0000 Saeed Mahameed wrote:
> > My concern is that driver may allocate a full page for each frame but
> > only DMA map the amount that can reasonably contain data given the
> > MTU.
> > To save on DMA syncs.
> > 
> > Today that wouldn't be a problem, because XDP_REDIRECT will re-map
> > the
> > page, and XDP_TX has the same MTU.
> 
> I am not worried about dma at all, i am worried about the xdp progs
> which are now allowed to extend packets beyond the mtu and do XDP_TX.
> but as i am thinking about this i just realized that this can already
> happen with xdp_adjust_head()..
> 
> but as you stated above this puts alot of assumptions on how driver
> should dma rx buffs 
> 
> > In this set xdp_data_hard_end is used both to find the end of memory
> > buffer, and end of DMA buffer. Implementation of
> > bpf_xdp_adjust_tail()
> > assumes anything < SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
> > from
> > the end is fair game.
> 
> but why skb_shared_info in particular though ? this assumes someone
> needs this tail for building skbs .. looks weird to me.

Fair, simplifies the internals, I guess.

> > So I was trying to say that we should warn driver authors that the
> > DMA
> > buffer can now grow / move beyond what the driver may expect in
> > XDP_TX.  
> 
> Ack, but can we do it by desing ? i.e instead of having hardcoded
> limits (e.g. SKB_DATA_ALIGN(shinfo)) in bpf_xdp_adjust_tail(), let the
> driver provide this, or any other restrictions, e.g mtu for tx, or
> driver specific memory model restrictions .. 

Right, actually for NFP we need to add the check already - looking at
the code - the DMA mapping does not cover anything beyond the headroom +
MTU.

> > Drivers can either DMA map enough memory, or handle the corner case
> > in
> > a special way.
> > 
> > IDK if that makes sense, we may be talking past each other :)  