Re: [bpf-next PATCH] xdp: accept that XDP headroom isn't always equal XDP_PACKET_HEADROOM

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Tue, 03 Mar 2020 13:12:09 +0100
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> Jesper Dangaard Brouer <brouer@redhat.com> writes:
> 
> > The Intel based drivers (ixgbe + i40e) have implemented XDP with
> > headroom 192 bytes and not the recommended 256 bytes defined by
> > XDP_PACKET_HEADROOM.  For generic-XDP, accept that this headroom
> > is also a valid size.
> >
> > Still for generic-XDP if headroom is less, still expand headroom to
> > XDP_PACKET_HEADROOM as this is the default in most XDP drivers.
> >
> > Tested on ixgbe with xdp_rxq_info --skb-mode and --action XDP_DROP:
> > - Before: 4,816,430 pps
> > - After : 7,749,678 pps
> > (Note that ixgbe in native mode XDP_DROP 14,704,539 pps)
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> >  include/uapi/linux/bpf.h |    1 +
> >  net/core/dev.c           |    4 ++--
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 906e9f2752db..14dc4f9fb3c8 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3312,6 +3312,7 @@ struct bpf_xdp_sock {
> >  };
> >  
> >  #define XDP_PACKET_HEADROOM 256
> > +#define XDP_PACKET_HEADROOM_MIN 192  
> 
> Do we need a comment here explaining why there are two values?

Good point, but I want to take it even further, lets discuss what these
defines should be used for (which is what the comment should say ;-)).

Maybe we should name it to reflect that this is used by generic XDP?
Or do we also want to change ixgbe and i40e to use this value?
(Looking at ixgbe code this is semi-dynamic xgbe_rx_offset(rx_ring) ->
IXGBE_SKB_PAD -> ixgbe_skb_pad())


An orthogonal question is why does XDP-generic have this limit?
(Luigi Rizzo's patch in this area is not adhering to this...)

XDP-native use headroom area for xdp_frame and metadata. The xdp_frame
is not relevant/used in XDP-generic.  The metadata is still relevant
for XDP-generic, as it's a communication channel to TC-BPF, but its
only 32 bytes.  IHMO it should be safe to reduce to 128 bytes.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer