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

[Saeed Mahameed <saeedm@mellanox.com>]

On Thu, 2020-04-16 at 15:02 +0200, Jesper Dangaard Brouer wrote:
> On Thu, 9 Apr 2020 00:50:02 +0000
> Saeed Mahameed <saeedm@mellanox.com> wrote:
> 
> > On Wed, 2020-04-08 at 13:50 +0200, Jesper Dangaard Brouer wrote:
> > > XDP have evolved to support several frame sizes, but xdp_buff was
> > > not
> > > updated with this information. The frame size (frame_sz) member
> > > of
> > > xdp_buff is introduced to know the real size of the memory the
> > > frame
> > > is
> > > delivered in.
> > > 
> > > When introducing this also make it clear that some tailroom is
> > > reserved/required when creating SKBs using build_skb().
> > > 
> > > It would also have been an option to introduce a pointer to
> > > data_hard_end (with reserved offset). The advantage with frame_sz
> > > is
> > > that (like rxq) drivers only need to setup/assign this value once
> > > per
> > > NAPI cycle. Due to XDP-generic (and some drivers) it's not
> > > possible
> > > to
> > > store frame_sz inside xdp_rxq_info, because it's varies per
> > > packet as
> > > it
> > > can be based/depend on packet length.
> > > 
> > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > > ---
> > >  include/net/xdp.h |   17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > > index 40c6d3398458..99f4374f6214 100644
> > > --- a/include/net/xdp.h
> > > +++ b/include/net/xdp.h
> > > @@ -6,6 +6,8 @@
> > >  #ifndef __LINUX_NET_XDP_H__
> > >  #define __LINUX_NET_XDP_H__
> > >  
> > > +#include <linux/skbuff.h> /* skb_shared_info */
> > > +  
> > 
> > I think it is wrong to make xdp.h depend on skbuff.h
> > we must keep xdp.h minimal and independent,
> 
> I agree, that it seems strange to have xdp.h include skbuff.h, and
> I'm
> not happy with that approach myself, but the alternatives all looked
> kind of ugly.
> 
> > the new macros should be defined in skbuff.h 
> 
> Moving #define xdp_data_hard_end(xdp) into skbuff.h also seems
> strange.
> 

So maybe we shouldn't have any dependencies by design, and let the
drivers decide how much tailroom they want to preserve, and remove the
hardcoded sizeof(skb_shinfo).. 

maybe per rxq ? on memory model registration ?


> 
> > >  /**
> > >   * DOC: XDP RX-queue information
> > >   *
> > > @@ -70,8 +72,23 @@ struct xdp_buff {
> > >  	void *data_hard_start;
> > >  	unsigned long handle;
> > >  	struct xdp_rxq_info *rxq;
> > > +	u32 frame_sz; /* frame size to deduct data_hard_end/reserved
> > > tailroom*/  
> > 
> > why u32 ? u16 should be more than enough.. 
> 
> Nope.  It need to be able to store PAGE_SIZE == 65536.
> 
> $ echo $((1<<12))
> 4096
> $ echo $((1<<16))
> 65536
> 
> $ printf "0x%X\n" 65536
> 0x10000
> 

:(

> 
> > >  };
> > >  
> > > +/* Reserve memory area at end-of data area.
> > > + *
> > > + * 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)))
> > > +  
> > 
> > this macro is not safe when unary operators are being used
> 
> The parentheses round (xdp) does make xdp_data_hard_end(&xdp) work
> correctly. What other cases are you worried about?
> 
> 

consider: 
xdp_data_hard_end(xdp_ptr++)