hi Sorin,
I had forgotten that we ref count ‘gOvsSwitchContext’ now, and I agree that the
ref count implementation you have is more efficient than the spin lock we used
previously. I just found a minor bug for which I sent a fix.
This patch itself looks good but for a couple of minor comments.
I’ll ack this now. You can address the comments and send out a v3.
Acked-by: Nithin Raju <nit...@vmware.com>
> On Apr 15, 2015, at 1:28 AM, Sorin Vinturis
> <svintu...@cloudbasesolutions.com> wrote:
>
> There is no need to use gOvsCtrlLock spinlock to guard the switch
> context, as there is now the switch context's reference count used
> for this purpose.
>
> Now the gOvsCtrlLock spinlock guards only one shared resource, the
> OVS_OPEN_INSTANCE global instance array.
>
> v2: Atomically test and set if the instance is in attach process, to protect
> filter attach routine against multiple calls.
> - NdisAcquireSpinLock(gOvsCtrlLock);
> if (gOvsSwitchContext) {
> - NdisReleaseSpinLock(gOvsCtrlLock);
> OVS_LOG_TRACE("Exit: Failed to create OVS Switch, only one datapath
> is"
> "supported, %p.", gOvsSwitchContext);
> goto cleanup;
> }
> - if (gOvsInAttach) {
> - NdisReleaseSpinLock(gOvsCtrlLock);
> +
> + if (InterlockedCompareExchange((LONG volatile *)&gOvsInAttach, 1, 0)) {
Pls. make ‘gOvsSwitchContext’ a volatile LONG. It is currently BOOLEAN.
> NTSTATUS
> @@ -163,12 +161,6 @@ OvsSubscribeDpIoctl(PVOID instanceP,
> if (instance->packetQueue && !join) {
> /* unsubscribe */
> OvsCleanupPacketQueue(instance);
> -
> - OvsAcquirePidHashLock();
> - /* Remove the instance from pidHashArray */
> - OvsDelPidInstance(gOvsSwitchContext, pid);
> - OvsReleasePidHashLock();
This seems to be an unrelated change. Should be removed from this patch.
thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
