datapath-windows: Make GET_PID a separate IOCTL

Added a new IOCTL in order to retrieve the PID from the kernel datapath.
The new method uses a direct and cleaner way, as opposed to the old way
of using a Netlink transaction, avoiding the unnecessary overhead.

Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/31
Acked-by: Nithin Raju <nithin@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Tested-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

4 files changed, 57 insertions, 82 deletions

diff --git a/datapath-windows/include/OvsDpInterfaceExt.h b/datapath-windows/include/OvsDpInterfaceExt.h
index 7e09caf77..e2353767d 100644
--- a/datapath-windows/include/OvsDpInterfaceExt.h
+++ b/datapath-windows/include/OvsDpInterfaceExt.h
@@ -29,22 +29,27 @@
 
 #define OVS_IOCTL_DEVICE_TYPE 45000
 
-/* We used Direct I/O (zero copy) for the buffers. */
 #define OVS_IOCTL_START   0x100
+/* We used Direct I/O (zero copy) for the buffers. */
+/* Non-Netlink-based IOCTLs. */
+#define OVS_IOCTL_GET_PID \
+    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x0, METHOD_BUFFERED,\
+              FILE_WRITE_ACCESS)
+/* Netlink-based IOCTLs. */
 #define OVS_IOCTL_READ \
-    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x0, METHOD_OUT_DIRECT,\
+    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 + 0x1, METHOD_OUT_DIRECT, \
+    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x2, METHOD_OUT_DIRECT, \
               FILE_READ_ACCESS)
 #define OVS_IOCTL_READ_PACKET \
-    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x2, METHOD_OUT_DIRECT, \
+    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x3, METHOD_OUT_DIRECT, \
               FILE_READ_ACCESS)
 #define OVS_IOCTL_WRITE \
-    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x3, METHOD_IN_DIRECT,\
+    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x4, METHOD_IN_DIRECT,\
               FILE_READ_ACCESS)
 #define OVS_IOCTL_TRANSACT \
-    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x4, METHOD_OUT_DIRECT,\
+    CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x5, METHOD_OUT_DIRECT,\
               FILE_WRITE_ACCESS)
 
 /*
@@ -75,7 +80,6 @@
 
 /* Commands available under the OVS_WIN_CONTROL_FAMILY. */
 enum ovs_win_control_cmd {
-    OVS_CTRL_CMD_WIN_GET_PID,
     OVS_CTRL_CMD_WIN_PEND_REQ,
     OVS_CTRL_CMD_WIN_PEND_PACKET_REQ,
     OVS_CTRL_CMD_MC_SUBSCRIBE_REQ,
diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 888c6ef79..e566ea9d0 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -87,8 +87,7 @@ typedef struct _NETLINK_FAMILY {
 } NETLINK_FAMILY, *PNETLINK_FAMILY;
 
 /* Handlers for the various netlink commands. */
-static NetlinkCmdHandler OvsGetPidCmdHandler,
-                         OvsPendEventCmdHandler,
+static NetlinkCmdHandler OvsPendEventCmdHandler,
                          OvsPendPacketCmdHandler,
                          OvsSubscribeEventCmdHandler,
                          OvsSubscribePacketCmdHandler,
@@ -110,6 +109,8 @@ static NTSTATUS HandleGetDpDump(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
                                 UINT32 *replyLen);
 static NTSTATUS HandleDpTransactionCommon(
                     POVS_USER_PARAMS_CONTEXT usrParamsCtx, UINT32 *replyLen);
+static NTSTATUS OvsGetPidHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
+                                    UINT32 *replyLen);
 
 /*
  * The various netlink families, along with the supported commands. Most of
@@ -120,11 +121,6 @@ static NTSTATUS HandleDpTransactionCommon(
 
 /* Netlink control family: this is a Windows specific family. */
 NETLINK_CMD nlControlFamilyCmdOps[] = {
-    { .cmd             = OVS_CTRL_CMD_WIN_GET_PID,
-      .handler         = OvsGetPidCmdHandler,
-      .supportedDevOp  = OVS_TRANSACTION_DEV_OP,
-      .validateDpIndex = FALSE,
-    },
     { .cmd = OVS_CTRL_CMD_WIN_PEND_REQ,
       .handler = OvsPendEventCmdHandler,
       .supportedDevOp = OVS_WRITE_DEV_OP,
@@ -736,6 +732,24 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
      * operation.
      */
     switch (code) {
+    case OVS_IOCTL_GET_PID:
+        /* Both input buffer and output buffer use the same location. */
+        outputBuffer = irp->AssociatedIrp.SystemBuffer;
+        if (outputBufferLen != 0) {
+            InitUserParamsCtx(irp, instance, 0, NULL,
+                              inputBuffer, inputBufferLen,
+                              outputBuffer, outputBufferLen,
+                              &usrParamsCtx);
+
+            ASSERT(outputBuffer);
+        } else {
+            status = STATUS_NDIS_INVALID_LENGTH;
+            goto done;
+        }
+
+        status = OvsGetPidHandler(&usrParamsCtx, &replyLen);
+        goto done;
+
     case OVS_IOCTL_TRANSACT:
         /* Both input buffer and output buffer are mandatory. */
         if (outputBufferLen != 0) {
@@ -947,11 +961,9 @@ ValidateNetlinkCmd(UINT32 devOp,
             }
 
             /* Validate the PID. */
-            if (ovsMsg->genlMsg.cmd != OVS_CTRL_CMD_WIN_GET_PID) {
-                if (ovsMsg->nlMsg.nlmsgPid != instance->pid) {
-                    status = STATUS_INVALID_PARAMETER;
-                    goto done;
-                }
+            if (ovsMsg->nlMsg.nlmsgPid != instance->pid) {
+                status = STATUS_INVALID_PARAMETER;
+                goto done;
             }
 
             status = STATUS_SUCCESS;
@@ -992,38 +1004,33 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
 
 /*
  * --------------------------------------------------------------------------
- *  Command Handler for 'OVS_CTRL_CMD_WIN_GET_PID'.
+ *  Handler for 'OVS_IOCTL_GET_PID'.
  *
  *  Each handle on the device is assigned a unique PID when the handle is
- *  created. On platforms that support netlink natively, the PID is available
- *  to userspace when the netlink socket is created. However, without native
- *  netlink support on Windows, OVS datapath generates the PID and lets the
- *  userspace query it.
- *
- *  This function implements the query.
+ *  created. This function passes the PID to userspace using METHOD_BUFFERED
+ *  method.
  * --------------------------------------------------------------------------
  */
 static NTSTATUS
-OvsGetPidCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
-                    UINT32 *replyLen)
+OvsGetPidHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
+                 UINT32 *replyLen)
 {
-    POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
-    POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
+    NTSTATUS status = STATUS_SUCCESS;
+    PUINT32 msgOut = (PUINT32)usrParamsCtx->outputBuffer;
 
     if (usrParamsCtx->outputLength >= sizeof *msgOut) {
         POVS_OPEN_INSTANCE instance =
             (POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance;
 
         RtlZeroMemory(msgOut, sizeof *msgOut);
-        msgOut->nlMsg.nlmsgSeq = msgIn->nlMsg.nlmsgSeq;
-        msgOut->nlMsg.nlmsgPid = instance->pid;
+        RtlCopyMemory(msgOut, &instance->pid, sizeof(*msgOut));
         *replyLen = sizeof *msgOut;
-        /* XXX: We might need to return the DP index as well. */
     } else {
-        return STATUS_NDIS_INVALID_LENGTH;
+        *replyLen = sizeof *msgOut;
+        status = STATUS_NDIS_INVALID_LENGTH;
     }
 
-    return STATUS_SUCCESS;
+    return status;
 }
 
 /*
diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index 97220b468..f25fe9a4f 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -319,7 +319,7 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     rc = OvsPutFlowIoctl(&mappedFlow, sizeof (struct OvsFlowPut),
                          &stats);
     if (rc != STATUS_SUCCESS) {
-        OVS_LOG_ERROR("OvsFlowPut failed.");
+        OVS_LOG_ERROR("OvsPutFlowIoctl failed.");
         goto done;
     }
 
diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
index df889d3eb..fab2a6681 100644
--- a/lib/netlink-socket.c
+++ b/lib/netlink-socket.c
@@ -273,63 +273,27 @@ nl_sock_destroy(struct nl_sock *sock)
 
 #ifdef _WIN32
 /* Reads the pid for 'sock' generated in the kernel datapath. The function
- * follows a transaction semantic. Eventually this function should call into
- * nl_transact. */
+ * uses a separate IOCTL instead of a transaction semantic to avoid unnecessary
+ * message overhead. */
 static int
 get_sock_pid_from_kernel(struct nl_sock *sock)
 {
-    struct nl_transaction txn;
-    struct ofpbuf request;
-    uint64_t request_stub[128];
-    struct ofpbuf reply;
-    uint64_t reply_stub[128];
-    struct ovs_header *ovs_header;
-    struct nlmsghdr *nlmsg;
-    uint32_t seq;
-    int retval;
-    DWORD bytes;
-    int ovs_msg_size = sizeof (struct nlmsghdr) + sizeof (struct genlmsghdr) +
-                       sizeof (struct ovs_header);
-
-    ofpbuf_use_stub(&request, request_stub, sizeof request_stub);
-    txn.request = &request;
-    ofpbuf_use_stub(&reply, reply_stub, sizeof reply_stub);
-    txn.reply = &reply;
-
-    seq = nl_sock_allocate_seq(sock, 1);
-    nl_msg_put_genlmsghdr(&request, 0, OVS_WIN_NL_CTRL_FAMILY_ID, 0,
-                          OVS_CTRL_CMD_WIN_GET_PID, OVS_WIN_CONTROL_VERSION);
-    nlmsg = nl_msg_nlmsghdr(txn.request);
-    nlmsg->nlmsg_seq = seq;
-
-    ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header);
-    ovs_header->dp_ifindex = 0;
-    ovs_header = ofpbuf_put_uninit(&reply, ovs_msg_size);
+    uint32_t pid = 0;
+    int retval = 0;
+    DWORD bytes = 0;
 
-    if (!DeviceIoControl(sock->handle, OVS_IOCTL_TRANSACT,
-                         txn.request->data, txn.request->size,
-                         txn.reply->data, txn.reply->size,
+    if (!DeviceIoControl(sock->handle, OVS_IOCTL_GET_PID,
+                         NULL, 0, &pid, sizeof(pid),
                          &bytes, NULL)) {
         retval = EINVAL;
-        goto done;
     } else {
-        if (bytes < ovs_msg_size) {
+        if (bytes < sizeof(pid)) {
             retval = EINVAL;
-            goto done;
-        }
-
-        nlmsg = nl_msg_nlmsghdr(txn.reply);
-        if (nlmsg->nlmsg_seq != seq) {
-            retval = EINVAL;
-            goto done;
+        } else {
+            sock->pid = pid;
         }
-        sock->pid = nlmsg->nlmsg_pid;
     }
-    retval = 0;
 
-done:
-    ofpbuf_uninit(&request);
-    ofpbuf_uninit(&reply);
     return retval;
 }
 #endif  /* _WIN32 */