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__