diff mbox series

[ovs-dev] datapath-windows: Remove the workaround in NAT for TCP checksum

Message ID 20170915220449.2648-1-kumaranand@vmware.com
State Accepted
Headers show
Series [ovs-dev] datapath-windows: Remove the workaround in NAT for TCP checksum | expand

Commit Message

Anand Kumar Sept. 15, 2017, 10:04 p.m. UTC 
When checksum offload is enabled, compute checksum using the
TCP pseudo header.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Actions.c | 60 +++++++--------------------------------
 1 file changed, 11 insertions(+), 49 deletions(-)

Comments

Sairam Venugopal Oct. 13, 2017, 8:51 p.m. UTC | #1 
Hi Alin,

Any update on this one? I believe you had raised this issue and had sent out a patch to address this earlier.

Thanks,
Sairam




On 9/15/17, 3:04 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:

>When checksum offload is enabled, compute checksum using the
>TCP pseudo header.
>
>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>---
> datapath-windows/ovsext/Actions.c | 60 +++++++--------------------------------
> 1 file changed, 11 insertions(+), 49 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
>index 41d1c7b..0af4a66 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1528,6 +1528,11 @@ OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
>                         ((BOOLEAN)csumInfo.Receive.UdpChecksumSucceeded ||
>                          (BOOLEAN)csumInfo.Receive.UdpChecksumFailed);
>         }
>+        if (l4Offload) {
>+            *checkField = IPPseudoChecksum(&newAddr, &ipHdr->daddr,
>+                tcpHdr ? IPPROTO_TCP : IPPROTO_UDP,
>+                ntohs(ipHdr->tot_len) - ipHdr->ihl * 4);
>+        }
>     } else {
>         addrField = &ipHdr->daddr;
>         if (tcpHdr) {
>@@ -1538,19 +1543,13 @@ OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
>             checkField = &udpHdr->check;
>         }
>     }
>+
>     if (*addrField != newAddr) {
>         UINT32 oldAddr = *addrField;
>-        if (checkField && *checkField != 0) {
>-            if (l4Offload) {
>-                /* Recompute IP pseudo checksum */
>-                *checkField = ~(*checkField);
>-                *checkField = ChecksumUpdate32(*checkField, oldAddr,
>-                                               newAddr);
>-                *checkField = ~(*checkField);
>-            } else {
>-                *checkField = ChecksumUpdate32(*checkField, oldAddr,
>-                                               newAddr);
>-            }
>+        if (checkField && *checkField != 0 && !l4Offload) {
>+            /* Recompute total checksum. */
>+            *checkField = ChecksumUpdate32(*checkField, oldAddr,
>+                                            newAddr);
>         }
>         if (ipHdr->check != 0) {
>             ipHdr->check = ChecksumUpdate32(ipHdr->check, oldAddr,
>@@ -1561,49 +1560,12 @@ OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
> 
>     if (portField && *portField != newPort) {
>         if (checkField && !l4Offload) {
>+            /* Recompute total checksum. */
>             *checkField = ChecksumUpdate16(*checkField, *portField,
>                                            newPort);
>         }
>         *portField = newPort;
>     }
>-    PNET_BUFFER_LIST curNbl = ovsFwdCtx->curNbl;
>-    PNET_BUFFER_LIST newNbl = NULL;
>-    if (layers->isTcp) {
>-        UINT32 mss = OVSGetTcpMSS(curNbl);
>-        if (mss) {
>-            OVS_LOG_TRACE("l4Offset %d", layers->l4Offset);
>-            newNbl = OvsTcpSegmentNBL(ovsFwdCtx->switchContext, curNbl, layers,
>-                                      mss, 0, FALSE);
>-            if (newNbl == NULL) {
>-                OVS_LOG_ERROR("Unable to segment NBL");
>-                return NDIS_STATUS_FAILURE;
>-            }
>-            /* Clear out LSO flags after this point */
>-            NET_BUFFER_LIST_INFO(newNbl, TcpLargeSendNetBufferListInfo) = 0;
>-        }
>-    }
>-    /* If we didn't split the packet above, make a copy now */
>-    if (newNbl == NULL) {
>-        csumInfo.Value = NET_BUFFER_LIST_INFO(curNbl,
>-                                              TcpIpChecksumNetBufferListInfo);
>-        OvsApplySWChecksumOnNB(layers, curNbl, &csumInfo);
>-    }
>-
>-    if (newNbl) {
>-        curNbl = newNbl;
>-        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>-                                    L"Complete after cloning NBL for encapsulation");
>-        OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
>-                             newNbl, ovsFwdCtx->srcVportNo, 0,
>-                             NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
>-                             ovsFwdCtx->completionList,
>-                             &ovsFwdCtx->layers, FALSE);
>-        ovsFwdCtx->curNbl = newNbl;
>-    }
>-
>-    NET_BUFFER_LIST_INFO(curNbl,
>-                         TcpIpChecksumNetBufferListInfo) = 0;
>-
>     return NDIS_STATUS_SUCCESS;
> }
> 
>-- 
>2.9.3.windows.1
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=SBcJfA7atJ6aGjBxtOlOR2r14HlYT8CY-K1DpMvgt4A&s=oP6eHfHcXJwo9vAhJbfEzIDJtQ6u-WkjnbIOxErltVY&e=
Alin-Gabriel Serdean Oct. 16, 2017, 2:22 p.m. UTC | #2 
Thanks so much for the patch!

Sorry for the long delay in testing.

Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Saturday, September 16, 2017 1:05 AM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Remove the workaround in
> NAT for TCP checksum
> 
> When checksum offload is enabled, compute checksum using the TCP
> pseudo header.
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
>  datapath-windows/ovsext/Actions.c | 60
+++++++-------------------------------
> -
>  1 file changed, 11 insertions(+), 49 deletions(-)
>
Alin Serdean Oct. 16, 2017, 2:33 p.m. UTC | #3 
It looks good and I managed to test it, sorry for the delay.

I had some issues while disabling the offloads on the hw adapter on Windows but that turned out to be a different issue.

I applied it on master, but I would like to apply it on branch-2.8 as well. What do you think?

Thanks,
Alin.

> -----Original Message-----
> From: Sairam Venugopal [mailto:vsairam@vmware.com]
> Sent: Friday, October 13, 2017 11:51 PM
> To: Anand Kumar <kumaranand@vmware.com>; dev@openvswitch.org
> Cc: Alin Serdean <aserdean@cloudbasesolutions.com>
> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Remove the workaround
> in NAT for TCP checksum
> 
> Hi Alin,
> 
> Any update on this one? I believe you had raised this issue and had sent out
> a patch to address this earlier.
> 
> Thanks,
> Sairam
> 
> 
> 
> 
> On 9/15/17, 3:04 PM, "ovs-dev-bounces@openvswitch.org on behalf of
> Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of
> kumaranand@vmware.com> wrote:
> 
> >When checksum offload is enabled, compute checksum using the TCP
> pseudo
> >header.
> >
> >Signed-off-by: Anand Kumar <kumaranand@vmware.com>
Anand Kumar Oct. 16, 2017, 8:22 p.m. UTC | #4 
Hi Alin,

Thanks for applying the patch. 
Yes, we need this on branch 2.8 as well.

Thanks,
Anand Kumar

On 10/16/17, 7:33 AM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote:

    It looks good and I managed to test it, sorry for the delay.
    
    I had some issues while disabling the offloads on the hw adapter on Windows but that turned out to be a different issue.
    
    I applied it on master, but I would like to apply it on branch-2.8 as well. What do you think?
    
    Thanks,
    Alin.
    
    > -----Original Message-----
    > From: Sairam Venugopal [mailto:vsairam@vmware.com]
    > Sent: Friday, October 13, 2017 11:51 PM
    > To: Anand Kumar <kumaranand@vmware.com>; dev@openvswitch.org
    > Cc: Alin Serdean <aserdean@cloudbasesolutions.com>
    > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Remove the workaround
    > in NAT for TCP checksum
    > 
    > Hi Alin,
    > 
    > Any update on this one? I believe you had raised this issue and had sent out
    > a patch to address this earlier.
    > 
    > Thanks,
    > Sairam
    > 
    > 
    > 
    > 
    > On 9/15/17, 3:04 PM, "ovs-dev-bounces@openvswitch.org on behalf of
    > Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of
    > kumaranand@vmware.com> wrote:
    > 
    > >When checksum offload is enabled, compute checksum using the TCP
    > pseudo
    > >header.
    > >
    > >Signed-off-by: Anand Kumar <kumaranand@vmware.com>
Alin Serdean Oct. 17, 2017, 12:29 a.m. UTC | #5 
Applied to branch-2.8 as well.

Thanks!

> -----Original Message-----
> From: Anand Kumar [mailto:kumaranand@vmware.com]
> Sent: Monday, October 16, 2017 11:22 PM
> To: Alin Serdean <aserdean@cloudbasesolutions.com>; Sairam Venugopal
> <vsairam@vmware.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] datapath-windows: Remove the workaround
> in NAT for TCP checksum
> 
> Hi Alin,
> 
> Thanks for applying the patch.
> Yes, we need this on branch 2.8 as well.
> 
> Thanks,
> Anand Kumar
> 
> On 10/16/17, 7:33 AM, "Alin Serdean" <aserdean@cloudbasesolutions.com>
> wrote:
> 
>     It looks good and I managed to test it, sorry for the delay.
> 
>     I had some issues while disabling the offloads on the hw adapter on
> Windows but that turned out to be a different issue.
> 
>     I applied it on master, but I would like to apply it on branch-2.8 as well.
> What do you think?
> 
>     Thanks,
>     Alin.
> 
>     > -----Original Message-----
>     > From: Sairam Venugopal [mailto:vsairam@vmware.com]
>     > Sent: Friday, October 13, 2017 11:51 PM
>     > To: Anand Kumar <kumaranand@vmware.com>; dev@openvswitch.org
>     > Cc: Alin Serdean <aserdean@cloudbasesolutions.com>
>     > Subject: Re: [ovs-dev] [PATCH] datapath-windows: Remove the
> workaround
>     > in NAT for TCP checksum
>     >
>     > Hi Alin,
>     >
>     > Any update on this one? I believe you had raised this issue and had sent
> out
>     > a patch to address this earlier.
>     >
>     > Thanks,
>     > Sairam
>     >
>     >
>     >
>     >
>     > On 9/15/17, 3:04 PM, "ovs-dev-bounces@openvswitch.org on behalf of
>     > Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of
>     > kumaranand@vmware.com> wrote:
>     >
>     > >When checksum offload is enabled, compute checksum using the TCP
>     > pseudo
>     > >header.
>     > >
>     > >Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
index 41d1c7b..0af4a66 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1528,6 +1528,11 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
                         ((BOOLEAN)csumInfo.Receive.UdpChecksumSucceeded ||
                          (BOOLEAN)csumInfo.Receive.UdpChecksumFailed);
         }
+        if (l4Offload) {
+            *checkField = IPPseudoChecksum(&newAddr, &ipHdr->daddr,
+                tcpHdr ? IPPROTO_TCP : IPPROTO_UDP,
+                ntohs(ipHdr->tot_len) - ipHdr->ihl * 4);
+        }
     } else {
         addrField = &ipHdr->daddr;
         if (tcpHdr) {
@@ -1538,19 +1543,13 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
             checkField = &udpHdr->check;
         }
     }
+
     if (*addrField != newAddr) {
         UINT32 oldAddr = *addrField;
-        if (checkField && *checkField != 0) {
-            if (l4Offload) {
-                /* Recompute IP pseudo checksum */
-                *checkField = ~(*checkField);
-                *checkField = ChecksumUpdate32(*checkField, oldAddr,
-                                               newAddr);
-                *checkField = ~(*checkField);
-            } else {
-                *checkField = ChecksumUpdate32(*checkField, oldAddr,
-                                               newAddr);
-            }
+        if (checkField && *checkField != 0 && !l4Offload) {
+            /* Recompute total checksum. */
+            *checkField = ChecksumUpdate32(*checkField, oldAddr,
+                                            newAddr);
         }
         if (ipHdr->check != 0) {
             ipHdr->check = ChecksumUpdate32(ipHdr->check, oldAddr,
@@ -1561,49 +1560,12 @@  OvsUpdateAddressAndPort(OvsForwardingContext *ovsFwdCtx,
 
     if (portField && *portField != newPort) {
         if (checkField && !l4Offload) {
+            /* Recompute total checksum. */
             *checkField = ChecksumUpdate16(*checkField, *portField,
                                            newPort);
         }
         *portField = newPort;
     }
-    PNET_BUFFER_LIST curNbl = ovsFwdCtx->curNbl;
-    PNET_BUFFER_LIST newNbl = NULL;
-    if (layers->isTcp) {
-        UINT32 mss = OVSGetTcpMSS(curNbl);
-        if (mss) {
-            OVS_LOG_TRACE("l4Offset %d", layers->l4Offset);
-            newNbl = OvsTcpSegmentNBL(ovsFwdCtx->switchContext, curNbl, layers,
-                                      mss, 0, FALSE);
-            if (newNbl == NULL) {
-                OVS_LOG_ERROR("Unable to segment NBL");
-                return NDIS_STATUS_FAILURE;
-            }
-            /* Clear out LSO flags after this point */
-            NET_BUFFER_LIST_INFO(newNbl, TcpLargeSendNetBufferListInfo) = 0;
-        }
-    }
-    /* If we didn't split the packet above, make a copy now */
-    if (newNbl == NULL) {
-        csumInfo.Value = NET_BUFFER_LIST_INFO(curNbl,
-                                              TcpIpChecksumNetBufferListInfo);
-        OvsApplySWChecksumOnNB(layers, curNbl, &csumInfo);
-    }
-
-    if (newNbl) {
-        curNbl = newNbl;
-        OvsCompleteNBLForwardingCtx(ovsFwdCtx,
-                                    L"Complete after cloning NBL for encapsulation");
-        OvsInitForwardingCtx(ovsFwdCtx, ovsFwdCtx->switchContext,
-                             newNbl, ovsFwdCtx->srcVportNo, 0,
-                             NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
-                             ovsFwdCtx->completionList,
-                             &ovsFwdCtx->layers, FALSE);
-        ovsFwdCtx->curNbl = newNbl;
-    }
-
-    NET_BUFFER_LIST_INFO(curNbl,
-                         TcpIpChecksumNetBufferListInfo) = 0;
-
     return NDIS_STATUS_SUCCESS;
 }