[PATCH net-next] gro: introduce gro_mac_header_len

Shlomo Pongratz reported GRO L2 header check was suited for Ethernet
only, and failed on IB/ipoib traffic.

He provided a patch faking a zeroed header to let GRO aggregates frames.

Roland Dreier, Herbert Xu, and others suggested we change GRO L2 header
check to be more generic.

This patch introduces a new netdevice field, gro_mac_header_len, giving
L2 header length, default to ETH_HLEN (14 bytes)

A device setup function can override this default value.

__napi_gro_receive() has special handling for the common case (Ethernet)
to avoid a memcmp() call and use an inline optimized function instead.

Signed-off-by: Eric Dumazet <redacted>
Reported-by: Shlomo Pongratz <shlomop-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Roland Dreier <redacted>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Herbert Xu <redacted>
---
 include/linux/netdevice.h |    1 +
 net/core/dev.c            |   11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0eac07c..d17192b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -1095,6 +1095,7 @@ struct net_device {
 	unsigned int		mtu;	/* interface MTU value		*/
 	unsigned short		type;	/* interface hardware type	*/
 	unsigned short		hard_header_len;	/* hardware hdr length	*/
+	unsigned int		gro_mac_header_len;
 
 	/* extra head- and tailroom the hardware may need, but not in all cases
 	 * can this be guaranteed, especially tailroom. Some cases also use

diff --git a/net/core/dev.c b/net/core/dev.c
index f124947..0b43939 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3491,14 +3491,20 @@ static inline gro_result_t
 __napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 {
 	struct sk_buff *p;
+	unsigned int maclen = skb->dev->gro_mac_header_len;
 
 	for (p = napi->gro_list; p; p = p->next) {
 		unsigned long diffs;
 
 		diffs = (unsigned long)p->dev ^ (unsigned long)skb->dev;
 		diffs |= p->vlan_tci ^ skb->vlan_tci;
-		diffs |= compare_ether_header(skb_mac_header(p),
-					      skb_gro_mac_header(skb));
+		if (maclen == ETH_HLEN)
+			diffs |= compare_ether_header(skb_mac_header(p),
+						      skb_gro_mac_header(skb));
+		else if (!diffs)
+			diffs = memcmp(skb_mac_header(p),
+				       skb_gro_mac_header(skb),
+				       maclen);
 		NAPI_GRO_CB(p)->same_flow = !diffs;
 		NAPI_GRO_CB(p)->flush = 0;
 	}

@@ -5962,6 +5968,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	INIT_LIST_HEAD(&dev->unreg_list);
 	INIT_LIST_HEAD(&dev->link_watch_list);
 	dev->priv_flags = IFF_XMIT_DST_RELEASE;
+	dev->gro_mac_header_len = ETH_HLEN;
 	setup(dev);
 
 	dev->num_tx_queues = txqs;


--

To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Eric Dumazet <redacted>
Date: Mon, 06 Feb 2012 17:27:07 +0100

Shlomo Pongratz reported GRO L2 header check was suited for Ethernet
only, and failed on IB/ipoib traffic.

He provided a patch faking a zeroed header to let GRO aggregates frames.

Roland Dreier, Herbert Xu, and others suggested we change GRO L2 header
check to be more generic.

This patch introduces a new netdevice field, gro_mac_header_len, giving
L2 header length, default to ETH_HLEN (14 bytes)

A device setup function can override this default value.

__napi_gro_receive() has special handling for the common case (Ethernet)
to avoid a memcmp() call and use an inline optimized function instead.

Signed-off-by: Eric Dumazet <redacted>
Reported-by: Shlomo Pongratz <shlomop-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

We really need an explanation, probably both in the commit message and
the comments next to this new struct member, explaining why in the world
we can't use ->hard_header_len for this.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Le lundi 06 février 2012 à 11:31 -0500, David Miller a écrit :

We really need an explanation, probably both in the commit message and
the comments next to this new struct member, explaining why in the world
we can't use ->hard_header_len for this.

OK, I added some information from Or Gerlitz in V2

Thanks !

[PATCH net-next V2] gro: introduce gro_mac_header_len

Shlomo Pongratz reported GRO L2 header check was suited for Ethernet
only, and failed on IB/ipoib traffic.

He provided a patch faking a zeroed header to let GRO aggregates frames.

Roland Dreier, Herbert Xu, and others suggested we change GRO L2 header
check to be more generic.

This patch introduces a new netdevice field, gro_mac_header_len, giving
L2 header length, default to ETH_HLEN (14 bytes)

A device setup function can override this default value.

gro_max_header_len can be different than hard_header_len because as Or
Gerlitz said :

	IPoIB advertizes hard_header_len which is bigger than the
	IPoIB header len, this is done such that skbs sent by the
	network stack have enough headroom for a "pseudoheader"
	which for few flows (e.g unicast arp replies and multicast)
	is placed there by the ipoib hard_header function and later
	used by the xmit function.

__napi_gro_receive() has special handling for the common case (Ethernet)
to avoid a memcmp() call and use an inline optimized function instead.

Signed-off-by: Eric Dumazet <redacted>
Reported-by: Shlomo Pongratz <redacted>
Cc: Roland Dreier <roland@kernel.org>
Cc: Or Gerlitz <redacted>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
V2: added a comment saying why we dont use hard_header_len but a new
field.

 include/linux/netdevice.h |    1 +
 net/core/dev.c            |   11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0eac07c..903bb6e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -1095,6 +1095,7 @@ struct net_device {
 	unsigned int		mtu;	/* interface MTU value		*/
 	unsigned short		type;	/* interface hardware type	*/
 	unsigned short		hard_header_len;	/* hardware hdr length	*/
+	unsigned int		gro_mac_header_len; /* L2 header length for GRO */
 
 	/* extra head- and tailroom the hardware may need, but not in all cases
 	 * can this be guaranteed, especially tailroom. Some cases also use

diff --git a/net/core/dev.c b/net/core/dev.c
index f124947..0b43939 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3491,14 +3491,20 @@ static inline gro_result_t
 __napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 {
 	struct sk_buff *p;
+	unsigned int maclen = skb->dev->gro_mac_header_len;
 
 	for (p = napi->gro_list; p; p = p->next) {
 		unsigned long diffs;
 
 		diffs = (unsigned long)p->dev ^ (unsigned long)skb->dev;
 		diffs |= p->vlan_tci ^ skb->vlan_tci;
-		diffs |= compare_ether_header(skb_mac_header(p),
-					      skb_gro_mac_header(skb));
+		if (maclen == ETH_HLEN)
+			diffs |= compare_ether_header(skb_mac_header(p),
+						      skb_gro_mac_header(skb));
+		else if (!diffs)
+			diffs = memcmp(skb_mac_header(p),
+				       skb_gro_mac_header(skb),
+				       maclen);
 		NAPI_GRO_CB(p)->same_flow = !diffs;
 		NAPI_GRO_CB(p)->flush = 0;
 	}

@@ -5962,6 +5968,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	INIT_LIST_HEAD(&dev->unreg_list);
 	INIT_LIST_HEAD(&dev->link_watch_list);
 	dev->priv_flags = IFF_XMIT_DST_RELEASE;
+	dev->gro_mac_header_len = ETH_HLEN;
 	setup(dev);
 
 	dev->num_tx_queues = txqs;

From: Eric Dumazet <redacted>
Date: Mon, 06 Feb 2012 17:47:14 +0100

[ Roland Dreier CC:'d ]

gro_max_header_len can be different than hard_header_len because as Or
Gerlitz said :

	IPoIB advertizes hard_header_len which is bigger than the
	IPoIB header len, this is done such that skbs sent by the
	network stack have enough headroom for a "pseudoheader"
	which for few flows (e.g unicast arp replies and multicast)
	is placed there by the ipoib hard_header function and later
	used by the xmit function.

Translation: IPoIB's path resolution mechanism is garbage

So if IPoIB path resolution was properly integrated into the neighbour
cache state machine, instead of being implemented awkwardly in the
device transmit path, this crap wouldn't be necessary right?

So here we have yet another incredibly painful side effect of how
IPoIB path resolution works.

Roland, I want you to seriously consider a way, any way, to get rid of
how IPoIB does path resolution.  It must be fully integrated into the
neighbour layer, the neighbour layer must be knowledgable about how
path resolution is a necessary step for a neighbour entry to enter the
valid state, and I want all of this awkward neighbour handling code
removed from the transmit path of IPoIB.

And finally it must not lie about it's hardware header length.

Then we won't need crap like what is being proposed here, a
"no_this_is_the_real_hard_header_len" struct member.  That's just
rediculious.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Le lundi 06 février 2012 à 11:58 -0500, David Miller a écrit :

From: Eric Dumazet <redacted>
Date: Mon, 06 Feb 2012 17:47:14 +0100

[ Roland Dreier CC:'d ]

quoted

gro_max_header_len can be different than hard_header_len because as Or
Gerlitz said :

	IPoIB advertizes hard_header_len which is bigger than the
	IPoIB header len, this is done such that skbs sent by the
	network stack have enough headroom for a "pseudoheader"
	which for few flows (e.g unicast arp replies and multicast)
	is placed there by the ipoib hard_header function and later
	used by the xmit function.

Translation: IPoIB's path resolution mechanism is garbage

So if IPoIB path resolution was properly integrated into the neighbour
cache state machine, instead of being implemented awkwardly in the
device transmit path, this crap wouldn't be necessary right?

So here we have yet another incredibly painful side effect of how
IPoIB path resolution works.

Roland, I want you to seriously consider a way, any way, to get rid of
how IPoIB does path resolution.  It must be fully integrated into the
neighbour layer, the neighbour layer must be knowledgable about how
path resolution is a necessary step for a neighbour entry to enter the
valid state, and I want all of this awkward neighbour handling code
removed from the transmit path of IPoIB.

And finally it must not lie about it's hardware header length.

Then we won't need crap like what is being proposed here, a
"no_this_is_the_real_hard_header_len" struct member.  That's just
rediculious.

OK, I'll resend my first patch then, using hard_header_len

On 2/6/2012 7:07 PM, Eric Dumazet wrote:

OK, I'll resend my first patch then, using hard_header_len

so, we will be back to square one... as the hard_header_len which 
advertized now by IPoIB will fail
the GRO L2 check... lets see where this discussion evolves.

Or.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shlomo Pongratz reported GRO L2 header check was suited for Ethernet
only, and failed on IB/ipoib traffic.

He provided a patch faking a zeroed header to let GRO aggregates frames.

Roland Dreier, Herbert Xu, and others suggested we change GRO L2 header
check to be more generic, ie not assuming L2 header is 14 bytes, but
taking into account hard_header_len.

__napi_gro_receive() has special handling for the common case (Ethernet)
to avoid a memcmp() call and use an inline optimized function instead.

Signed-off-by: Eric Dumazet <redacted>
Reported-by: Shlomo Pongratz <shlomop-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Roland Dreier <redacted>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Herbert Xu <redacted>
Tested-by: Sean Hefty <redacted>
---
 net/core/dev.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f124947..763a0ed 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3491,14 +3491,20 @@ static inline gro_result_t
 __napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 {
 	struct sk_buff *p;
+	unsigned int maclen = skb->dev->hard_header_len;
 
 	for (p = napi->gro_list; p; p = p->next) {
 		unsigned long diffs;
 
 		diffs = (unsigned long)p->dev ^ (unsigned long)skb->dev;
 		diffs |= p->vlan_tci ^ skb->vlan_tci;
-		diffs |= compare_ether_header(skb_mac_header(p),
-					      skb_gro_mac_header(skb));
+		if (maclen == ETH_HLEN)
+			diffs |= compare_ether_header(skb_mac_header(p),
+						      skb_gro_mac_header(skb));
+		else if (!diffs)
+			diffs = memcmp(skb_mac_header(p),
+				       skb_gro_mac_header(skb),
+				       maclen);
 		NAPI_GRO_CB(p)->same_flow = !diffs;
 		NAPI_GRO_CB(p)->flush = 0;
 	}


--

To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Eric Dumazet <redacted>
Date: Wed, 08 Feb 2012 19:51:50 +0100

Shlomo Pongratz reported GRO L2 header check was suited for Ethernet
only, and failed on IB/ipoib traffic.

He provided a patch faking a zeroed header to let GRO aggregates frames.

Roland Dreier, Herbert Xu, and others suggested we change GRO L2 header
check to be more generic, ie not assuming L2 header is 14 bytes, but
taking into account hard_header_len.

__napi_gro_receive() has special handling for the common case (Ethernet)
to avoid a memcmp() call and use an inline optimized function instead.

Signed-off-by: Eric Dumazet <redacted>
Reported-by: Shlomo Pongratz <shlomop-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Roland Dreier <redacted>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Herbert Xu <redacted>
Tested-by: Sean Hefty <redacted>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html