Eitan,
I get the following error when I try to apply this patch. Looks like there's a
new line in build-aux/extract-odp-netlink-windows-dp-h.
---
[nithin@pa-dbc1122 openvswitch-review]$ git am <
../tmp/ovs-dev-v3-datapath-windows-Event-read-handler.patch
Applying: datapath-windows Event read handler
/dbc/pa-dbc1122/nithin/nithin-ovs-int-1/openvswitch-review/.git/rebase-apply/patch:13:
new blank line at EOF.
+
warning: 1 line adds whitespace errors.
---
Also, I am not very convinced that we cannot reuse the READ_IOCTL, but I am ok
with your approach provided the end-to-end (including userspace code) looks
simpler overall. Maybe including the userspace code would make things clearer.
Other comments are minor.
Thanks for working on this esp. when not everything is ironed out :)
On Sep 26, 2014, at 5:03 AM, Eitan Eliahu <elia...@vmware.com>
wrote:
> The Read event handler is executed when user mode issues a socket receive on
> an MC socket associated with the event queue. A new IOCTL READ command is
> used to differentiate between transaction based and packet miss sockets.
> An entry for the handler will be added once the Vport family implementation
> checked in.
> User mode code for setting the socket type will follow
>
> Signed-off-by: Eitan Eliahu <elia...@vmware.com>
> ---
> build-aux/extract-odp-netlink-windows-dp-h | 4 +
> datapath-windows/include/OvsDpInterfaceExt.h | 10 +-
> datapath-windows/ovsext/Datapath.c | 150 ++++++++++++++++++++++++++-
> datapath-windows/ovsext/Event.c | 42 ++++++++
> datapath-windows/ovsext/Event.h | 2 +
> 5 files changed, 205 insertions(+), 3 deletions(-)
>
> diff --git a/build-aux/extract-odp-netlink-windows-dp-h
> b/build-aux/extract-odp-netlink-windows-dp-h
> index 70514cb..82d95f1 100755
> --- a/build-aux/extract-odp-netlink-windows-dp-h
> +++ b/build-aux/extract-odp-netlink-windows-dp-h
> @@ -23,3 +23,7 @@ s,#include <linux/if_ether\.h>,\n#ifndef ETH_ADDR_LEN \
>
> # Use OVS's own ETH_ADDR_LEN instead of Linux-specific ETH_ALEN.
> s/ETH_ALEN/ETH_ADDR_LEN/
> +
> +s/OVS_VPORT_CMD_GET,/OVS_VPORT_CMD_GET,\
> + OVS_VPORT_CMD_NOTIFY,/
Do we need to call it OVS_VPORT_CMD_NOTIFY? If we can call it with a generic
name such as OVS_CTRL_CMD_NOTIFY, we can make this as part of the command
extensions in OvsDpInterfaceExt.h. That way we cam avoid sneaking in commands
in the standard interface to the datapath.
> +
> diff --git a/datapath-windows/include/OvsDpInterfaceExt.h
> b/datapath-windows/include/OvsDpInterfaceExt.h
> index 64fd20e..be1e49f 100644
> --- a/datapath-windows/include/OvsDpInterfaceExt.h
> +++ b/datapath-windows/include/OvsDpInterfaceExt.h
> @@ -34,11 +34,17 @@
> #define OVS_IOCTL_READ \
> CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x0,
> METHOD_OUT_DIRECT,\
> FILE_READ_ACCESS)
> +#define OVS_IOCTL_READ_PACKET \
> + CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x1,
> METHOD_OUT_DIRECT, \
> + FILE_READ_ACCESS)
> +#define OVS_IOCTL_READ_EVENT \
> + CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x2,
> METHOD_OUT_DIRECT, \
> + FILE_READ_ACCESS)
> #define OVS_IOCTL_WRITE \
> - CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x1,
> METHOD_IN_DIRECT,\
> + CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x3,
> METHOD_IN_DIRECT,\
> FILE_READ_ACCESS)
> #define OVS_IOCTL_TRANSACT \
> - CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x2,
> METHOD_OUT_DIRECT,\
> + CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x4,
> METHOD_OUT_DIRECT,\
> FILE_WRITE_ACCESS)
>
> /*
> diff --git a/datapath-windows/ovsext/Datapath.c
> b/datapath-windows/ovsext/Datapath.c
> index 0dfdd57..3102e66 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -99,7 +99,8 @@ static NetlinkCmdHandler OvsGetPidCmdHandler,
> OvsGetDpCmdHandler,
> OvsPendEventCmdHandler,
> OvsSubscribeEventCmdHandler,
> - OvsSetDpCmdHandler;
> + OvsSetDpCmdHandler,
> + OvsReadEventCmdHandler;
The handler function does not seem to be specific to the VPORT event.
Also, I don't see this function listed as a handler for any of the commands. Is
that coming in a subsequent patch?
>
> static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> UINT32 *replyLen);
> @@ -620,6 +621,29 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> devOp = OVS_TRANSACTION_DEV_OP;
> break;
>
> + case OVS_IOCTL_READ_EVENT:
> + /* This IOCTL is used to read events */
> + if (outputBufferLen != 0) {
> + status = MapIrpOutputBuffer(irp, outputBufferLen,
> + sizeof *ovsMsg, &outputBuffer);
> + if (status != STATUS_SUCCESS) {
> + goto done;
> + }
> + ASSERT(outputBuffer);
> + } else {
> + status = STATUS_NDIS_INVALID_LENGTH;
> + goto done;
> + }
> + inputBuffer = NULL;
> + inputBufferLen = 0;
> +
> + ovsMsg = &ovsMsgReadOp;
> + ovsMsg->nlMsg.nlmsgType = OVS_WIN_NL_VPORT_FAMILY_ID;
> + /* An "artificial" command so we can use NL family function table*/
> + ovsMsg->genlMsg.cmd = OVS_VPORT_CMD_NOTIFY;
The other values in the OVS_MESSAGE are not being set. Is it not required? At
the minimum, the dpif index should be set I think.
> + devOp = OVS_READ_DEV_OP;
> + break;
> +
IMO, we should be able to read events using the OVS_IOCTL_READ operation
itself. I had implemented this for transactions using a write-read model, but
eventually did not send it out since we ended up using TRANSACTION ioctl. As
you can expect, the code looked something like this in the READ_IOCTL:
/* Check if there are any transaction replies queued up. */
if (!IsListEmpty(&instance->transactionReply)) {
/* Code to Send up the reply. */
goto done;
}
/* Check we are in the middle of a dump operation. */
if (instance->dumpState.ovsMsg == NULL) {
}
As you know, events, packets and dump operations are mutually exclusive. It is
straight forward to have some enforcements such as:
1. A descriptor that has subscribed to OVS_CTRL_CMD_MC_SUBSCRIBE_REQ cannot
start a dump operation.
2. While in the middle of a dump operation, a descriptor cannot call into:
OVS_CTRL_CMD_MC_SUBSCRIBE_REQ.
3. A descriptor that has subscribed to OVS_CTRL_CMD_MC_SUBSCRIBE_REQ cannot
receive packets.
4. A descriptor that has subscribed to receiving packets, cannot also have an
event queue associated with it using OVS_CTRL_CMD_MC_SUBSCRIBE_REQ.
All this said, I am ok with the changes you have made provided the end-to-end
(including userspace code) looks simpler overall. Maybe including the userspace
code would make things clearer.
> case OVS_IOCTL_READ:
> /* Output buffer is mandatory. */
> if (outputBufferLen != 0) {
> @@ -924,6 +948,7 @@ OvsPendEventCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
> return status;
> }
>
> +
> /*
> * --------------------------------------------------------------------------
> * Handler for the subscription for the event queue
> @@ -1214,4 +1239,127 @@ MapIrpOutputBuffer(PIRP irp,
> return STATUS_SUCCESS;
> }
>
> +/*
> + * --------------------------------------------------------------------------
> + * Utility function to fill up information about the state of a port in a
> reply
> + * to* userspace.
> + * Assumes that 'gOvsCtrlLock' lock is acquired.
> + * --------------------------------------------------------------------------
> + */
> +static NTSTATUS
> +OvsPortFillInfo(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> + POVS_EVENT_ENTRY eventEntry,
> + PNL_BUFFER nlBuf)
> +{
> + NTSTATUS status;
> + BOOLEAN rc;
> + OVS_MESSAGE msgOutTmp;
> + PNL_MSG_HDR nlMsg;
> + POVS_VPORT_ENTRY vport;
> +
> + ASSERT(NlBufAt(nlBuf, 0, 0) != 0 && nlBuf->bufRemLen >= sizeof
> msgOutTmp);
> +
> + msgOutTmp.nlMsg.nlmsgType = OVS_WIN_NL_VPORT_FAMILY_ID;
Do we want to include this as part of the VPORT_FAMILY? IMO, this should be the
OVS_WIN_NL_CTRL_FAMILY_ID since event subscribe etc commands are on the
OVS_WIN_CONTROL_FAMILY family.
> + msgOutTmp.nlMsg.nlmsgFlags = 0; /* XXX: ? */
> + msgOutTmp.nlMsg.nlmsgSeq = 0; /* driver intiated messages should have */
The comment seems incomplete.
> + msgOutTmp.nlMsg.nlmsgPid = usrParamsCtx->ovsInstance->pid;
> +
> + msgOutTmp.genlMsg.version = nlVportFamilyOps.version;
> + 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)) {
> + msgOutTmp.genlMsg.cmd = OVS_VPORT_CMD_NEW;
> + } else if (eventEntry->status &
> + (OVS_EVENT_LINK_DOWN | OVS_EVENT_DISCONNECT)) {
> + msgOutTmp.genlMsg.cmd = OVS_VPORT_CMD_DEL;
> + } else {
> + ASSERT(FALSE);
> + return STATUS_UNSUCCESSFUL;
> + }
I am not sure how the end behavior would be here if userspace sees a
OVS_VPORT_CMD_DEL, but sees the vport in vport dump. I agree that once we have
netdev, userspace can have more useful information at hand to detect changes.
Until then, I think we should just ignore LINK_UP and LINK_DOWN and not treat
them as CMD_NEW and CMD_DEL respectively. Esp. given that userspace has no way
to detect the link state today. In other words, we should just post
OVS_EVENT_CONNECT as OVS_VPORT_CMD_NEW, and OVS_EVENT_DISCONNECT as
OVS_VPORT_CMD_DEL and leave it at that. OVS_EVENT_LINK_UP and
OVS_EVENT_LINK_DOWN can be ignored, and no events should be posted.
> + msgOutTmp.ovsHdr.dp_ifindex = gOvsSwitchContext->dpNo;
> +
> + rc = NlMsgPutHead(nlBuf, (PCHAR)&msgOutTmp, sizeof msgOutTmp);
> + if (!rc) {
> + status = STATUS_INVALID_BUFFER_SIZE;
> + goto cleanup;
> + }
> +
> + vport = OvsFindVportByPortNo(gOvsSwitchContext, eventEntry->portNo);
> + if (!vport) {
> + status = STATUS_DEVICE_DOES_NOT_EXIST;
> + goto cleanup;
> + }
> +
> + rc = NlMsgPutTailU32(nlBuf, OVS_VPORT_ATTR_PORT_NO, eventEntry->portNo)
> ||
> + NlMsgPutTailU32(nlBuf, OVS_VPORT_ATTR_TYPE, vport->ovsType) ||
> + NlMsgPutTailString(nlBuf, OVS_VPORT_ATTR_NAME, vport->ovsName);
> + if (!rc) {
> + status = STATUS_INVALID_BUFFER_SIZE;
> + goto cleanup;
> + }
> +
> + /* XXXX Should we add the port stats attributes?*/
> + nlMsg = (PNL_MSG_HDR)NlBufAt(nlBuf, 0, 0);
> + nlMsg->nlmsgLen = NlBufSize(nlBuf);
> + status = STATUS_SUCCESS;
> +
> +cleanup:
> + return status;
> +}
> +
> +
> +/*
> + * --------------------------------------------------------------------------
> + * Handler for reading events from the driver event queue. This handler is
> + * executed when user modes issues a socket receive on a socket assocaited
> + * with the MC group for events.
> + * XXX user mode should read multiple events in one system call
> + * --------------------------------------------------------------------------
> + */
> +static NTSTATUS
> +OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> + UINT32 *replyLen)
> +{
> +#ifdef DBG
> + POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
> + POVS_OPEN_INSTANCE instance =
> + (POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance;
> +#endif
> + NL_BUFFER nlBuf;
> + NTSTATUS status;
> + OVS_EVENT_ENTRY eventEntry;
> +
> + ASSERT(usrParamsCtx->devOp == OVS_READ_DEV_OP);
> +
> + /* Should never read events with a dump socket */
> + ASSERT(instance->dumpState.ovsMsg == NULL);
Cool, you have this enforcement. We reuse this and demultiplex the READ_IOCTL.
> +
> + /* Must have an event queue */
> + ASSERT(instance->eventQueue != NULL);
> +
> + /* Output buffer has been validated while validating read dev op. */
> + ASSERT(msgOut != NULL && usrParamsCtx->outputLength >= sizeof *msgOut);
> +
> + NlBufInit(&nlBuf, usrParamsCtx->outputBuffer,
> usrParamsCtx->outputLength);
> +
> + OvsAcquireCtrlLock();
> + if (!gOvsSwitchContext) {
> + status = STATUS_SUCCESS;
> + goto cleanup;
> + }
> +
> + /* remove an event entry from the event queue */
> + status = OvsRemoveEventEntry(usrParamsCtx->ovsInstance, &eventEntry);
> + if (status != STATUS_SUCCESS) {
> + goto cleanup;
> + }
> +
> + status = OvsPortFillInfo(usrParamsCtx, &eventEntry, &nlBuf);
> +
> +cleanup:
> + OvsReleaseCtrlLock();
> + *replyLen = NlBufSize(&nlBuf);;
If there's a failure, there's a chance that *replyLen will be set to a value >
0. This might be OK, but most of the other code sets *replyLen to a value > 0,
only when there's success.
> + return status;
> +}
> #endif /* OVS_USE_NL_INTERFACE */
> diff --git a/datapath-windows/ovsext/Event.c b/datapath-windows/ovsext/Event.c
> index fec3485..467771d 100644
> --- a/datapath-windows/ovsext/Event.c
> +++ b/datapath-windows/ovsext/Event.c
> @@ -292,6 +292,7 @@ done_event_subscribe:
> return status;
> }
>
> +#if defined OVS_USE_NL_INTERFACE && OVS_USE_NL_INTERFACE == 0
> /*
> * --------------------------------------------------------------------------
> * Poll event queued in the event queue. always synchronous.
> @@ -376,6 +377,7 @@ event_poll_done:
> OVS_LOG_TRACE("Exit: numEventPolled: %d", numEntry);
> return STATUS_SUCCESS;
> }
> +#endif /* OVS_USE_NL_INTERFACE */
>
>
> /*
> @@ -494,3 +496,43 @@ OvsWaitEventIoctl(PIRP irp,
> OVS_LOG_TRACE("Exit: return status: %#x", status);
> return status;
> }
> +
> +/*
> + *--------------------------------------------------------------------------
> + * Poll event queued in the event queue.always synchronous.
> + *
> + * Results:
> + * STATUS_SUCCESS event was dequeued
> + * STATUS_UNSUCCESSFUL the queue is empty.
> + * --------------------------------------------------------------------------
> + */
> +NTSTATUS
> +OvsRemoveEventEntry(POVS_OPEN_INSTANCE instance,
> + POVS_EVENT_ENTRY entry)
> +{
> + NTSTATUS status = STATUS_UNSUCCESSFUL;
> + POVS_EVENT_QUEUE queue;
> + POVS_EVENT_QUEUE_ELEM elem;
> +
> + OvsAcquireEventQueueLock();
> +
> + queue = (POVS_EVENT_QUEUE)instance->eventQueue;
> +
> + if (queue == NULL) {
> + ASSERT(queue);
> + goto remove_event_done;
> + }
> +
> + if (queue->numElems) {
> + elem = (POVS_EVENT_QUEUE_ELEM)RemoveHeadList(&queue->elemList);
> + entry->portNo = elem->portNo;
> + entry->status = elem->status;
> + OvsFreeMemory(elem);
> + queue->numElems--;
> + status = STATUS_SUCCESS;
> + }
> +
> +remove_event_done:
> + OvsReleaseEventQueueLock();
> + return status;
> +}
> diff --git a/datapath-windows/ovsext/Event.h b/datapath-windows/ovsext/Event.h
> index f4801b9..a43a0bb 100644
> --- a/datapath-windows/ovsext/Event.h
> +++ b/datapath-windows/ovsext/Event.h
> @@ -47,4 +47,6 @@ NTSTATUS OvsPollEventIoctl(PFILE_OBJECT fileObject, PVOID
> inputBuffer,
> UINT32 outputLength, UINT32 *replyLen);
> NTSTATUS OvsWaitEventIoctl(PIRP irp, PFILE_OBJECT fileObject,
> PVOID inputBuffer, UINT32 inputLength);
> +NTSTATUS OvsRemoveEventEntry(PVOID instance, POVS_EVENT_ENTRY entry);
> +
> #endif /* __EVENT_H_ */
> --
> 1.9.4.msysgit.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=ubrOpIWavCMqX4l4j1LEVpTfDj%2FD5Qyn8KCoJIBGvzo%3D%0A&m=D32NoB29antmSYUS0H7v1fJSyW72eHVWreqXKIjegmM%3D%0A&s=2f507d5bb59ce07b57300f7259baa0d2159b4015c2fbb81677205ddbadb85ed1
Thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
