Hello Nithin,
[QUOTE]
Is this a pre-cursor for implementing the persistent ports feature that the
cloudbase repo had implemented.
[/QUOTE]
Yes.
I think it is better to drop the "persistent port" term. When I had designed it
long ago, I chose the name "persistent" because it "persists" during hyper-v
switch port creation and destruction (its lifetime is determined by the netlink
vport commands only).
Anyway, I was thinking how better to name it: "vport" is used on linux, but on
Windows we have hyper-v ports, which are vports as well. I had thought of
OFPort, but open flow ports are userspace ports, and the userspace deals with
of ports and dp ports.
Therefore, I think it would be best to call them datapath ports.
[QUOTE]
Can the persistent port patch be implemented on the of the VPORT array data
structure we have today in OvsSwitch.h, OvsVport.h and OvsVport.c.
[/QUOTE]
I don't think it would work very well. I mean, it could be quite intricate.
Basically, the following workflows can happen, for vport creation:
workflow1:
a) turn on VM1 (hyper-v port vmPort1 and hyper-v nic vmNic1 created
automatically)
b) set vm port friendly name "port1" for the port o vmNic1 (port oid update
happens)
c) create dp port "port1"
workflow2:
a) create dp port "port2"
b) set vm port friendly name "port2" for the port of vmNic2 (vmNic of VM2)
c) turn on VM2 (hyper-v port vmPort2 with port friendly name = "port2", and
hyper-v nic vmNic2 created automatically)
workflow3:
a) create dp port "port3"
b) turn on VM3 (hyper-v port vmPort3 and hyper-v nic vmNic3 created
automatically)
c) (much later on) set vm port friendly name "port3" for the port o vmNic3
workflow4:
a) create dp port "port4"
b) set vm port friendly name "port4" for the port of vmNic4 (vmNic of VM4)
b) turn on VM4 (hyper-v port vmPort4 with port friendly name = "port4" and
hyper-v nic vmNic4 created automatically)
c) (much later on) rename vm port name "port4" -> "my_port4"
Considerations for having only one vport array:
We can also have persistent / dp ports that have no hyper-v switch ports (will
never, or not yet), and hyper-v switch ports that have no persistent / dp port
counterpart (will never, or not yet).
Where we have a 'mapping' between hyper-v switch ports and dp ports, that
mapping would become broken if the vport friendly name would be modified later
on.
And this "mapping" means that at some point in time you may have two different
kinds of ports (hyper-v switch port created, dp port created) that don't know
YET they represent the same thing.
So this means that whenever that happens (i.e. they represent the same thing,
i.e. hv port friendly name = dp port name), you'll need to merge a vport2 -> a
vport1 (certain info) and destroy vport2.
Creation of a vport would behave differently for the workflows above: a
"create" would mean a lookup, then either "set" or create" or "merge + delete".
A deletion of a vport would mean setting "deleted" on either the hyper-v port
part or dp port part. And you can actually destroy the vport only when both
parts are marked as 'deleted'.
A rename of a hyper-v switch port friendly name would sometimes mean creating
two vports out of one (break mapping).
If we keep a separate array for dp ports (fixed sized array) and a separate
array for hyper-v switch ports:
a) we would have logical separation between hyper-v switch ports and dp ports.
this means the hyper-v switch port array will be managed separately from dp
port array.
b) we would have clearer code
c) we could do a constant-time lookup by number on the dp port array (i.e.
indexing), and a constant-time lookup by port id on the hyper-v switch port
array (i.e. indexing).
During packet processing, the vport lookup is always done via dp port number,
so I think that constant-time lookup here would be more appropriate.
[QUOTE]
My understanding is that using WMI, userspace sets the name into the
'FriendlyName' field of the hyper-v port/NIC
[/QUOTE]
It sets the FriendlyName of the hyper-v switch port.
[QUOTE]
As long as the kernel driver reads this name, it should get access to the
persistent name for the port.
[/QUOTE]
The persistent / dp port name will be the same as the hyper-v switch port
friendly name, when both represent the same actual port.
At port create or port update, the dp ports would be looked up for a dp port
whose dp port name matches port params' hyper-v switch port friendly name.
[QUOTE]
Is there more to this patch?
[/QUOTE]
This patch was the fixed sized array only. You meant, if there is more to the
vport patch?
Thanks!
Sam
________________________________________
From: Nithin Raju [nit...@vmware.com]
Sent: Saturday, August 16, 2014 9:57 AM
To: Samuel Ghinet
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH 12/15] datapath-windows: Add FixedSizedArray
Samuel,
Is this a pre-cursor for implementing the persistent ports feature that the
cloudbase repo had implemented. We really liked that feature to be able to set
the port name from userspace.
Can the persistent port patch be implemented on the of the VPORT array data
structure we have today in OvsSwitch.h, OvsVport.h and OvsVport.c. My
understanding is that using WMI, userspace sets the name into the
'FriendlyName' field of the hyper-v port/NIC. As long as the kernel driver
reads this name, it should get access to the persistent name for the port. Is
there more to this patch?
Pls. do look at the implementation of 'vportArray' in OVS_SWITCH_CONTEXT.
There's support the following lookup functions:
OvsFindVportByPortIdAndNicIndex()
OvsFindVportByOvsName()
OvsFindVportByPortNo()
I'll look at the code change and give more comments.
thanks,
Nithin
On Aug 6, 2014, at 9:15 AM, Samuel Ghinet <sghi...@cloudbasesolutions.com>
wrote:
> Add FixedSizedArray
>
> This fixed sized array implementation is meant to be used for datapath ports,
> hyper-v switch ports and
> hyper-v switch nics. It is a fixed array of MAXUINT16 pointers to fixed sized
> array items.
> The OVS_FXDARRAY_ITEM supports reference counting.
> The fixed sized array will offer constant-time lookup based on port number,
> which is almost always the
> kind of port lookup we do in the driver (the other one is by name, which is
> at km-um communication only).
>
> Also, for hyper-v switch ports:
> A closer look at NDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO, we see
> that the
> port id there is 16bit wide. Therefore, we could use, in the future the fixed
> sized array for hyper-v switch
> nics and ids as well.
>
> In the future we may be able to build upon it a more complex structure, such
> as a flexible array, if the need will arise.
>
> Signed-off-by: Samuel Ghinet <sghi...@cloudbasesolutions.com>
> ---
> datapath-windows/ovsext/Core/FixedSizedArray.c | 194 +++++++++++++++++++++++++
> datapath-windows/ovsext/Core/FixedSizedArray.h | 110 ++++++++++++++
> datapath-windows/ovsext/ovsext.vcxproj | 2 +
> datapath-windows/ovsext/ovsext.vcxproj.filters | 8 +-
> datapath-windows/ovsext/precomp.h | 1 +
> 5 files changed, 314 insertions(+), 1 deletion(-)
> create mode 100644 datapath-windows/ovsext/Core/FixedSizedArray.c
> create mode 100644 datapath-windows/ovsext/Core/FixedSizedArray.h
>
> diff --git a/datapath-windows/ovsext/Core/FixedSizedArray.c
> b/datapath-windows/ovsext/Core/FixedSizedArray.c
> new file mode 100644
> index 0000000..3f63753
> --- /dev/null
> +++ b/datapath-windows/ovsext/Core/FixedSizedArray.c
> @@ -0,0 +1,194 @@
> +/*
> +Copyright 2014 Cloudbase Solutions Srl
> +
> +Licensed under the Apache License, Version 2.0 (the "License");
> +you may not use this file except in compliance with the License.
> +You may obtain a copy of the License at
> +
> +http ://www.apache.org/licenses/LICENSE-2.0
> +
> +Unless required by applicable law or agreed to in writing, software
> +distributed under the License is distributed on an "AS IS" BASIS,
> +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +See the License for the specific language governing permissions and
> +limitations under the License.
> +*/
> +
> +#include "FixedSizedArray.h"
> +
> +_Use_decl_annotations_
> +BOOLEAN FxdArray_FindNextFree_Unsafe(_In_ const OVS_FIXED_SIZED_ARRAY*
> pArray, _Inout_ UINT16* pFirst)
> +{
> + UINT16 first = 0;
> +
> + OVS_CHECK(pFirst);
> +
> + first = *pFirst;
> +
> + //we have set the 'firstFree' to a port => we must find the next free
> port to set firstFree = null_port
> + //we start searching a free slot in [first, end]
> + while (first < OVS_MAX_ARRAY_SIZE && pArray->array[first])
> + {
> + first++;
> + }
> +
> + //if we found a free slot => this is the free port we return
> + if (first < OVS_MAX_ARRAY_SIZE)
> + {
> + if (!pArray->array[first])
> + {
> + *pFirst = first;
> + return TRUE;
> + }
> + }
> +
> + //else, search [0, first)
> + for (first = 0; first < pArray->firstFree; ++first)
> + {
> + if (!pArray->array[first])
> + {
> + *pFirst = first;
> + return TRUE;
> + }
> + }
> +
> + return FALSE;
> +}
> +
> +_Use_decl_annotations_
> +OVS_ERROR FxdArray_AddByNumber_Unsafe(_Inout_ OVS_FIXED_SIZED_ARRAY* pArray,
> const OVS_FXDARRAY_ITEM* pItem, UINT16 number)
> +{
> + UINT16 first = pArray->firstFree;
> + OVS_ERROR error = OVS_ERROR_NOERROR;
> +
> + if (NULL != pArray->array[number])
> + {
> + return OVS_ERROR_EXIST;
> + }
> +
> + pArray->array[number] = OVS_CONST_CAST(pItem);
> + pArray->count++;
> +
> + if (first == 0)
> + {
> + OVS_CHECK(number <= MAXUINT16);
> +
> + first = (UINT16)number;
> + }
> +
> + if (first != number)
> + {
> + error = OVS_ERROR_INVAL;
> + goto Cleanup;
> + }
> +
> + if (first == number)
> + {
> + //we have set the 'firstFree' to a port => we must find the next
> free port to set firstFree = null_port
> + if (!FxdArray_FindNextFree_Unsafe(pArray, &first))
> + {
> + OVS_CHECK(pArray->count == MAXUINT16);
> +
> + LOG_ERROR("all available ports are used!\n");
> + error = OVS_ERROR_NOSPC;
> + goto Cleanup;
> + }
> +
> + pArray->firstFree = first;
> + }
> +
> +Cleanup:
> + if (error)
> + {
> + //found no room for new port
> + pArray->array[number] = NULL;
> + pArray->count--;
> + }
> +
> + return error;
> +}
> +
> +_Use_decl_annotations_
> +BOOLEAN FxdArray_Add_Unsafe(_Inout_ OVS_FIXED_SIZED_ARRAY* pArray, const
> OVS_FXDARRAY_ITEM* pItem, UINT16* pNumber)
> +{
> + UINT16 first = pArray->firstFree;
> + UINT16 number = 0;
> + BOOLEAN ok = TRUE;
> +
> + OVS_CHECK(NULL == pArray->array[first]);
> + number = first;
> +
> + pArray->array[number] = OVS_CONST_CAST(pItem);
> + pArray->count++;
> +
> + if (!FxdArray_FindNextFree_Unsafe(pArray, &first))
> + {
> + OVS_CHECK(pArray->count == MAXUINT16);
> +
> + LOG_ERROR("all available ports are used!\n");
> + ok = FALSE;
> + goto Cleanup;
> + }
> +
> + pArray->firstFree = first;
> +
> +Cleanup:
> + if (ok)
> + {
> + *pNumber = number;
> + }
> + else
> + {
> + //found no room for new port
> + pArray->array[number] = NULL;
> + pArray->count--;
> + }
> +
> + return ok;
> +}
> +
> +_Use_decl_annotations_
> +OVS_FXDARRAY_ITEM* FxdArray_Find_Unsafe(const OVS_FIXED_SIZED_ARRAY* pArray,
> FxdArrayCondition condition, const VOID* pCondData)
> +{
> + OVS_FXDARRAY_ITEM* pOutItem = NULL;
> +
> + OVS_FXDARRAY_FOR_EACH(pArray, pCurItem, /*if*/ condition(pCurItem,
> (UINT_PTR)pCondData),
> + pOutItem = OVS_REFCOUNT_REFERENCE(pCurItem)
> + );
> +
> + return pOutItem;
> +}
> +
> +_Use_decl_annotations_
> +OVS_FXDARRAY_ITEM* FxdArray_Find_Ref(const OVS_FIXED_SIZED_ARRAY* pArray,
> FxdArrayCondition condition, const VOID* pCondData)
> +{
> + OVS_FXDARRAY_ITEM* pOutItem = NULL;
> + LOCK_STATE_EX lockState;
> +
> + FXARRAY_LOCK_READ(pArray, &lockState);
> +
> + pOutItem = FxdArray_Find_Unsafe(pArray, condition, pCondData);
> +
> + FXARRAY_UNLOCK(pArray, &lockState);
> +
> + return pOutItem;
> +}
> +
> +BOOLEAN FxdArray_Remove_Unsafe(OVS_FIXED_SIZED_ARRAY* pArray,
> OVS_FXDARRAY_ITEM* pItem, UINT16 number)
> +{
> + if (pArray->array[number] != pItem)
> + {
> + LOG_ERROR(__FUNCTION__ "item not found: %u\n", number);
> + return FALSE;
> + }
> +
> + OVS_CHECK(number <= 0xFFFF);
> + pArray->array[number] = NULL;
> +
> + pArray->firstFree = number;
> +
> + OVS_CHECK(pArray->count > 0);
> + --(pArray->count);
> +
> + return TRUE;
> +}
> diff --git a/datapath-windows/ovsext/Core/FixedSizedArray.h
> b/datapath-windows/ovsext/Core/FixedSizedArray.h
> new file mode 100644
> index 0000000..08dddb3
> --- /dev/null
> +++ b/datapath-windows/ovsext/Core/FixedSizedArray.h
> @@ -0,0 +1,110 @@
> +/*
> +Copyright 2014 Cloudbase Solutions Srl
> +
> +Licensed under the Apache License, Version 2.0 (the "License");
> +you may not use this file except in compliance with the License.
> +You may obtain a copy of the License at
> +
> +http ://www.apache.org/licenses/LICENSE-2.0
> +
> +Unless required by applicable law or agreed to in writing, software
> +distributed under the License is distributed on an "AS IS" BASIS,
> +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +See the License for the specific language governing permissions and
> +limitations under the License.
> +*/
> +
> +#pragma once
> +
> +#include "precomp.h"
> +
> +#include "Error.h"
> +
> +#define OVS_MAX_ARRAY_SIZE MAXUINT16
> +
> +typedef struct _OVS_FXDARRAY_ITEM OVS_FXDARRAY_ITEM;
> +
> +typedef BOOLEAN (*FxdArrayCondition)(OVS_FXDARRAY_ITEM* pItem, UINT_PTR
> data);
> +
> +//specific item entries inherit OVS_FXDARRAY_ITEM
> +typedef struct _OVS_FXDARRAY_ITEM
> +{
> + //must be the first field in the struct
> + OVS_REF_COUNT refCount;
> +
> + NDIS_RW_LOCK_EX* pRwLock;
> +}OVS_FXDARRAY_ITEM, *POVS_FXDARRAY_ITEM;
> +
> +#define FXDITEM_LOCK_READ(pItem, pLockState)
> NdisAcquireRWLockRead((pItem)->pRwLock, pLockState, 0)
> +#define FXDITEM_LOCK_WRITE(pItem, pLockState)
> NdisAcquireRWLockWrite((pItem)->pRwLock, pLockState, 0)
> +#define FXDITEM_UNLOCK(pItem, pLockState)
> NdisReleaseRWLock((pItem)->pRwLock, pLockState)
> +
> +typedef struct _OVS_FIXED_SIZED_ARRAY
> +{
> + NDIS_RW_LOCK_EX* pRwLock;
> +
> + OVS_FXDARRAY_ITEM* array[OVS_MAX_ARRAY_SIZE];
> + UINT16 count;
> + UINT16 firstFree;
> +}OVS_FIXED_SIZED_ARRAY;
> +
> +#define FXDARRAY_LOCK_READ(pArray, pLockState)
> NdisAcquireRWLockRead((pArray)->pRwLock, pLockState, 0)
> +#define FXDARRAY_LOCK_WRITE(pArray, pLockState)
> NdisAcquireRWLockWrite((pArray)->pRwLock, pLockState, 0)
> +#define FXDARRAY_UNLOCK(pArray, pLockState)
> NdisReleaseRWLock((pArray)->pRwLock, pLockState)
> +#define FXDARRAY_UNLOCK_IF(pArray, pLockState, locked) { if ((locked) &&
> (pArray)) FXDARRAY_UNLOCK((pArray), pLockState); }
> +
> +#define OVS_FXDARRAY_FOR_EACH(pArray, pCurItem, condition, code) \
> +{ \
> + ULONG countProcessed = 0; \
> + \
> + for (ULONG i = 0; i < OVS_MAX_ARRAY_SIZE; ++i) \
> + { \
> + OVS_FXDARRAY_ITEM* pCurItem = (pArray)->array[i]; \
> + \
> + if (pCurItem) \
> + { \
> + LOCK_STATE_EX lockState = { 0 }; \
> + \
> + FXDITEM_LOCK_READ(pCurItem, &lockState); \
> + \
> + if ((condition)) \
> + { \
> + code; \
> + \
> + FXDITEM_UNLOCK(pCurItem, &lockState); \
> + break; \
> + } \
> + \
> + FXDITEM_UNLOCK(pCurItem, &lockState); \
> + \
> + ++countProcessed; \
> + } \
> + \
> + if (countProcessed >= (pArray)->count) \
> + { \
> + break; \
> + } \
> + } \
> + \
> + OVS_CHECK(countProcessed == (pArray)->count); \
> +}
> +
> +/**********************************************************************************/
> +
> +//unsafe = you must lock with FXARRAY lock
> +BOOLEAN FxdArray_FindNextFree_Unsafe(_In_ const OVS_FIXED_SIZED_ARRAY*
> pPorts, _Inout_ UINT16* pFirst);
> +
> +//unsafe = you must lock with FXARRAY lock
> +OVS_ERROR FxdArray_AddByNumber_Unsafe(_Inout_ OVS_FIXED_SIZED_ARRAY* pArray,
> _In_ const OVS_FXDARRAY_ITEM* pItem, UINT16 number);
> +
> +//unsafe = you must lock with FXARRAY lock
> +BOOLEAN FxdArray_Add_Unsafe(_Inout_ OVS_FIXED_SIZED_ARRAY* pArray, const
> OVS_FXDARRAY_ITEM* pItem, _Out_ UINT16* pNumber);
> +
> +_Ret_maybenull_
> +OVS_FXDARRAY_ITEM* FxdArray_Find_Unsafe(_In_ const OVS_FIXED_SIZED_ARRAY*
> pArray, FxdArrayCondition condition, _In_ const VOID* pCondData);
> +
> +_Ret_maybenull_
> +OVS_FXDARRAY_ITEM* FxdArray_Find_Ref(_In_ const OVS_FIXED_SIZED_ARRAY*
> pArray, FxdArrayCondition condition, _In_ const VOID* pCondData);
> +
> +_Ret_maybenull_
> +BOOLEAN FxdArray_Remove_Unsafe(_Inout_ OVS_FIXED_SIZED_ARRAY* pArray, _In_
> OVS_FXDARRAY_ITEM* pItem, UINT16 number);
> diff --git a/datapath-windows/ovsext/ovsext.vcxproj
> b/datapath-windows/ovsext/ovsext.vcxproj
> index 99aa6f6..2c62cab 100644
> --- a/datapath-windows/ovsext/ovsext.vcxproj
> +++ b/datapath-windows/ovsext/ovsext.vcxproj
> @@ -72,6 +72,7 @@
> <ItemGroup Label="WrappedTaskItems">
> <ClInclude Include="Core\Debug.h" />
> <ClInclude Include="Core\Error.h" />
> + <ClInclude Include="Core\FixedSizedArray.h" />
> <ClInclude Include="Core\IpHelper.h" />
> <ClInclude Include="Core\Jhash.h" />
> <ClInclude Include="Core\List.h" />
> @@ -136,6 +137,7 @@
> <ItemGroup>
> <ClCompile Include="Core\Debug.c" />
> <ClCompile Include="Core\Driver.c" />
> + <ClCompile Include="Core\FixedSizedArray.c" />
> <ClCompile Include="Core\IpHelper.c" />
> <ClCompile Include="Core\Jhash.c" />
> <ClCompile Include="Core\SpookyHash.c" />
> diff --git a/datapath-windows/ovsext/ovsext.vcxproj.filters
> b/datapath-windows/ovsext/ovsext.vcxproj.filters
> index fc3ba3b..96cfac6 100644
> --- a/datapath-windows/ovsext/ovsext.vcxproj.filters
> +++ b/datapath-windows/ovsext/ovsext.vcxproj.filters
> @@ -86,6 +86,9 @@
> <ClInclude Include="Core\RefCount.h">
> <Filter>Core</Filter>
> </ClInclude>
> + <ClInclude Include="Core\FixedSizedArray.h">
> + <Filter>Core</Filter>
> + </ClInclude>
> </ItemGroup>
> <ItemGroup>
> <ResourceCompile Include="ovsext.rc" />
> @@ -178,5 +181,8 @@
> <ClCompile Include="Core\SpookyHash.c">
> <Filter>Core</Filter>
> </ClCompile>
> + <ClCompile Include="Core\FixedSizedArray.c">
> + <Filter>Core</Filter>
> + </ClCompile>
> </ItemGroup>
> -</Project>
> \ No newline at end of file
> +</Project>
> diff --git a/datapath-windows/ovsext/precomp.h
> b/datapath-windows/ovsext/precomp.h
> index 529d7de..3fd5da2 100644
> --- a/datapath-windows/ovsext/precomp.h
> +++ b/datapath-windows/ovsext/precomp.h
> @@ -24,4 +24,5 @@
> #include "Core/Debug.h"
> #include "Core/Types.h"
> #include "Core/Util.h"
> +#include "Core/RefCount.h"
> #include "OpenFlow/Pub.h"
> --
> 1.8.3.msysgit.0
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=ubrOpIWavCMqX4l4j1LEVpTfDj%2FD5Qyn8KCoJIBGvzo%3D%0A&m=wcVRh0eqISAneUor3%2B%2B8Gg4MfjDK0veLK6unFg1wLSc%3D%0A&s=2000cec8aea1eaa07088b993a333ff7f24be6696c7cbc622b299c06062c24f3e
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
