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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
