Re: Measuring/Debugging XDP Performance

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

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

On Tue, 28 Jan 2020 12:42:21 -0600
Christian Deacon <gamemann@gflclan.com> wrote:

> I understand the `e1000e` driver doesn't have XDP support upstream. 
> Therefore, I tried implementing the driver patch that adds XDP support 
> here <https://github.com/adjavon/e1000e_xdp>. Unfortunately, this patch 
> was based off of kernel 4.10.2 (I'm using 4.19). Therefore, I had to 
> manually implement the patch code (this is my first time messing with 
> NIC driver code). Sadly, it doesn't seem like the patch worked based off 
> of the 'perf' results which are attached. I still see "do_xdp_generic" 

I do think it would be more valuable to implement XDP for driver igb.

Given igb is a "slow" 1Gbit/s interface, we could also just fix
XDP-generic to avoid the reallocation of the SKB for every packet...
I've attached two patches which does exactly that. It sounds like you
know howto apply patches and recompile your kernel(?)

I've tested this on my testlab.  The igb hardware seems is limited
to 1.2Mpps.  Using an XDP_DROP test program, I observe the CPU
processing packets use 50% CPU time before, and 36% CPU time after the
patches.

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

[-- Attachment #2: 01-04-xdp-headroom --]
[-- Type: application/octet-stream, Size: 1808 bytes --]

xdp: accept that XDP headroom isn't always equal XDP_PACKET_HEADROOM

From: Jesper Dangaard Brouer <brouer@redhat.com>

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.

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 f1d74a2bd234..2d93cb1f9433 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3291,6 +3291,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 38bc35da39f7..c54ed9066724 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4531,11 +4531,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_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;
 

[-- Attachment #3: 02-igb-adjust-headroom --]
[-- Type: application/octet-stream, Size: 2669 bytes --]

igb: increase headroom to cooperate with XDP-generic

From: Jesper Dangaard Brouer <brouer@redhat.com>

Increase the packet headroom to 192 bytes.

This works, but due to NIC TimeStamp header (IGB_TS_HDR_LEN = 16),
providing 192 bytes headroom, cause the driver to use order-1 pages
(8192 bytes) to store packets.  This is only a problem if the TS
header is combined with a VLAN header and is a MTU sized packets.

A solution would be to detect if HW timestamp is enabled, but I
couldn't find an easy test.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb.h      |    9 +++++++--
 drivers/net/ethernet/intel/igb/igb_main.c |    3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 49b5fa9d4783..4c689fe947d1 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -137,10 +137,15 @@ struct vf_mac_filter {
 #define IGB_RX_HDR_LEN		IGB_RXBUFFER_256
 #define IGB_TS_HDR_LEN		16
 
-#define IGB_SKB_PAD		(NET_SKB_PAD + NET_IP_ALIGN)
+/* Headroom 192 fit MTU size Ethernet frame into 2K with skb_shared_info */
+#define IGB_SKB_PAD		192
+// FIXME: What about NET_IP_ALIGN?
+
 #if (PAGE_SIZE < 8192)
 #define IGB_MAX_FRAME_BUILD_SKB \
 	(SKB_WITH_OVERHEAD(IGB_RXBUFFER_2048) - IGB_SKB_PAD - IGB_TS_HDR_LEN)
+	 // Hmm... this -IGB_TS_HDR_LEN might be a problem
+	// 2048 - 320 - 192 - 16 = 1520
 #else
 #define IGB_MAX_FRAME_BUILD_SKB (IGB_RXBUFFER_2048 - IGB_TS_HDR_LEN)
 #endif
@@ -348,7 +353,7 @@ static inline unsigned int igb_rx_pg_order(struct igb_ring *ring)
 {
 #if (PAGE_SIZE < 8192)
 	if (ring_uses_large_buffer(ring))
-		return 1;
+		return 1; // page order-1 = 8192 bytes
 #endif
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b46bff8fe056..8ed3db27d4d2 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4567,7 +4567,7 @@ static void igb_set_rx_buffer_len(struct igb_adapter *adapter,
 #if (PAGE_SIZE < 8192)
 	if (adapter->max_frame_size <= IGB_MAX_FRAME_BUILD_SKB)
 		return;
-
+	// Issue (1522 <= 1520) --> set_ring_uses_large_buffer
 	set_ring_uses_large_buffer(rx_ring);
 #endif
 }
@@ -6256,6 +6256,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+	// 1500 + 14 + 4 + 4 = 1522
 
 	/* adjust max frame to be at least the size of a standard frame */
 	if (max_frame < (ETH_FRAME_LEN + ETH_FCS_LEN))