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

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

[Jesper Dangaard Brouer]

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
 
 /* User return codes for XDP prog type.
  * A valid XDP program must return one of these defined values. All other
diff --git a/net/core/dev.c b/net/core/dev.c
index 4770dde3448d..9c941cd38b13 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4518,11 +4518,11 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 		return XDP_PASS;
 
 	/* XDP packets must be linear and must have sufficient headroom
-	 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
+	 * of XDP_PACKET_HEADROOM_MIN bytes. This is the guarantee that also
 	 * native XDP provides, thus we need to do it here as well.
 	 */
 	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
-	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
+	    skb_headroom(skb) < XDP_PACKET_HEADROOM_MIN) {
 		int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
 		int troom = skb->tail + skb->data_len - skb->end;
 

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

[Toke Høiland-Jørgensen]

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?

-Toke

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

[Jesper Dangaard Brouer]

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

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

[Alexei Starovoitov]

On Tue, Mar 03, 2020 at 12:46:58PM +0100, Jesper Dangaard Brouer wrote:
> 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

why expose it in uapi?

>  /* User return codes for XDP prog type.
>   * A valid XDP program must return one of these defined values. All other
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4770dde3448d..9c941cd38b13 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4518,11 +4518,11 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  		return XDP_PASS;
>  
>  	/* XDP packets must be linear and must have sufficient headroom
> -	 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
> +	 * of XDP_PACKET_HEADROOM_MIN bytes. This is the guarantee that also
>  	 * native XDP provides, thus we need to do it here as well.
>  	 */
>  	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
> -	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> +	    skb_headroom(skb) < XDP_PACKET_HEADROOM_MIN) {
>  		int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);

this looks odd. It's comparing against 192, but doing math with 256.
I guess that's ok, but needs a clear comment.
How about just doing 'skb_headroom(skb) < 192' here.
Or #define 192 right before this function with a comment about ixgbe?

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

[John Fastabend]

Alexei Starovoitov wrote:
> On Tue, Mar 03, 2020 at 12:46:58PM +0100, Jesper Dangaard Brouer wrote:
> > 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.

The reason is to fit two packets on a 4k page. The driver itself
is fairly flexible at this point. I think we should reconsider
pushing down the headroom required in the program metadata and
configuring it at runtime. At the moment the drivers are wasting
half a page for no good reason in most cases I suspect. What is the
use case for >192B headroom? I've not found an actual user who
has complained yet.

Resurrecting an old debate here so probably doesn't need to
stall this patch.

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

But why do we care about generic-XDP performance? Seems users should
just use XDP proper on ixgbe and i40e its supported.

> > 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
> 
> why expose it in uapi?
> 
> >  /* User return codes for XDP prog type.
> >   * A valid XDP program must return one of these defined values. All other
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 4770dde3448d..9c941cd38b13 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4518,11 +4518,11 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
> >  		return XDP_PASS;
> >  
> >  	/* XDP packets must be linear and must have sufficient headroom
> > -	 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
> > +	 * of XDP_PACKET_HEADROOM_MIN bytes. This is the guarantee that also
> >  	 * native XDP provides, thus we need to do it here as well.
> >  	 */
> >  	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
> > -	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> > +	    skb_headroom(skb) < XDP_PACKET_HEADROOM_MIN) {
> >  		int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
> 
> this looks odd. It's comparing against 192, but doing math with 256.
> I guess that's ok, but needs a clear comment.
> How about just doing 'skb_headroom(skb) < 192' here.
> Or #define 192 right before this function with a comment about ixgbe?

Or just let ixgbe/i40e be slow? I guess I'm missing some context?

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

[Luigi Rizzo]

On Fri, Mar 6, 2020 at 5:06 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Alexei Starovoitov wrote:
> > On Tue, Mar 03, 2020 at 12:46:58PM +0100, Jesper Dangaard Brouer wrote:
...
> > > 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)
> > >
>
> But why do we care about generic-XDP performance? Seems users should
> just use XDP proper on ixgbe and i40e its supported.

I think the point was to show the performance benefit of skipping the
normalization (admittedly for a specific workload, tinygrams;
my other patch to control xdpgeneric_linearize covered a different range
of packet sizes).

On a side note I think it would be more useful to report times in ns/pkt,
as they can be applied to other drivers too. Specifically here I would
have written:

  Before: average 207 ns/pkt (1s / 4.816 Mpps)
  After:  average 129 ns/pkt (1s / 7.750 Mpps)

cheers
luigi

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

[Jesper Dangaard Brouer]

On Fri, 06 Mar 2020 08:06:35 -0800
John Fastabend <john.fastabend@gmail.com> wrote:

> Alexei Starovoitov wrote:
> > On Tue, Mar 03, 2020 at 12:46:58PM +0100, Jesper Dangaard Brouer wrote:  
[...]
> > > 
> > > 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)
> > >   
> 
> But why do we care about generic-XDP performance? Seems users should
> just use XDP proper on ixgbe and i40e its supported.
>
[...]
> 
> Or just let ixgbe/i40e be slow? I guess I'm missing some context?

The context originates from an email thread[1] on XDP-newbies list, that
had a production setup (anycast routing of gaming traffic[3]) that used
XDP and they used XDP-generic (actually without realizing it).  They
were using Intel igb driver (that don't have native-XDP), and changing
to e.g. ixgbe (or i40e) is challenging given it requires physical access
to the PoP (Points of Presence) and upgrading to a 10G port at the PoP
also have costs associated.

Why not simply use TC-BPF (cls_bpf) instead of XDP.  I've actually been
promoting that more people should use TC-BPF, and also in combination[2].
The reason it makes sense to stick with XDP here is to allow them to
deploy the same software on their PoP servers, regardless of which
NIC driver is available.

Performance wise, I will admit that I've explicitly chosen not to
optimize XDP-generic, and I've even seen it as a good thing that we
have this reallocation penalty.  Given the uniform software deployment
argument and my measurements in[1] I've changed my mind.  For the igb
driver I'm not motivated to implement XDP-native, because a newer Intel
CPU can handle wirespeed even-with the reallocations, but it is just
wasteful to do these reallocations.  "Allowing" these 1Gbit/s NICs to
work more optimally with XDP-generic, will allow us to ignore
converting these drivers to XDP-native, and as HW gets upgraded they
will transition seamlessly to XDP-native.


[1] https://www.spinics.net/lists/xdp-newbies/msg01548.html
[2] https://github.com/xdp-project/xdp-cpumap-tc
[3] https://gitlab.com/Dreae/compressor/
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

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

[John Fastabend]

Jesper Dangaard Brouer wrote:
> On Fri, 06 Mar 2020 08:06:35 -0800
> John Fastabend <john.fastabend@gmail.com> wrote:
> 
> > Alexei Starovoitov wrote:
> > > On Tue, Mar 03, 2020 at 12:46:58PM +0100, Jesper Dangaard Brouer wrote:  
> [...]
> > > > 
> > > > 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)
> > > >   
> > 
> > But why do we care about generic-XDP performance? Seems users should
> > just use XDP proper on ixgbe and i40e its supported.
> >
> [...]
> > 
> > Or just let ixgbe/i40e be slow? I guess I'm missing some context?
> 
> The context originates from an email thread[1] on XDP-newbies list, that
> had a production setup (anycast routing of gaming traffic[3]) that used
> XDP and they used XDP-generic (actually without realizing it).  They
> were using Intel igb driver (that don't have native-XDP), and changing
> to e.g. ixgbe (or i40e) is challenging given it requires physical access
> to the PoP (Points of Presence) and upgrading to a 10G port at the PoP
> also have costs associated.

OK maybe igb should get xdp...

I get the wanting to run an XDP program across multiple cards even when
they don't have XDP support.

> 
> Why not simply use TC-BPF (cls_bpf) instead of XDP.  I've actually been
> promoting that more people should use TC-BPF, and also in combination[2].
> The reason it makes sense to stick with XDP here is to allow them to
> deploy the same software on their PoP servers, regardless of which
> NIC driver is available.

Sounds reasonable to me.

> 
> Performance wise, I will admit that I've explicitly chosen not to
> optimize XDP-generic, and I've even seen it as a good thing that we
> have this reallocation penalty.  Given the uniform software deployment
> argument and my measurements in[1] I've changed my mind.  For the igb
> driver I'm not motivated to implement XDP-native, because a newer Intel
> CPU can handle wirespeed even-with the reallocations, but it is just
> wasteful to do these reallocations.  "Allowing" these 1Gbit/s NICs to
> work more optimally with XDP-generic, will allow us to ignore
> converting these drivers to XDP-native, and as HW gets upgraded they
> will transition seamlessly to XDP-native.

OK, might be nice to put the details in the commit message? The original
patch seemed to be mostly about 140e and ixgbe but in those case the
user (from above xdp example) would just use XDP. So more about 1gbps
and missing xdpsupport?

> 
> 
> [1] https://www.spinics.net/lists/xdp-newbies/msg01548.html
> [2] https://github.com/xdp-project/xdp-cpumap-tc
> [3] https://gitlab.com/Dreae/compressor/
> -- 
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>