Thanks Jesse so much for the reviewing the patch! This is my first attempt to
contribute to the ovs community and your prompt reply gave me so much
encouragement and inspiration.
I am not sure why Windows kernel doesn't compute UDP checksum for all tunnels.
I added Nithin to the thread to see if he has some idea. My wild guess is that
it tries to save some computational workload.
You are absolutely correct in all your feedbacks about the options. I actually
misinterpret the RFC doc and thought the option data is fixed length. I'll
address all your concerns in a follow up patch soon.
Thanks again,
Yin
________________________________________
From: Jesse Gross <je...@kernel.org>
Sent: Tuesday, May 17, 2016 6:06 PM
To: Yin Lin
Cc: ovs dev
Subject: Re: [ovs-dev] [PATCH] Add Geneve support on Windows datapath.
On Tue, May 17, 2016 at 5:02 PM, Yin Lin <li...@vmware.com> wrote:
> diff --git a/datapath-windows/ovsext/Geneve.c
> b/datapath-windows/ovsext/Geneve.c
> new file mode 100644
> index 0000000..d3eed86
> --- /dev/null
> +++ b/datapath-windows/ovsext/Geneve.c
> +NDIS_STATUS OvsEncapGeneve(POVS_VPORT_ENTRY vport,
> + PNET_BUFFER_LIST curNbl,
> + OvsIPv4TunnelKey *tunKey,
> + POVS_SWITCH_CONTEXT switchContext,
> + POVS_PACKET_HDR_INFO layers,
> + PNET_BUFFER_LIST *newNbl)
[...]
I'm not planning on reviewing the whole patch since I don't know the
Windows implementation well but I have a couple of quick comments on
the protocol implementation itself:
> + /* UDP header */
> + udpHdr = (UDPHdr *)((PCHAR)ipHdr + sizeof *ipHdr);
> + udpHdr->source = htons(tunKey->flow_hash | MAXINT16);
> + udpHdr->dest = htons(vportGeneve->dstPort);
> + udpHdr->len = htons(NET_BUFFER_DATA_LENGTH(curNb) - headRoom +
> + sizeof *udpHdr + sizeof *geneveHdr +
> tunKey->tunOptLen);
> + udpHdr->check = 0;
Is the UDP checksum computed anywhere? If userspace requests it then
we should fill it in. Conversely, on the receive side, it looks like
the checksum is verified if present but OVS_TNL_F_CSUM is not set to
report this to userspace.
It's possible that I might be missing something though since I also
don't see where this is done for other tunneling protocols.
> + /* Geneve header */
> + geneveHdr = (GeneveHdr *)((PCHAR)udpHdr + sizeof *udpHdr);
> + geneveHdr->version = GENEVE_VER;
> + geneveHdr->optLen = tunKey->tunOptLen / 4;
> + geneveHdr->oam = !!(tunKey->flags & OVS_TNL_F_OAM);
> + geneveHdr->critical = 0;
> + geneveHdr->reserved1 = 0;
> + geneveHdr->protocol = htons(ETH_P_TEB);
> + geneveHdr->vni = GENEVE_TUNNELID_TO_VNI(tunKey->tunnelId);
> + geneveHdr->reserved2 = 0;
> +
> + /* Geneve header options */
> + optHdr = (GeneveOptionHdr *)(geneveHdr + 1);
> + memcpy(optHdr, TunnelKeyGetOptions(tunKey), tunKey->tunOptLen);
> + geneveHdr->critical = OvsTunnelKeyIsCritical(tunKey) ? 1 : 0;
> + for (i = 0; i < tunKey->tunOptLen; i++) {
> + if (optHdr[i].type & GENEVE_CRIT_OPT_TYPE) {
> + geneveHdr->critical = 1;
> + }
> + }
> + }
Looking at the way that the geneveHdr->critical bit is generated, a
few things seem suspicious:
* The options are themselves variable length, so we can't just treat
them as an array of headers - we'll need to look at the length for
each option to find the next header. (The for loop counter also seems
to be comparing an index against a byte length, causing us to read out
of bounds.)
* It looks like the check is done twice - once in
OvsTunnelKeyIsCritical and again in the for loop below it.
* It would be nice to do this operation once per flow at setup time
instead of per packet.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
