[RFC PATCH 28/60] hyper_dmabuf: address several synchronization issues

[Dongwon Kim <dongwon.kim@intel.com>]

From: Mateusz Polrola <mateuszx.potrola@intel.com>

This patch addresses several synchronization issues while sharing
DMA_BUF with another VM.

1. Set WAIT_AFTER_SYNC_REQ to false by default to prevent possible
   performance degradation when waing for the response for every
   syncrhonization request to exporter VM.

2. Removed HYPER_DMABUF_OPS_RELEASE_FINAL message - now exporter can
   automatically detect when there are no more consumers of DMA_BUF
   so importer VM doesn't have to send out this message.

3. Renamed HYPER_DMABUF_FIRST_EXPORT into HYPER_DMABUF_EXPORT_FD

4. Introduced HYPER_DMABUF_EXPORT_FD_FAILED message to undo
   HYPER_DMABUF_FIRST_EXPORT in case of any failure while executing
   hyper_dmabuf_export_fd_ioctl

5. Waiting until other VM processes all pending requests when ring
   buffers are all full.

6. Create hyper_dmabuf.h with definitions of driver interface under
   include/uapi/xen/

Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c        | 21 ++---
 drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c      | 17 +++-
 drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.h      | 74 +----------------
 drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c        | 30 +++++--
 drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h        |  4 +-
 .../xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c    | 24 ++++--
 .../xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c   |  5 +-
 include/uapi/xen/hyper_dmabuf.h                    | 96 ++++++++++++++++++++++
 8 files changed, 163 insertions(+), 108 deletions(-)
 create mode 100644 include/uapi/xen/hyper_dmabuf.h

diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c
index a0b3946..5a034ffb 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c
@@ -187,10 +187,7 @@ int hyper_dmabuf_cleanup_sgt_info(struct hyper_dmabuf_sgt_info *sgt_info, int fo
 	 * side.
 	 */
 	if (!force &&
-	    (!list_empty(&sgt_info->va_kmapped->list) ||
-	    !list_empty(&sgt_info->va_vmapped->list) ||
-	    !list_empty(&sgt_info->active_sgts->list) ||
-	    !list_empty(&sgt_info->active_attached->list))) {
+	    sgt_info->importer_exported) {
 		dev_warn(hyper_dmabuf_private.device, "dma-buf is used by importer\n");
 		return -EPERM;
 	}
@@ -259,7 +256,7 @@ int hyper_dmabuf_cleanup_sgt_info(struct hyper_dmabuf_sgt_info *sgt_info, int fo
 	return 0;
 }
 
-#define WAIT_AFTER_SYNC_REQ 1
+#define WAIT_AFTER_SYNC_REQ 0
 
 inline int hyper_dmabuf_sync_request(int id, int dmabuf_ops)
 {
@@ -431,17 +428,11 @@ static void hyper_dmabuf_ops_release(struct dma_buf *dma_buf)
 	final_release = sgt_info && !sgt_info->valid &&
 		        !sgt_info->num_importers;
 
-	if (final_release) {
-		ret = hyper_dmabuf_sync_request(sgt_info->hyper_dmabuf_id,
-						HYPER_DMABUF_OPS_RELEASE_FINAL);
-	} else {
-		ret = hyper_dmabuf_sync_request(sgt_info->hyper_dmabuf_id,
-						HYPER_DMABUF_OPS_RELEASE);
-	}
-
+	ret = hyper_dmabuf_sync_request(sgt_info->hyper_dmabuf_id,
+					HYPER_DMABUF_OPS_RELEASE);
 	if (ret < 0) {
-		dev_err(hyper_dmabuf_private.device,
-			"hyper_dmabuf::%s Error:send dmabuf sync request failed\n", __func__);
+		dev_warn(hyper_dmabuf_private.device,
+			 "hyper_dmabuf::%s Error:send dmabuf sync request failed\n", __func__);
 	}
 
 	/*
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
index 19ca725..58b115a 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
@@ -35,6 +35,7 @@
 #include <linux/dma-buf.h>
 #include <linux/delay.h>
 #include <linux/list.h>
+#include <xen/hyper_dmabuf.h>
 #include "hyper_dmabuf_struct.h"
 #include "hyper_dmabuf_ioctl.h"
 #include "hyper_dmabuf_list.h"
@@ -282,12 +283,17 @@ static int hyper_dmabuf_export_fd_ioctl(void *data)
 	/* send notification for export_fd to exporter */
 	operand = sgt_info->hyper_dmabuf_id;
 
+	dev_dbg(hyper_dmabuf_private.device, "Exporting fd of buffer %d\n", operand);
+
 	req = kcalloc(1, sizeof(*req), GFP_KERNEL);
-	hyper_dmabuf_create_request(req, HYPER_DMABUF_FIRST_EXPORT, &operand);
+	hyper_dmabuf_create_request(req, HYPER_DMABUF_EXPORT_FD, &operand);
 
 	ret = ops->send_req(HYPER_DMABUF_DOM_ID(operand), req, true);
 
 	if (ret < 0) {
+		/* in case of timeout other end eventually will receive request, so we need to undo it */
+		hyper_dmabuf_create_request(req, HYPER_DMABUF_EXPORT_FD_FAILED, &operand);
+		ops->send_req(HYPER_DMABUF_DOM_ID(operand), req, false);
 		kfree(req);
 		dev_err(hyper_dmabuf_private.device, "Failed to create sgt or notify exporter\n");
 		sgt_info->num_importers--;
@@ -298,12 +304,12 @@ static int hyper_dmabuf_export_fd_ioctl(void *data)
 
 	if (ret == HYPER_DMABUF_REQ_ERROR) {
 		dev_err(hyper_dmabuf_private.device,
-			"Buffer invalid\n");
+			"Buffer invalid %d, cannot import\n", operand);
 		sgt_info->num_importers--;
 		mutex_unlock(&hyper_dmabuf_private.lock);
 		return -EINVAL;
 	} else {
-		dev_dbg(hyper_dmabuf_private.device, "Can import buffer\n");
+		dev_dbg(hyper_dmabuf_private.device, "Can import buffer %d\n", operand);
 		ret = 0;
 	}
 
@@ -322,7 +328,12 @@ static int hyper_dmabuf_export_fd_ioctl(void *data)
 						   &sgt_info->refs_info);
 
 		if (!data_pages) {
+			dev_err(hyper_dmabuf_private.device, "Cannot map pages of buffer %d\n", operand);
 			sgt_info->num_importers--;
+			req = kcalloc(1, sizeof(*req), GFP_KERNEL);
+			hyper_dmabuf_create_request(req, HYPER_DMABUF_EXPORT_FD_FAILED, &operand);
+			ops->send_req(HYPER_DMABUF_DOM_ID(operand), req, false);
+			kfree(req);
 			mutex_unlock(&hyper_dmabuf_private.lock);
 			return -EINVAL;
 		}
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.h
index 558964c..8355e30 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.h
@@ -22,8 +22,8 @@
  *
  */
 
-#ifndef __LINUX_PUBLIC_HYPER_DMABUF_IOCTL_H__
-#define __LINUX_PUBLIC_HYPER_DMABUF_IOCTL_H__
+#ifndef __HYPER_DMABUF_IOCTL_H__
+#define __HYPER_DMABUF_IOCTL_H__
 
 typedef int (*hyper_dmabuf_ioctl_t)(void *data);
 
@@ -42,72 +42,4 @@ struct hyper_dmabuf_ioctl_desc {
 			.name = #ioctl			\
 	}
 
-#define IOCTL_HYPER_DMABUF_TX_CH_SETUP \
-_IOC(_IOC_NONE, 'G', 0, sizeof(struct ioctl_hyper_dmabuf_tx_ch_setup))
-struct ioctl_hyper_dmabuf_tx_ch_setup {
-	/* IN parameters */
-	/* Remote domain id */
-	int remote_domain;
-};
-
-#define IOCTL_HYPER_DMABUF_RX_CH_SETUP \
-_IOC(_IOC_NONE, 'G', 1, sizeof(struct ioctl_hyper_dmabuf_rx_ch_setup))
-struct ioctl_hyper_dmabuf_rx_ch_setup {
-	/* IN parameters */
-	/* Source domain id */
-	int source_domain;
-};
-
-#define IOCTL_HYPER_DMABUF_EXPORT_REMOTE \
-_IOC(_IOC_NONE, 'G', 2, sizeof(struct ioctl_hyper_dmabuf_export_remote))
-struct ioctl_hyper_dmabuf_export_remote {
-	/* IN parameters */
-	/* DMA buf fd to be exported */
-	int dmabuf_fd;
-	/* Domain id to which buffer should be exported */
-	int remote_domain;
-	/* exported dma buf id */
-	int hyper_dmabuf_id;
-	int private[4];
-};
-
-#define IOCTL_HYPER_DMABUF_EXPORT_FD \
-_IOC(_IOC_NONE, 'G', 3, sizeof(struct ioctl_hyper_dmabuf_export_fd))
-struct ioctl_hyper_dmabuf_export_fd {
-	/* IN parameters */
-	/* hyper dmabuf id to be imported */
-	int hyper_dmabuf_id;
-	/* flags */
-	int flags;
-	/* OUT parameters */
-	/* exported dma buf fd */
-	int fd;
-};
-
-#define IOCTL_HYPER_DMABUF_UNEXPORT \
-_IOC(_IOC_NONE, 'G', 4, sizeof(struct ioctl_hyper_dmabuf_unexport))
-struct ioctl_hyper_dmabuf_unexport {
-	/* IN parameters */
-	/* hyper dmabuf id to be unexported */
-	int hyper_dmabuf_id;
-	/* delay in ms by which unexport processing will be postponed */
-	int delay_ms;
-	/* OUT parameters */
-	/* Status of request */
-	int status;
-};
-
-#define IOCTL_HYPER_DMABUF_QUERY \
-_IOC(_IOC_NONE, 'G', 5, sizeof(struct ioctl_hyper_dmabuf_query))
-struct ioctl_hyper_dmabuf_query {
-	/* in parameters */
-	/* hyper dmabuf id to be queried */
-	int hyper_dmabuf_id;
-	/* item to be queried */
-	int item;
-	/* OUT parameters */
-	/* Value of queried item */
-	int info;
-};
-
-#endif //__LINUX_PUBLIC_HYPER_DMABUF_DRV_H__
+#endif //__HYPER_DMABUF_IOCTL_H__
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
index 6e24442..3111cdc 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
@@ -79,9 +79,10 @@ void hyper_dmabuf_create_request(struct hyper_dmabuf_req *req,
 		req->operands[0] = operands[0];
 		break;
 
-	case HYPER_DMABUF_FIRST_EXPORT:
-		/* dmabuf fd is being created on imported side for first time */
-		/* command : HYPER_DMABUF_FIRST_EXPORT,
+	case HYPER_DMABUF_EXPORT_FD:
+	case HYPER_DMABUF_EXPORT_FD_FAILED:
+		/* dmabuf fd is being created on imported side or importing failed */
+		/* command : HYPER_DMABUF_EXPORT_FD or HYPER_DMABUF_EXPORT_FD_FAILED,
 		 * operands0 : hyper_dmabuf_id
 		 */
 		req->operands[0] = operands[0];
@@ -244,8 +245,10 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_req *req)
 	}
 
 	/* synchronous dma_buf_fd export */
-	if (req->command == HYPER_DMABUF_FIRST_EXPORT) {
+	if (req->command == HYPER_DMABUF_EXPORT_FD) {
 		/* find a corresponding SGT for the id */
+		dev_dbg(hyper_dmabuf_private.device,
+			"Processing HYPER_DMABUF_EXPORT_FD %d\n", req->operands[0]);
 		exp_sgt_info = hyper_dmabuf_find_exported(req->operands[0]);
 
 		if (!exp_sgt_info) {
@@ -254,17 +257,32 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_req *req)
 			req->status = HYPER_DMABUF_REQ_ERROR;
 		} else if (!exp_sgt_info->valid) {
 			dev_dbg(hyper_dmabuf_private.device,
-				"Buffer no longer valid - cannot export\n");
+				"Buffer no longer valid - cannot export fd %d\n", req->operands[0]);
 			req->status = HYPER_DMABUF_REQ_ERROR;
 		} else {
 			dev_dbg(hyper_dmabuf_private.device,
-				"Buffer still valid - can export\n");
+				"Buffer still valid - can export fd%d\n", req->operands[0]);
 			exp_sgt_info->importer_exported++;
 			req->status = HYPER_DMABUF_REQ_PROCESSED;
 		}
 		return req->command;
 	}
 
+	if (req->command == HYPER_DMABUF_EXPORT_FD_FAILED) {
+		dev_dbg(hyper_dmabuf_private.device,
+			"Processing HYPER_DMABUF_EXPORT_FD_FAILED %d\n", req->operands[0]);
+		exp_sgt_info = hyper_dmabuf_find_exported(req->operands[0]);
+
+		if (!exp_sgt_info) {
+			dev_err(hyper_dmabuf_private.device,
+				"critical err: requested sgt_info can't be found %d\n", req->operands[0]);
+			req->status = HYPER_DMABUF_REQ_ERROR;
+		} else {
+			exp_sgt_info->importer_exported--;
+			req->status = HYPER_DMABUF_REQ_PROCESSED;
+		}
+		return req->command;
+	}
 
 	dev_dbg(hyper_dmabuf_private.device,
 		"%s: putting request to workqueue\n", __func__);
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
index 8b3c857..50ce617 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
@@ -43,7 +43,8 @@ struct hyper_dmabuf_resp {
 
 enum hyper_dmabuf_command {
 	HYPER_DMABUF_EXPORT = 0x10,
-	HYPER_DMABUF_FIRST_EXPORT,
+	HYPER_DMABUF_EXPORT_FD,
+	HYPER_DMABUF_EXPORT_FD_FAILED,
 	HYPER_DMABUF_NOTIFY_UNEXPORT,
 	HYPER_DMABUF_OPS_TO_REMOTE,
 	HYPER_DMABUF_OPS_TO_SOURCE,
@@ -55,7 +56,6 @@ enum hyper_dmabuf_ops {
 	HYPER_DMABUF_OPS_MAP,
 	HYPER_DMABUF_OPS_UNMAP,
 	HYPER_DMABUF_OPS_RELEASE,
-	HYPER_DMABUF_OPS_RELEASE_FINAL,
 	HYPER_DMABUF_OPS_BEGIN_CPU_ACCESS,
 	HYPER_DMABUF_OPS_END_CPU_ACCESS,
 	HYPER_DMABUF_OPS_KMAP_ATOMIC,
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c
index a74e800..0eded61 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c
@@ -152,13 +152,25 @@ int hyper_dmabuf_remote_sync(int id, int ops)
 		kfree(sgtl);
 		break;
 
-	case HYPER_DMABUF_OPS_RELEASE_FINAL:
+	case HYPER_DMABUF_OPS_RELEASE:
+		dev_dbg(hyper_dmabuf_private.device,
+			"Buffer %d released, references left: %d\n",
+			 sgt_info->hyper_dmabuf_id,
+			 sgt_info->importer_exported -1);
+                sgt_info->importer_exported--;
+		/* If there are still importers just break, if no then continue with final cleanup */
+		if (sgt_info->importer_exported)
+			break;
+
 		/*
 		 * Importer just released buffer fd, check if there is any other importer still using it.
 		 * If not and buffer was unexported, clean up shared data and remove that buffer.
 		 */
-		 if (list_empty(&sgt_info->active_attached->list) &&
-		     !sgt_info->valid) {
+		dev_dbg(hyper_dmabuf_private.device,
+			"Buffer %d final released\n", sgt_info->hyper_dmabuf_id);
+
+		if (!sgt_info->valid && !sgt_info->importer_exported &&
+		    !sgt_info->unexport_scheduled) {
 			hyper_dmabuf_cleanup_sgt_info(sgt_info, false);
 			hyper_dmabuf_remove_exported(id);
 			kfree(sgt_info);
@@ -168,12 +180,6 @@ int hyper_dmabuf_remote_sync(int id, int ops)
 
 		break;
 
-	case HYPER_DMABUF_OPS_RELEASE:
-		/* place holder */
-                sgt_info->importer_exported--;
-
-		break;
-
 	case HYPER_DMABUF_OPS_BEGIN_CPU_ACCESS:
 		ret = dma_buf_begin_cpu_access(sgt_info->dma_buf, DMA_BIDIRECTIONAL);
 		if (ret) {
diff --git a/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c b/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c
index 2cc35e3..ce9862a 100644
--- a/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c
+++ b/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c
@@ -519,8 +519,9 @@ int hyper_dmabuf_xen_send_req(int domid, struct hyper_dmabuf_req *req, int wait)
 
 	ring = &ring_info->ring_front;
 
-	if (RING_FULL(ring))
-		return -EBUSY;
+	while (RING_FULL(ring)) {
+		usleep_range(100, 120);
+	}
 
 	new_req = RING_GET_REQUEST(ring, ring->req_prod_pvt);
 	if (!new_req) {
diff --git a/include/uapi/xen/hyper_dmabuf.h b/include/uapi/xen/hyper_dmabuf.h
new file mode 100644
index 0000000..2eff3a8e
--- /dev/null
+++ b/include/uapi/xen/hyper_dmabuf.h
@@ -0,0 +1,96 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#ifndef __LINUX_PUBLIC_HYPER_DMABUF_H__
+#define __LINUX_PUBLIC_HYPER_DMABUF_H__
+
+#define IOCTL_HYPER_DMABUF_TX_CH_SETUP \
+_IOC(_IOC_NONE, 'G', 0, sizeof(struct ioctl_hyper_dmabuf_tx_ch_setup))
+struct ioctl_hyper_dmabuf_tx_ch_setup {
+	/* IN parameters */
+	/* Remote domain id */
+	int remote_domain;
+};
+
+#define IOCTL_HYPER_DMABUF_RX_CH_SETUP \
+_IOC(_IOC_NONE, 'G', 1, sizeof(struct ioctl_hyper_dmabuf_rx_ch_setup))
+struct ioctl_hyper_dmabuf_rx_ch_setup {
+	/* IN parameters */
+	/* Source domain id */
+	int source_domain;
+};
+
+#define IOCTL_HYPER_DMABUF_EXPORT_REMOTE \
+_IOC(_IOC_NONE, 'G', 2, sizeof(struct ioctl_hyper_dmabuf_export_remote))
+struct ioctl_hyper_dmabuf_export_remote {
+	/* IN parameters */
+	/* DMA buf fd to be exported */
+	int dmabuf_fd;
+	/* Domain id to which buffer should be exported */
+	int remote_domain;
+	/* exported dma buf id */
+	int hyper_dmabuf_id;
+	int private[4];
+};
+
+#define IOCTL_HYPER_DMABUF_EXPORT_FD \
+_IOC(_IOC_NONE, 'G', 3, sizeof(struct ioctl_hyper_dmabuf_export_fd))
+struct ioctl_hyper_dmabuf_export_fd {
+	/* IN parameters */
+	/* hyper dmabuf id to be imported */
+	int hyper_dmabuf_id;
+	/* flags */
+	int flags;
+	/* OUT parameters */
+	/* exported dma buf fd */
+	int fd;
+};
+
+#define IOCTL_HYPER_DMABUF_UNEXPORT \
+_IOC(_IOC_NONE, 'G', 4, sizeof(struct ioctl_hyper_dmabuf_unexport))
+struct ioctl_hyper_dmabuf_unexport {
+	/* IN parameters */
+	/* hyper dmabuf id to be unexported */
+	int hyper_dmabuf_id;
+	/* delay in ms by which unexport processing will be postponed */
+	int delay_ms;
+	/* OUT parameters */
+	/* Status of request */
+	int status;
+};
+
+#define IOCTL_HYPER_DMABUF_QUERY \
+_IOC(_IOC_NONE, 'G', 5, sizeof(struct ioctl_hyper_dmabuf_query))
+struct ioctl_hyper_dmabuf_query {
+	/* in parameters */
+	/* hyper dmabuf id to be queried */
+	int hyper_dmabuf_id;
+	/* item to be queried */
+	int item;
+	/* OUT parameters */
+	/* Value of queried item */
+	int info;
+};
+
+#endif //__LINUX_PUBLIC_HYPER_DMABUF_H__
-- 
2.7.4