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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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++)
