net: do not disable sg for AoE

PATCH In review Ed Cashin 3 participants 9 messages Sep 19, 2012 – Sep 20, 2012

Subsystems (4)

Networking Network drivers Block layer ATA over Ethernet (aoe) driver

lore

Share & tools

lore

Ed Cashin

author

2012-09-19 00:20 UTC #0 lore raw

PATCH 1/1 net: do not disable sg for AoE

A change in a series of VLAN-related changes appears to have inadvertently disabled the use of the scatter gather feature of network cards for transmission of non-IP ethernet protocols like ATA over Ethernet (AoE). Below is a reference to the commit that introduces a "harmonize_features" function that turns off scatter gather when the NIC does not support hardware checksumming for the ethernet protocol of an sk buff.

commit f01a5236bd4b140198fbcc550f085e8361fd73fa
Author: Jesse Gross <[email protected]>
Date: Sun Jan 9 06:23:31 2011 +0000

net offloading: Generalize netif_get_vlan_features().

The can_checksum_protocol function is not equipped to consider a protocol that does not require checksumming. Calling it for a protocol that requires no checksum is inappropriate.

The patch below has harmonize_features call can_checksum_protocol when the protocol needs a checksum, so that the network layer is not forced to perform unnecessary skb linearization on the transmission of AoE packets. Unnecessary linearization results in decreased performance and increased memory pressure, as reported here:

http://www.spinics.net/lists/linux-mm/msg15184.html

The problem has probably not been widely experienced yet, because only recently has the kernel.org-distributed aoe driver acquired the ability to use payloads of over a page in size, with the patchset recently included in the mm tree:

https://lkml.org/lkml/2012/8/28/140

The coraid.com-distributed aoe driver already could use payloads of greater than a page in size, but its users generally do not use the newest kernels.

Signed-off-by Ed Cashin <[email protected]>

---
 net/core/dev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/core/dev.c +2 -1

index d7fe32c..5531159 100644
@@ -2134,7 +2134,8 @@ static bool can_checksum_protocol(netdev_features_t features, __be16 protocol)
 static netdev_features_t harmonize_features(struct sk_buff *skb,
 	__be16 protocol, netdev_features_t features)
 {
-	if (!can_checksum_protocol(features, protocol)) {
+	if (protocol != htons(ETH_P_AOE) &&
+	    !can_checksum_protocol(features, protocol)) {
 		features &= ~NETIF_F_ALL_CSUM;
 		features &= ~NETIF_F_SG;
 	} else if (illegal_highdma(skb->dev, skb)) {

Signature · 1 line

1.7.2.5

Ben Hutchings

2012-09-19 19:55 UTC #1 lore raw

Quoted 12 lines · Ed Cashin

A change in a series of VLAN-related changes appears to have inadvertently disabled the use of the scatter gather feature of network cards for transmission of non-IP ethernet protocols like ATA over Ethernet (AoE). Below is a reference to the commit that introduces a "harmonize_features" function that turns off scatter gather when the NIC does not support hardware checksumming for the ethernet protocol of an sk buff.
commit f01a5236bd4b140198fbcc550f085e8361fd73fa
Author: Jesse Gross <[email protected]>
Date: Sun Jan 9 06:23:31 2011 +0000

Quoted 5 lines

net offloading: Generalize netif_get_vlan_features().
The can_checksum_protocol function is not equipped to consider a protocol that does not require checksumming. Calling it for a protocol that requires no checksum is inappropriate.

Right, but...

[...]

net/core/dev.c static bool can_checksum_protocol(netdev_features_t features, __be16 protocol) · line 2137

Quoted 10 lines

net/core/dev.c +1 -1

index d7fe32c..5531159 100644
@@ -2134,7 +2134,8 @@ static bool can_checksum_protocol(netdev_features_t features, __be16 protocol)
 static netdev_features_t harmonize_features(struct sk_buff *skb,
 	__be16 protocol, netdev_features_t features)
 {
-	if (!can_checksum_protocol(features, protocol)) {
+	if (protocol != htons(ETH_P_AOE) &&

the generic way to check that would be skb->ip_summed != CHECKSUM_NONE.

Ben.

Quoted 4 lines

net/core/dev.c +1 -0

+	    !can_checksum_protocol(features, protocol)) {
 		features &= ~NETIF_F_ALL_CSUM;
 		features &= ~NETIF_F_SG;
 	} else if (illegal_highdma(skb->dev, skb)) {

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

David Miller

maintainer

2012-09-19 20:00 UTC #7 lore raw

In reply to Ed Cashin

Quoted 2 lines

Date: Tue, 18 Sep 2012 20:20:36 -0400

Please do not incorporate the date of the commit in your local tree into the Date: field advertised in your outgoing emails when you post patches.

You did not post this on September 18th, you posted it on September 19th.

When you post patches the way you did, it screws up the ordering of my patch queue at:

http://patchwork.ozlabs.org/project/netdev/list/

since it is sorted by the Date: in the patch's email.

Therefore if you just use the normal email posting date it makes my life a lot easier and it makes sure patches will be looked at and processed in the correct order.

Thanks.

Ed Cashin

author

2012-09-19 20:14 UTC #8 lore raw

Quoted 3 lines · David Miller

Please do not incorporate the date of the commit in your local tree into the Date: field advertised in your outgoing emails when you post patches.

Noted. I will remove the Date field added by git-format-patch from now on.

Sorry for the inconvenience.

Signature · 2 lines

  Ed Cashin
  [email protected]

Ed Cashin

author

2012-09-20 01:16 UTC #2 lore raw

cover do not disable sg when packet requires no checksum

In reply to Ben Hutchings

This two-part patchset replaces an earlier net-only patch that added an explicit check for the AoE protocol to harmonize_features in net/core/dev.c.

Following the suggestion of Ben Hutchings, this patchset makes the decision in the network layer protocol agnostic instead of using ETH_P_AOE as a special case.

Ed L. Cashin (2):
aoe: mark AoE packets as requiring no checksumming
net: do not disable sg for packets requiring no checksum

 drivers/block/aoe/aoecmd.c |    1 +
 net/core/dev.c             |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

Signature · 1 line

1.7.2.5

Ed Cashin

author

2012-09-20 01:17 UTC #3 lore raw

PATCH 1/2 aoe: mark AoE packets as requiring no checksumming

In order for the network layer to see that AoE requires no checksumming in a generic way, packets should be marked CHECKSUM_NONE.

Rather than relying on the current behavior of alloc_skb, this change causes the aoe driver to explicitly mark its packets as requiring no checksum.

Signed-off-by Ed Cashin <[email protected]>

---
 drivers/block/aoe/aoecmd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/block/aoe/aoecmd.c +1 -0

index de0435e..0ba1b63 100644
@@ -35,6 +35,7 @@ new_skb(ulong len)
 		skb_reset_mac_header(skb);
 		skb_reset_network_header(skb);
 		skb->protocol = __constant_htons(ETH_P_AOE);
+		skb->ip_summed = CHECKSUM_NONE;
 	}
 	return skb;
 }

Signature · 1 line

1.7.2.5

Ed Cashin

author

2012-09-20 01:20 UTC #6 lore raw

PATCH 2/2 net: do not disable sg for packets requiring no checksum

In reply to Ed Cashin

commit f01a5236bd4b140198fbcc550f085e8361fd73fa
Author: Jesse Gross <[email protected]>
Date: Sun Jan 9 06:23:31 2011 +0000

net offloading: Generalize netif_get_vlan_features().

The can_checksum_protocol function is not equipped to consider a protocol that does not require checksumming. Calling it for a protocol that requires no checksum is inappropriate.

http://www.spinics.net/lists/linux-mm/msg15184.html

https://lkml.org/lkml/2012/8/28/140

The coraid.com-distributed aoe driver already could use payloads of greater than a page in size, but its users generally do not use the newest kernels.

Signed-off-by Ed Cashin <[email protected]>

---
 net/core/dev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/core/dev.c +2 -1

index d7fe32c..9b934d4 100644
@@ -2134,7 +2134,8 @@ static bool can_checksum_protocol(netdev_features_t features, __be16 protocol)
 static netdev_features_t harmonize_features(struct sk_buff *skb,
 	__be16 protocol, netdev_features_t features)
 {
-	if (!can_checksum_protocol(features, protocol)) {
+	if (skb->ip_summed != CHECKSUM_NONE &&
+	    !can_checksum_protocol(features, protocol)) {
 		features &= ~NETIF_F_ALL_CSUM;
 		features &= ~NETIF_F_SG;
 	} else if (illegal_highdma(skb->dev, skb)) {

Signature · 1 line

1.7.2.5

Ben Hutchings

2012-09-20 01:25 UTC #4 lore raw

Re: [PATCH 1/2] aoe: mark AoE packets as requiring no checksumming

In reply to Ed Cashin

Quoted 3 lines · Ed Cashin

In order for the network layer to see that AoE requires no checksumming in a generic way, packets should be marked CHECKSUM_NONE.

Which they are by default.

Quoted 14 lines

Rather than relying on the current behavior of alloc_skb, this change causes the aoe driver to explicitly mark its packets as requiring no checksum.
Signed-off-by Ed Cashin <[email protected]>
---
 drivers/block/aoe/aoecmd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/block/aoe/aoecmd.c +0 -0
index de0435e..0ba1b63 100644

drivers/block/aoe/aoecmd.c new_skb(ulong len) · line 38

Quoted 4 lines

drivers/block/aoe/aoecmd.c +1 -0

@@ -35,6 +35,7 @@ new_skb(ulong len)
 		skb_reset_mac_header(skb);
 		skb_reset_network_header(skb);
 		skb->protocol = __constant_htons(ETH_P_AOE);
+		skb->ip_summed = CHECKSUM_NONE;

Can be written as:

		skb_checksum_none_assert(skb);

if you really want to be sure.

Ben.

Quoted 3 lines

drivers/block/aoe/aoecmd.c +0 -0
 	}
 	return skb;
 }

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Ed Cashin

author

2012-09-20 01:40 UTC #5 lore raw

Re: [PATCH 1/2] aoe: mark AoE packets as requiring no checksumming

Quoted 2 lines · Ben Hutchings

Can be written as:
		skb_checksum_none_assert(skb);

Another good idea. I'll resend.

Signature · 2 lines

  Ed Cashin
  [email protected]