Acked-by: Sairam Venugopal <vsai...@vmware.com>
On 11/18/15, 8:13 AM, "Nithin Raju" <nit...@vmware.com> wrote: >Turns out that we don't need to generate an event is practically >useful only in case of a port disconnect to let userspace know. >Hence, this event is being posted from HvDisconnectNic(). > >In case of a new port appearing, it seems that userspace is not >interested in a new port unless it was added by userspace itself. >In my tests, userspce would end up deleting the port when it got >a new port notification, despite the port existing in OVSDB. > >The reasoning seems simple enough: >- On Linux, OVS is integrated with the hypervisor (libvirt for eg) >and a port (ie. netdev) gets created in the Linux kernel and then >get added to OVSDB. When vswitchd picks up the port addition in OVSDB, >it adds the port in the OVS kernel DP. >- If the kernel netdev does not exist while OVS userspace tries to >create the port in OVS kernel DP, port addition fails. Moreover, the >only way to re-add the port is to trigger userspace to re-add the port >by deleting the port in OVSDB and re-adding it. > >With this patch, I have verified that if a VIF gets disconnected on the >Hyper-V switch, it disappears from the OVS kernel DP as well. > >Signed-off-by: Nithin Raju <nit...@vmware.com> >--- > datapath-windows/ovsext/Datapath.c | 18 ++----- > datapath-windows/ovsext/DpInternal.h | 7 ++- > datapath-windows/ovsext/Event.c | 97 >+++++++++++++----------------------- > datapath-windows/ovsext/Event.h | 5 +- > datapath-windows/ovsext/Switch.c | 2 +- > datapath-windows/ovsext/Vport.c | 65 +++++++++++------------- > 6 files changed, 79 insertions(+), 115 deletions(-) > >diff --git a/datapath-windows/ovsext/Datapath.c >b/datapath-windows/ovsext/Datapath.c >index b3dbd71..a9a218d 100644 >--- a/datapath-windows/ovsext/Datapath.c >+++ b/datapath-windows/ovsext/Datapath.c >@@ -1497,7 +1497,6 @@ OvsPortFillInfo(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > BOOLEAN ok; > OVS_MESSAGE msgOutTmp; > PNL_MSG_HDR nlMsg; >- POVS_VPORT_ENTRY vport; > > ASSERT(NlBufAt(nlBuf, 0, 0) != 0 && nlBuf->bufRemLen >= sizeof >msgOutTmp); > >@@ -1512,9 +1511,9 @@ OvsPortFillInfo(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > msgOutTmp.genlMsg.reserved = 0; > > /* we don't have netdev yet, treat link up/down a adding/removing a >port*/ >- if (eventEntry->status & (OVS_EVENT_LINK_UP | OVS_EVENT_CONNECT)) { >+ if (eventEntry->type & (OVS_EVENT_LINK_UP | OVS_EVENT_CONNECT)) { > msgOutTmp.genlMsg.cmd = OVS_VPORT_CMD_NEW; >- } else if (eventEntry->status & >+ } else if (eventEntry->type & > (OVS_EVENT_LINK_DOWN | OVS_EVENT_DISCONNECT)) { > msgOutTmp.genlMsg.cmd = OVS_VPORT_CMD_DEL; > } else { >@@ -1529,17 +1528,11 @@ OvsPortFillInfo(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > goto cleanup; > } > >- vport = OvsFindVportByPortNo(gOvsSwitchContext, eventEntry->portNo); >- if (!vport) { >- status = STATUS_DEVICE_DOES_NOT_EXIST; >- goto cleanup; >- } >- > ok = NlMsgPutTailU32(nlBuf, OVS_VPORT_ATTR_PORT_NO, >eventEntry->portNo) && >- NlMsgPutTailU32(nlBuf, OVS_VPORT_ATTR_TYPE, vport->ovsType) && >+ NlMsgPutTailU32(nlBuf, OVS_VPORT_ATTR_TYPE, >eventEntry->ovsType) && > NlMsgPutTailU32(nlBuf, OVS_VPORT_ATTR_UPCALL_PID, >- vport->upcallPid) && >- NlMsgPutTailString(nlBuf, OVS_VPORT_ATTR_NAME, vport->ovsName); >+ eventEntry->upcallPid) && >+ NlMsgPutTailString(nlBuf, OVS_VPORT_ATTR_NAME, >eventEntry->ovsName); > if (!ok) { > status = STATUS_INVALID_BUFFER_SIZE; > goto cleanup; >@@ -1606,4 +1599,3 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > cleanup: > return status; > } >- >diff --git a/datapath-windows/ovsext/DpInternal.h >b/datapath-windows/ovsext/DpInternal.h >index 8de48a2..4b58ae8 100644 >--- a/datapath-windows/ovsext/DpInternal.h >+++ b/datapath-windows/ovsext/DpInternal.h >@@ -287,8 +287,11 @@ enum { > > > typedef struct _OVS_EVENT_ENTRY { >- uint32_t portNo; >- uint32_t status; >+ UINT32 portNo; >+ OVS_VPORT_TYPE ovsType; >+ UINT32 upcallPid; >+ CHAR ovsName[OVS_MAX_PORT_NAME_LENGTH]; >+ UINT32 type; > } OVS_EVENT_ENTRY, *POVS_EVENT_ENTRY; > > #define OVS_DEFAULT_PORT_NO 0xffffffff >diff --git a/datapath-windows/ovsext/Event.c >b/datapath-windows/ovsext/Event.c >index cca9575..c210da3 100644 >--- a/datapath-windows/ovsext/Event.c >+++ b/datapath-windows/ovsext/Event.c >@@ -31,7 +31,6 @@ > LIST_ENTRY ovsEventQueue; > static NDIS_SPIN_LOCK eventQueueLock; > UINT32 ovsNumEventQueue; >-UINT32 ovsNumPollAll; > > NTSTATUS > OvsInitEventQueue() >@@ -112,57 +111,37 @@ OvsCleanupEvent(POVS_OPEN_INSTANCE instance) > * >-------------------------------------------------------------------------- > */ > VOID >-OvsPostEvent(UINT32 portNo, >- UINT32 status) >+OvsPostEvent(POVS_EVENT_ENTRY event) > { > POVS_EVENT_QUEUE_ELEM elem; > POVS_EVENT_QUEUE queue; > PLIST_ENTRY link; >- BOOLEAN triggerPollAll = FALSE; > LIST_ENTRY list; >- PLIST_ENTRY entry; >+ PLIST_ENTRY entry; > PIRP irp; > > InitializeListHead(&list); > >- OVS_LOG_TRACE("Enter: portNo: %#x, status: %#x", portNo, status); >+ OVS_LOG_TRACE("Enter: portNo: %#x, status: %#x", event->portNo, >+ event->type); > > OvsAcquireEventQueueLock(); > > LIST_FORALL(&ovsEventQueue, link) { > queue = CONTAINING_RECORD(link, OVS_EVENT_QUEUE, queueLink); >- if ((status & queue->mask) == 0 || >- queue->pollAll) { >+ if ((event->type & queue->mask) == 0) { > continue; > } >- if (queue->numElems > (OVS_MAX_VPORT_ARRAY_SIZE >> 1) || >- portNo == OVS_DEFAULT_PORT_NO) { >- queue->pollAll = TRUE; >- } else { >- elem = (POVS_EVENT_QUEUE_ELEM)OvsAllocateMemoryWithTag( >- sizeof(*elem), OVS_EVENT_POOL_TAG); >- if (elem == NULL) { >- queue->pollAll = TRUE; >- } else { >- elem->portNo = portNo; >- elem->status = (status & queue->mask); >- InsertTailList(&queue->elemList, &elem->link); >- queue->numElems++; >- OVS_LOG_INFO("Queue: %p, numElems: %d", >- queue, queue->numElems); >- } >- } >- if (queue->pollAll) { >- PLIST_ENTRY curr, next; >- triggerPollAll = TRUE; >- ovsNumPollAll++; >- LIST_FORALL_SAFE(&queue->elemList, curr, next) { >- RemoveEntryList(curr); >- elem = CONTAINING_RECORD(curr, OVS_EVENT_QUEUE_ELEM, >link); >- OvsFreeMemoryWithTag(elem, OVS_EVENT_POOL_TAG); >- } >- queue->numElems = 0; >- } >+ event->type &= queue->mask; >+ >+ elem = (POVS_EVENT_QUEUE_ELEM)OvsAllocateMemoryWithTag( >+ sizeof(*elem), OVS_EVENT_POOL_TAG); >+ RtlCopyMemory(&elem->event, event, sizeof elem->event); >+ InsertTailList(&queue->elemList, &elem->link); >+ queue->numElems++; >+ OVS_LOG_INFO("Queue: %p, numElems: %d", >+ queue, queue->numElems); >+ > if (queue->pendingIrp != NULL) { > PDRIVER_CANCEL cancelRoutine; > irp = queue->pendingIrp; >@@ -180,8 +159,6 @@ OvsPostEvent(UINT32 portNo, > OVS_LOG_INFO("Wakeup thread with IRP: %p", irp); > OvsCompleteIrpRequest(irp, 0, STATUS_SUCCESS); > } >- OVS_LOG_TRACE("Exit: triggered pollAll: %s", >- (triggerPollAll ? "TRUE" : "FALSE")); > } > > >@@ -255,7 +232,6 @@ OvsSubscribeEventIoctl(PFILE_OBJECT fileObject, > queue->mask = request->mask; > queue->pendingIrp = NULL; > queue->numElems = 0; >- queue->pollAll = TRUE; /* always poll all in the begining */ > InsertHeadList(&ovsEventQueue, &queue->queueLink); > ovsNumEventQueue++; > instance->eventQueue = queue; >@@ -360,11 +336,13 @@ OvsWaitEventIoctl(PIRP irp, > PVOID inputBuffer, > UINT32 inputLength) > { >- NTSTATUS status; >+ NTSTATUS status = STATUS_SUCCESS; > POVS_EVENT_POLL poll; > POVS_EVENT_QUEUE queue; > POVS_OPEN_INSTANCE instance; > BOOLEAN cancelled = FALSE; >+ PDRIVER_CANCEL cancelRoutine; >+ > OVS_LOG_TRACE("Enter: inputLength: %u", inputLength); > > if (inputLength < sizeof (OVS_EVENT_POLL)) { >@@ -377,38 +355,36 @@ OvsWaitEventIoctl(PIRP irp, > > instance = OvsGetOpenInstance(fileObject, poll->dpNo); > if (instance == NULL) { >- OvsReleaseEventQueueLock(); >- OVS_LOG_TRACE("Exit: Can not find open instance, dpNo: %d", >poll->dpNo); >+ OVS_LOG_TRACE("Exit: Can not find open instance, dpNo: %d", >+ poll->dpNo); > return STATUS_INVALID_PARAMETER; > } > > queue = (POVS_EVENT_QUEUE)instance->eventQueue; > if (queue == NULL) { >- OvsReleaseEventQueueLock(); > OVS_LOG_TRACE("Exit: Event queue does not exist"); >- return STATUS_INVALID_PARAMETER; >+ status = STATUS_INVALID_PARAMETER; >+ goto unlock; > } > if (queue->pendingIrp) { >- OvsReleaseEventQueueLock(); > OVS_LOG_TRACE("Exit: Event queue already in pending state"); >- return STATUS_DEVICE_BUSY; >+ status = STATUS_DEVICE_BUSY; >+ goto unlock; > } > >- status = (queue->numElems != 0 || queue->pollAll) ? >- STATUS_SUCCESS : STATUS_PENDING; >- if (status == STATUS_PENDING) { >- PDRIVER_CANCEL cancelRoutine; >- IoMarkIrpPending(irp); >- IoSetCancelRoutine(irp, OvsCancelIrp); >- if (irp->Cancel) { >- cancelRoutine = IoSetCancelRoutine(irp, NULL); >- if (cancelRoutine) { >- cancelled = TRUE; >- } >- } else { >- queue->pendingIrp = irp; >+ IoMarkIrpPending(irp); >+ IoSetCancelRoutine(irp, OvsCancelIrp); >+ if (irp->Cancel) { >+ cancelRoutine = IoSetCancelRoutine(irp, NULL); >+ if (cancelRoutine) { >+ cancelled = TRUE; > } >+ } else { >+ queue->pendingIrp = irp; >+ status = STATUS_PENDING; > } >+ >+unlock: > OvsReleaseEventQueueLock(); > if (cancelled) { > OvsCompleteIrpRequest(irp, 0, STATUS_CANCELLED); >@@ -446,8 +422,7 @@ OvsRemoveEventEntry(POVS_OPEN_INSTANCE instance, > > if (queue->numElems) { > elem = (POVS_EVENT_QUEUE_ELEM)RemoveHeadList(&queue->elemList); >- entry->portNo = elem->portNo; >- entry->status = elem->status; >+ *entry = elem->event; > OvsFreeMemoryWithTag(elem, OVS_EVENT_POOL_TAG); > queue->numElems--; > status = STATUS_SUCCESS; >diff --git a/datapath-windows/ovsext/Event.h >b/datapath-windows/ovsext/Event.h >index a43a0bb..b087875 100644 >--- a/datapath-windows/ovsext/Event.h >+++ b/datapath-windows/ovsext/Event.h >@@ -19,8 +19,7 @@ > > typedef struct _OVS_EVENT_QUEUE_ELEM { > LIST_ENTRY link; >- UINT32 portNo; >- UINT32 status; >+ OVS_EVENT_ENTRY event; > } OVS_EVENT_QUEUE_ELEM, *POVS_EVENT_QUEUE_ELEM; > > typedef struct _OVS_EVENT_QUEUE { >@@ -39,7 +38,7 @@ VOID OvsCleanupEventQueue(VOID); > struct _OVS_OPEN_INSTANCE; > > VOID OvsCleanupEvent(struct _OVS_OPEN_INSTANCE *instance); >-VOID OvsPostEvent(UINT32 portNo, UINT32 status); >+VOID OvsPostEvent(POVS_EVENT_ENTRY event); > NTSTATUS OvsSubscribeEventIoctl(PFILE_OBJECT fileObject, PVOID >inputBuffer, > UINT32 inputLength); > NTSTATUS OvsPollEventIoctl(PFILE_OBJECT fileObject, PVOID inputBuffer, >diff --git a/datapath-windows/ovsext/Switch.c >b/datapath-windows/ovsext/Switch.c >index a783ea1..6ddf5dc 100644 >--- a/datapath-windows/ovsext/Switch.c >+++ b/datapath-windows/ovsext/Switch.c >@@ -564,7 +564,7 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext) > OvsClearAllSwitchVports(switchContext); > goto cleanup; > } >- OvsPostEvent(OVS_DEFAULT_PORT_NO, OVS_DEFAULT_EVENT_STATUS); >+ // OvsPostEvent(OVS_DEFAULT_PORT_NO, OVS_DEFAULT_EVENT_STATUS); > > cleanup: > if (status != NDIS_STATUS_SUCCESS) { >diff --git a/datapath-windows/ovsext/Vport.c >b/datapath-windows/ovsext/Vport.c >index 69e9513..48845da 100644 >--- a/datapath-windows/ovsext/Vport.c >+++ b/datapath-windows/ovsext/Vport.c >@@ -301,6 +301,8 @@ HvDeletePort(POVS_SWITCH_CONTEXT switchContext, > /* > * >-------------------------------------------------------------------------- > * Function to process addition of a NIC connection on the Hyper-V >switch. >+ * XXX: Posting an event to DPIF is incorrect here. However, it might be >useful >+ * to post an event to netdev-windows.c. > * >-------------------------------------------------------------------------- > */ > NDIS_STATUS >@@ -308,8 +310,6 @@ HvCreateNic(POVS_SWITCH_CONTEXT switchContext, > PNDIS_SWITCH_NIC_PARAMETERS nicParam) > { > POVS_VPORT_ENTRY vport; >- UINT32 portNo = 0; >- UINT32 event = 0; > NDIS_STATUS status = NDIS_STATUS_SUCCESS; > > LOCK_STATE_EX lockState; >@@ -389,13 +389,6 @@ HvCreateNic(POVS_SWITCH_CONTEXT switchContext, > AssignNicNameSpecial(vport); > } > >- portNo = vport->portNo; >- if (vport->ovsState == OVS_STATE_CONNECTED) { >- event = OVS_EVENT_CONNECT | OVS_EVENT_LINK_UP; >- } else if (vport->ovsState == OVS_STATE_NIC_CREATED) { >- event = OVS_EVENT_CONNECT; >- } >- > add_nic_done: > NdisReleaseRWLock(switchContext->dispatchLock, &lockState); > if (status == STATUS_SUCCESS && >@@ -404,9 +397,6 @@ add_nic_done: > nicParam->NicIndex != 0))) { > AssignNicNameSpecial(vport); > } >- if (portNo != OVS_DPPORT_NUMBER_INVALID && event) { >- OvsPostEvent(portNo, event); >- } > > done: > VPORT_NIC_EXIT(nicParam); >@@ -426,7 +416,7 @@ HvConnectNic(POVS_SWITCH_CONTEXT switchContext, > { > LOCK_STATE_EX lockState; > POVS_VPORT_ENTRY vport; >- UINT32 portNo = 0; >+ UINT32 portNo; > > VPORT_NIC_ENTER(nicParam); > >@@ -456,9 +446,6 @@ HvConnectNic(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) { > OvsInternalAdapterUp(portNo, &nicParam->NetCfgInstanceId); > } >@@ -479,8 +466,7 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext, > { > POVS_VPORT_ENTRY vport; > LOCK_STATE_EX lockState; >- >- UINT32 status = 0, portNo = 0; >+ UINT32 event = 0; > > VPORT_NIC_ENTER(nicParam); > >@@ -512,7 +498,7 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext, > case NdisSwitchNicTypeEmulated: > if (!RtlEqualMemory(vport->vmMacAddress, nicParam->VMMacAddress, > sizeof (vport->vmMacAddress))) { >- status |= OVS_EVENT_MAC_CHANGE; >+ event |= OVS_EVENT_MAC_CHANGE; > RtlCopyMemory(vport->vmMacAddress, nicParam->VMMacAddress, > sizeof (vport->vmMacAddress)); > } >@@ -524,26 +510,31 @@ HvUpdateNic(POVS_SWITCH_CONTEXT switchContext, > sizeof (vport->permMacAddress))) { > RtlCopyMemory(vport->permMacAddress, >nicParam->PermanentMacAddress, > sizeof (vport->permMacAddress)); >- status |= OVS_EVENT_MAC_CHANGE; >+ event |= OVS_EVENT_MAC_CHANGE; > } > if (!RtlEqualMemory(vport->currMacAddress, >nicParam->CurrentMacAddress, > sizeof (vport->currMacAddress))) { > RtlCopyMemory(vport->currMacAddress, nicParam->CurrentMacAddress, > sizeof (vport->currMacAddress)); >- status |= OVS_EVENT_MAC_CHANGE; >+ event |= OVS_EVENT_MAC_CHANGE; > } > > if (vport->mtu != nicParam->MTU) { > vport->mtu = nicParam->MTU; >- status |= OVS_EVENT_MTU_CHANGE; >+ event |= OVS_EVENT_MTU_CHANGE; > } > vport->numaNodeId = nicParam->NumaNodeId; >- portNo = vport->portNo; > > NdisReleaseRWLock(switchContext->dispatchLock, &lockState); >- if (status && portNo) { >- OvsPostEvent(portNo, status); >- } >+ >+ /* >+ * XXX: Not sure what kind of event to post here. DPIF is not >interested in >+ * changes to MAC address. Netdev-windows might be intrested, though. >+ * That said, if the name chagnes, not clear what kind of event to be >+ * posted. We might have to delete the vport, and have userspace >recreate >+ * it. >+ */ >+ > update_nic_done: > VPORT_NIC_EXIT(nicParam); > } >@@ -558,9 +549,9 @@ HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext, > PNDIS_SWITCH_NIC_PARAMETERS nicParam) > { > POVS_VPORT_ENTRY vport; >- UINT32 portNo = 0; > LOCK_STATE_EX lockState; > BOOLEAN isInternalPort = FALSE; >+ OVS_EVENT_ENTRY event; > > VPORT_NIC_ENTER(nicParam); > >@@ -585,16 +576,25 @@ HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext, > > vport->nicState = NdisSwitchNicStateDisconnected; > vport->ovsState = OVS_STATE_NIC_CREATED; >- portNo = vport->portNo; > > if (vport->ovsType == OVS_VPORT_TYPE_INTERNAL) { > isInternalPort = TRUE; > } > >+ event.portNo = vport->portNo; >+ event.ovsType = vport->ovsType; >+ event.upcallPid = vport->upcallPid; >+ RtlCopyMemory(&event.ovsName, &vport->ovsName, sizeof event.ovsName); >+ event.type = OVS_EVENT_LINK_DOWN; >+ > NdisReleaseRWLock(switchContext->dispatchLock, &lockState); > >- /* XXX if portNo != INVALID or always? */ >- OvsPostEvent(portNo, OVS_EVENT_LINK_DOWN); >+ /* >+ * Delete the port from the hash tables accessible to userspace. >After this >+ * point, userspace should not be able to access this port. >+ */ >+ OvsRemoveAndDeleteVport(NULL, switchContext, vport, FALSE, TRUE); >+ OvsPostEvent(&event); > > if (isInternalPort) { > OvsInternalAdapterDown(); >@@ -615,7 +615,6 @@ HvDeleteNic(POVS_SWITCH_CONTEXT switchContext, > { > LOCK_STATE_EX lockState; > POVS_VPORT_ENTRY vport; >- UINT32 portNo = 0; > > VPORT_NIC_ENTER(nicParam); > /* Wait for lists to be initialized. */ >@@ -640,21 +639,17 @@ HvDeleteNic(POVS_SWITCH_CONTEXT switchContext, > vport->nicState = NdisSwitchNicStateUnknown; > vport->ovsState = OVS_STATE_PORT_CREATED; > >- portNo = vport->portNo; > if (vport->portType == NdisSwitchPortTypeExternal && > vport->nicIndex != 0) { > OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, FALSE); > } > > NdisReleaseRWLock(switchContext->dispatchLock, &lockState); >- /* XXX if portNo != INVALID or always? */ >- OvsPostEvent(portNo, OVS_EVENT_DISCONNECT); > > done: > VPORT_NIC_EXIT(nicParam); > } > >- > /* > * OVS Vport related functionality. > */ >-- >1.8.5.6 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma >n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc >ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=1jjlB9liar4XJYcgk6bPbix0jYPc4Y >idkOASwGN9MUc&s=-xxYGtfB93ExNKHdTSOPXOw19R00_zryOriRuVJLq-U&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev