| Message ID | 20230926214251.58503-1-nnac123@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 51e7a66666e0ca9642c59464ef8359f0ac604d41 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ibmveth: Remove condition to recompute TCP header checksum. | expand |
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));
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!
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));