Hi Jesse,
Thanks for the comments. My responses inlined.
>>> /* 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.
For Geneve, 252 would have been sufficient. The fact that we want to keep
it generic is a good reason to maximize the number of bytes we can pack.
But, I still find the 255 as an odd number odd :)
You are right about alignment.
>>>+#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.
Sure, we don’t support it in Hyper-V datapath. But, it is probably ok to
keep the define.
>>>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.
What should be the behavior if the VNI is 0? Is the packet considered a
valid packet?
>
>>>+ 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.
Thanks for correcting. I was mistaken.
>
>>>+ 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.
Sure.
>
>> 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.
Thanks for the info. I am assuming that ALL the options are part of the
flow key - regardless of whether they are critical or not. So, once
userspace sets up the flow in the datapath, any subsequent packet with the
same Geneve header + options will match the flow.
Thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
