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