[ovs-dev] [PATCH v3 2/3] datapath-windows: Add multiple switch internal ports

Alin Serdean aserdean at cloudbasesolutions.com
Sat Aug 13 01:07:27 UTC 2016


I missed a couple of comments. Here are my comments:
>-        OvsCopyPortParamsFromVport(virtExtVport, &portParam);
>+        char convertString[256];
>+        RtlZeroMemory(convertString, 256);
>         NdisReleaseRWLock(switchContext->dispatchLock, &lockState);

Sai - Do you need to release this lock here for 
OvsConvertIfCountedStrToAnsiStr?
This lock again gets acquired afterwards.

Alin: We need to release the lock to get back to passive level. OvsConvertIfCountedStrToAnsiStr calls RtlUnicodeStringToAnsiString(https://msdn.microsoft.com/en-us/library/windows/hardware/ff562969(v=vs.85).aspx ) which needs to be called at passive level, if we still have the lock we are at dispatch


>-        status = HvCreatePort(switchContext, &portParam,
>-                              nicParam->NicIndex);
>+        status = OvsConvertIfCountedStrToAnsiStr(&portFriendlyName,
>+                                                 convertString,
>+                                                 
>OVS_MAX_PORT_NAME_LENGTH);
>         NdisAcquireRWLockWrite(switchContext->dispatchLock, &lockState,

>         if (status != NDIS_STATUS_SUCCESS) {
>-            goto add_nic_done;

Sai - switchContext->dispatchLock needs to be released here.
Alin: rebase issue, it should go to add_nic_done;

>+            goto done;
>+        }
>+        POVS_VPORT_ENTRY ovsVport = OvsFindVportByOvsName(switchContext,
>+                                                          convertString);
>+        if (ovsVport != NULL) {
>+            UpdateSwitchCtxWithVport(switchContext, ovsVport, FALSE);
>+        } else {
>+            NDIS_SWITCH_PORT_PARAMETERS portParam;
>+            POVS_VPORT_ENTRY virtExtVport =
>+                (POVS_VPORT_ENTRY)switchContext->virtualExternalVport;
>+
>+            ASSERT(virtExtVport);
>+            OvsCopyPortParamsFromVport(virtExtVport, &portParam);
>+            NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
>+            status = HvCreatePort(switchContext, &portParam,
>+                                  nicParam->NicIndex);
>+            NdisAcquireRWLockWrite(switchContext->dispatchLock, 
>&lockState, 0);
>+            if (status != NDIS_STATUS_SUCCESS) {

Sai - switchContext->dispatchLock needs to be released here.


>+                goto done;
Alin: rebase issue, it should go to add_nic_done;
>+            }


>+        if (hvDelete && vport->isAbsentOnHv == FALSE) {
>+            switchContext->internalPortId = 0;
Alin: this is not needed removed in next version.
>+            switchContext->countInternalVports--;

Sai - Is this ASSERT really necessary? 
Alin - yes, the assert is needed for roleover

>+            ASSERT(switchContext->countInternalVports >= 0);
>+            OvsInternalAdapterDown(vport->portNo, 
>vport->netCfgInstanceId);

> -----Original Message-----
> From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Alin
> Serdean
> Sent: Saturday, August 13, 2016 12:24 AM
> To: Sairam Venugopal <vsairam at vmware.com>; dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v3 2/3] datapath-windows: Add multiple
> switch internal ports
> 
> Hi Sai,
> 
> Thanks a lot for the review!
> 
> Comments inlined.
> 
> Alin.
> 


More information about the dev mailing list