From: Nithin Raju Date: Fri, 24 Oct 2014 00:33:13 +0000 (-0700) Subject: datapath-windows: Update vport add code. X-Git-Tag: v2.4.0~1108 X-Git-Url: http://git.cascardo.eti.br/?a=commitdiff_plain;h=35f20164e225240e443b9140313ce75539c841d3;p=cascardo%2Fovs.git datapath-windows: Update vport add code. In this patch, we make the following updates to the vport add code: 1. Clarify the roles of the different hash tables, so it is easier to read/write code in the future. 2. Update OvsNewVportCmdHandler() to support adding bridge-internal ports. 3. Fixes in OvsNewVportCmdHandler() to support adding external port. Earlier, we'd hit ASSERTs. 4. I could not figure out way to add a port of type OVS_PORT_TYPE_INTERNAL with name "internal" to the confdb using ovs-vsctl.exe. And, this is needed in order to add the Hyper-V internal port from userspace. To workaround this problem, we treat a port of type OVS_PORT_TYPE_NETDEV with name "internal" as a request to add the Hyper-V internal port. This is a workaround. The end result is that there's a discrepancy between the port type in the datpaath v/s confdb, but this has not created any trouble in testing so far. If this ends up becoming an issue, we can mark the Hyper-V internal port to be of type OVS_PORT_TYPE_NETDEV. No harm. 5. Because of changes indicated in #1, we also update the vport dump code to look at the correct hash table for ports added from userspace. 6. Add a OvsGetTunnelVport() for convenience. 7. Update ASSERTs() while cleaning up the switch. 8. Nuke OvsGetExternalVport() and OvsGetExternalMtu(). Signed-off-by: Nithin Raju Acked-by: Alin Gabriel Serdean Tested-by: Alin Gabriel Serdean Acked-by: Ankur Sharma Signed-off-by: Ben Pfaff --- diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c index 55b19a089..1b504a276 100644 --- a/datapath-windows/ovsext/Datapath.c +++ b/datapath-windows/ovsext/Datapath.c @@ -1513,7 +1513,8 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx, NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, NDIS_RWL_AT_DISPATCH_LEVEL); - if (gOvsSwitchContext->numHvVports > 0) { + if (gOvsSwitchContext->numHvVports > 0 || + gOvsSwitchContext->numNonHvVports > 0) { /* inBucket: the bucket, used for lookup */ UINT32 inBucket = instance->dumpState.index[0]; /* inIndex: index within the given bucket, used for lookup */ @@ -1525,7 +1526,7 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx, for (i = inBucket; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) { PLIST_ENTRY head, link; - head = &(gOvsSwitchContext->portIdHashArray[i]); + head = &(gOvsSwitchContext->portNoHashArray[i]); POVS_VPORT_ENTRY vport = NULL; outIndex = 0; @@ -1537,18 +1538,15 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx, * inIndex + 1 vport from the bucket. */ if (outIndex >= inIndex) { - vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portIdLink); - - if (vport->portNo != OVS_DPPORT_NUMBER_INVALID) { - OvsCreateMsgFromVport(vport, msgIn, - usrParamsCtx->outputBuffer, - usrParamsCtx->outputLength, - gOvsSwitchContext->dpNo); - ++outIndex; - break; - } else { - vport = NULL; - } + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink); + + ASSERT(vport->portNo != OVS_DPPORT_NUMBER_INVALID); + OvsCreateMsgFromVport(vport, msgIn, + usrParamsCtx->outputBuffer, + usrParamsCtx->outputLength, + gOvsSwitchContext->dpNo); + ++outIndex; + break; } ++outIndex; @@ -1748,7 +1746,9 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, PCHAR portName; ULONG portNameLen; UINT32 portType; - UINT32 hash; + BOOLEAN isBridgeInternal = FALSE; + BOOLEAN vportAllocated = FALSE; + BOOLEAN addInternalPortAsNetdev = FALSE; static const NL_POLICY ovsVportPolicy[] = { [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32, .optional = TRUE }, @@ -1798,38 +1798,49 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, goto Cleanup; } - if (portType == OVS_VPORT_TYPE_INTERNAL) { + if (portName && portType == OVS_VPORT_TYPE_NETDEV && + !strcmp(OVS_DPPORT_INTERNAL_NAME_A, portName)) { + addInternalPortAsNetdev = TRUE; + } + + if (portName && portType == OVS_VPORT_TYPE_INTERNAL && + strcmp(OVS_DPPORT_INTERNAL_NAME_A, portName)) { + isBridgeInternal = TRUE; + } + + if (portType == OVS_VPORT_TYPE_INTERNAL && !isBridgeInternal) { vport = gOvsSwitchContext->internalVport; } else if (portType == OVS_VPORT_TYPE_NETDEV) { - if (!strcmp(portName, "external")) { - vport = gOvsSwitchContext->virtualExternalVport; - } else { - vport = OvsFindVportByHvName(gOvsSwitchContext, portName); - } + /* External ports can also be looked up like VIF ports. */ + vport = OvsFindVportByHvName(gOvsSwitchContext, portName); } else { - /* XXX: change when other tunneling ports are added */ - ASSERT(portType == OVS_VPORT_TYPE_VXLAN); - - if (gOvsSwitchContext->vxlanVport) { - nlError = NL_ERROR_EXIST; - goto Cleanup; - } + ASSERT(OvsIsTunnelVportType(portType) || + (portType == OVS_VPORT_TYPE_INTERNAL && isBridgeInternal)); + ASSERT(OvsGetTunnelVport(gOvsSwitchContext, portType) == NULL || + !OvsIsTunnelVportType(portType)); vport = (POVS_VPORT_ENTRY)OvsAllocateVport(); if (vport == NULL) { nlError = NL_ERROR_NOMEM; goto Cleanup; } + vportAllocated = TRUE; - vport->ovsState = OVS_STATE_PORT_CREATED; + if (OvsIsTunnelVportType(portType)) { + nlError = OvsInitTunnelVport(vport, portType, VXLAN_UDP_PORT); + } else { + OvsInitBridgeInternalVport(vport); + } - /* - * XXX: when we allow configuring the vxlan udp port, we should read - * this from vport->options instead! - */ - nlError = OvsInitVxlanTunnel(vport, VXLAN_UDP_PORT); - if (nlError != NL_ERROR_SUCCESS) { - goto Cleanup; + if (nlError == NL_ERROR_SUCCESS) { + vport->ovsState = OVS_STATE_CONNECTED; + vport->nicState = NdisSwitchNicStateConnected; + + /* + * Allow the vport to be deleted, because there is no + * corresponding hyper-v switch part. + */ + vport->hvDeleted = TRUE; } } @@ -1837,21 +1848,21 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, nlError = NL_ERROR_INVAL; goto Cleanup; } - if (vport->portNo != OVS_DPPORT_NUMBER_INVALID) { nlError = NL_ERROR_EXIST; goto Cleanup; } - /* Fill the data in vport */ - vport->ovsType = portType; - + /* Initialize the vport with OVS specific properties. */ + if (addInternalPortAsNetdev != TRUE) { + vport->ovsType = portType; + } if (vportAttrs[OVS_VPORT_ATTR_PORT_NO] != NULL) { /* - * XXX: when we implement the limit for ovs port number to be - * MAXUINT16, we'll need to check the port number received from the - * userspace. - */ + * XXX: when we implement the limit for ovs port number to be + * MAXUINT16, we'll need to check the port number received from the + * userspace. + */ vport->portNo = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_PORT_NO]); } else { vport->portNo = OvsComputeVportNo(gOvsSwitchContext); @@ -1866,42 +1877,19 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, ASSERT(portNameLen <= OVS_MAX_PORT_NAME_LENGTH); RtlCopyMemory(vport->ovsName, portName, portNameLen); - /* if we don't have options, then vport->portOptions will be NULL */ vport->portOptions = vportAttrs[OVS_VPORT_ATTR_OPTIONS]; /* - * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath, - * we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set, - * it means we have an array of pids, instead of a single pid. - * ATM we assume we have one pid only. - */ + * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath, + * we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set, + * it means we have an array of pids, instead of a single pid. + * ATM we assume we have one pid only. + */ vport->upcallPid = NlAttrGetU32(vportAttrs[OVS_VPORT_ATTR_UPCALL_PID]); - if (vport->ovsType == OVS_VPORT_TYPE_VXLAN) { - gOvsSwitchContext->vxlanVport = vport; - } else if (vport->ovsType == OVS_VPORT_TYPE_INTERNAL) { - gOvsSwitchContext->internalVport = vport; - gOvsSwitchContext->internalPortId = vport->portId; - } else if (vport->ovsType == OVS_VPORT_TYPE_NETDEV && - vport->isExternal) { - gOvsSwitchContext->virtualExternalVport = vport; - gOvsSwitchContext->virtualExternalPortId = vport->portId; - } - - /* - * insert the port into the hash array of ports: by port number and ovs - * and ovs (datapath) port name. - * NOTE: OvsJhashWords has portNo as "1" word. This is ok, because the - * portNo is stored in 2 bytes only (max port number = MAXUINT16). - */ - hash = OvsJhashWords(&vport->portNo, 1, OVS_HASH_BASIS); - InsertHeadList(&gOvsSwitchContext->portNoHashArray[hash & OVS_VPORT_MASK], - &vport->portNoLink); - - hash = OvsJhashBytes(vport->ovsName, portNameLen, OVS_HASH_BASIS); - InsertHeadList(&gOvsSwitchContext->ovsPortNameHashArray[hash & OVS_VPORT_MASK], - &vport->ovsNameLink); + status = InitOvsVportCommon(gOvsSwitchContext, vport); + ASSERT(status == STATUS_SUCCESS); status = OvsCreateMsgFromVport(vport, msgIn, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength, @@ -1916,7 +1904,7 @@ Cleanup: POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; - if (vport && vport->ovsType == OVS_VPORT_TYPE_VXLAN) { + if (vport && vportAllocated == TRUE) { OvsRemoveAndDeleteVport(gOvsSwitchContext, vport); } diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c index a540926e8..2b68037c2 100644 --- a/datapath-windows/ovsext/Switch.c +++ b/datapath-windows/ovsext/Switch.c @@ -433,6 +433,7 @@ OvsUninitSwitchContext(POVS_SWITCH_CONTEXT switchContext) /* We need to do cleanup for tunnel port here. */ ASSERT(switchContext->numHvVports == 0); + ASSERT(switchContext->numNonHvVports == 0); NdisFreeRWLock(switchContext->dispatchLock); switchContext->dispatchLock = NULL; @@ -493,12 +494,6 @@ cleanup: return status; } -PVOID -OvsGetExternalVport() -{ - return gOvsSwitchContext->virtualExternalVport; -} - /* * -------------------------------------------------------------------------- diff --git a/datapath-windows/ovsext/Switch.h b/datapath-windows/ovsext/Switch.h index 11d9df623..8df250089 100644 --- a/datapath-windows/ovsext/Switch.h +++ b/datapath-windows/ovsext/Switch.h @@ -133,20 +133,28 @@ typedef struct _OVS_SWITCH_CONTEXT POVS_VPORT_ENTRY vxlanVport; - PLIST_ENTRY ovsPortNameHashArray; // based on ovsName + /* + * 'portIdHashArray' ONLY contains ports that exist on the Hyper-V switch, + * namely: VIF (vNIC) ports, external port and Hyper-V internal port. + * 'numHvVports' counts the ports in 'portIdHashArray'. + * + * 'portNoHashArray' ONLY contains ports that are added from OVS userspace, + * regardless of whether that port exists on the Hyper-V switch or not. + * Tunnel ports and bridge-internal ports are examples of ports that do not + * exist on the Hyper-V switch, and 'numNonHvVports' counts such ports in + * 'portNoHashArray'. + * + * 'ovsPortNameHashArray' contains the same entries as 'portNoHashArray' but + * hashed on a different key. + */ PLIST_ENTRY portIdHashArray; // based on Hyper-V portId PLIST_ENTRY portNoHashArray; // based on ovs port number + PLIST_ENTRY ovsPortNameHashArray; // based on ovsName PLIST_ENTRY pidHashArray; // based on packet pids NDIS_SPIN_LOCK pidHashLock; // Lock for pidHash table - /* - * 'numPhysicalNics' is the number of physical external NICs. - * 'numHvVports' is the number of Hyper-V switch ports added to OVS - * via the NDIS callbacks. - * 'numNonHvVports' is the number of ports added from userspace that are - * not on the Hyper-V switch. Eg. tunnel ports. - */ - UINT32 numPhysicalNics; + UINT32 numPhysicalNics; // the number of physical + // external NICs. UINT32 numHvVports; UINT32 numNonHvVports; diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index 11ec07db3..1e8154ed9 100644 --- a/datapath-windows/ovsext/Vport.c +++ b/datapath-windows/ovsext/Vport.c @@ -728,8 +728,6 @@ OvsInitTunnelVport(POVS_VPORT_ENTRY vport, { NTSTATUS status = STATUS_SUCCESS; - UNREFERENCED_PARAMETER(dstPort); - vport->isBridgeInternal = FALSE; vport->ovsType = ovsType; vport->ovsState = OVS_STATE_PORT_CREATED; @@ -739,8 +737,7 @@ OvsInitTunnelVport(POVS_VPORT_ENTRY vport, case OVS_VPORT_TYPE_GRE64: break; case OVS_VPORT_TYPE_VXLAN: - /* Will be enabled in later. */ - /* status = OvsInitVxlanTunnel(vport, dstPort); */ + status = OvsInitVxlanTunnel(vport, dstPort); break; default: ASSERT(0); @@ -889,6 +886,10 @@ InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext, switchContext->vxlanVport = vport; switchContext->numNonHvVports++; break; + case OVS_VPORT_TYPE_INTERNAL: + if (vport->isBridgeInternal) { + switchContext->numNonHvVports++; + } default: break; } @@ -1055,6 +1056,7 @@ OvsInitConfiguredSwitchNics(POVS_SWITCH_CONTEXT switchContext) nicParam->NicIndex != 0) { POVS_VPORT_ENTRY virtExtVport = (POVS_VPORT_ENTRY)switchContext->virtualExternalVport; + vport = OvsAllocateVport(); if (vport) { OvsInitPhysNicVport(vport, virtExtVport, @@ -1125,7 +1127,7 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT switchContext) LIST_FORALL_SAFE(head, link, next) { POVS_VPORT_ENTRY vport; vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink); - ASSERT(OvsIsTunnelVportType(vport->portType) || + ASSERT(OvsIsTunnelVportType(vport->ovsType) || (vport->ovsType == OVS_VPORT_TYPE_INTERNAL && vport->isBridgeInternal)); OvsRemoveAndDeleteVport(switchContext, vport); diff --git a/datapath-windows/ovsext/Vport.h b/datapath-windows/ovsext/Vport.h index bf9028e74..b4e4fca06 100644 --- a/datapath-windows/ovsext/Vport.h +++ b/datapath-windows/ovsext/Vport.h @@ -179,6 +179,18 @@ OvsIsTunnelVportType(OVS_VPORT_TYPE ovsType) ovsType == OVS_VPORT_TYPE_GRE64; } +static __inline POVS_VPORT_ENTRY +OvsGetTunnelVport(POVS_SWITCH_CONTEXT switchContext, + OVS_VPORT_TYPE ovsType) +{ + switch(ovsType) { + case OVS_VPORT_TYPE_VXLAN: + return switchContext->vxlanVport; + default: + return NULL; + } +} + static __inline BOOLEAN OvsIsInternalVportType(OVS_VPORT_TYPE ovsType) { @@ -194,12 +206,6 @@ OvsIsBridgeInternalVport(POVS_VPORT_ENTRY vport) return vport->isBridgeInternal == TRUE; } -static __inline UINT32 -OvsGetExternalMtu() -{ - return ((POVS_VPORT_ENTRY) OvsGetExternalVport())->mtu; -} - VOID OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT switchContext, POVS_VPORT_ENTRY vport);