Re: [PATCH] net: Make skb_segment not to compute checksum if network controller supports checksumming
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2020-02-23 02:41:58
On Thu, Feb 20, 2020 at 11:14 PM Yadu Kishore [off-list ref] wrote:
Problem: TCP checksum in the output path is not being offloaded during GSO in the following case: The network driver does not support scatter-gather but supports checksum offload with NETIF_F_HW_CSUM. Cause: skb_segment calls skb_copy_and_csum_bits if the network driver does not announce NETIF_F_SG. It does not check if the driver supports NETIF_F_HW_CSUM. So for devices which might want to offload checksum but do not support SG there is currently no way to do so if GSO is enabled. Solution: In skb_segment check if the network controller does checksum and if so call skb_copy_bits instead of skb_copy_and_csum_bits. Testing: Without the patch, ran iperf TCP traffic with NETIF_F_HW_CSUM enabled in the network driver. Observed the TCP checksum offload is not happening since the skbs received by the driver in the output path have skb->ip_summed set to CHECKSUM_NONE. With the patch ran iperf TCP traffic and observed that TCP checksum is being offloaded with skb->ip_summed set to CHECKSUM_PARTIAL.
Did you measure a cycle efficiency improvement? As discussed in the referred email thread, the kernel uses checksum_and_copy because it is generally not significantly more expensive than copy alone. skb_segment already is a very complex function. New code needs to offer a tangible benefit.
Also tested with the patch by disabling NETIF_F_HW_CSUM in the driver to cover the newly introduced if-else code path in skb_segment. In-Reply-To: CABGOaVTY6BrzJTYEtVXwawzP7-D8sb1KASDWFk15v0QFaJVbUg@mail.gmail.com
This does not seem to be a commonly used tag. And indeed differs from the actual In-Reply-To in the email headers. Perhaps Link: https://lore.kernel.org/netdev/CABGOaVTY6BrzJTYEtVXwawzP7-D8sb1KASDWFk15v0QFaJVbUg@mail.gmail.com/ (local)
quoted hunk ↗ jump to hunk
Signed-off-by: Yadu Kishore <redacted> --- net/core/skbuff.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 1365a55..82a5b53 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c@@ -3926,14 +3926,22 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, goto perform_csum_check; if (!sg) { - if (!nskb->remcsum_offload) - nskb->ip_summed = CHECKSUM_NONE; - SKB_GSO_CB(nskb)->csum = - skb_copy_and_csum_bits(head_skb, offset, - skb_put(nskb, len), - len, 0); - SKB_GSO_CB(nskb)->csum_start = - skb_headroom(nskb) + doffset; + if (!csum) { + if (!nskb->remcsum_offload) + nskb->ip_summed = CHECKSUM_NONE; + SKB_GSO_CB(nskb)->csum = + skb_copy_and_csum_bits(head_skb, offset, + skb_put(nskb, + len), + len, 0); + SKB_GSO_CB(nskb)->csum_start = + skb_headroom(nskb) + doffset; + } else { + nskb->ip_summed = CHECKSUM_PARTIAL;
Is this not already handled by __copy_skb_header above? If ip_summed has to be initialized, so have csum_start and csum_offset. That call should have initialized all three.
+ skb_copy_bits(head_skb, offset,
+ skb_put(nskb, len),
+ len);
+ }
continue;
}
--
2.7.4