[ovs-dev] [PATCH 3/3 v2] datapath-windows: Add GRE TEB support for windows datapath
Nithin Raju
nithin at vmware.com
Thu Dec 10 18:22:29 UTC 2015
Hi Alin,
I’ve replied to comments where you had a question. Will wait for the v3.
-----Original Message-----
From: Alin Serdean <aserdean at cloudbasesolutions.com>
Date: Thursday, December 10, 2015 at 9:10 AM
To: Nithin Raju <nithin at vmware.com>, "dev at openvswitch.org"
<dev at openvswitch.org>
Subject: RE: [ovs-dev] [PATCH 3/3 v2] datapath-windows: Add GRE TEB
support for windows datapath
>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.
Sounds good.
>> >+ 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
Sure, but what about LSO v2?
>
>> >+ }
>> >+
>> >+ 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.
I am just saying that you are assuming the caller set *newNbl to NULL. It
might be best to not make that assumption or assert at the top that
*newNbl is 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’ and ‘curMdl’ have been updated and that misled me into thinking
‘curNbl’ has been updated too.
>>
>> >+ 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?
I don’t know why they are present in VXLAN/STT code. We can remove it.
>>
>> >+ 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://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_
>rfc2784&d=BQIF_g&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B
>40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=J1TlK3sQRnkaOx0hoVo-Yic6yLDD_SEkJh4RY
>lVZhbg&s=RKca1pB3pcuka8EKTyDupkd1AfOoBEhOlrAHHzyLYwg&e= ) and not
>2874(https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_rfc
>_rfc2874.txt&d=BQIF_g&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQ
>cdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=J1TlK3sQRnkaOx0hoVo-Yic6yLDD_SEk
>Jh4RYlVZhbg&s=HKl6fk72da_pjp-6d9iMV6BA-BPvfnweFLQw6h83JUY&e= ) .
>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