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

Saeed Mahameed
2020-04-09T23:07:51Z

On Wed, 2020-04-08 at 18:13 -0700, Jakub Kicinski wrote:
> On Thu, 9 Apr 2020 00:48:30 +0000 Saeed Mahameed wrote:
> > > > + * This macro reserves tailroom in the XDP buffer by limiting
> > > > the
> > > > + * XDP/BPF data access to data_hard_end. Notice same area
> > > > (and
> > > > size)
> > > > + * is used for XDP_PASS, when constructing the SKB via
> > > > build_skb().
> > > > + */
> > > > +#define xdp_data_hard_end(xdp) \
> > > > + ((xdp)->data_hard_start + (xdp)->frame_sz - \
> > > > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
> > >
> > > I think it should be said somewhere that the drivers are expected
> > > to
> > > DMA map memory up to xdp_data_hard_end(xdp).
> > >
> >
> > but this works on a specific xdp buff, drivers work with mtu
> >
> > and what if the driver want to have this as an option per packet
> > ..
> > i.e.: if there is enough tail room, then build_skb, otherwise
> > alloc new skb, copy headers, setup data frags.. etc
> >
> > having such limitations on driver can be very strict, i think the
> > decision must remain dynamic per frame..
> >
> > of-course drivers should optimize to preserve enough tail room for
> > all
> > rx packets..
>
> 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.

> 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 ..

> 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 :)