hi Alin,
The change looks good overall, but for the comments I had. I am ok with not
addressing some of the comments since I have not looked at the future patches
yet.
There are some concerns I have about double freeing an external port. When we
add an external port, the sequence is something like this (as I observed at
least):
OvsCreatePort(): nicIndex = 0
OvsCreateNic(): nicIndex = 0
OvsCreateNic(): nicIndex = 1
OvsConnectNic(): nicIndex = 1
I don't know if the new code handles this. The previous code handed this in
kind of a complicated way by checking for nicIndex in OvsInitVportCommon() and
OvsCreateNic().
This patch as such is fine. If there are bugs, we can address them in future
patches.
Acked-by: Nithin Raju <nit...@vmware.com>
On Sep 30, 2014, at 7:54 AM, Samuel Ghinet <sghi...@cloudbasesolutions.com>
wrote:
> For this, the old method of finding vports based on the
> ovs port numbers is removed. Now, the lookup of a vport
> by ovs port number is done the same way as the lookup by
> hyper-v switch port id.
Hmm, the only concern I have is that we call into OvsFindVportByPortNo() many
times. We should make a good effort that the hash table has as minimum
collisions as possible. I don't mind using a bigger hash bucker just for
portNo. Eg. for every packet, we call OvsFindVportByPortNo() at lease 2x of
number of output ports.
>
> This is done so that the kernel is able to interact with
> the userspace correctly when using vport channels.
> The problem manifested in lib/dpif-netlink.c, at the function
> vport_add_channels.
>
> This patch removes the field vportArray from OVS_SWITCH_CONTEXT.
> In its place, portNoHashArray is set. In the OVS_VPORT_ENTRY
> struct, we also add portNoLink. This new method will do lookup
> and insertions of vports by ovs (datapath) port numbers the same
> way it is done for hyper-v switch port ids.
>
> This patch implicitly removes the indexes, which were previously
> used in insertions and lookups on ovs port numbers. The removal
> of the index also means that the vxlan vport can no longer be
> looked up the same way as it was done before: now we hold a pointer
> to it, vxlanVport in OVS_SWITCH_CONTEXT. For the moment, we can
> have only one vxlan vport. When more will be allowed, this field
> will turn into a list of vxlan ports.
>
> The invalid port number value (held in OVS_DPPORT_NUMBER_INVALID)
> is now changed from 0 to MAXUINT16, the same as it is on linux.
Ok, we can remove the OVS_DEFAULT_PORT_NO now, and udpate other port number
fields to be 16-bit too.
>
> The function OvsComputeVportNo has also been removed, since the
> computation of a vport port number can no longer be done like this.
> When vport add will be added, a new, updated OvsComputeVportNo
> function will be added.
>
> Also, in OvsInitVportCommon, we no longer need to (and no longer can)
> initialize vport->ovsName, nor vport->ovsNameLen, because they will
> be initialized by the netlink command vport add. Since the netlink
> command vport add will generate numbers for the datapath (ovs) port
> numbers and set the port names, we cannot insert the vport into the
> hash array of port numbers here, nor into the hash array of port names.
>
> Signed-off-by: Samuel Ghinet <sghi...@cloudbasesolutions.com>
> ---
> datapath-windows/ovsext/Actions.c | 27 ++++--
> datapath-windows/ovsext/Switch.c | 28 ++-----
> datapath-windows/ovsext/Switch.h | 18 ++--
> datapath-windows/ovsext/Tunnel.c | 2 +-
> datapath-windows/ovsext/Vport.c | 169 ++++++++++----------------------------
> datapath-windows/ovsext/Vport.h | 56 +++----------
> 6 files changed, 94 insertions(+), 206 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Actions.c
> b/datapath-windows/ovsext/Actions.c
> index 180b6b8..cf44140 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -205,7 +205,7 @@ OvsDetectTunnelRxPkt(OvsForwardingContext *ovsFwdCtx,
> if (!flowKey->ipKey.nwFrag &&
> flowKey->ipKey.nwProto == IPPROTO_UDP &&
> flowKey->ipKey.l4.tpDst == VXLAN_UDP_PORT_NBO) {
> - tunnelVport = OvsGetTunnelVport(OVS_VPORT_TYPE_VXLAN);
> + tunnelVport = ovsFwdCtx->switchContext->vxlanVport;
> ovsActionStats.rxVxlan++;
> }
>
> @@ -271,9 +271,14 @@ OvsDetectTunnelPkt(OvsForwardingContext *ovsFwdCtx,
> * If the packet will not be encapsulated, consume the tunnel context
> * by clearing it.
> */
> - if (ovsFwdCtx->srcVportNo != OVS_DEFAULT_PORT_NO &&
> - !OvsIsVifVportNo(ovsFwdCtx->srcVportNo)) {
> - ovsFwdCtx->tunKey.dst = 0;
> + if (ovsFwdCtx->srcVportNo != OVS_DEFAULT_PORT_NO) {
> +
> + POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(
> + ovsFwdCtx->switchContext, ovsFwdCtx->srcVportNo);
> +
> + if (!vport || vport->ovsType != OVS_VPORT_TYPE_NETDEV) {
> + ovsFwdCtx->tunKey.dst = 0;
> + }
There's intermixing of OVS_DEFAULT_PORT_NO and OVS_DPPORT_NUMBER_INVALID.
> }
>
> /* Tunnel the packet only if tunnel context is set. */
> @@ -1468,9 +1473,17 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
> PNL_ATTR queueAttr;
> POVS_PACKET_QUEUE_ELEM elem;
> UINT32 queueId = OVS_DEFAULT_PACKET_QUEUE;
> - //XXX confusing that portNo is actually portId for external port.
> - BOOLEAN isRecv = (portNo == switchContext->externalPortId)
> - || OvsIsTunnelVportNo(portNo);
> + BOOLEAN isRecv = FALSE;
> +
> + POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(switchContext,
> + portNo);
> +
> + if (vport) {
> + if (vport->isExternal ||
> + OvsIsTunnelVportType(vport->ovsType)) {
> + isRecv = TRUE;
> + }
> + }
>
> queueAttr = NlAttrFindNested(a, OVS_USERSPACE_ATTR_PID);
> userdataAttr = NlAttrFindNested(a, OVS_USERSPACE_ATTR_USERDATA);
> diff --git a/datapath-windows/ovsext/Switch.c
> b/datapath-windows/ovsext/Switch.c
> index ca95bdd..fbf5e80 100644
> --- a/datapath-windows/ovsext/Switch.c
> +++ b/datapath-windows/ovsext/Switch.c
> @@ -354,8 +354,8 @@ OvsInitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> switchContext->dispatchLock =
> NdisAllocateRWLock(switchContext->NdisFilterHandle);
>
> - switchContext->vportArray =
> - (PVOID *)OvsAllocateMemory(sizeof (PVOID) *
> OVS_MAX_VPORT_ARRAY_SIZE);
> + switchContext->portNoHashArray = (PLIST_ENTRY)
> + OvsAllocateMemory(sizeof(LIST_ENTRY) * OVS_MAX_VPORT_ARRAY_SIZE);
> switchContext->ovsPortNameHashArray = (PLIST_ENTRY)
> OvsAllocateMemory(sizeof (LIST_ENTRY) * OVS_MAX_VPORT_ARRAY_SIZE);
> switchContext->portIdHashArray = (PLIST_ENTRY)
> @@ -367,14 +367,14 @@ OvsInitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> }
> if (status != NDIS_STATUS_SUCCESS ||
> switchContext->dispatchLock == NULL ||
> - switchContext->vportArray == NULL ||
> + switchContext->portNoHashArray == NULL ||
> switchContext->ovsPortNameHashArray == NULL ||
> switchContext->portIdHashArray == NULL) {
> if (switchContext->dispatchLock) {
> NdisFreeRWLock(switchContext->dispatchLock);
> }
> - if (switchContext->vportArray) {
> - OvsFreeMemory(switchContext->vportArray);
> + if (switchContext->portNoHashArray) {
> + OvsFreeMemory(switchContext->portNoHashArray);
> }
> if (switchContext->ovsPortNameHashArray) {
> OvsFreeMemory(switchContext->ovsPortNameHashArray);
> @@ -395,13 +395,13 @@ OvsInitSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> for (i = 0; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
> InitializeListHead(&switchContext->portIdHashArray[i]);
> }
> - RtlZeroMemory(switchContext->vportArray,
> - sizeof (PVOID) * OVS_MAX_VPORT_ARRAY_SIZE);
> + for (i = 0; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
> + InitializeListHead(&switchContext->portNoHashArray[i]);
> + }
>
> switchContext->isActivated = FALSE;
> switchContext->isActivateFailed = FALSE;
> switchContext->dpNo = OVS_DP_NUMBER;
> - switchContext->lastPortIndex = OVS_MAX_VPORT_ARRAY_SIZE -1;
> ovsTimeIncrementPerTick = KeQueryTimeIncrement() / 10000;
> OVS_LOG_TRACE("Exit: Succesfully initialized switchContext: %p",
> switchContext);
> @@ -419,7 +419,7 @@ OvsCleanupSwitchContext(POVS_SWITCH_CONTEXT switchContext)
> NdisFreeRWLock(switchContext->dispatchLock);
> OvsFreeMemory(switchContext->ovsPortNameHashArray);
> OvsFreeMemory(switchContext->portIdHashArray);
> - OvsFreeMemory(switchContext->vportArray);
> + OvsFreeMemory(switchContext->portNoHashArray);
> OvsDeleteFlowTable(&switchContext->datapath);
> OvsCleanupBufferPool(switchContext);
> OVS_LOG_TRACE("Exit: Delete switchContext: %p", switchContext);
> @@ -469,16 +469,6 @@ cleanup:
> }
>
> PVOID
> -OvsGetVportFromIndex(UINT16 index)
> -{
> - if (index < OVS_MAX_VPORT_ARRAY_SIZE &&
> - !OVS_IS_VPORT_ENTRY_NULL(gOvsSwitchContext, index)) {
> - return gOvsSwitchContext->vportArray[index];
> - }
> - return NULL;
> -}
> -
> -PVOID
> OvsGetExternalVport()
> {
> return gOvsSwitchContext->externalVport;
> diff --git a/datapath-windows/ovsext/Switch.h
> b/datapath-windows/ovsext/Switch.h
> index 5cf65b9..83a725c 100644
> --- a/datapath-windows/ovsext/Switch.h
> +++ b/datapath-windows/ovsext/Switch.h
> @@ -43,10 +43,6 @@
> #define OVS_INTERNAL_VPOR_END 72
> #define OVS_VM_VPORT_START 72
> #define OVS_VM_VPORT_MAX 0xffff
We don't need these constants either.
> -#define OVS_VPORT_INDEX(_portNo) ((_portNo) & 0xffffff)
> -#define OVS_VPORT_PORT_NO(_index, _gen) \
> - (((_index) & 0xffffff) | ((UINT32)(_gen) << 24))
> -#define OVS_VPORT_GEN(portNo) (portNo >> 24)
>
> #define OVS_MAX_PHYS_ADAPTERS 32
> #define OVS_MAX_IP_VPOR 32
> @@ -109,13 +105,18 @@ typedef struct _OVS_SWITCH_CONTEXT
> POVS_VPORT_ENTRY externalVport; // the virtual adapter vport
> POVS_VPORT_ENTRY internalVport;
>
> - PVOID *vportArray;
> - PLIST_ENTRY ovsPortNameHashArray; // based on ovsName
> - PLIST_ENTRY portIdHashArray; // based on portId
> + /*
> + * XXX when we support multiple VXLAN ports, we will need a list entry
> + * instead
> + */
> + POVS_VPORT_ENTRY vxlanVport;
> +
> + PLIST_ENTRY ovsPortNameHashArray; // based on ovsName
> + PLIST_ENTRY portIdHashArray; // based on
> portId
> + PLIST_ENTRY portNoHashArray; // based on
> ovs port number
>
> UINT32 numPhysicalNics;
> UINT32 numVports; // include validation port
> - UINT32 lastPortIndex;
>
> /* Lock taken over the switch. This protects the ports on the switch. */
> PNDIS_RW_LOCK_EX dispatchLock;
> @@ -165,7 +166,6 @@ OvsReleaseDatapath(OVS_DATAPATH *datapath,
> }
>
>
> -PVOID OvsGetVportFromIndex(UINT16 index);
> PVOID OvsGetExternalVport();
>
> #endif /* __SWITCH_H_ */
> diff --git a/datapath-windows/ovsext/Tunnel.c
> b/datapath-windows/ovsext/Tunnel.c
> index 304ab59..64e279c 100644
> --- a/datapath-windows/ovsext/Tunnel.c
> +++ b/datapath-windows/ovsext/Tunnel.c
> @@ -284,7 +284,7 @@ OvsInjectPacketThroughActions(PNET_BUFFER_LIST pNbl,
>
> SendFlags |= NDIS_SEND_FLAGS_DISPATCH_LEVEL;
>
> - vport = OvsGetTunnelVport(OVS_VPORT_TYPE_VXLAN);
> + vport = gOvsSwitchContext->vxlanVport;
>
> if (vport == NULL){
> status = STATUS_UNSUCCESSFUL;
> diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
> index 28319a4..443e71e 100644
> --- a/datapath-windows/ovsext/Vport.c
> +++ b/datapath-windows/ovsext/Vport.c
> @@ -211,7 +211,7 @@ HvOnCreateNic(POVS_SWITCH_CONTEXT switchContext,
>
> add_nic_done:
> NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
> - if (portNo && event) {
> + if (portNo != OVS_DPPORT_NUMBER_INVALID && event) {
> OvsPostEvent(portNo, event);
> }
>
> @@ -260,6 +260,7 @@ HvOnConnectNic(POVS_SWITCH_CONTEXT switchContext,
>
> NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
>
> + /* XXX only if portNo != INVALID or always? */
> OvsPostEvent(portNo, OVS_EVENT_LINK_UP);
>
> if (nicParam->NicType == NdisSwitchNicTypeInternal) {
> @@ -384,6 +385,7 @@ HvOnDisconnectNic(POVS_SWITCH_CONTEXT switchContext,
>
> NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
>
> + /* XXX if portNo != INVALID or always? */
> OvsPostEvent(portNo, OVS_EVENT_LINK_DOWN);
>
> if (isInternalPort) {
> @@ -432,6 +434,7 @@ HvOnDeleteNic(POVS_SWITCH_CONTEXT switchContext,
> vport->ovsState = OVS_STATE_PORT_CREATED;
>
> NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
> + /* XXX if portNo != INVALID or always? */
> OvsPostEvent(portNo, OVS_EVENT_DISCONNECT);
>
> done:
> @@ -446,14 +449,15 @@ POVS_VPORT_ENTRY
> OvsFindVportByPortNo(POVS_SWITCH_CONTEXT switchContext,
> UINT32 portNo)
> {
> - if (OVS_VPORT_INDEX(portNo) < OVS_MAX_VPORT_ARRAY_SIZE) {
> - if (OVS_IS_VPORT_ENTRY_NULL(switchContext, OVS_VPORT_INDEX(portNo)))
> {
> - return NULL;
> - } else {
> - POVS_VPORT_ENTRY vport;
> - vport = (POVS_VPORT_ENTRY)
> - switchContext->vportArray[OVS_VPORT_INDEX(portNo)];
> - return vport->portNo == portNo ? vport : NULL;
> + POVS_VPORT_ENTRY vport;
> + PLIST_ENTRY head, link;
> + UINT32 hash = OvsJhashBytes((const VOID *)&portNo, sizeof(portNo),
> + OVS_HASH_BASIS);
> + head = &(switchContext->portNoHashArray[hash & OVS_VPORT_MASK]);
> + LIST_FORALL(head, link) {
> + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink);
> + if (vport->portNo == portNo) {
> + return vport;
> }
> }
> return NULL;
> @@ -485,18 +489,7 @@ OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT
> switchContext,
> NDIS_SWITCH_NIC_INDEX index)
> {
> if (portId == switchContext->externalPortId) {
> - if (index == 0) {
> - return (POVS_VPORT_ENTRY)switchContext->externalVport;
> - } else if (index > OVS_MAX_PHYS_ADAPTERS) {
> - return NULL;
> - }
> - if (OVS_IS_VPORT_ENTRY_NULL(switchContext,
> - index + OVS_EXTERNAL_VPORT_START)) {
> - return NULL;
> - } else {
> - return (POVS_VPORT_ENTRY)switchContext->vportArray[
> - index + OVS_EXTERNAL_VPORT_START];
> - }
> + return (POVS_VPORT_ENTRY)switchContext->externalVport;
> } else if (switchContext->internalPortId == portId) {
> return (POVS_VPORT_ENTRY)switchContext->internalVport;
> } else {
> @@ -515,72 +508,6 @@ OvsFindVportByPortIdAndNicIndex(POVS_SWITCH_CONTEXT
> switchContext,
> }
> }
>
> -UINT32
> -OvsComputeVportNo(POVS_SWITCH_CONTEXT switchContext,
> - UINT32 nicIndex,
> - OVS_VPORT_TYPE ovsType,
> - BOOLEAN isExternal)
> -{
> - UINT32 index = 0xffffff, i = 0;
> - UINT64 gen;
> -
> - if (isExternal) {
> - if (nicIndex == 0) {
> - return 0; // not a valid portNo
> - } else if (nicIndex > OVS_MAX_PHYS_ADAPTERS) {
> - return 0;
> - } else {
> - index = nicIndex + OVS_EXTERNAL_VPORT_START;
> - }
> - }
> -
> - switch (ovsType) {
> - case OVS_VPORT_TYPE_INTERNAL:
> - index = OVS_INTERNAL_VPORT_DEFAULT_INDEX;
> - break;
> - case OVS_VPORT_TYPE_NETDEV:
> - index = switchContext->lastPortIndex + 1;
> - if (index == OVS_MAX_VPORT_ARRAY_SIZE) {
> - index = OVS_VM_VPORT_START;
> - }
> - while (!OVS_IS_VPORT_ENTRY_NULL(switchContext, index) &&
> - i < (OVS_MAX_VPORT_ARRAY_SIZE - OVS_VM_VPORT_START)) {
> - index++;
> - i++;
> - if (index == OVS_MAX_VPORT_ARRAY_SIZE) {
> - index = OVS_VM_VPORT_START;
> - }
> - }
> - if (i == (OVS_MAX_VPORT_ARRAY_SIZE - OVS_VM_VPORT_START)) {
> - return 0; // not available
> - }
> - switchContext->lastPortIndex = index;
> - break;
> - case OVS_VPORT_TYPE_GRE:
> - index = OVS_GRE_VPORT_INDEX;
> - break;
> - case OVS_VPORT_TYPE_GRE64:
> - index = OVS_GRE64_VPORT_INDEX;
> - break;
> - case OVS_VPORT_TYPE_VXLAN:
> - index = OVS_VXLAN_VPORT_INDEX;
> - break;
> - default:
> - ASSERT(isExternal);
> - }
> - if (index > OVS_MAX_VPORT_ARRAY_SIZE) {
> - return 0;
> - }
> - gen = (UINT64)switchContext->vportArray[index];
> - if (gen > 0xff) {
> - return 0;
> - } else if (gen == 0) {
> - gen++;
> - }
> - return OVS_VPORT_PORT_NO(index, (UINT32)gen);
> -}
> -
> -
> static POVS_VPORT_ENTRY
> OvsAllocateVport(VOID)
> {
> @@ -591,6 +518,12 @@ OvsAllocateVport(VOID)
> }
> RtlZeroMemory(vport, sizeof (OVS_VPORT_ENTRY));
> vport->ovsState = OVS_STATE_UNKNOWN;
> + vport->portNo = OVS_DPPORT_NUMBER_INVALID;
> +
> + InitializeListHead(&vport->ovsNameLink);
> + InitializeListHead(&vport->portIdLink);
> + InitializeListHead(&vport->portNoLink);
> +
> return vport;
> }
>
> @@ -699,63 +632,45 @@ OvsInitPhysNicVport(POVS_VPORT_ENTRY vport,
> }
> static NDIS_STATUS
> OvsInitVportCommon(POVS_SWITCH_CONTEXT switchContext,
> -POVS_VPORT_ENTRY vport)
> + POVS_VPORT_ENTRY vport)
> {
> UINT32 hash;
> - size_t len;
> - if (vport->portType != NdisSwitchPortTypeExternal ||
> - vport->nicIndex != 0) {
> - vport->portNo = OvsComputeVportNo(switchContext, vport->nicIndex,
> - vport->ovsType, vport->portType == NdisSwitchPortTypeExternal);
> - if (vport->portNo == OVS_DPPORT_NUMBER_INVALID) {
> - return NDIS_STATUS_RESOURCES;
> - }
> - ASSERT(OVS_IS_VPORT_ENTRY_NULL(switchContext,
> - OVS_VPORT_INDEX(vport->portNo)));
>
> - switchContext->vportArray[OVS_VPORT_INDEX(vport->portNo)] = vport;
> - }
Adding an external port is a little complicated. We get two calls to
OvsCreateNic(). The code above kind of handled this case. Anyway, we need to
simplify this code. So, I am OK with getting rid of it.
> + ASSERT(vport->portNo == OVS_DPPORT_NUMBER_INVALID);
> +
> switch (vport->portType) {
> case NdisSwitchPortTypeExternal:
> if (vport->nicIndex == 0) {
> switchContext->externalPortId = vport->portId;
> switchContext->externalVport = vport;
> - RtlStringCbPrintfA(vport->ovsName, OVS_MAX_PORT_NAME_LENGTH - 1,
> - "external.virtualAdapter");
> - }
> - else {
> + } else {
> switchContext->numPhysicalNics++;
The else code is not really serving the purpose it is supposed to. But, I am ok
with it. Pls. add a XXX: comment saying we need to handle this in the future.
> - RtlStringCbPrintfA(vport->ovsName, OVS_MAX_PORT_NAME_LENGTH - 1,
> - "external.%lu", (UINT32)vport->nicIndex);
> }
> break;
> case NdisSwitchPortTypeInternal:
> switchContext->internalPortId = vport->portId;
> switchContext->internalVport = vport;
> - RtlStringCbPrintfA(vport->ovsName, OVS_MAX_PORT_NAME_LENGTH - 1,
> - "internal");
> break;
> case NdisSwitchPortTypeSynthetic:
> - RtlStringCbPrintfA(vport->ovsName, OVS_MAX_PORT_NAME_LENGTH - 1,
> - "vmNICSyn.%lx", vport->portNo);
> break;
> case NdisSwitchPortTypeEmulated:
> - RtlStringCbPrintfA(vport->ovsName, OVS_MAX_PORT_NAME_LENGTH - 1,
> - "vmNICEmu.%lx", vport->portNo);
> break;
> }
> - StringCbLengthA(vport->ovsName, OVS_MAX_PORT_NAME_LENGTH - 1, &len);
> - vport->ovsNameLen = (UINT32)len;
> +
> if (vport->portType == NdisSwitchPortTypeExternal &&
> vport->nicIndex == 0) {
> return NDIS_STATUS_SUCCESS;
> }
> - hash = OvsJhashBytes(vport->ovsName, vport->ovsNameLen, OVS_HASH_BASIS);
> - InsertHeadList(&switchContext->ovsPortNameHashArray[hash &
> OVS_VPORT_MASK],
> - &vport->ovsNameLink);
> +
> + /*
> + * NOTE: OvsJhashWords has portId as "1" word. This should be ok, even
> + * though sizeof(NDIS_SWITCH_PORT_ID) = 4, not 2, because the
> + * hyper-v switch seems to use only 2 bytes out of 4.
> + */
Pls. add a XXX comment, we'll revisit this in the future.
> hash = OvsJhashWords(&vport->portId, 1, OVS_HASH_BASIS);
> InsertHeadList(&switchContext->portIdHashArray[hash & OVS_VPORT_MASK],
> &vport->portIdLink);
> +
> switchContext->numVports++;
> return NDIS_STATUS_SUCCESS;
> }
> @@ -765,8 +680,6 @@ static VOID
> OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT switchContext,
> POVS_VPORT_ENTRY vport)
> {
> - UINT64 gen = vport->portNo >> 24;
> -
> if (vport->isExternal) {
> if (vport->nicIndex == 0) {
> ASSERT(switchContext->numPhysicalNics == 0);
> @@ -799,9 +712,7 @@ OvsRemoveAndDeleteVport(POVS_SWITCH_CONTEXT switchContext,
>
> RemoveEntryList(&vport->ovsNameLink);
> RemoveEntryList(&vport->portIdLink);
> - gen = (gen + 1) & 0xff;
> - switchContext->vportArray[OVS_VPORT_INDEX(vport->portNo)] =
> - (PVOID)(UINT64)gen;
> + RemoveEntryList(&vport->portNoLink);
> switchContext->numVports--;
> OvsFreeMemory(vport);
> }
> @@ -925,14 +836,18 @@ cleanup:
> VOID
> OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT switchContext)
> {
> - UINT32 i;
> + for (UINT hash = 0; hash < OVS_MAX_VPORT_ARRAY_SIZE; ++hash) {
> + PLIST_ENTRY head, link;
> +
> + head = &(switchContext->portNoHashArray[hash & OVS_VPORT_MASK]);
> + LIST_FORALL(head, link) {
> + POVS_VPORT_ENTRY vport;
>
> - for (i = 0; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
> - if (!OVS_IS_VPORT_ENTRY_NULL(switchContext, i)) {
> - OvsRemoveAndDeleteVport(switchContext,
> - (POVS_VPORT_ENTRY)switchContext->vportArray[i]);
> + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink);
> + OvsRemoveAndDeleteVport(switchContext, vport);
> }
> }
> +
> if (switchContext->externalVport) {
> OvsRemoveAndDeleteVport(switchContext,
> (POVS_VPORT_ENTRY)switchContext->externalVport);
I don't know if we need the special code to remove a vport based on
'switchContext->externalVport'. All the Vports should be part of
portNoHashArray. So, if you flush that list, it should be sufficient. Did you
encounter any double frees in your testing?
> diff --git a/datapath-windows/ovsext/Vport.h b/datapath-windows/ovsext/Vport.h
> index fce16e2..678c7c5 100644
> --- a/datapath-windows/ovsext/Vport.h
> +++ b/datapath-windows/ovsext/Vport.h
> @@ -17,7 +17,18 @@
> #ifndef __VPORT_H_
> #define __VPORT_H_ 1
>
> -#define OVS_DPPORT_NUMBER_INVALID 0
> +#define OVS_MAX_DPPORTS MAXUINT16
> +#define OVS_DPPORT_NUMBER_INVALID OVS_MAX_DPPORTS
> +/*
> + * The local port (0) is a reserved port, that is not allowed to be be
> + * created by the netlink command vport add. On linux, this port is created
> + * at netlink command datapath new. However, on windows, we do not need to
> + * create it, and more, we shouldn't. The userspace attempts to create two
> + * internal vports, the LOCAL port (0) and the internal port (with any other
> + * port number). The non-LOCAL internal port is used in the userspace when it
> + * requests the internal port.
> +*/
> +#define OVS_DPPORT_LOCAL 0
Pls. call this OVS_DPPORT_NUMBER_LOCAL.
>
> #include "Switch.h"
>
> @@ -67,6 +78,7 @@ typedef struct _OVS_VPORT_FULL_STATS {
> typedef struct _OVS_VPORT_ENTRY {
> LIST_ENTRY ovsNameLink;
> LIST_ENTRY portIdLink;
> + LIST_ENTRY portNoLink;
>
> OVS_VPORT_STATE ovsState;
> OVS_VPORT_TYPE ovsType;
> @@ -99,9 +111,6 @@ typedef struct _OVS_VPORT_ENTRY {
>
> struct _OVS_SWITCH_CONTEXT;
>
> -#define OVS_IS_VPORT_ENTRY_NULL(_SwitchContext, _i) \
> - ((UINT64)(_SwitchContext)->vportArray[_i] <= 0xff)
> -
> POVS_VPORT_ENTRY
> OvsFindVportByPortNo(struct _OVS_SWITCH_CONTEXT *switchContext,
> UINT32 portNo);
> @@ -148,49 +157,10 @@ OvsIsInternalVportType(OVS_VPORT_TYPE ovsType)
> return ovsType == OVS_VPORT_TYPE_INTERNAL;
> }
>
> -static __inline BOOLEAN
> -OvsIsTunnelVportNo(UINT32 portNo)
> -{
> - UINT32 idx = OVS_VPORT_INDEX(portNo);
> - return (idx >= OVS_TUNNEL_INDEX_START && idx <= OVS_TUNNEL_INDEX_END);
> -}
OvsIsTunnelVportNo() can can still be useful, but we can add it back later.
> -
> -static __inline BOOLEAN
> -OvsIsVifVportNo(UINT32 portNo)
> -{
> - UINT32 idx = OVS_VPORT_INDEX(portNo);
> - return (idx >= OVS_VM_VPORT_START && idx <= OVS_VM_VPORT_MAX);
> -}
> -
> -static __inline POVS_VPORT_ENTRY
> -OvsGetTunnelVport(OVS_VPORT_TYPE type)
> -{
> - ASSERT(OvsIsTunnelVportType(type));
> - switch(type) {
> - case OVS_VPORT_TYPE_VXLAN:
> - return (POVS_VPORT_ENTRY)
> OvsGetVportFromIndex(OVS_VXLAN_VPORT_INDEX);
> - default:
> - ASSERT(! "OvsGetTunnelVport not implemented for this tunnel.");
> - }
> -
> - return NULL;
> -}
> -
> -static __inline PVOID
> -OvsGetVportPriv(OVS_VPORT_TYPE type)
> -{
> - return OvsGetTunnelVport(type)->priv;
> -}
> -
> static __inline UINT32
> OvsGetExternalMtu()
> {
> return ((POVS_VPORT_ENTRY) OvsGetExternalVport())->mtu;
> }
>
> -UINT32 OvsComputeVportNo(POVS_SWITCH_CONTEXT switchContext,
> - UINT32 nicIndex,
> - OVS_VPORT_TYPE ovsType,
> - BOOLEAN isExternal);
> -
> #endif /* __VPORT_H_ */
> --
> 1.8.3.msysgit.0
>
Thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
