On Wed, Jun 15, 2016 at 3:43 PM, Nithin Raju <nit...@vmware.com> wrote:
> -----Original Message-----
> From: Yin Lin <li...@vmware.com>
> Date: Monday, June 13, 2016 at 1:39 PM
> To: "dev@openvswitch.org" <dev@openvswitch.org>, Nithin Raju
> <nit...@vmware.com>
> Subject: [PATCH v6] datapath-windows: Add Geneve suppor
>
>>diff --git a/datapath-windows/ovsext/DpInternal.h
>>b/datapath-windows/ovsext/DpInternal.h
>>index 07bc180..1620186 100644
>>--- a/datapath-windows/ovsext/DpInternal.h
>>+++ b/datapath-windows/ovsext/DpInternal.h
>>@@ -128,10 +128,18 @@ typedef struct L2Key {
>> } L2Key; /* Size of 24 byte. */
>>
>> /* Number of packet attributes required to store OVS tunnel key. */
>>-#define NUM_PKT_ATTR_REQUIRED 3
>>+#define NUM_PKT_ATTR_REQUIRED 35
>>+#define TUN_OPT_MAX_LEN 255
>
> Wouldn¹t it have been better for alignment purposes to have
> TUN_OPT_MAX_LEN as 252, and tunOptLen to be a UINT32. Since the size of
> the tunnel options has to be a multiple of 4, the last 3 bytes are going
> to be unused anyway. That way you don¹t have to go through any of the
> alignment math later.
>
> I know this is the same on Linux code also, but might be worth finding out
> if there¹s any strong reason to keep this 255 and not the more convenient
> 252.
The reason is to avoid making this overly tunnel specific.
I don't really see the benefits making it 252 bytes plus a 4 byte
length though. The length field is representing the amount of data in
the array, so 1 byte is completely sufficient. And the alignment is
rounding to 8 byte multiples but Geneve metadata is in 4 byte chunks,
so I don't see how it could be avoided.
>>diff --git a/datapath-windows/ovsext/Flow.h
>>b/datapath-windows/ovsext/Flow.h
>>index d39db45..0744d30 100644
>>--- a/datapath-windows/ovsext/Flow.h
>>+++ b/datapath-windows/ovsext/Flow.h
>>@@ -87,10 +87,17 @@ VOID MapTunAttrToFlowPut(PNL_ATTR *keyAttrs, PNL_ATTR
>>*tunAttrs,
>> OvsFlowKey *destKey);
>> UINT32 OvsFlowKeyAttrSize(void);
>> UINT32 OvsTunKeyAttrSize(void);
>>+NTSTATUS OvsTunnelAttrToIPv4TunnelKey(PNL_ATTR attr, OvsIPv4TunnelKey
>>*tunKey);
>>
>> /* Flags for tunneling */
>> #define OVS_TNL_F_DONT_FRAGMENT (1 << 0)
>> #define OVS_TNL_F_CSUM (1 << 1)
>> #define OVS_TNL_F_KEY (1 << 2)
>>+#define OVS_TNL_F_OAM (1 << 3)
>>+#define OVS_TNL_F_CRT_OPT (1 << 4)
>>+#define OVS_TNL_F_GENEVE_OPT (1 << 5)
>>+#define OVS_TNL_F_VXLAN_OPT (1 << 6)
>
> Does VXLAN support options? If not, let¹s remove this define;
OVS supports extensions to VXLAN, some of which use the tunnel option space.
>>diff --git a/datapath-windows/ovsext/Geneve.c
>>b/datapath-windows/ovsext/Geneve.c
>>new file mode 100644
>>index 0000000..8190a15
>>--- /dev/null
>>+++ b/datapath-windows/ovsext/Geneve.c
[...]
>>+ geneveHdr = (GeneveHdr *)((PCHAR)udpHdr + sizeof *udpHdr);
>>+ if (geneveHdr->protocol != htons(ETH_P_TEB)) {
>>+ status = STATUS_NDIS_INVALID_PACKET;
>>+ goto dropNbl;
>>+ }
>>+ tunKey->flags = OVS_TNL_F_KEY;
>>+ if (geneveHdr->oam) {
>>+ tunKey->flags |= OVS_TNL_F_OAM;
>>+ }
>
> VXLAN code has been recently updated to set OVS_TNL_F_KEY conditionally.
> Can you pls. udpate Geneve code as well?
The key is always present in the Geneve header, so I'm not sure what
it would be conditional on.
>>+ tunKey->tunnelId = GENEVE_VNI_TO_TUNNELID(geneveHdr->vni);
>>+ tunKey->tunOptLen = (uint8)geneveHdr->optLen * 4;
>
> This can obviously overflow right? Max value is 0x3f * 8. I really think
> you should use uint32 for ŒtunKey->tunOptLen¹.
Geneve options are 4 byte multiples, not 8, so the max length is 252.
>>+ if (tunKey->tunOptLen > TUN_OPT_MAX_LEN ||
>>+ packetLength < tunnelSize + tunKey->tunOptLen) {
>>+ status = NDIS_STATUS_INVALID_LENGTH;
>>+ goto dropNbl;
>>+ }
>>+ /* Clear out the receive flag for the inner packet. */
>>+ NET_BUFFER_LIST_INFO(curNbl, TcpIpChecksumNetBufferListInfo) = 0;
>>+
>>+ NdisAdvanceNetBufferDataStart(curNb, tunnelSize, FALSE, NULL);
>>+ if (tunKey->tunOptLen > 0) {
>>+ optStart = NdisGetDataBuffer(curNb, tunKey->tunOptLen,
>>+ TunnelKeyGetOptions(tunKey), 1 /*no
>>align*/, 0);
>>+
>>+ /* If data is contiguous in the buffer, NdisGetDataBuffer will
>>not copy
>>+ data to the storage. Manual copy is needed. */
>>+ if (optStart != TunnelKeyGetOptions(tunKey)) {
>>+ memcpy(TunnelKeyGetOptions(tunKey), optStart,
>>tunKey->tunOptLen);
>>+ }
>>+ NdisAdvanceNetBufferDataStart(curNb, tunKey->tunOptLen, FALSE,
>>NULL);
>>+ }
>
> Don¹t we need to set the OVS_TNL_F_CRT_OPT flag based on Geneve header?
This flag doesn't get reported to userspace so as long as it isn't
added when the flow is setup by the datapath, there's no need to scan
the options on a per-packet basis.
> Also, who takes care of validating if we can handle all of the critical
> options? Are the critical options part of the flow key?
Userspace handles critical vs. non-critical options. The datapath just
needs to faithfully report what was received.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
