[ovs-dev] [PATCH 3/3 v2] datapath-windows: Add GRE TEB support for windows datapath

Alin Serdean aserdean at cloudbasesolutions.com
Thu Dec 10 17:10:50 UTC 2015


Thanks for the review Nithin trimming the reply a bit so we have more visibility.

Regarding the sequence number, I was keeping in mind GRE64 supported by OVS but that is deprecated now so we can remove it.


> >+    RtlZeroMemory(grePort, sizeof(*grePort));
> >+    grePort->dstPort = udpDestPort;
> 
> There¹s no ŒdstPort¹ for GRE.
>
[Alin Gabriel Serdean: ] I was thinking to keep the same structure for all. I will remove it
 
> >+    vport->priv = (PVOID)grePort;
> >+    return STATUS_SUCCESS;
> >+        }
> 
> How about LSO v2?
> 
> Also, why are we doing S/W TSO? Doesn¹t the H/W NIC support TSO + GRE
> encap?
>
[Alin Gabriel Serdean: ] From the commit message:
(hardware offloads will be added in a separate patch) with LSO(TSO) support
I am missing an actual card to test that part of the code
 
> >+    }
> >+
> >+    vportGre = (POVS_GRE_VPORT)GetOvsVportPriv(vport);
> >+    ASSERT(vportGre);
> >+
> >+    /* If we didn't split the packet above, make a copy now */
> >+    if (*newNbl == NULL) {
> 
> Pls. set *newNbl to NULL before the possible split.
[Alin Gabriel Serdean: ] This code would execute only if *newNbl is NULL so I don't see any added value to re set it to NULL.
> 
> >+        *newNbl = OvsPartialCopyNBL(switchContext, curNbl, 0, headRoom,
> >+                                    FALSE /*NBL info*/);\
> >+        if (*newNbl == NULL) {
> >+            OVS_LOG_ERROR("Unable to copy NBL");
> >+            return NDIS_STATUS_FAILURE;
> >+        }
> >+        /*
> >+         * To this point we do not have VXLAN offloading.
> >+         * Apply defined checksums
> >+         */
> 
> Since the last argument to OvsPartialCopyNBL() is FALSE, checksum info
> would not have got copied over to ŒnewNbl¹. Populating ŒcsumInfo¹
> before the copy would fix the bug. I agree this is a bug in VXLAN code as well.
> 
> Also, the comment should be updated to say GRE.
[Alin Gabriel Serdean: ] I will update the comment. Regarding csumInfo it is done over curNbl not newNbl. This isn't a bug here nor in VXLAN code.
> 
> >+        curNb = NET_BUFFER_LIST_FIRST_NB(*newNbl);
> >+        curMdl = NET_BUFFER_CURRENT_MDL(curNb);
> >+        bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
> >+
> >LowPagePriority);
> >+        if (!bufferStart) {
> >+            status = NDIS_STATUS_RESOURCES;
> >+            goto ret_error;
> >+        }
> >+
> >+        NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO csumInfo;
> >+        csumInfo.Value = NET_BUFFER_LIST_INFO(curNbl,
> >+
> >TcpIpChecksumNetBufferListInfo);

> >+
> >+        if (tunKey->flags & OVS_TNL_F_SEQ) {
> >+            RtlZeroMemory(currentOffset, 4);
> >+            currentOffset += 4;
> >+#if DBG
> >+            counterHeadRoom -= 4;
> >+#endif
> >+        }
> 
> Seq number can be removed, IMO.
[Alin Gabriel Serdean: ] Agreed
> 
> >+
> >+typedef struct _OVS_GRE_VPORT {
> >+    UINT16 dstPort;
[Alin Gabriel Serdean: ] this can be removed.
> >+    UINT64 inPkts;
> >+    UINT64 outPkts;
> >+    UINT64 slowInPkts;
> >+    UINT64 slowOutPkts;
> >+    UINT64 filterID;
> 
> All the fields above seem to be unused right now.
[Alin Gabriel Serdean: ] The same applied for VXLAN/STT they are not used. So I wanted to keep the common structure. Should I remove them from VXLAN/STT when I respin the patch?
> 
> >+    UINT64 ipId;
> >+    /*
> >+    * To be filled
> >+    */
> >+} OVS_GRE_VPORT, *POVS_GRE_VPORT;
> >+
> >+
> >+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> >+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >+ * |C| |K|S| Reserved0       | Ver |         Protocol Type         |
> >+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >+ * |      Checksum (optional)      |       Reserved1 (Optional)    |
> >+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >+ * |                         Key (optional)                        |
> >+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >+ * |                 Sequence Number (Optional)                    |
> >+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >+ */
> 
> Maybe we should just say 2874 since we don¹t use a bulk of the logic in 2890.
[Alin Gabriel Serdean: ] Thinking miss typed 2784 (https://tools.ietf.org/html/rfc2784) and not 2874(https://www.ietf.org/rfc/rfc2874.txt) .
I think the explanation in 2890 is far better, as there is no direct explanation about the key / sequence fields.
> 
> OVS userspace does not have support for OVS_TNL_F_SEQ. So, how would
> this flag be set at all? I¹d suggest removing it if we are not using it. Less code
> the better.
> 
[Alin Gabriel Serdean: ] Sure I can remove it.
> OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT
> >usrParamsCtx,
> >             UINT16 transportPortDest = 0;
> >
> >             switch (portType) {
> >+            case OVS_VPORT_TYPE_GRE:
> >+                OvsCleanupGreTunnel(vport);
> >+                break;
> 
> Why we are calling ŒOvsCleanupGreTunnel¹ here? The code is only trying to
> determine the ŒtransportPortDest¹ which in case of GRE should be 0 or
> some stand
> 
[Alin Gabriel Serdean: ] I think I forgot to delete that bit before I sent out. I will remove it in v3.
> >             case OVS_VPORT_TYPE_VXLAN:
> >                 transportPortDest = VXLAN_UDP_PORT;
> >                 break;
> >diff --git a/datapath-windows/ovsext/Vport.h
> >b/datapath-windows/ovsext/Vport.h
> >index e9f3b03..b11cf79 100644
> >--- a/datapath-windows/ovsext/Vport.h
> >+++ b/datapath-windows/ovsext/Vport.h
> >@@ -17,9 +17,10 @@
> > #ifndef __VPORT_H_
> > #define __VPORT_H_ 1




More information about the dev mailing list