[PATCH net] ibmveth: Remove condition to recompute TCP header checksum.

[PATCH net] ibmveth: Remove condition to recompute TCP header checksum.

[Nick Child]

From: David Wilder <dwilder@us.ibm.com>

In some OVS environments the TCP pseudo header checksum may need to be
recomputed. Currently this is only done when the interface instance is
configured for "Trunk Mode". We found the issue also occurs in some
Kubernetes environments, these environments do not use "Trunk Mode",
therefor the condition is removed.

Performance tests with this change show only a fractional decrease in
throughput (< 0.2%).

Fixes: 7525de2516fb ("ibmveth: Set CHECKSUM_PARTIAL if NULL TCP CSUM.")
Signed-off-by: David Wilder <dwilder@us.ibm.com>
Reviewed-by: Nick Child <nnac123@linux.ibm.com>
---
Hello, I (Nick Child) am submitting on behalf of the
author (David Wilder) since he is having patch submission issues.
Apologies for any inconvenience.


 drivers/net/ethernet/ibm/ibmveth.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 113fcb3e353e..748ee25cee8d 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -1303,24 +1303,23 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
 	 * the user space for finding a flow. During this process, OVS computes
 	 * checksum on the first packet when CHECKSUM_PARTIAL flag is set.
 	 *
-	 * So, re-compute TCP pseudo header checksum when configured for
-	 * trunk mode.
+	 * So, re-compute TCP pseudo header checksum.
 	 */
+
 	if (iph_proto == IPPROTO_TCP) {
 		struct tcphdr *tcph = (struct tcphdr *)(skb->data + iphlen);
+
 		if (tcph->check == 0x0000) {
 			/* Recompute TCP pseudo header checksum  */
-			if (adapter->is_active_trunk) {
-				tcphdrlen = skb->len - iphlen;
-				if (skb_proto == ETH_P_IP)
-					tcph->check =
-					 ~csum_tcpudp_magic(iph->saddr,
-					iph->daddr, tcphdrlen, iph_proto, 0);
-				else if (skb_proto == ETH_P_IPV6)
-					tcph->check =
-					 ~csum_ipv6_magic(&iph6->saddr,
-					&iph6->daddr, tcphdrlen, iph_proto, 0);
-			}
+			tcphdrlen = skb->len - iphlen;
+			if (skb_proto == ETH_P_IP)
+				tcph->check =
+				 ~csum_tcpudp_magic(iph->saddr,
+				iph->daddr, tcphdrlen, iph_proto, 0);
+			else if (skb_proto == ETH_P_IPV6)
+				tcph->check =
+				 ~csum_ipv6_magic(&iph6->saddr,
+				&iph6->daddr, tcphdrlen, iph_proto, 0);
 			/* Setup SKB fields for checksum offload */
 			skb_partial_csum_set(skb, iphlen,
 					     offsetof(struct tcphdr, check));
-- 
2.39.3

Re: [PATCH net] ibmveth: Remove condition to recompute TCP header checksum.

[Jacob Keller]

On 9/26/2023 2:42 PM, Nick Child wrote:
> From: David Wilder <dwilder@us.ibm.com>
> 
> In some OVS environments the TCP pseudo header checksum may need to be
> recomputed. Currently this is only done when the interface instance is
> configured for "Trunk Mode". We found the issue also occurs in some
> Kubernetes environments, these environments do not use "Trunk Mode",
> therefor the condition is removed.
> 
> Performance tests with this change show only a fractional decrease in
> throughput (< 0.2%).
> 
> Fixes: 7525de2516fb ("ibmveth: Set CHECKSUM_PARTIAL if NULL TCP CSUM.")
> Signed-off-by: David Wilder <dwilder@us.ibm.com>
> Reviewed-by: Nick Child <nnac123@linux.ibm.com>
> ---
> Hello, I (Nick Child) am submitting on behalf of the
> author (David Wilder) since he is having patch submission issues.
> Apologies for any inconvenience.
> 

I think you're supposed to add your own Signed-off-by when sending
patches on behalf of another author, but its clear who the author is
with the From line, and you called it out here too. Thanks! Just a note
for future submissions on behalf of others. From
Documentation/process/submitting-patches.rst:


> Any further SoBs (Signed-off-by:'s) following the author's SoB are from
> people handling and transporting the patch, but were not involved in its
> development. SoB chains should reflect the **real** route a patch took
> as it was propagated to the maintainers and ultimately to Linus, with
> the first SoB entry signalling primary authorship of a single author.
>> When to use Acked-by:, Cc:, and Co-developed-by:
> ------------------------------------------------
> 
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch's delivery path.
> 

I don't think this should require a resend, but its good to note for the
future :)

Patch looks good. I assume there isn't a different "fast" check that can
be done to determine if we need the recomputation, and it doesn't hit
the performance too badly. The simplification and ensuring that no one
else hits this error in the future is worth the slight cost I think.
Correctness comes before speed.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> 
>  drivers/net/ethernet/ibm/ibmveth.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 113fcb3e353e..748ee25cee8d 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1303,24 +1303,23 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
>  	 * the user space for finding a flow. During this process, OVS computes
>  	 * checksum on the first packet when CHECKSUM_PARTIAL flag is set.
>  	 *
> -	 * So, re-compute TCP pseudo header checksum when configured for
> -	 * trunk mode.
> +	 * So, re-compute TCP pseudo header checksum.
>  	 */
> +
>  	if (iph_proto == IPPROTO_TCP) {
>  		struct tcphdr *tcph = (struct tcphdr *)(skb->data + iphlen);
> +
>  		if (tcph->check == 0x0000) {
>  			/* Recompute TCP pseudo header checksum  */
> -			if (adapter->is_active_trunk) {
> -				tcphdrlen = skb->len - iphlen;
> -				if (skb_proto == ETH_P_IP)
> -					tcph->check =
> -					 ~csum_tcpudp_magic(iph->saddr,
> -					iph->daddr, tcphdrlen, iph_proto, 0);
> -				else if (skb_proto == ETH_P_IPV6)
> -					tcph->check =
> -					 ~csum_ipv6_magic(&iph6->saddr,
> -					&iph6->daddr, tcphdrlen, iph_proto, 0);
> -			}
> +			tcphdrlen = skb->len - iphlen;
> +			if (skb_proto == ETH_P_IP)
> +				tcph->check =
> +				 ~csum_tcpudp_magic(iph->saddr,
> +				iph->daddr, tcphdrlen, iph_proto, 0);
> +			else if (skb_proto == ETH_P_IPV6)
> +				tcph->check =
> +				 ~csum_ipv6_magic(&iph6->saddr,
> +				&iph6->daddr, tcphdrlen, iph_proto, 0);
>  			/* Setup SKB fields for checksum offload */
>  			skb_partial_csum_set(skb, iphlen,
>  					     offsetof(struct tcphdr, check));

Re: [PATCH net] ibmveth: Remove condition to recompute TCP header checksum.

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 26 Sep 2023 16:42:51 -0500 you wrote:
> From: David Wilder <dwilder@us.ibm.com>
> 
> In some OVS environments the TCP pseudo header checksum may need to be
> recomputed. Currently this is only done when the interface instance is
> configured for "Trunk Mode". We found the issue also occurs in some
> Kubernetes environments, these environments do not use "Trunk Mode",
> therefor the condition is removed.
> 
> [...]

Here is the summary with links:
  - [net] ibmveth: Remove condition to recompute TCP header checksum.
    https://git.kernel.org/netdev/net/c/51e7a66666e0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html