[ovs-dev] [PATCH 6/7] datapath-windows: updates to vport add code
Alin Serdean
aserdean at cloudbasesolutions.com
Fri Oct 24 22:15:10 UTC 2014
Acked-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com>
Tested-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com>
-----Mesaj original-----
De la: dev [mailto:dev-bounces at openvswitch.org] În numele Nithin Raju
Trimis: Friday, October 24, 2014 3:33 AM
Către: dev at openvswitch.org
Subiect: [ovs-dev] [PATCH 6/7] datapath-windows: updates to 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 <nithin at vmware.com>
---
datapath-windows/ovsext/Datapath.c | 138 ++++++++++++++++-------------------
datapath-windows/ovsext/Switch.c | 7 +--
datapath-windows/ovsext/Switch.h | 26 +++++---
datapath-windows/ovsext/Vport.c | 12 ++--
datapath-windows/ovsext/Vport.h | 18 +++--
5 files changed, 100 insertions(+), 101 deletions(-)
diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index aede487..bd267f6 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 ab00a07..ac2b509 100644
--- a/datapath-windows/ovsext/Switch.c
+++ b/datapath-windows/ovsext/Switch.c
@@ -428,6 +428,7 @@ OvsCleanupSwitchContext(POVS_SWITCH_CONTEXT switchContext)
/* We need to do cleanup for tunnel port here. */
ASSERT(switchContext->numHvVports == 0);
+ ASSERT(switchContext->numNonHvVports == 0);
NdisFreeRWLock(switchContext->dispatchLock);
NdisFreeSpinLock(&(switchContext->pidHashLock));
@@ -483,12 +484,6 @@ cleanup:
return status;
}
-PVOID
-OvsGetExternalVport()
-{
- return gOvsSwitchContext->virtualExternalVport;
-}
-
/*
* --------------------------------------------------------------------------
diff --git a/datapath-windows/ovsext/Switch.h b/datapath-windows/ovsext/Switch.h
index 11d9df6..8df2500 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 11ec07d..1e8154e 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 bf9028e..b4e4fca 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);
--
1.7.4.1
_______________________________________________
dev mailing list
dev at openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list