| Message ID | 1447863219-20084-4-git-send-email-nithin@vmware.com |
|---|---|
| State | Superseded |
| Headers | show |
Acked-by: Sairam Venugopal <vsairam@vmware.com> On 11/18/15, 8:13 AM, "Nithin Raju" <nithin@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 <nithin@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=
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > -----Mesaj original----- > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Nithin Raju > Trimis: Wednesday, November 18, 2015 6:14 PM > Către: dev@openvswitch.org > Subiect: [ovs-dev] [PATCH 3/6 v2] datapath-windows: cleanup events code > > 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 <nithin@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 > http://openvswitch.org/mailman/listinfo/dev
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. */
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 <nithin@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(-)