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