Sorin,
I’m treating this change as a continuation of the other changed titled “support
for custom VXLAN tunnel port”.
High level comment is:
There may be a STT port and UDP port with the same destination port number. We
might need to specify the tunneling protocol or the L4 protocol as key as well.
Looks good overall. I’d encourage you to send the two patches as a series.
I had the following inlined comments:
> diff --git a/datapath-windows/ovsext/Actions.c
> b/datapath-windows/ovsext/Actions.c
> index a93fe03..20d2f1e 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -203,9 +203,10 @@ OvsDetectTunnelRxPkt(OvsForwardingContext *ovsFwdCtx,
> * packets only if they are at least VXLAN header size.
> */
> if (!flowKey->ipKey.nwFrag &&
> - flowKey->ipKey.nwProto == IPPROTO_UDP &&
> - flowKey->ipKey.l4.tpDst == VXLAN_UDP_PORT_NBO) {
> - tunnelVport = ovsFwdCtx->switchContext->vxlanVport;
> + flowKey->ipKey.nwProto == IPPROTO_UDP) {
> + UINT16 dstPort = htons(flowKey->ipKey.l4.tpDst);
> + tunnelVport = OvsFindTunnelVportByDstPort(ovsFwdCtx->switchContext,
> + dstPort);
> ovsActionStats.rxVxlan++;
> }
Incrementing ovsActionStats.rxVxlan should be done only if tunnelVport != NULL.
I support, you could move the increment to the next ‘if’ block, where we check
if tunnelVport != NULL.
One more comment is that, there may be a STT port and UDP port with the same
destination port number. We might need to specify the tunneling protocol or the
L4 protocol as key as well.
> @@ -1318,6 +1319,7 @@ OvsExecuteSetAction(OvsForwardingContext *ovsFwdCtx,
> status = OvsTunnelAttrToIPv4TunnelKey((PNL_ATTR)a, &tunKey);
> ASSERT(status == NDIS_STATUS_SUCCESS);
> tunKey.flow_hash = (uint16)(hash ? *hash : OvsHashFlow(key));
> + tunKey.dst_port = key->ipKey.l4.tpDst;
> RtlCopyMemory(&ovsFwdCtx->tunKey, &tunKey, sizeof ovsFwdCtx->tunKey);
XXX: tunKey.dst_port is already in NBO.
>
> break;
> diff --git a/datapath-windows/ovsext/Switch.c
> b/datapath-windows/ovsext/Switch.c
> index cf5e3c4..665aff4 100644
> --- a/datapath-windows/ovsext/Switch.c
> +++ b/datapath-windows/ovsext/Switch.c
> @@ -366,6 +366,8 @@ OvsInitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> OvsAllocateMemory(sizeof (LIST_ENTRY) * OVS_MAX_VPORT_ARRAY_SIZE);
> switchContext->pidHashArray = (PLIST_ENTRY)
> OvsAllocateMemory(sizeof(LIST_ENTRY) * OVS_MAX_PID_ARRAY_SIZE);
> + switchContext->tunnelVportsArray = (PLIST_ENTRY)
> + OvsAllocateMemory(sizeof(LIST_ENTRY) * OVS_MAX_VPORT_ARRAY_SIZE);
> status = OvsAllocateFlowTable(&switchContext->datapath, switchContext);
>
> if (status == NDIS_STATUS_SUCCESS) {
> @@ -376,7 +378,8 @@ OvsInitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> switchContext->portNoHashArray == NULL ||
> switchContext->ovsPortNameHashArray == NULL ||
> switchContext->portIdHashArray== NULL ||
> - switchContext->pidHashArray == NULL) {
> + switchContext->pidHashArray == NULL ||
> + switchContext->tunnelVportsArray == NULL) {
> if (switchContext->dispatchLock) {
> NdisFreeRWLock(switchContext->dispatchLock);
> }
> @@ -394,6 +397,10 @@ OvsInitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> OvsFreeMemory(switchContext->pidHashArray);
> }
>
> + if (switchContext->tunnelVportsArray) {
> + OvsFreeMemory(switchContext->tunnelVportsArray);
> + }
> +
> OvsDeleteFlowTable(&switchContext->datapath);
> OvsCleanupBufferPool(switchContext);
>
> @@ -403,12 +410,9 @@ OvsInitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
>
> for (i = 0; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
> InitializeListHead(&switchContext->ovsPortNameHashArray[i]);
> - }
> - for (i = 0; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
> InitializeListHead(&switchContext->portIdHashArray[i]);
> - }
> - for (i = 0; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
> InitializeListHead(&switchContext->portNoHashArray[i]);
> + InitializeListHead(&switchContext->tunnelVportsArray[i]);
> }
>
> for (i = 0; i < OVS_MAX_PID_ARRAY_SIZE; i++) {
> @@ -445,6 +449,8 @@ OvsUninitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> switchContext->portNoHashArray = NULL;
> OvsFreeMemory(switchContext->pidHashArray);
> switchContext->pidHashArray = NULL;
> + OvsFreeMemory(switchContext->tunnelVportsArray);
> + switchContext->tunnelVportsArray = NULL;
> OvsDeleteFlowTable(&switchContext->datapath);
> OvsCleanupBufferPool(switchContext);
> OVS_LOG_TRACE("Exit: Delete switchContext: %p", switchContext);
> diff --git a/datapath-windows/ovsext/Switch.h
> b/datapath-windows/ovsext/Switch.h
> index 7960072..f2558e5 100644
> --- a/datapath-windows/ovsext/Switch.h
> +++ b/datapath-windows/ovsext/Switch.h
> @@ -132,7 +132,7 @@ typedef struct _OVS_SWITCH_CONTEXT
> POVS_VPORT_ENTRY virtualExternalVport; // the virtual adapter
> vport
> POVS_VPORT_ENTRY internalVport;
>
> - POVS_VPORT_ENTRY vxlanVport;
> + PLIST_ENTRY tunnelVportsArray;
A comment explaining ‘tunnelVportsArray’ would help code reading. One detail
that can be useful is if it only a particular tunnel or all tunnel types that
get hashed here.
> /*
> * 'portIdHashArray' ONLY contains ports that exist on the Hyper-V switch,
> diff --git a/datapath-windows/ovsext/Tunnel.c
> b/datapath-windows/ovsext/Tunnel.c
> index fed58f1..002f180 100644
> --- a/datapath-windows/ovsext/Tunnel.c
> +++ b/datapath-windows/ovsext/Tunnel.c
> @@ -285,7 +285,8 @@ OvsInjectPacketThroughActions(PNET_BUFFER_LIST pNbl,
>
> SendFlags |= NDIS_SEND_FLAGS_DISPATCH_LEVEL;
>
> - vport = gOvsSwitchContext->vxlanVport;
> + vport = OvsFindTunnelVportByDstPort(gOvsSwitchContext,
> + htons(tunnelKey.dst_port));
>
> if (vport == NULL){
> status = STATUS_UNSUCCESSFUL;
> diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
> index 968c112..bc5c23b 100644
> --- a/datapath-windows/ovsext/Vport.c
> +++ b/datapath-windows/ovsext/Vport.c
> @@ -574,6 +574,25 @@ OvsFindVportByPortNo(POVS_SWITCH_CONTEXT switchContext,
>
>
> POVS_VPORT_ENTRY
> +OvsFindTunnelVportByDstPort(POVS_SWITCH_CONTEXT switchContext,
> + UINT16 dstPort)
> +{
> + 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 (((POVS_VXLAN_VPORT)vport->priv)->dstPort == dstPort) {
> + return vport;
> + }
> + }
> + return NULL;
> +}
> +
> +
> +POVS_VPORT_ENTRY
> OvsFindVportByOvsName(POVS_SWITCH_CONTEXT switchContext,
> PSTR name)
> {
> @@ -999,8 +1018,8 @@ InitHvVportCommon(POVS_SWITCH_CONTEXT switchContext,
> * --------------------------------------------------------------------------
> * Functionality common to any port added from OVS userspace.
> *
> - * Inserts the port into 'portIdHashArray', 'ovsPortNameHashArray' and caches
> - * the pointer in the 'switchContext' if needed.
> + * Inserts the port into 'portNoHashArray', 'ovsPortNameHashArray' and in
> + * 'tunnelVportsArray' if appropriate.
> * --------------------------------------------------------------------------
> */
> NDIS_STATUS
> @@ -1011,10 +1030,17 @@ InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext,
>
> switch(vport->ovsType) {
> case OVS_VPORT_TYPE_VXLAN:
> - //ASSERT(switchContext->vxlanVport == NULL);
> - switchContext->vxlanVport = vport;
> + {
> + POVS_VXLAN_VPORT vxlanVport = (POVS_VXLAN_VPORT)vport->priv;
> + hash = OvsJhashBytes(&vxlanVport->dstPort,
> + sizeof(vxlanVport->dstPort),
> + OVS_HASH_BASIS);
> + InsertHeadList(
> + &gOvsSwitchContext->tunnelVportsArray[hash & OVS_VPORT_MASK],
> + &vport->tunnelVportLink);
> switchContext->numNonHvVports++;
> break;
> + }
> case OVS_VPORT_TYPE_INTERNAL:
> if (vport->isBridgeInternal) {
> switchContext->numNonHvVports++;
> @@ -1096,7 +1122,8 @@ OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT
> switchContext,
> break;
> case OVS_VPORT_TYPE_VXLAN:
> OvsCleanupVxlanTunnel(vport);
> - switchContext->vxlanVport = NULL;
> + RemoveEntryList(&vport->tunnelVportLink);
> + InitializeListHead(&vport->tunnelVportLink);
> break;
> case OVS_VPORT_TYPE_GRE:
> case OVS_VPORT_TYPE_GRE64:
> @@ -1295,19 +1322,6 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT
> switchContext)
> vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portIdLink);
> OvsRemoveAndDeleteVport(switchContext, vport, TRUE, TRUE, NULL);
> }
> - }
> - /*
> - * Remove 'virtualExternalVport' as well. This port is not part of the
> - * 'portIdHashArray'.
> - */
> - if (switchContext->virtualExternalVport) {
> - OvsRemoveAndDeleteVport(switchContext,
> - (POVS_VPORT_ENTRY)switchContext->virtualExternalVport, TRUE,
> TRUE,
> - NULL);
> - }
> -
> - for (UINT hash = 0; hash < OVS_MAX_VPORT_ARRAY_SIZE; hash++) {
> - PLIST_ENTRY head, link, next;
>
> head = &(switchContext->portNoHashArray[hash & OVS_VPORT_MASK]);
> LIST_FORALL_SAFE(head, link, next) {
> @@ -1320,9 +1334,18 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT
> switchContext)
> }
> }
>
> + /*
> + * Remove 'virtualExternalVport' as well. This port is not part of the
> + * 'portIdHashArray'.
> + */
> + if (switchContext->virtualExternalVport) {
> + OvsRemoveAndDeleteVport(switchContext,
> + (POVS_VPORT_ENTRY)switchContext->virtualExternalVport, TRUE,
> TRUE,
> + NULL);
> + }
> +
XXX: Moving the code for ‘if (switchContext->virtualExternalVport) might trip
the following ASSERT:
1316 ASSERT(OvsIsTunnelVportType(vport->ovsType) ||
1317 (vport->ovsType == OVS_VPORT_TYPE_INTERNAL &&
1318 vport->isBridgeInternal) || vport->isPresentOnHv ==
TRUE);
> ASSERT(switchContext->virtualExternalVport == NULL);
> ASSERT(switchContext->internalVport == NULL);
> - ASSERT(switchContext->vxlanVport == NULL);
> }
>
>
> @@ -2033,8 +2056,6 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
> } else {
> ASSERT(OvsIsTunnelVportType(portType) ||
> (portType == OVS_VPORT_TYPE_INTERNAL && isBridgeInternal));
> - //ASSERT(OvsGetTunnelVport(gOvsSwitchContext, portType) == NULL ||
> - // !OvsIsTunnelVportType(portType));
>
> vport = (POVS_VPORT_ENTRY)OvsAllocateVport();
> if (vport == NULL) {
> diff --git a/datapath-windows/ovsext/Vport.h b/datapath-windows/ovsext/Vport.h
> index 348fbfd..1d8e253 100644
> --- a/datapath-windows/ovsext/Vport.h
> +++ b/datapath-windows/ovsext/Vport.h
> @@ -84,6 +84,7 @@ typedef struct _OVS_VPORT_ENTRY {
> LIST_ENTRY ovsNameLink;
> LIST_ENTRY portIdLink;
> LIST_ENTRY portNoLink;
> + LIST_ENTRY tunnelVportLink;
>
> OVS_VPORT_STATE ovsState;
> OVS_VPORT_TYPE ovsType;
> @@ -135,10 +136,8 @@ typedef struct _OVS_VPORT_ENTRY {
>
> struct _OVS_SWITCH_CONTEXT;
>
> -POVS_VPORT_ENTRY
> -OvsFindVportByPortNo(struct _OVS_SWITCH_CONTEXT *switchContext,
> - UINT32 portNo);
> -
> +POVS_VPORT_ENTRY OvsFindVportByPortNo(POVS_SWITCH_CONTEXT switchContext,
> + UINT32 portNo);
> /* "name" is null-terminated */
> POVS_VPORT_ENTRY OvsFindVportByOvsName(POVS_SWITCH_CONTEXT switchContext,
> PSTR name);
> @@ -147,6 +146,8 @@ 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);
>
> NDIS_STATUS OvsAddConfiguredSwitchPorts(struct _OVS_SWITCH_CONTEXT
> *switchContext);
> NDIS_STATUS OvsInitConfiguredSwitchNics(struct _OVS_SWITCH_CONTEXT
> *switchContext);
> @@ -182,11 +183,12 @@ OvsIsTunnelVportType(OVS_VPORT_TYPE ovsType)
>
> static __inline POVS_VPORT_ENTRY
> OvsGetTunnelVport(POVS_SWITCH_CONTEXT switchContext,
> - OVS_VPORT_TYPE ovsType)
> + OVS_VPORT_TYPE ovsType,
> + UINT16 dstPort)
One more comment is that, there may be a STT port and UDP port with the same
destination port number. We might need to specify the tunneling protocol or the
L4 protocol as key as well.
OvsGetTunnelVport() doesn’t seem to be used anymore.
thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
