[PATCH net-next 0/2] net, igb, mlx4: Copy exact header to SKB linear buffer

[PATCH net-next 0/2] net, igb, mlx4: Copy exact header to SKB linear buffer

[Amir Vadai]

Hi,

This patchset consist of 2 patches written by Ido:
First patch is making the header length computation function from igb and igbe
generlized at net/core/utils.c.
Second patch is using this helper function to copy the exact packet header to
the linear buffer instead of constant length.

Patches were applied on top of commit 79e0f1c: "ipv6: Need to sock_put on csum
error"

Thanks,
Amir

Ido Shamay (2):
  net: Expose header length compution function
  net/mlx4_en: Copy exact header to SKB linear part

 drivers/net/ethernet/intel/igb/igb_main.c     | 109 +-----------------------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 116 +-------------------------
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |  12 +--
 include/linux/net.h                           |   2 +
 net/core/utils.c                              | 104 +++++++++++++++++++++++
 5 files changed, 115 insertions(+), 228 deletions(-)

-- 
1.8.3.4

[PATCH net-next 1/2] net: Expose header length compution function

[Amir Vadai]

From: Ido Shamay <idos@mellanox.com>

This commit exposes an header length compution generic function,
introduced in some of Intel's Etherent drivers (igb and ixgbe),
to net public namespace under the name __net_get_headlen,
where it can be used by other vendors and removes code duplication.

Signed-off-by: Ido Shamay <idos@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c     | 109 +-----------------------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 116 +-------------------------
 include/linux/net.h                           |   2 +
 net/core/utils.c                              | 104 +++++++++++++++++++++++
 4 files changed, 108 insertions(+), 223 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index bfcda8a..6f3d2a8 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6736,113 +6736,6 @@ static bool igb_is_non_eop(struct igb_ring *rx_ring,
 }
 
 /**
- *  igb_get_headlen - determine size of header for LRO/GRO
- *  @data: pointer to the start of the headers
- *  @max_len: total length of section to find headers in
- *
- *  This function is meant to determine the length of headers that will
- *  be recognized by hardware for LRO, and GRO offloads.  The main
- *  motivation of doing this is to only perform one pull for IPv4 TCP
- *  packets so that we can do basic things like calculating the gso_size
- *  based on the average data per packet.
- **/
-static unsigned int igb_get_headlen(unsigned char *data,
-				    unsigned int max_len)
-{
-	union {
-		unsigned char *network;
-		/* l2 headers */
-		struct ethhdr *eth;
-		struct vlan_hdr *vlan;
-		/* l3 headers */
-		struct iphdr *ipv4;
-		struct ipv6hdr *ipv6;
-	} hdr;
-	__be16 protocol;
-	u8 nexthdr = 0;	/* default to not TCP */
-	u8 hlen;
-
-	/* this should never happen, but better safe than sorry */
-	if (max_len < ETH_HLEN)
-		return max_len;
-
-	/* initialize network frame pointer */
-	hdr.network = data;
-
-	/* set first protocol and move network header forward */
-	protocol = hdr.eth->h_proto;
-	hdr.network += ETH_HLEN;
-
-	/* handle any vlan tag if present */
-	if (protocol == htons(ETH_P_8021Q)) {
-		if ((hdr.network - data) > (max_len - VLAN_HLEN))
-			return max_len;
-
-		protocol = hdr.vlan->h_vlan_encapsulated_proto;
-		hdr.network += VLAN_HLEN;
-	}
-
-	/* handle L3 protocols */
-	if (protocol == htons(ETH_P_IP)) {
-		if ((hdr.network - data) > (max_len - sizeof(struct iphdr)))
-			return max_len;
-
-		/* access ihl as a u8 to avoid unaligned access on ia64 */
-		hlen = (hdr.network[0] & 0x0F) << 2;
-
-		/* verify hlen meets minimum size requirements */
-		if (hlen < sizeof(struct iphdr))
-			return hdr.network - data;
-
-		/* record next protocol if header is present */
-		if (!(hdr.ipv4->frag_off & htons(IP_OFFSET)))
-			nexthdr = hdr.ipv4->protocol;
-	} else if (protocol == htons(ETH_P_IPV6)) {
-		if ((hdr.network - data) > (max_len - sizeof(struct ipv6hdr)))
-			return max_len;
-
-		/* record next protocol */
-		nexthdr = hdr.ipv6->nexthdr;
-		hlen = sizeof(struct ipv6hdr);
-	} else {
-		return hdr.network - data;
-	}
-
-	/* relocate pointer to start of L4 header */
-	hdr.network += hlen;
-
-	/* finally sort out TCP */
-	if (nexthdr == IPPROTO_TCP) {
-		if ((hdr.network - data) > (max_len - sizeof(struct tcphdr)))
-			return max_len;
-
-		/* access doff as a u8 to avoid unaligned access on ia64 */
-		hlen = (hdr.network[12] & 0xF0) >> 2;
-
-		/* verify hlen meets minimum size requirements */
-		if (hlen < sizeof(struct tcphdr))
-			return hdr.network - data;
-
-		hdr.network += hlen;
-	} else if (nexthdr == IPPROTO_UDP) {
-		if ((hdr.network - data) > (max_len - sizeof(struct udphdr)))
-			return max_len;
-
-		hdr.network += sizeof(struct udphdr);
-	}
-
-	/* If everything has gone correctly hdr.network should be the
-	 * data section of the packet and will be the end of the header.
-	 * If not then it probably represents the end of the last recognized
-	 * header.
-	 */
-	if ((hdr.network - data) < max_len)
-		return hdr.network - data;
-	else
-		return max_len;
-}
-
-/**
  *  igb_pull_tail - igb specific version of skb_pull_tail
  *  @rx_ring: rx descriptor ring packet is being transacted on
  *  @rx_desc: pointer to the EOP Rx descriptor
@@ -6886,7 +6779,7 @@ static void igb_pull_tail(struct igb_ring *rx_ring,
 	/* we need the header to contain the greater of either ETH_HLEN or
 	 * 60 bytes if the skb->len is less than 60 for skb_pad.
 	 */
-	pull_len = igb_get_headlen(va, IGB_RX_HDR_LEN);
+	pull_len = __net_get_headlen(va, IGB_RX_HDR_LEN);
 
 	/* align pull length to size of long to optimize memcpy performance */
 	skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8089ea9..77d1d2e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1527,120 +1527,6 @@ void ixgbe_alloc_rx_buffers(struct ixgbe_ring *rx_ring, u16 cleaned_count)
 		ixgbe_release_rx_desc(rx_ring, i);
 }
 
-/**
- * ixgbe_get_headlen - determine size of header for RSC/LRO/GRO/FCOE
- * @data: pointer to the start of the headers
- * @max_len: total length of section to find headers in
- *
- * This function is meant to determine the length of headers that will
- * be recognized by hardware for LRO, GRO, and RSC offloads.  The main
- * motivation of doing this is to only perform one pull for IPv4 TCP
- * packets so that we can do basic things like calculating the gso_size
- * based on the average data per packet.
- **/
-static unsigned int ixgbe_get_headlen(unsigned char *data,
-				      unsigned int max_len)
-{
-	union {
-		unsigned char *network;
-		/* l2 headers */
-		struct ethhdr *eth;
-		struct vlan_hdr *vlan;
-		/* l3 headers */
-		struct iphdr *ipv4;
-		struct ipv6hdr *ipv6;
-	} hdr;
-	__be16 protocol;
-	u8 nexthdr = 0;	/* default to not TCP */
-	u8 hlen;
-
-	/* this should never happen, but better safe than sorry */
-	if (max_len < ETH_HLEN)
-		return max_len;
-
-	/* initialize network frame pointer */
-	hdr.network = data;
-
-	/* set first protocol and move network header forward */
-	protocol = hdr.eth->h_proto;
-	hdr.network += ETH_HLEN;
-
-	/* handle any vlan tag if present */
-	if (protocol == htons(ETH_P_8021Q)) {
-		if ((hdr.network - data) > (max_len - VLAN_HLEN))
-			return max_len;
-
-		protocol = hdr.vlan->h_vlan_encapsulated_proto;
-		hdr.network += VLAN_HLEN;
-	}
-
-	/* handle L3 protocols */
-	if (protocol == htons(ETH_P_IP)) {
-		if ((hdr.network - data) > (max_len - sizeof(struct iphdr)))
-			return max_len;
-
-		/* access ihl as a u8 to avoid unaligned access on ia64 */
-		hlen = (hdr.network[0] & 0x0F) << 2;
-
-		/* verify hlen meets minimum size requirements */
-		if (hlen < sizeof(struct iphdr))
-			return hdr.network - data;
-
-		/* record next protocol if header is present */
-		if (!(hdr.ipv4->frag_off & htons(IP_OFFSET)))
-			nexthdr = hdr.ipv4->protocol;
-	} else if (protocol == htons(ETH_P_IPV6)) {
-		if ((hdr.network - data) > (max_len - sizeof(struct ipv6hdr)))
-			return max_len;
-
-		/* record next protocol */
-		nexthdr = hdr.ipv6->nexthdr;
-		hlen = sizeof(struct ipv6hdr);
-#ifdef IXGBE_FCOE
-	} else if (protocol == htons(ETH_P_FCOE)) {
-		if ((hdr.network - data) > (max_len - FCOE_HEADER_LEN))
-			return max_len;
-		hlen = FCOE_HEADER_LEN;
-#endif
-	} else {
-		return hdr.network - data;
-	}
-
-	/* relocate pointer to start of L4 header */
-	hdr.network += hlen;
-
-	/* finally sort out TCP/UDP */
-	if (nexthdr == IPPROTO_TCP) {
-		if ((hdr.network - data) > (max_len - sizeof(struct tcphdr)))
-			return max_len;
-
-		/* access doff as a u8 to avoid unaligned access on ia64 */
-		hlen = (hdr.network[12] & 0xF0) >> 2;
-
-		/* verify hlen meets minimum size requirements */
-		if (hlen < sizeof(struct tcphdr))
-			return hdr.network - data;
-
-		hdr.network += hlen;
-	} else if (nexthdr == IPPROTO_UDP) {
-		if ((hdr.network - data) > (max_len - sizeof(struct udphdr)))
-			return max_len;
-
-		hdr.network += sizeof(struct udphdr);
-	}
-
-	/*
-	 * If everything has gone correctly hdr.network should be the
-	 * data section of the packet and will be the end of the header.
-	 * If not then it probably represents the end of the last recognized
-	 * header.
-	 */
-	if ((hdr.network - data) < max_len)
-		return hdr.network - data;
-	else
-		return max_len;
-}
-
 static void ixgbe_set_rsc_gso_size(struct ixgbe_ring *ring,
 				   struct sk_buff *skb)
 {
@@ -1799,7 +1685,7 @@ static void ixgbe_pull_tail(struct ixgbe_ring *rx_ring,
 	 * we need the header to contain the greater of either ETH_HLEN or
 	 * 60 bytes if the skb->len is less than 60 for skb_pad.
 	 */
-	pull_len = ixgbe_get_headlen(va, IXGBE_RX_HDR_SIZE);
+	pull_len = __net_get_headlen(va, IXGBE_RX_HDR_SIZE);
 
 	/* align pull length to size of long to optimize memcpy performance */
 	skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
diff --git a/include/linux/net.h b/include/linux/net.h
index 94734a6..1ca3776 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -248,6 +248,8 @@ do {								\
 bool __net_get_random_once(void *buf, int nbytes, bool *done,
 			   struct static_key *done_key);
 
+int __net_get_headlen(unsigned char *data, unsigned int max_len);
+
 #ifdef HAVE_JUMP_LABEL
 #define ___NET_RANDOM_STATIC_KEY_INIT ((struct static_key) \
 		{ .enabled = ATOMIC_INIT(0), .entries = (void *)1 })
diff --git a/net/core/utils.c b/net/core/utils.c
index 2f737bf..5189d6f 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -26,13 +26,17 @@
 #include <linux/percpu.h>
 #include <linux/init.h>
 #include <linux/ratelimit.h>
+#include <linux/if_vlan.h>
 
 #include <net/sock.h>
 #include <net/net_ratelimit.h>
+#include <net/ip.h>
 
 #include <asm/byteorder.h>
 #include <asm/uaccess.h>
 
+#include <scsi/fc/fc_fcoe.h>
+
 int net_msg_warn __read_mostly = 1;
 EXPORT_SYMBOL(net_msg_warn);
 
@@ -387,3 +391,103 @@ bool __net_get_random_once(void *buf, int nbytes, bool *done,
 	return true;
 }
 EXPORT_SYMBOL(__net_get_random_once);
+
+int __net_get_headlen(unsigned char *data, unsigned int max_len)
+{
+	union {
+		unsigned char *network;
+		/* l2 headers */
+		struct ethhdr *eth;
+		struct vlan_hdr *vlan;
+		/* l3 headers */
+		struct iphdr *ipv4;
+		struct ipv6hdr *ipv6;
+	} hdr;
+	__be16 protocol;
+	u8 nexthdr = 0;	/* default to not TCP */
+	u8 hlen;
+
+	/* this should never happen, but better safe than sorry */
+	if (max_len < ETH_HLEN)
+		return max_len;
+
+	/* initialize network frame pointer */
+	hdr.network = data;
+
+	/* set first protocol and move network header forward */
+	protocol = hdr.eth->h_proto;
+	hdr.network += ETH_HLEN;
+
+	/* handle any vlan tag if present */
+	if (protocol == htons(ETH_P_8021Q)) {
+		if ((hdr.network - data) > (max_len - VLAN_HLEN))
+			return max_len;
+
+		protocol = hdr.vlan->h_vlan_encapsulated_proto;
+		hdr.network += VLAN_HLEN;
+	}
+
+	/* handle L3 protocols */
+	if (protocol == htons(ETH_P_IP)) {
+		if ((hdr.network - data) > (max_len - sizeof(struct iphdr)))
+			return max_len;
+
+		/* access ihl as a u8 to avoid unaligned access on ia64 */
+		hlen = (hdr.network[0] & 0x0F) << 2;
+
+		/* verify hlen meets minimum size requirements */
+		if (hlen < sizeof(struct iphdr))
+			return hdr.network - data;
+
+		/* record next protocol if header is present */
+		if (!(hdr.ipv4->frag_off & htons(IP_OFFSET)))
+			nexthdr = hdr.ipv4->protocol;
+	} else if (protocol == htons(ETH_P_IPV6)) {
+		if ((hdr.network - data) > (max_len - sizeof(struct ipv6hdr)))
+			return max_len;
+
+		/* record next protocol */
+		nexthdr = hdr.ipv6->nexthdr;
+		hlen = sizeof(struct ipv6hdr);
+	} else if (protocol == htons(ETH_P_FCOE)) {
+		if ((hdr.network - data) > (max_len - FCOE_HEADER_LEN))
+			return max_len;
+		hlen = FCOE_HEADER_LEN;
+	} else {
+		return hdr.network - data;
+	}
+
+	/* relocate pointer to start of L4 header */
+	hdr.network += hlen;
+
+	/* finally sort out TCP/UDP */
+	if (nexthdr == IPPROTO_TCP) {
+		if ((hdr.network - data) > (max_len - sizeof(struct tcphdr)))
+			return max_len;
+
+		/* access doff as a u8 to avoid unaligned access on ia64 */
+		hlen = (hdr.network[12] & 0xF0) >> 2;
+
+		/* verify hlen meets minimum size requirements */
+		if (hlen < sizeof(struct tcphdr))
+			return hdr.network - data;
+
+		hdr.network += hlen;
+	} else if (nexthdr == IPPROTO_UDP) {
+		if ((hdr.network - data) > (max_len - sizeof(struct udphdr)))
+			return max_len;
+
+		hdr.network += sizeof(struct udphdr);
+	}
+
+	/* If everything has gone correctly hdr.network should be the
+	 * data section of the packet and will be the end of the header.
+	 * If not then it probably represents the end of the last recognized
+	 * header.
+	 */
+	if ((hdr.network - data) < max_len)
+		return hdr.network - data;
+	else
+		return max_len;
+}
+EXPORT_SYMBOL(__net_get_headlen);
-- 
1.8.3.4

[PATCH net-next 2/2] net/mlx4_en: Copy exact header to SKB linear part

[Amir Vadai]

From: Ido Shamay <idos@mellanox.com>

When copy received packet header to the linear section of the SKB,
Copy the exact header (best effort) and not the max possible header,
using the new network helper function __net_get_headlen.
It will return the size of the header up to the latest known header.

Signed-off-by: Ido Shamay <idos@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index ba049ae..e8b1787 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -566,6 +566,7 @@ static struct sk_buff *mlx4_en_rx_skb(struct mlx4_en_priv *priv,
 	void *va;
 	int used_frags;
 	dma_addr_t dma;
+	int hdrlen;
 
 	skb = netdev_alloc_skb(priv->dev, SMALL_PACKET_SIZE + NET_IP_ALIGN);
 	if (!skb) {
@@ -598,15 +599,16 @@ static struct sk_buff *mlx4_en_rx_skb(struct mlx4_en_priv *priv,
 		skb_shinfo(skb)->nr_frags = used_frags;
 
 		/* Copy headers into the skb linear buffer */
-		memcpy(skb->data, va, HEADER_COPY_SIZE);
-		skb->tail += HEADER_COPY_SIZE;
+		hdrlen = __net_get_headlen(va, SMALL_PACKET_SIZE);
+		memcpy(skb->data, va, ALIGN(hdrlen, sizeof(long)));
+		skb->tail += hdrlen;
 
 		/* Skip headers in first fragment */
-		skb_shinfo(skb)->frags[0].page_offset += HEADER_COPY_SIZE;
+		skb_shinfo(skb)->frags[0].page_offset += hdrlen;
 
 		/* Adjust size of first fragment */
-		skb_frag_size_sub(&skb_shinfo(skb)->frags[0], HEADER_COPY_SIZE);
-		skb->data_len = length - HEADER_COPY_SIZE;
+		skb_frag_size_sub(&skb_shinfo(skb)->frags[0], hdrlen);
+		skb->data_len = length - hdrlen;
 	}
 	return skb;
 }
-- 
1.8.3.4

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Alexander Duyck]

On 05/08/2014 05:50 AM, Amir Vadai wrote:
> From: Ido Shamay <idos@mellanox.com>
> 
> This commit exposes an header length compution generic function,
> introduced in some of Intel's Etherent drivers (igb and ixgbe),
> to net public namespace under the name __net_get_headlen,
> where it can be used by other vendors and removes code duplication.
> 
> Signed-off-by: Ido Shamay <idos@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>

Thanks for doing this.  It was something I was meaning to do but just
haven't had the time to do it.

Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

Re: [PATCH net-next 1/2] net: Expose header length compution function

[David Miller]

From: Amir Vadai <amirv@mellanox.com>
Date: Thu,  8 May 2014 15:50:33 +0300

> From: Ido Shamay <idos@mellanox.com>
> 
> This commit exposes an header length compution generic function,
> introduced in some of Intel's Etherent drivers (igb and ixgbe),
> to net public namespace under the name __net_get_headlen,
> where it can be used by other vendors and removes code duplication.
> 
> Signed-off-by: Ido Shamay <idos@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>

First of all, this appears (at least to me) to duplicate a lot of
what the existing flow dissector can do.

For example, you can probably call skb_flow_dissect() and the
filled in flow_keys has much of what you need.  If it doesn't
have everything you need consider extending it.

Even if that won't work out, you must add a detailed comment, with
kernel doc, above the declaration for this new interface in the
header file.

I honestly still have no idea exactly what this function does and
exactly what it's trying to achieve.

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Alexander Duyck]

On 05/09/2014 01:24 PM, David Miller wrote:
> From: Amir Vadai <amirv@mellanox.com>
> Date: Thu,  8 May 2014 15:50:33 +0300
> 
>> From: Ido Shamay <idos@mellanox.com>
>>
>> This commit exposes an header length compution generic function,
>> introduced in some of Intel's Etherent drivers (igb and ixgbe),
>> to net public namespace under the name __net_get_headlen,
>> where it can be used by other vendors and removes code duplication.
>>
>> Signed-off-by: Ido Shamay <idos@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> 
> First of all, this appears (at least to me) to duplicate a lot of
> what the existing flow dissector can do.
> 
> For example, you can probably call skb_flow_dissect() and the
> filled in flow_keys has much of what you need.  If it doesn't
> have everything you need consider extending it.
> 
> Even if that won't work out, you must add a detailed comment, with
> kernel doc, above the declaration for this new interface in the
> header file.
> 
> I honestly still have no idea exactly what this function does and
> exactly what it's trying to achieve.

Actually the flow keys with the exception of maybe thoff and ip_proto
are pretty much useless to us for our purpose.  As is we are left to
then go off and compute the header length of the transport before we get
to the information we need.

The two functions are very similar though.  It might be worth while to
actually park the two next to each other so that it is obvious that when
one gets updated to support a new protocol the other should as well.  So
for example the headlen function could pick up some of the tunnel stuff.

The main difference between __net_get_headlen() and skb_flow_dissect()
is that skb_flow_dissect is meant to work on a packet headed in either
direction, as such there are multiple calls to skb_copy_bits and all the
copies out of the paged area that go with it.  __net_get_headlen() is
meant to be called by the base netdev on a linear buffer to parse
through everything from the start of the Ethernet header to the start of
the data section and determine the length of that region.  So one
obvious difference is that we care about the length of non-l4 headers,
whereas skb_flow_dissect does not.

Thanks,

Alex

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Eric Dumazet]

On Sat, 2014-05-10 at 10:12 -0700, Alexander Duyck wrote:

> Actually the flow keys with the exception of maybe thoff and ip_proto
> are pretty much useless to us for our purpose.  As is we are left to
> then go off and compute the header length of the transport before we get
> to the information we need.
> 
> The two functions are very similar though.  It might be worth while to
> actually park the two next to each other so that it is obvious that when
> one gets updated to support a new protocol the other should as well.  So
> for example the headlen function could pick up some of the tunnel stuff.
> 
> The main difference between __net_get_headlen() and skb_flow_dissect()
> is that skb_flow_dissect is meant to work on a packet headed in either
> direction, as such there are multiple calls to skb_copy_bits and all the
> copies out of the paged area that go with it.  __net_get_headlen() is
> meant to be called by the base netdev on a linear buffer to parse
> through everything from the start of the Ethernet header to the start of
> the data section and determine the length of that region.  So one
> obvious difference is that we care about the length of non-l4 headers,
> whereas skb_flow_dissect does not.

Its actually possible to prepare an skb with the needed parts so that
skb_flow_dissect() has no copy to do.

Its a 10 lines helper maybe.

I already mentioned this in the past, I fail to understand your
resistance.

I will provide it unless someone beats me, because its the week end and
I have other plans at the moment.

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Alexander Duyck]

On 05/10/2014 10:49 AM, Eric Dumazet wrote:
> On Sat, 2014-05-10 at 10:12 -0700, Alexander Duyck wrote:
>
>> Actually the flow keys with the exception of maybe thoff and ip_proto
>> are pretty much useless to us for our purpose.  As is we are left to
>> then go off and compute the header length of the transport before we get
>> to the information we need.
>>
>> The two functions are very similar though.  It might be worth while to
>> actually park the two next to each other so that it is obvious that when
>> one gets updated to support a new protocol the other should as well.  So
>> for example the headlen function could pick up some of the tunnel stuff.
>>
>> The main difference between __net_get_headlen() and skb_flow_dissect()
>> is that skb_flow_dissect is meant to work on a packet headed in either
>> direction, as such there are multiple calls to skb_copy_bits and all the
>> copies out of the paged area that go with it.  __net_get_headlen() is
>> meant to be called by the base netdev on a linear buffer to parse
>> through everything from the start of the Ethernet header to the start of
>> the data section and determine the length of that region.  So one
>> obvious difference is that we care about the length of non-l4 headers,
>> whereas skb_flow_dissect does not.
> Its actually possible to prepare an skb with the needed parts so that
> skb_flow_dissect() has no copy to do.
>
> Its a 10 lines helper maybe.
>
> I already mentioned this in the past, I fail to understand your
> resistance.
>
> I will provide it unless someone beats me, because its the week end and
> I have other plans at the moment.

I'm more of a fan of purpose built functions in hot-path.  In the case
of skb_flow_dissect, it is meant to collect the inputs for a Jenkins
hash.  If we also expand it to get the length my concern is that it may
do both, but it won't be very efficient at doing either, and that
doesn't even take into account that somebody at some point might want
the flow dissector to not do things like coalesce IPv6 addresses to
support things like a Toeplitz hash which would slow things down further.

I can wait for the patch. I don't really see what you're talking about
since we are trying to linearize the header portion of the buffers and
for jumbos frames all 2K of the buffer has been used so you can't do any
tricks like use a paged frag for the head.

Thanks,

Alex

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Eric Dumazet]

On Sat, 2014-05-10 at 14:53 -0700, Alexander Duyck wrote:

> I'm more of a fan of purpose built functions in hot-path.  In the case
> of skb_flow_dissect, it is meant to collect the inputs for a Jenkins
> hash.

Not really.

And having multiple flow dissectors is really a lot of trouble for us,
and contributes to code bloat.

>   If we also expand it to get the length my concern is that it may
> do both, but it won't be very efficient at doing either, and that
> doesn't even take into account that somebody at some point might want
> the flow dissector to not do things like coalesce IPv6 addresses to
> support things like a Toeplitz hash which would slow things down further.
> 
> I can wait for the patch. I don't really see what you're talking about
> since we are trying to linearize the header portion of the buffers and
> for jumbos frames all 2K of the buffer has been used so you can't do any
> tricks like use a paged frag for the head.


I've tested the following with a mlx4, and it indeed speeds up GRE
traffic, as GRO packets can now contain 17 MSS instead of 8.

(Pulling payload means GRO had to use 2 'frags' per MSS)

 drivers/net/ethernet/mellanox/mlx4/en_rx.c |   13 ++++++----
 include/linux/skbuff.h                     |    1 
 net/core/flow_dissector.c                  |   23 +++++++++++++++++++
 3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index e8c0d2b832b7..ff996b7c9a89 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -586,6 +586,7 @@ static struct sk_buff *mlx4_en_rx_skb(struct mlx4_en_priv *priv,
 		skb_copy_to_linear_data(skb, va, length);
 		skb->tail += length;
 	} else {
+		unsigned int hlen;
 		/* Move relevant fragments to skb */
 		used_frags = mlx4_en_complete_rx_desc(priv, rx_desc, frags,
 							skb, length);
@@ -595,16 +596,18 @@ static struct sk_buff *mlx4_en_rx_skb(struct mlx4_en_priv *priv,
 		}
 		skb_shinfo(skb)->nr_frags = used_frags;
 
+		hlen = eth_frame_headlen(va, length);
+
 		/* Copy headers into the skb linear buffer */
-		memcpy(skb->data, va, HEADER_COPY_SIZE);
-		skb->tail += HEADER_COPY_SIZE;
+		memcpy(skb->data, va, hlen);
+		skb->tail += hlen;
 
 		/* Skip headers in first fragment */
-		skb_shinfo(skb)->frags[0].page_offset += HEADER_COPY_SIZE;
+		skb_shinfo(skb)->frags[0].page_offset += hlen;
 
 		/* Adjust size of first fragment */
-		skb_frag_size_sub(&skb_shinfo(skb)->frags[0], HEADER_COPY_SIZE);
-		skb->data_len = length - HEADER_COPY_SIZE;
+		skb_frag_size_sub(&skb_shinfo(skb)->frags[0], hlen);
+		skb->data_len = length - hlen;
 	}
 	return skb;
 }
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 7a9beeb1c458..4b3d8999a11c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3066,6 +3066,7 @@ bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off);
 int skb_checksum_setup(struct sk_buff *skb, bool recalculate);
 
 u32 __skb_get_poff(const struct sk_buff *skb);
+u32 eth_frame_headlen(void *data, unsigned int len);
 
 /**
  * skb_head_is_locked - Determine if the skb->head is locked down
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 107ed12a5323..55d4831493c1 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -323,6 +323,29 @@ u32 __skb_get_poff(const struct sk_buff *skb)
 	return poff;
 }
 
+
+/* Helper to find length of headers in an ethernet frame.
+ * This can help drivers to pull exact amount of bytes into
+ * skb->head to get optimal GRO performance.
+ * TODO: Could also return rxhash while we do a complete flow dissection.
+ */
+u32 eth_frame_headlen(void *data, unsigned int len)
+{
+	const struct ethhdr *eth = data;
+	struct sk_buff skb;
+
+	if (unlikely(len < ETH_HLEN))
+		return len;
+
+	skb.protocol = eth->h_proto;
+	skb.head = skb.data = data + ETH_HLEN;
+	skb_reset_network_header(&skb);
+	skb.len = len - ETH_HLEN;
+	skb.data_len = 0;
+	return __skb_get_poff(&skb) + ETH_HLEN;
+}
+EXPORT_SYMBOL(eth_frame_headlen);
+
 static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
 {
 #ifdef CONFIG_XPS

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Alexander Duyck]

On 05/19/2014 02:01 PM, Eric Dumazet wrote:
> On Sat, 2014-05-10 at 14:53 -0700, Alexander Duyck wrote:
> 
>> I'm more of a fan of purpose built functions in hot-path.  In the case
>> of skb_flow_dissect, it is meant to collect the inputs for a Jenkins
>> hash.
> 
> Not really.
> 
> And having multiple flow dissectors is really a lot of trouble for us,
> and contributes to code bloat.
> 
>>   If we also expand it to get the length my concern is that it may
>> do both, but it won't be very efficient at doing either, and that
>> doesn't even take into account that somebody at some point might want
>> the flow dissector to not do things like coalesce IPv6 addresses to
>> support things like a Toeplitz hash which would slow things down further.
>>
>> I can wait for the patch. I don't really see what you're talking about
>> since we are trying to linearize the header portion of the buffers and
>> for jumbos frames all 2K of the buffer has been used so you can't do any
>> tricks like use a paged frag for the head.
> 

So it looks like you did kind of what I expected you would, only you
allocated a temporary sk_buff on the stack and then pointed the head to
the start of the page.  I'm not really a fan of this approach though it
does give me a couple ideas.

One thought I just had though, what if we were to do something like
create an eth_build_skb function?  It would essentially be a cross
between eth_type_trans, your new eth_frame_headlen function, and
build_skb.  It would allow us to avoid the unnecessary allocation of an
skb on the stack and avoid any unnecessary data duplication since we
already would be doing a number of the eth_type_trans steps in your
eth_frame_headlen function.  The one limitation is that we would need to
allocate a block of memory for the head, but that would be done after we
figure out what the size of the header is.

If I get a chance I might try coding it up on Friday to see what
something like that might look like.

Thanks,

Alex

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Eric Dumazet]

On Wed, 2014-05-21 at 08:03 -0700, Alexander Duyck wrote:

> So it looks like you did kind of what I expected you would, only you
> allocated a temporary sk_buff on the stack and then pointed the head to
> the start of the page.  I'm not really a fan of this approach though it
> does give me a couple ideas.
> 
> One thought I just had though, what if we were to do something like
> create an eth_build_skb function?

Well, it all depends if you use napi_get_frags() / napi_gro_frags(),
which are normally the way to get very fast GRO processing, since
you don't even have to allocate memory for the skbs at all, since
skb will likely be recycled in napi_reuse_skb()

>   It would essentially be a cross
> between eth_type_trans, your new eth_frame_headlen function, and
> build_skb.  It would allow us to avoid the unnecessary allocation of an
> skb on the stack and avoid any unnecessary data duplication since we
> already would be doing a number of the eth_type_trans steps in your
> eth_frame_headlen function.  The one limitation is that we would need to
> allocate a block of memory for the head, but that would be done after we
> figure out what the size of the header is.

'Allocating' an skb on stack has no cost. Exactly 0 added instructions.

It only increases the size of stack, and at this point we are before all
the networking stacks, so it is safe.

Have you seen the eBPF stuff adding more stack usage than this ?

#define MAX_BPF_STACK 512

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Alexander Duyck]

On 05/21/2014 08:51 AM, Eric Dumazet wrote:
> On Wed, 2014-05-21 at 08:03 -0700, Alexander Duyck wrote:
>
>> So it looks like you did kind of what I expected you would, only you
>> allocated a temporary sk_buff on the stack and then pointed the head to
>> the start of the page.  I'm not really a fan of this approach though it
>> does give me a couple ideas.
>>
>> One thought I just had though, what if we were to do something like
>> create an eth_build_skb function?
> Well, it all depends if you use napi_get_frags() / napi_gro_frags(),
> which are normally the way to get very fast GRO processing, since
> you don't even have to allocate memory for the skbs at all, since
> skb will likely be recycled in napi_reuse_skb()

Another thought would be to possibly look into a GRO type approach. 
Something where we could place the length parsing functions in the
offload_callbacks.  If we could do that then we could just integrate the
functionality with GRO and make use of those callbacks.  Basically it
would require doing the parsing as a part of napi_frags_skb() so that
when we do the pull we get the full header length in one shot so that
the entire frame is linear right from the start.

Actually the more I think about this now the more it makes sense.  We
could probably pull out all the skb_gro_header_hard/skb_gro_header_slow
length bits from the existing gro_receive functions and place them in
another piece of the code.

>>   It would essentially be a cross
>> between eth_type_trans, your new eth_frame_headlen function, and
>> build_skb.  It would allow us to avoid the unnecessary allocation of an
>> skb on the stack and avoid any unnecessary data duplication since we
>> already would be doing a number of the eth_type_trans steps in your
>> eth_frame_headlen function.  The one limitation is that we would need to
>> allocate a block of memory for the head, but that would be done after we
>> figure out what the size of the header is.
> 'Allocating' an skb on stack has no cost. Exactly 0 added instructions.
>
> It only increases the size of stack, and at this point we are before all
> the networking stacks, so it is safe.
>
> Have you seen the eBPF stuff adding more stack usage than this ?
>
> #define MAX_BPF_STACK 512

We have had stack smashing issues in the past with the ixgbe interrupt
handlers and it wasn't consuming much memory on the stack as I recall. 
I prefer to err on the side of caution.

Also the more I think about it I am not really comfortable putting a
partially initialized sk_buff through any function calls.  It seems like
it is setting somebody up for a failure because if at some point the
code changes and needs some other field out of the skb it won't be
initialized here unless they catch this tricky bit of code.

Re: [PATCH net-next 1/2] net: Expose header length compution function

[Eric Dumazet]

On Wed, 2014-05-21 at 09:45 -0700, Alexander Duyck wrote:

> We have had stack smashing issues in the past with the ixgbe interrupt
> handlers and it wasn't consuming much memory on the stack as I recall. 
> I prefer to err on the side of caution.

Well, if we can not temporarily use 256 bytes in a leaf function in rx
handler of a driver, how IP + TCP stack will ever work ?

> Also the more I think about it I am not really comfortable putting a
> partially initialized sk_buff through any function calls.  It seems like
> it is setting somebody up for a failure because if at some point the
> code changes and needs some other field out of the skb it won't be
> initialized here unless they catch this tricky bit of code.

We are speaking of flow_dissect, which is a leaf function if 
data_len = 0

The fields I setup are the only ones that can be read from flow
dissection.

We look at a frame, given a starting point (network header) and protocol
(skb->protocol).

Apparent complexity is apparent only : skb_header_pointer() is basically
a nop if data_len = 0

Sure, you can add a

#ifdef DEBUG_FLOW_DISSECT
   memset(&skb, 0x6b, sizeof(skb));
#endif

And I did/tried that before sending the patch ;)