Acked-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com>
> -----Mesaj original----- > De la: dev [mailto:dev-boun...@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 <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 > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
