[ovs-dev] [PATCH v1] datapath-windows Event read handler

Eitan Eliahu eliahue at vmware.com
Thu Sep 25 05:46:32 UTC 2014


We planned to keep the legacy device code in tact until we make progress with the NL device. We will remove the legacy device soon. Yes, this compilation variable will help us to find these code segments.
Thank you,
Eitan

From: Alin Serdean [mailto:aserdean at cloudbasesolutions.com]
Sent: Wednesday, September 24, 2014 10:44 PM
To: Eitan Eliahu; dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler

None what so ever. Was just wondering if there was a hidden meaning to it that I missed :).

Alin.

De la: Eitan Eliahu [mailto:eliahue at vmware.com]
Trimis: Thursday, September 25, 2014 8:41 AM
Către: Alin Serdean; dev at openvswitch.org<mailto:dev at openvswitch.org>
Subiect: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler

Yes, I do. Do you see any issue with that?
Thanks,
Eitan

From: Alin Serdean [mailto:aserdean at cloudbasesolutions.com]
Sent: Wednesday, September 24, 2014 10:39 PM
To: Eitan Eliahu; dev at openvswitch.org<mailto:dev at openvswitch.org>
Subject: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler

It was not guarded until now so was wondering why guard it now :), unless that you want to use this as a mark for later on.

Alin.

De la: Eitan Eliahu [mailto:eliahue at vmware.com]
Trimis: Thursday, September 25, 2014 8:34 AM
Către: Alin Serdean; dev at openvswitch.org<mailto:dev at openvswitch.org>
Subiect: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler

Why this code you are talking about is different from the code which is already guarded? Both are for the legacy device so they need to be treated the same? Did I miss your point?
Thanks,
Eitan

From: Alin Serdean [mailto:aserdean at cloudbasesolutions.com]
Sent: Wednesday, September 24, 2014 10:22 PM
To: Eitan Eliahu; dev at openvswitch.org<mailto:dev at openvswitch.org>
Subject: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler


I am talking only about the following:

@@ -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 */



Not the code which is already guarded :).



Line 12: new blank line at EOF.

Seems like a blank line



I would prefer using something without disabling warnings.



Thank you,

Alin.



-----Mesaj original-----
De la: Eitan Eliahu [mailto:eliahue at vmware.com]
Trimis: Thursday, September 25, 2014 8:11 AM
Către: Alin Serdean; dev at openvswitch.org<mailto:dev at openvswitch.org>
Subiect: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler



If we remove OVS_USE_NL_INTERFACE the legacy dpif device should be active rather the NL device. Basically, we have all code which is used by the legacy device excluded by using this compilation flag. Are you suggesting that we will remove all thse code segments?



On the white space  I have " git diff --check" returns with no erros. I am not sure where is the white space.



The while(FALSE) style is used throughout the WinDDK code. But I will change it so we all be happy.



Thank you!

Eitan



-----Original Message-----

From: Alin Serdean [mailto:aserdean at cloudbasesolutions.com]

Sent: Wednesday, September 24, 2014 10:00 PM

To: Eitan Eliahu; dev at openvswitch.org<mailto:dev at openvswitch.org>

Subject: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler





-----Mesaj original-----

De la: Eitan Eliahu [mailto:eliahue at vmware.com]

Trimis: Thursday, September 25, 2014 7:48 AM

Către: Alin Serdean; dev at openvswitch.org<mailto:dev at openvswitch.org>

Subiect: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler



Alin, this warning is used to warn about constant in a condition. We shouldn't have these cases at all. There are other parts in the code which uses the while(FALSE) condition.

I do not think we should add more that is my opinion.



OVS_USE_NL_INTERFACE is used to exclude code which used for the dpif device. There are many code segments that excluded. I suggest that we will remove all the dpif code in a different change.

OvsPollEventIoctl is only called from OvsDeviceControl(Ioctl.c) so I do not see any other reason besides just raising awareness. Maybe there is some piece I missed.



OvsRemoveEventEntry is not exactly the opposite function of OvsPostEvent.

OvsPostEvent insert an element to the queue but also triggers the event through completing the event pending IRP.

You have a point here.



Please let me know your thoughts.

Thanks,

Eitan



-----Original Message-----

From: Alin Serdean [mailto:aserdean at cloudbasesolutions.com]

Sent: Wednesday, September 24, 2014 9:13 PM

To: Eitan Eliahu; dev at openvswitch.org<mailto:dev at openvswitch.org>

Subject: RE: [ovs-dev] [PATCH v1] datapath-windows Event read handler



1. ovs-dev-v1-datapath-windows-Event-read-handler.patch:12: new blank line at EOF.

+

warning: 1 line adds whitespace errors.



2. OvsPostEvent adds elements to the queue can you please find a similar name for OvsRemoveEventEntry



3. Really don't  like that we have to push in:

+#pragma warning( push )

+#pragma warning( disable:4127 )



Just for

do {

} while (FALSE)

Could you please rewrite that part.



4. At first glance adding:

#if defined OVS_USE_NL_INTERFACE && OVS_USE_NL_INTERFACE == 0 Seems unnecessary just to mark probably a code for deletion.



Alin.



-----Mesaj original-----

De la: dev [mailto:dev-bounces at openvswitch.org] În numele Eitan Eliahu

Trimis: Thursday, September 25, 2014 12:58 PM

Către: dev at openvswitch.org<mailto:dev at openvswitch.org>

Subiect: [ovs-dev] [PATCH v1] datapath-windows Event read handler



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 <eliahue at vmware.com<mailto:eliahue at vmware.com>>

---

build-aux/extract-odp-netlink-windows-dp-h   |   4 +

datapath-windows/include/OvsDpInterfaceExt.h |  10 +-

datapath-windows/ovsext/Datapath.c           | 162 ++++++++++++++++++++++++++-

datapath-windows/ovsext/Event.c              |  42 +++++++

datapath-windows/ovsext/Event.h              |   2 +

5 files changed, 217 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,/

+

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..fa8fa64 100644

--- a/datapath-windows/ovsext/Datapath.c

+++ b/datapath-windows/ovsext/Datapath.c

@@ -22,6 +22,9 @@

  */

#if defined OVS_USE_NL_INTERFACE && OVS_USE_NL_INTERFACE == 1



+#pragma warning( push )

+#pragma warning( disable:4127 )

+

#include "precomp.h"

#include "Datapath.h"

#include "Jhash.h"

@@ -99,7 +102,8 @@ static NetlinkCmdHandler OvsGetPidCmdHandler,

                          OvsGetDpCmdHandler,

                          OvsPendEventCmdHandler,

                          OvsSubscribeEventCmdHandler,

-                         OvsSetDpCmdHandler;

+                         OvsSetDpCmdHandler,

+                         OvsReadEventCmdHandler;



 static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT usrParamsCtx,

                                        UINT32 *replyLen); @@ -620,6 +624,30 @@ 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;

+        devOp = OVS_READ_DEV_OP;

+        break;

+

     case OVS_IOCTL_READ:

         /* Output buffer is mandatory. */

         if (outputBufferLen != 0) {

@@ -924,6 +952,7 @@ OvsPendEventCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,

     return status;

}



+

/*

  * --------------------------------------------------------------------------

  *  Handler for the subscription for the event queue @@ -1214,4 +1243,135 @@ 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;

+    msgOutTmp.nlMsg.nlmsgFlags = 0;  /* XXX: ? */

+    msgOutTmp.nlMsg.nlmsgSeq = 0; /* driver intiated messages should have */

+    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;

+    }

+    msgOutTmp.ovsHdr.dp_ifindex = gOvsSwitchContext->dpNo;

+

+    do {

+

+        rc = NlMsgPutHead(nlBuf, (PCHAR)&msgOutTmp, sizeof msgOutTmp);

+        if (!rc) {

+            status = STATUS_INVALID_BUFFER_SIZE;

+            break;

+        }

+

+        vport = OvsFindVportByPortNo(gOvsSwitchContext, eventEntry->portNo);

+        if (!vport) {

+            status = STATUS_DEVICE_DOES_NOT_EXIST;

+            break;

+        }

+

+        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;

+            break;

+        }

+

+        /* XXXX Should we add the port stats attributes?*/

+        nlMsg = (PNL_MSG_HDR)NlBufAt(nlBuf, 0, 0);

+        nlMsg->nlmsgLen = NlBufSize(nlBuf);

+        rc = STATUS_SUCCESS;

+    } while (FALSE);

+

+    return rc;

+}

+

+

+/*

+ *

+-----------------------------------------------------------------------

+---

+ * 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);

+

+    /* 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);;

+    return status;

+}

+

+#pragma warning( push )

#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 at openvswitch.org<mailto:dev at openvswitch.org>

https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=MU4xsiSuEqxx%2Bv4WfYKm0Cl0rVjWQbNQuqXGJ%2Bz04OE%3D%0A&s=7d8cff33d70e75c82739dbc6a9941f3d97e3d96992f62a4f516ed82eb49c99ba



More information about the dev mailing list