Skip to contentSkip to conversation

do not disable sg when packet requires no checksum

PATCH In review Ed Cashin 2 participants 4 messages Sep 20, 2012 – Sep 21, 2012

Subsystems (4)
lore
Share & tools
lore

Ed Cashin

author
#0 lore raw

cover do not disable sg when packet requires no checksum

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 suggestions of Ben Hutchings, this patchset makes the decision in the network layer protocol agnostic instead of using ETH_P_AOE as a special case. It relies on fresh skbs being CHECKSUM_NONE but makes that explicit with an assertion.

Ed L. Cashin (2):
aoe: assert AoE packets marked as requiring no checksum
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
author
#1 lore raw

PATCH 1/2 aoe: assert AoE packets marked as requiring no checksum

In order for the network layer to see that AoE requires no checksumming in a generic way, the packets must be marked as requiring no checksum, so we make this requirement explicit with the assertion.

---
 drivers/block/aoe/aoecmd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/block/aoe/aoecmd.c +1 -0
index de0435e..887f68f 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_checksum_none_assert(skb);
 	}
 	return skb;
 }
Signature · 1 line
1.7.2.5
author
#2 lore raw

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

In reply to 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

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.

---
 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
maintainer
#3 lore raw
In reply to Ed Cashin

From: Ed Cashin <[email protected]>
Date: Wed, 19 Sep 2012 18:46:07 -0700

Quoted 4 lines

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.

Quoted 8 lines

Following the suggestions of Ben Hutchings, this patchset makes the decision in the network layer protocol agnostic instead of using ETH_P_AOE as a special case. It relies on fresh skbs being CHECKSUM_NONE but makes that explicit with an assertion.

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

Applied and queued up for -stable, thanks Ed.