| Message ID | 1464756440-3608-1-git-send-email-nithin@vmware.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Thanks Nithin for working on this! The fix overall looks pretty good.
Can we get rid of OvsFindTunnelVportByPortType since it doesn't make much
sense and is dangerous to call if user doesn't understand what it actually
does?
Also, there is some misalignment in the following code. Did you use tab?
- if (tunnelVport) {
+ tunnelVport =
+ OvsFindTunnelVportByDstPortAndNWProto(ovsFwdCtx->switchContext,
+ dstPort,
+ flowKey->ipKey.nwProto);
+ if (tunnelVport) {
+ switch(tunnelVport->ovsType) {
+ case OVS_VPORT_TYPE_STT:
ovsActionStats.rxStt++;
- }
On Tue, May 31, 2016 at 9:47 PM, Nithin Raju <nithin@vmware.com> wrote:
> In Actions.c, based on the IP Protocol type and L4 port of
> the outer packet, we lookup the tunnel port. The function
> that made this happen took the tunnel type as an argument.
> Semantically, is is better to pass the IP protocol type and
> let the lookup code map IP protocol type to tunnel type.
>
> In the vport add code, we make sure that we block tunnel
> port addition if there's already a tunnel port that uses
> the same IP protocol type and L4 port number.
>
> This is required for Geneve port lookups the references to
> which can find in the patch.
>
> Signed-off-by: Nithin Raju <nithin@vmware.com>
> ---
> datapath-windows/ovsext/Actions.c | 39 +++++++++++-------------
> datapath-windows/ovsext/Tunnel.c | 6 ++--
> datapath-windows/ovsext/Vport.c | 64
> +++++++++++++++++++++++++++++++++++++--
> datapath-windows/ovsext/Vport.h | 9 ++++--
> 4 files changed, 88 insertions(+), 30 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Actions.c
> b/datapath-windows/ovsext/Actions.c
> index 4edf7d0..53be718 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -217,30 +217,27 @@ OvsDetectTunnelRxPkt(OvsForwardingContext *ovsFwdCtx,
> */
> if (!flowKey->ipKey.nwFrag) {
> UINT16 dstPort = htons(flowKey->ipKey.l4.tpDst);
> - switch (flowKey->ipKey.nwProto) {
> - case IPPROTO_GRE:
> - tunnelVport =
> OvsFindTunnelVportByPortType(ovsFwdCtx->switchContext,
> -
> OVS_VPORT_TYPE_GRE);
> - if (tunnelVport) {
> - ovsActionStats.rxGre++;
> - }
> - break;
> - case IPPROTO_TCP:
> - tunnelVport =
> OvsFindTunnelVportByDstPort(ovsFwdCtx->switchContext,
> - dstPort,
> - OVS_VPORT_TYPE_STT);
> - if (tunnelVport) {
> + tunnelVport =
> +
> OvsFindTunnelVportByDstPortAndNWProto(ovsFwdCtx->switchContext,
> + dstPort,
> + flowKey->ipKey.nwProto);
> + if (tunnelVport) {
> + switch(tunnelVport->ovsType) {
> + case OVS_VPORT_TYPE_STT:
> ovsActionStats.rxStt++;
> - }
> - break;
> - case IPPROTO_UDP:
> - tunnelVport =
> OvsFindTunnelVportByDstPort(ovsFwdCtx->switchContext,
> - dstPort,
> -
> OVS_VPORT_TYPE_VXLAN);
> - if (tunnelVport) {
> + break;
> + case OVS_VPORT_TYPE_VXLAN:
> ovsActionStats.rxVxlan++;
> + break;
> +#if 0
> + case OVS_VPORT_TYPE_GENEVE:
> + ovsActionStats.rxGeneve++;
> + break;
> +#endif
> + case OVS_VPORT_TYPE_GRE:
> + ovsActionStats.rxGre++;
> + break;
> }
> - break;
> }
> }
>
> diff --git a/datapath-windows/ovsext/Tunnel.c
> b/datapath-windows/ovsext/Tunnel.c
> index 97d2020..c5aae1a 100644
> --- a/datapath-windows/ovsext/Tunnel.c
> +++ b/datapath-windows/ovsext/Tunnel.c
> @@ -285,9 +285,9 @@ OvsInjectPacketThroughActions(PNET_BUFFER_LIST pNbl,
>
> SendFlags |= NDIS_SEND_FLAGS_DISPATCH_LEVEL;
>
> - vport = OvsFindTunnelVportByDstPort(gOvsSwitchContext,
> - htons(tunnelKey.dst_port),
> - OVS_VPORT_TYPE_VXLAN);
> + vport = OvsFindTunnelVportByDstPortAndType(gOvsSwitchContext,
> +
> htons(tunnelKey.dst_port),
> + OVS_VPORT_TYPE_VXLAN);
>
> if (vport == NULL){
> status = STATUS_UNSUCCESSFUL;
> diff --git a/datapath-windows/ovsext/Vport.c
> b/datapath-windows/ovsext/Vport.c
> index 222b2c1..f5eeaa5 100644
> --- a/datapath-windows/ovsext/Vport.c
> +++ b/datapath-windows/ovsext/Vport.c
> @@ -691,9 +691,9 @@ OvsFindVportByPortNo(POVS_SWITCH_CONTEXT switchContext,
>
>
> POVS_VPORT_ENTRY
> -OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext,
> - UINT16 dstPort,
> - OVS_VPORT_TYPE ovsPortType)
> +OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT switchContext,
> + UINT16 dstPort,
> + OVS_VPORT_TYPE ovsPortType)
> {
> POVS_VPORT_ENTRY vport;
> PLIST_ENTRY head, link;
> @@ -711,6 +711,41 @@ OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT
> switchContext,
> }
>
> POVS_VPORT_ENTRY
> +OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT switchContext,
> + UINT16 dstPort,
> + UINT8 nwProto)
> +{
> + POVS_VPORT_ENTRY vport;
> + PLIST_ENTRY head, link;
> + UINT32 hash = OvsJhashBytes((const VOID *)&dstPort, sizeof(dstPort),
> + OVS_HASH_BASIS);
> + head = &(switchContext->tunnelVportsArray[hash & OVS_VPORT_MASK]);
> + LIST_FORALL(head, link) {
> + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, tunnelVportLink);
> + if (GetPortFromPriv(vport) == dstPort) {
> + switch (nwProto) {
> + case IPPROTO_UDP:
> + if (/* nwProto != OVS_VPORT_TYPE_GENEVE || */
> + vport->ovsType != OVS_VPORT_TYPE_VXLAN) {
> + continue;
> + }
> + break;
> + case IPPROTO_TCP:
> + if (vport->ovsType != OVS_VPORT_TYPE_STT) {
> + continue;
> + }
> + break;
> + case IPPROTO_GRE:
> + default:
> + break;
> + }
> + return vport;
> + }
> + }
> + return NULL;
> +}
> +
> +POVS_VPORT_ENTRY
> OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext,
> OVS_VPORT_TYPE ovsPortType)
> {
> @@ -2222,15 +2257,20 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
>
> if (OvsIsTunnelVportType(portType)) {
> UINT16 transportPortDest = 0;
> + UINT8 nwProto;
> + POVS_VPORT_ENTRY dupVport;
>
> switch (portType) {
> case OVS_VPORT_TYPE_GRE:
> + nwProto = IPPROTO_GRE;
> break;
> case OVS_VPORT_TYPE_VXLAN:
> transportPortDest = VXLAN_UDP_PORT;
> + nwProto = IPPROTO_UDP;
> break;
> case OVS_VPORT_TYPE_STT:
> transportPortDest = STT_TCP_PORT;
> + nwProto = IPPROTO_TCP;
> break;
> default:
> nlError = NL_ERROR_INVAL;
> @@ -2245,6 +2285,22 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
> }
> }
>
> + /*
> + * We don't allow two tunnel ports on identical N/W protocol
> and
> + * L4 port number. This is applicable even if the two ports
> are of
> + * different tunneling types.
> + */
> + dupVport =
> + OvsFindTunnelVportByDstPortAndNWProto(gOvsSwitchContext,
> + transportPortDest,
> + nwProto);
> + if (dupVport) {
> + OVS_LOG_ERROR("Vport for N/W proto and port already
> exists,"
> + " type: %u, dst port: %u, name: %s",
> dupVport->ovsType,
> + transportPortDest, dupVport->ovsName);
> + goto Cleanup;
> + }
> +
> status = OvsInitTunnelVport(usrParamsCtx,
> vport,
> portType,
> @@ -2319,6 +2375,8 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
> gOvsSwitchContext->dpNo);
>
> *replyLen = msgOut->nlMsg.nlmsgLen;
> + OVS_LOG_INFO("Created new vport, name: %s, type: %u", vport->ovsName,
> + vport->ovsType);
>
> Cleanup:
> NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
> diff --git a/datapath-windows/ovsext/Vport.h
> b/datapath-windows/ovsext/Vport.h
> index 373896d..f0a9acd 100644
> --- a/datapath-windows/ovsext/Vport.h
> +++ b/datapath-windows/ovsext/Vport.h
> @@ -145,9 +145,12 @@ POVS_VPORT_ENTRY
> OvsFindVportByHvNameA(POVS_SWITCH_CONTEXT switchContext,
> POVS_VPORT_ENTRY OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT
> switchContext,
> NDIS_SWITCH_PORT_ID
> portId,
> NDIS_SWITCH_NIC_INDEX
> index);
> -POVS_VPORT_ENTRY OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT
> switchContext,
> - UINT16 dstPort,
> - OVS_VPORT_TYPE ovsVportType);
> +POVS_VPORT_ENTRY OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT
> switchContext,
> + UINT16 dstPort,
> + OVS_VPORT_TYPE
> ovsPortType);
> +POVS_VPORT_ENTRY
> OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT switchContext,
> + UINT16 dstPort,
> + UINT8 nwProto);
> POVS_VPORT_ENTRY OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT
> switchContext,
> OVS_VPORT_TYPE ovsPortType);
>
> --
> 2.7.1.windows.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
Thanks for the review.
From: Yin Lin <yinlin10@gmail.com<mailto:yinlin10@gmail.com>>
Date: Thursday, June 2, 2016 at 3:00 PM
To: Nithin Raju <nithin@vmware.com<mailto:nithin@vmware.com>>
Cc: "dev@openvswitch.org<mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org>>
Subject: Re: [ovs-dev] [PATCH] datapath-windows: use ip proto for tunnel port lookup
Thanks Nithin for working on this! The fix overall looks pretty good.
Can we get rid of OvsFindTunnelVportByPortType since it doesn't make much sense and is dangerous to call if user doesn't understand what it actually does?
Done in the v2.
Also, there is some misalignment in the following code. Did you use tab?
- if (tunnelVport) {
+ tunnelVport =
+ OvsFindTunnelVportByDstPortAndNWProto(ovsFwdCtx->switchContext,
+ dstPort,
+ flowKey->ipKey.nwProto);
+ if (tunnelVport) {
+ switch(tunnelVport->ovsType) {
+ case OVS_VPORT_TYPE_STT:
ovsActionStats.rxStt++;
- }
Alignment looks correct. I have checked it after applying the patch.
-- Nithin
Hi Nithin, Thanks for the patch. Beside a few small nits regarding whitespace and extra comments, please see considerations to GRE tunnels in inlined comments. Alin. > - if (tunnelVport) { > + break; > + case OVS_VPORT_TYPE_VXLAN: > ovsActionStats.rxVxlan++; > + break; > +#if 0 > + case OVS_VPORT_TYPE_GENEVE: > + ovsActionStats.rxGeneve++; > + break; > +#endif [Alin Gabriel Serdean: ] I think you can fan out the ifdef and Yin can add them later > + case OVS_VPORT_TYPE_GRE: > + ovsActionStats.rxGre++; > + break; > } > - break; > } > } > > diff --git a/datapath-windows/ovsext/Tunnel.c b/datapath- > windows/ovsext/Tunnel.c > index 97d2020..c5aae1a 100644 > --- a/datapath-windows/ovsext/Tunnel.c > +++ b/datapath-windows/ovsext/Tunnel.c > @@ -285,9 +285,9 @@ OvsInjectPacketThroughActions(PNET_BUFFER_LIST > pNbl, > > SendFlags |= NDIS_SEND_FLAGS_DISPATCH_LEVEL; > > - vport = OvsFindTunnelVportByDstPort(gOvsSwitchContext, > - htons(tunnelKey.dst_port), > - OVS_VPORT_TYPE_VXLAN); > + vport = OvsFindTunnelVportByDstPortAndType(gOvsSwitchContext, > + htons(tunnelKey.dst_port), > + > + OVS_VPORT_TYPE_VXLAN); > > if (vport == NULL){ > status = STATUS_UNSUCCESSFUL; diff --git a/datapath- > windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index > 222b2c1..f5eeaa5 100644 > --- a/datapath-windows/ovsext/Vport.c > +++ b/datapath-windows/ovsext/Vport.c > @@ -691,9 +691,9 @@ OvsFindVportByPortNo(POVS_SWITCH_CONTEXT > switchContext, > > > POVS_VPORT_ENTRY > -OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, > - UINT16 dstPort, > - OVS_VPORT_TYPE ovsPortType) > +OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT > switchContext, > + UINT16 dstPort, > + OVS_VPORT_TYPE ovsPortType) > { > POVS_VPORT_ENTRY vport; > PLIST_ENTRY head, link; > @@ -711,6 +711,41 @@ > OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, } > > POVS_VPORT_ENTRY > +OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT > switchContext, > + UINT16 dstPort, > + UINT8 nwProto) { > + POVS_VPORT_ENTRY vport; > + PLIST_ENTRY head, link; > + UINT32 hash = OvsJhashBytes((const VOID *)&dstPort, sizeof(dstPort), > + OVS_HASH_BASIS); > + head = &(switchContext->tunnelVportsArray[hash & > OVS_VPORT_MASK]); > + LIST_FORALL(head, link) { > + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, > tunnelVportLink); > + if (GetPortFromPriv(vport) == dstPort) { > + switch (nwProto) { [Alin Gabriel Serdean: ] This will break in case we have a GRE tunnel set up. They rely only on the IP protocol; their destination port will be always set to zero. We can have packets which have l4 port zero and a gre tunnel which will result in a misinterpreted patcket. Please leave the function OvsFindTunnelVportByPortType the way it was and create a new one. > + case IPPROTO_UDP: > + if (/* nwProto != OVS_VPORT_TYPE_GENEVE || */ [Alin Gabriel Serdean: ] I think you can fan out the comment and Yin can add them later > + vport->ovsType != OVS_VPORT_TYPE_VXLAN) { > + continue; > + } > + break; > + case IPPROTO_TCP: > + if (vport->ovsType != OVS_VPORT_TYPE_STT) { > + continue; > + } > + break; > + case IPPROTO_GRE: > + default: > + break; > + } > + return vport; > + } > + } > + return NULL; > +} > + > +POVS_VPORT_ENTRY > OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, > OVS_VPORT_TYPE ovsPortType) { @@ -2222,15 +2257,20 @@ > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > > if (OvsIsTunnelVportType(portType)) { > UINT16 transportPortDest = 0; > + UINT8 nwProto; > + POVS_VPORT_ENTRY dupVport; > > switch (portType) { > case OVS_VPORT_TYPE_GRE: > + nwProto = IPPROTO_GRE; > break; > case OVS_VPORT_TYPE_VXLAN: > transportPortDest = VXLAN_UDP_PORT; > + nwProto = IPPROTO_UDP; > break; > case OVS_VPORT_TYPE_STT: > transportPortDest = STT_TCP_PORT; > + nwProto = IPPROTO_TCP; > break; > default: > nlError = NL_ERROR_INVAL; @@ -2245,6 +2285,22 @@ > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > } > } > > + /* > + * We don't allow two tunnel ports on identical N/W protocol and > + * L4 port number. This is applicable even if the two ports are of > + * different tunneling types. > + */ > + dupVport = > + OvsFindTunnelVportByDstPortAndNWProto(gOvsSwitchContext, > + transportPortDest, > + nwProto); [Alin Gabriel Serdean: ] Please skip this check in the case of GRE( it relies only on the IP protocol). > + if (dupVport) { > + OVS_LOG_ERROR("Vport for N/W proto and port already exists," [Alin Gabriel Serdean: ] ../ovs-dev-v2-datapath-windows-use-ip-proto-for-tunnel-port-lookup.patch:162: trailing whitespace. > + " type: %u, dst port: %u, name: %s", dupVport->ovsType, > + transportPortDest, dupVport->ovsName); > + goto Cleanup; > + } > + > status = OvsInitTunnelVport(usrParamsCtx, > vport, > portType, @@ -2319,6 +2375,8 @@ > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > gOvsSwitchContext->dpNo); > > *replyLen = msgOut->nlMsg.nlmsgLen; > + OVS_LOG_INFO("Created new vport, name: %s, type: %u", vport- > >ovsName, > + vport->ovsType); > > Cleanup: > NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); diff - > -git a/datapath-windows/ovsext/Vport.h b/datapath- > windows/ovsext/Vport.h index 373896d..f0a9acd 100644 > --- a/datapath-windows/ovsext/Vport.h > +++ b/datapath-windows/ovsext/Vport.h > @@ -145,9 +145,12 @@ POVS_VPORT_ENTRY > OvsFindVportByHvNameA(POVS_SWITCH_CONTEXT switchContext, > POVS_VPORT_ENTRY > OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT > switchContext, > NDIS_SWITCH_PORT_ID portId, > NDIS_SWITCH_NIC_INDEX index); - > POVS_VPORT_ENTRY > OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, > - UINT16 dstPort, > - OVS_VPORT_TYPE ovsVportType); > +POVS_VPORT_ENTRY > OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT > switchContext, > + UINT16 dstPort, > + OVS_VPORT_TYPE > +ovsPortType); POVS_VPORT_ENTRY > OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT > switchContext, > + UINT16 dstPort, > + UINT8 nwProto); > POVS_VPORT_ENTRY > OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, > OVS_VPORT_TYPE ovsPortType); > > -- > 2.7.1.windows.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Please disregard this review I it was intended for the V2 (https://patchwork.ozlabs.org/patch/629912/) Thanks, Alin. > -----Mesaj original----- > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Alin Serdean > Trimis: Tuesday, June 14, 2016 7:01 AM > Către: Nithin Raju <nithin@vmware.com>; dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] datapath-windows: use ip proto for tunnel > port lookup > > Hi Nithin, > > Thanks for the patch. Beside a few small nits regarding whitespace and extra > comments, please see considerations to GRE tunnels in inlined comments. > > Alin. > > - if (tunnelVport) { > > + break; > > + case OVS_VPORT_TYPE_VXLAN: > > ovsActionStats.rxVxlan++; > > + break; > > +#if 0 > > + case OVS_VPORT_TYPE_GENEVE: > > + ovsActionStats.rxGeneve++; > > + break; > > +#endif > [Alin Gabriel Serdean: ] I think you can fan out the ifdef and Yin can add them > later > > + case OVS_VPORT_TYPE_GRE: > > + ovsActionStats.rxGre++; > > + break; > > } > > - break; > > } > > } > > > > diff --git a/datapath-windows/ovsext/Tunnel.c b/datapath- > > windows/ovsext/Tunnel.c index 97d2020..c5aae1a 100644 > > --- a/datapath-windows/ovsext/Tunnel.c > > +++ b/datapath-windows/ovsext/Tunnel.c > > @@ -285,9 +285,9 @@ > OvsInjectPacketThroughActions(PNET_BUFFER_LIST > > pNbl, > > > > SendFlags |= NDIS_SEND_FLAGS_DISPATCH_LEVEL; > > > > - vport = OvsFindTunnelVportByDstPort(gOvsSwitchContext, > > - htons(tunnelKey.dst_port), > > - OVS_VPORT_TYPE_VXLAN); > > + vport = OvsFindTunnelVportByDstPortAndType(gOvsSwitchContext, > > + > > + htons(tunnelKey.dst_port), > > + > > + OVS_VPORT_TYPE_VXLAN); > > > > if (vport == NULL){ > > status = STATUS_UNSUCCESSFUL; diff --git a/datapath- > > windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index > > 222b2c1..f5eeaa5 100644 > > --- a/datapath-windows/ovsext/Vport.c > > +++ b/datapath-windows/ovsext/Vport.c > > @@ -691,9 +691,9 @@ OvsFindVportByPortNo(POVS_SWITCH_CONTEXT > > switchContext, > > > > > > POVS_VPORT_ENTRY > > -OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, > > - UINT16 dstPort, > > - OVS_VPORT_TYPE ovsPortType) > > +OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT > > switchContext, > > + UINT16 dstPort, > > + OVS_VPORT_TYPE ovsPortType) > > { > > POVS_VPORT_ENTRY vport; > > PLIST_ENTRY head, link; > > @@ -711,6 +711,41 @@ > > OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, } > > > > POVS_VPORT_ENTRY > > +OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT > > switchContext, > > + UINT16 dstPort, > > + UINT8 nwProto) { > > + POVS_VPORT_ENTRY vport; > > + PLIST_ENTRY head, link; > > + UINT32 hash = OvsJhashBytes((const VOID *)&dstPort, sizeof(dstPort), > > + OVS_HASH_BASIS); > > + head = &(switchContext->tunnelVportsArray[hash & > > OVS_VPORT_MASK]); > > + LIST_FORALL(head, link) { > > + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, > > tunnelVportLink); > > + if (GetPortFromPriv(vport) == dstPort) { > > + switch (nwProto) { > [Alin Gabriel Serdean: ] This will break in case we have a GRE tunnel set up. > They rely only on the IP protocol; their destination port will be always set to > zero. We can have packets which have l4 port zero and a gre tunnel which will > result in a misinterpreted patcket. Please leave the function > OvsFindTunnelVportByPortType the way it was and create a new one. > > + case IPPROTO_UDP: > > + if (/* nwProto != OVS_VPORT_TYPE_GENEVE || */ > [Alin Gabriel Serdean: ] I think you can fan out the comment and Yin can add > them later > > + vport->ovsType != OVS_VPORT_TYPE_VXLAN) { > > + continue; > > + } > > + break; > > + case IPPROTO_TCP: > > + if (vport->ovsType != OVS_VPORT_TYPE_STT) { > > + continue; > > + } > > + break; > > + case IPPROTO_GRE: > > + default: > > + break; > > + } > > + return vport; > > + } > > + } > > + return NULL; > > +} > > + > > +POVS_VPORT_ENTRY > > OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, > > OVS_VPORT_TYPE ovsPortType) { @@ > > -2222,15 +2257,20 @@ > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > > usrParamsCtx, > > > > if (OvsIsTunnelVportType(portType)) { > > UINT16 transportPortDest = 0; > > + UINT8 nwProto; > > + POVS_VPORT_ENTRY dupVport; > > > > switch (portType) { > > case OVS_VPORT_TYPE_GRE: > > + nwProto = IPPROTO_GRE; > > break; > > case OVS_VPORT_TYPE_VXLAN: > > transportPortDest = VXLAN_UDP_PORT; > > + nwProto = IPPROTO_UDP; > > break; > > case OVS_VPORT_TYPE_STT: > > transportPortDest = STT_TCP_PORT; > > + nwProto = IPPROTO_TCP; > > break; > > default: > > nlError = NL_ERROR_INVAL; @@ -2245,6 +2285,22 @@ > > OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > } > > } > > > > + /* > > + * We don't allow two tunnel ports on identical N/W protocol and > > + * L4 port number. This is applicable even if the two ports are of > > + * different tunneling types. > > + */ > > + dupVport = > > + OvsFindTunnelVportByDstPortAndNWProto(gOvsSwitchContext, > > + transportPortDest, > > + nwProto); > [Alin Gabriel Serdean: ] Please skip this check in the case of GRE( it relies only > on the IP protocol). > > + if (dupVport) { > > + OVS_LOG_ERROR("Vport for N/W proto and port already exists," > [Alin Gabriel Serdean: ] ../ovs-dev-v2-datapath-windows-use-ip-proto-for- > tunnel-port-lookup.patch:162: trailing whitespace. > > + " type: %u, dst port: %u, name: %s", dupVport->ovsType, > > + transportPortDest, dupVport->ovsName); > > + goto Cleanup; > > + } > > + > > status = OvsInitTunnelVport(usrParamsCtx, > > vport, > > portType, @@ -2319,6 +2375,8 > > @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > > gOvsSwitchContext->dpNo); > > > > *replyLen = msgOut->nlMsg.nlmsgLen; > > + OVS_LOG_INFO("Created new vport, name: %s, type: %u", vport- > > >ovsName, > > + vport->ovsType); > > > > Cleanup: > > NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); > > diff - -git a/datapath-windows/ovsext/Vport.h b/datapath- > > windows/ovsext/Vport.h index 373896d..f0a9acd 100644 > > --- a/datapath-windows/ovsext/Vport.h > > +++ b/datapath-windows/ovsext/Vport.h > > @@ -145,9 +145,12 @@ POVS_VPORT_ENTRY > > OvsFindVportByHvNameA(POVS_SWITCH_CONTEXT switchContext, > > POVS_VPORT_ENTRY > OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT > > switchContext, > > NDIS_SWITCH_PORT_ID portId, > > > > NDIS_SWITCH_NIC_INDEX index); - POVS_VPORT_ENTRY > > OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, > > - UINT16 dstPort, > > - OVS_VPORT_TYPE ovsVportType); > > +POVS_VPORT_ENTRY > > OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT > > switchContext, > > + UINT16 dstPort, > > + OVS_VPORT_TYPE > > +ovsPortType); POVS_VPORT_ENTRY > > OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT > > switchContext, > > + UINT16 dstPort, > > + UINT8 > > + nwProto); > > POVS_VPORT_ENTRY > > OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, > > OVS_VPORT_TYPE > > ovsPortType); > > > > -- > > 2.7.1.windows.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Hi Alin, Thanks for the review. I¹ve taken care of the extra whitespace and also the dead (placeholder) code. >> --- a/datapath-windows/ovsext/Vport.c > >> +++ b/datapath-windows/ovsext/Vport.c > >> POVS_VPORT_ENTRY > >> +OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT > >> switchContext, > >> + UINT16 dstPort, > >> + UINT8 nwProto) { > >> + POVS_VPORT_ENTRY vport; > >> + PLIST_ENTRY head, link; > >> + UINT32 hash = OvsJhashBytes((const VOID *)&dstPort, >>sizeof(dstPort), > >> + OVS_HASH_BASIS); > >> + head = &(switchContext->tunnelVportsArray[hash & > >> OVS_VPORT_MASK]); > >> + LIST_FORALL(head, link) { > >> + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, > >> tunnelVportLink); > >> + if (GetPortFromPriv(vport) == dstPort) { > >> + switch (nwProto) { > >[Alin Gabriel Serdean: ] This will break in case we have a GRE tunnel set >up. They rely only on the IP protocol; their destination port will be >always set to zero. We can have packets which have l4 port zero and a gre >tunnel which will result in a misinterpreted patcket. Please leave the >function OvsFindTunnelVportByPortType the way it was and create a new one. [Nithin]: There¹s no notion of a L4 port in a GRE packet, hence we use Œ0¹ as a convenient placeholder L4 port. What is the concern here? I don¹t mind leaving the function OvsFindTunnelVportByPortType() alone, but I am not sure how the patch is wrong. I¹ll add OvsFindTunnelVportByPortType() back. As such, I don¹t see any functionality breakage here. >> > >> + /* > >> + * We don't allow two tunnel ports on identical N/W >>protocol and > >> + * L4 port number. This is applicable even if the two >>ports are of > >> + * different tunneling types. > >> + */ > >> + dupVport = > >> + >>OvsFindTunnelVportByDstPortAndNWProto(gOvsSwitchContext, > >> + >>transportPortDest, > >> + nwProto); > >[Alin Gabriel Serdean: ] Please skip this check in the case of GRE( it >relies only on the IP protocol). [Nithin]: The code is simpler this way. No? I can add a comment to clarify and an ASSERT as well. BTW, we already make assumptions in the code w.r.t the L4 port number for GRE ports. Pls. have a look at the following code: NDIS_STATUS InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext, POVS_VPORT_ENTRY vport) { UINT32 hash; switch(vport->ovsType) { case OVS_VPORT_TYPE_GRE: case OVS_VPORT_TYPE_VXLAN: case OVS_VPORT_TYPE_STT: { UINT16 dstPort = GetPortFromPriv(vport); <<<<< L4 port # is 0 for GRE port. hash = OvsJhashBytes(&dstPort, sizeof(dstPort), OVS_HASH_BASIS); InsertHeadList( &gOvsSwitchContext->tunnelVportsArray[hash & OVS_VPORT_MASK], &vport->tunnelVportLink); switchContext->numNonHvVports++; break; } I¹ll send out a v3. Pls. have a look. Thanks, -- Nithin
> > [Nithin]: The code is simpler this way. No? I can add a comment to clarify and > an ASSERT as well. BTW, we already make assumptions in the code w.r.t the > L4 port number for GRE ports. Pls. have a look at the following code: [Alin Gabriel Serdean: ] The problem is not we make an assumption on the port being zero. The problem is related to the IP protocol match. I think my comment was unclear see the comment sent out on v3 of this patch. > > NDIS_STATUS > InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext, > POVS_VPORT_ENTRY vport) > { > UINT32 hash; > > switch(vport->ovsType) { > case OVS_VPORT_TYPE_GRE: > case OVS_VPORT_TYPE_VXLAN: > case OVS_VPORT_TYPE_STT: > { > UINT16 dstPort = GetPortFromPriv(vport); <<<<< L4 port # is 0 for > GRE port. > hash = OvsJhashBytes(&dstPort, > sizeof(dstPort), > OVS_HASH_BASIS); > InsertHeadList( > &gOvsSwitchContext->tunnelVportsArray[hash & > OVS_VPORT_MASK], > &vport->tunnelVportLink); > switchContext->numNonHvVports++; > break; > } > > > I¹ll send out a v3. Pls. have a look. > > Thanks, > -- Nithin
diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c index 4edf7d0..53be718 100644 --- a/datapath-windows/ovsext/Actions.c +++ b/datapath-windows/ovsext/Actions.c @@ -217,30 +217,27 @@ OvsDetectTunnelRxPkt(OvsForwardingContext *ovsFwdCtx, */ if (!flowKey->ipKey.nwFrag) { UINT16 dstPort = htons(flowKey->ipKey.l4.tpDst); - switch (flowKey->ipKey.nwProto) { - case IPPROTO_GRE: - tunnelVport = OvsFindTunnelVportByPortType(ovsFwdCtx->switchContext, - OVS_VPORT_TYPE_GRE); - if (tunnelVport) { - ovsActionStats.rxGre++; - } - break; - case IPPROTO_TCP: - tunnelVport = OvsFindTunnelVportByDstPort(ovsFwdCtx->switchContext, - dstPort, - OVS_VPORT_TYPE_STT); - if (tunnelVport) { + tunnelVport = + OvsFindTunnelVportByDstPortAndNWProto(ovsFwdCtx->switchContext, + dstPort, + flowKey->ipKey.nwProto); + if (tunnelVport) { + switch(tunnelVport->ovsType) { + case OVS_VPORT_TYPE_STT: ovsActionStats.rxStt++; - } - break; - case IPPROTO_UDP: - tunnelVport = OvsFindTunnelVportByDstPort(ovsFwdCtx->switchContext, - dstPort, - OVS_VPORT_TYPE_VXLAN); - if (tunnelVport) { + break; + case OVS_VPORT_TYPE_VXLAN: ovsActionStats.rxVxlan++; + break; +#if 0 + case OVS_VPORT_TYPE_GENEVE: + ovsActionStats.rxGeneve++; + break; +#endif + case OVS_VPORT_TYPE_GRE: + ovsActionStats.rxGre++; + break; } - break; } } diff --git a/datapath-windows/ovsext/Tunnel.c b/datapath-windows/ovsext/Tunnel.c index 97d2020..c5aae1a 100644 --- a/datapath-windows/ovsext/Tunnel.c +++ b/datapath-windows/ovsext/Tunnel.c @@ -285,9 +285,9 @@ OvsInjectPacketThroughActions(PNET_BUFFER_LIST pNbl, SendFlags |= NDIS_SEND_FLAGS_DISPATCH_LEVEL; - vport = OvsFindTunnelVportByDstPort(gOvsSwitchContext, - htons(tunnelKey.dst_port), - OVS_VPORT_TYPE_VXLAN); + vport = OvsFindTunnelVportByDstPortAndType(gOvsSwitchContext, + htons(tunnelKey.dst_port), + OVS_VPORT_TYPE_VXLAN); if (vport == NULL){ status = STATUS_UNSUCCESSFUL; diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index 222b2c1..f5eeaa5 100644 --- a/datapath-windows/ovsext/Vport.c +++ b/datapath-windows/ovsext/Vport.c @@ -691,9 +691,9 @@ OvsFindVportByPortNo(POVS_SWITCH_CONTEXT switchContext, POVS_VPORT_ENTRY -OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, - UINT16 dstPort, - OVS_VPORT_TYPE ovsPortType) +OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT switchContext, + UINT16 dstPort, + OVS_VPORT_TYPE ovsPortType) { POVS_VPORT_ENTRY vport; PLIST_ENTRY head, link; @@ -711,6 +711,41 @@ OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, } POVS_VPORT_ENTRY +OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT switchContext, + UINT16 dstPort, + UINT8 nwProto) +{ + POVS_VPORT_ENTRY vport; + PLIST_ENTRY head, link; + UINT32 hash = OvsJhashBytes((const VOID *)&dstPort, sizeof(dstPort), + OVS_HASH_BASIS); + head = &(switchContext->tunnelVportsArray[hash & OVS_VPORT_MASK]); + LIST_FORALL(head, link) { + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, tunnelVportLink); + if (GetPortFromPriv(vport) == dstPort) { + switch (nwProto) { + case IPPROTO_UDP: + if (/* nwProto != OVS_VPORT_TYPE_GENEVE || */ + vport->ovsType != OVS_VPORT_TYPE_VXLAN) { + continue; + } + break; + case IPPROTO_TCP: + if (vport->ovsType != OVS_VPORT_TYPE_STT) { + continue; + } + break; + case IPPROTO_GRE: + default: + break; + } + return vport; + } + } + return NULL; +} + +POVS_VPORT_ENTRY OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, OVS_VPORT_TYPE ovsPortType) { @@ -2222,15 +2257,20 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, if (OvsIsTunnelVportType(portType)) { UINT16 transportPortDest = 0; + UINT8 nwProto; + POVS_VPORT_ENTRY dupVport; switch (portType) { case OVS_VPORT_TYPE_GRE: + nwProto = IPPROTO_GRE; break; case OVS_VPORT_TYPE_VXLAN: transportPortDest = VXLAN_UDP_PORT; + nwProto = IPPROTO_UDP; break; case OVS_VPORT_TYPE_STT: transportPortDest = STT_TCP_PORT; + nwProto = IPPROTO_TCP; break; default: nlError = NL_ERROR_INVAL; @@ -2245,6 +2285,22 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, } } + /* + * We don't allow two tunnel ports on identical N/W protocol and + * L4 port number. This is applicable even if the two ports are of + * different tunneling types. + */ + dupVport = + OvsFindTunnelVportByDstPortAndNWProto(gOvsSwitchContext, + transportPortDest, + nwProto); + if (dupVport) { + OVS_LOG_ERROR("Vport for N/W proto and port already exists," + " type: %u, dst port: %u, name: %s", dupVport->ovsType, + transportPortDest, dupVport->ovsName); + goto Cleanup; + } + status = OvsInitTunnelVport(usrParamsCtx, vport, portType, @@ -2319,6 +2375,8 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, gOvsSwitchContext->dpNo); *replyLen = msgOut->nlMsg.nlmsgLen; + OVS_LOG_INFO("Created new vport, name: %s, type: %u", vport->ovsName, + vport->ovsType); Cleanup: NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); diff --git a/datapath-windows/ovsext/Vport.h b/datapath-windows/ovsext/Vport.h index 373896d..f0a9acd 100644 --- a/datapath-windows/ovsext/Vport.h +++ b/datapath-windows/ovsext/Vport.h @@ -145,9 +145,12 @@ POVS_VPORT_ENTRY OvsFindVportByHvNameA(POVS_SWITCH_CONTEXT switchContext, POVS_VPORT_ENTRY OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT switchContext, NDIS_SWITCH_PORT_ID portId, NDIS_SWITCH_NIC_INDEX index); -POVS_VPORT_ENTRY OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext, - UINT16 dstPort, - OVS_VPORT_TYPE ovsVportType); +POVS_VPORT_ENTRY OvsFindTunnelVportByDstPortAndType(POVS_SWITCH_CONTEXT switchContext, + UINT16 dstPort, + OVS_VPORT_TYPE ovsPortType); +POVS_VPORT_ENTRY OvsFindTunnelVportByDstPortAndNWProto(POVS_SWITCH_CONTEXT switchContext, + UINT16 dstPort, + UINT8 nwProto); POVS_VPORT_ENTRY OvsFindTunnelVportByPortType(POVS_SWITCH_CONTEXT switchContext, OVS_VPORT_TYPE ovsPortType);
In Actions.c, based on the IP Protocol type and L4 port of the outer packet, we lookup the tunnel port. The function that made this happen took the tunnel type as an argument. Semantically, is is better to pass the IP protocol type and let the lookup code map IP protocol type to tunnel type. In the vport add code, we make sure that we block tunnel port addition if there's already a tunnel port that uses the same IP protocol type and L4 port number. This is required for Geneve port lookups the references to which can find in the patch. Signed-off-by: Nithin Raju <nithin@vmware.com> --- datapath-windows/ovsext/Actions.c | 39 +++++++++++------------- datapath-windows/ovsext/Tunnel.c | 6 ++-- datapath-windows/ovsext/Vport.c | 64 +++++++++++++++++++++++++++++++++++++-- datapath-windows/ovsext/Vport.h | 9 ++++-- 4 files changed, 88 insertions(+), 30 deletions(-)