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

From: Dongwon Kim
Date: Tue Dec 19 2017 - 14:37:09 EST

From: Mateusz Polrola <mateuszx.potrola@xxxxxxxxx>

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@xxxxxxxxx>
---
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