[RFC PATCH 13/60] hyper_dmabuf: postponing cleanup of hyper_DMABUF

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

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

Immediate clean up of buffer is not possible if the buffer is
actively used by importer. In this case, we need to postpone
freeing hyper_DMABUF until the last consumer unmaps and releases
the buffer on impoter VM. New reference count is added for tracking
usage by importers.

Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c        | 37 ++++++++++++++--
 drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c      | 34 +++++++++++----
 drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c        | 49 +++++++---------------
 drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h        |  1 -
 .../xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c    | 14 +++++--
 drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h     |  9 +++-
 6 files changed, 95 insertions(+), 49 deletions(-)

diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c
index 06bd8e5..f258981 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_imp.c
@@ -9,6 +9,7 @@
 #include "hyper_dmabuf_imp.h"
 #include "xen/hyper_dmabuf_xen_comm.h"
 #include "hyper_dmabuf_msg.h"
+#include "hyper_dmabuf_list.h"
 
 #define REFS_PER_PAGE (PAGE_SIZE/sizeof(grant_ref_t))
 
@@ -104,7 +105,7 @@ struct sg_table* hyper_dmabuf_create_sgt(struct page **pages,
 
 	ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
 	if (ret) {
-		sg_free_table(sgt);
+		hyper_dmabuf_free_sgt(sgt);
 		return NULL;
 	}
 
@@ -129,6 +130,7 @@ struct sg_table* hyper_dmabuf_create_sgt(struct page **pages,
 void hyper_dmabuf_free_sgt(struct sg_table* sgt)
 {
 	sg_free_table(sgt);
+	kfree(sgt);
 }
 
 /*
@@ -583,6 +585,9 @@ int hyper_dmabuf_cleanup_sgt_info(struct hyper_dmabuf_sgt_info *sgt_info, int fo
 		kfree(attachl);
 	}
 
+	/* Start cleanup of buffer in reverse order to exporting */
+	hyper_dmabuf_cleanup_gref_table(sgt_info);
+
 	/* unmap dma-buf */
 	dma_buf_unmap_attachment(sgt_info->active_attached->attach,
 				 sgt_info->active_sgts->sgt,
@@ -594,8 +599,6 @@ int hyper_dmabuf_cleanup_sgt_info(struct hyper_dmabuf_sgt_info *sgt_info, int fo
 	/* close connection to dma-buf completely */
 	dma_buf_put(sgt_info->dma_buf);
 
-	hyper_dmabuf_cleanup_gref_table(sgt_info);
-
 	kfree(sgt_info->active_sgts);
 	kfree(sgt_info->active_attached);
 	kfree(sgt_info->va_kmapped);
@@ -694,6 +697,9 @@ static struct sg_table* hyper_dmabuf_ops_map(struct dma_buf_attachment *attachme
 	ret = hyper_dmabuf_sync_request_and_wait(sgt_info->hyper_dmabuf_id,
 						HYPER_DMABUF_OPS_MAP);
 
+	kfree(page_info->pages);
+	kfree(page_info);
+
 	if (ret < 0) {
 		printk("hyper_dmabuf::%s Error:send dmabuf sync request failed\n", __func__);
 	}
@@ -741,12 +747,34 @@ static void hyper_dmabuf_ops_release(struct dma_buf *dmabuf)
 
 	sgt_info = (struct hyper_dmabuf_imported_sgt_info *)dmabuf->priv;
 
+	if (sgt_info) {
+		/* dmabuf fd is being released - decrease refcount */
+		sgt_info->ref_count--;
+
+		/* if no one else in that domain is using that buffer, unmap it for now */
+		if (sgt_info->ref_count == 0) {
+			hyper_dmabuf_cleanup_imported_pages(sgt_info);
+			hyper_dmabuf_free_sgt(sgt_info->sgt);
+			sgt_info->sgt = NULL;
+		}
+	}
+
 	ret = hyper_dmabuf_sync_request_and_wait(sgt_info->hyper_dmabuf_id,
 						HYPER_DMABUF_OPS_RELEASE);
 
 	if (ret < 0) {
 		printk("hyper_dmabuf::%s Error:send dmabuf sync request failed\n", __func__);
 	}
+
+	/*
+	 * Check if buffer is still valid and if not remove it from imported list.
+	 * That has to be done after sending sync request
+	 */
+	if (sgt_info && sgt_info->ref_count == 0 &&
+	    sgt_info->flags == HYPER_DMABUF_SGT_INVALID) {
+		hyper_dmabuf_remove_imported(sgt_info->hyper_dmabuf_id);
+		kfree(sgt_info);
+	}
 }
 
 static int hyper_dmabuf_ops_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction dir)
@@ -944,6 +972,9 @@ int hyper_dmabuf_export_fd(struct hyper_dmabuf_imported_sgt_info *dinfo, int fla
 
 	fd = dma_buf_fd(dmabuf, flags);
 
+	/* dmabuf fd is exported for given bufer - increase its ref count */
+	dinfo->ref_count++;
+
 	return fd;
 }
 
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
index a222c1b..c57acafe 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
@@ -135,6 +135,7 @@ static int hyper_dmabuf_export_remote(void *data)
 	/* TODO: We might need to consider using port number on event channel? */
 	sgt_info->hyper_dmabuf_rdomain = export_remote_attr->remote_domain;
 	sgt_info->dma_buf = dma_buf;
+	sgt_info->flags = 0;
 
 	sgt_info->active_sgts = kcalloc(1, sizeof(struct sgt_list), GFP_KERNEL);
 	sgt_info->active_attached = kcalloc(1, sizeof(struct attachment_list), GFP_KERNEL);
@@ -233,6 +234,15 @@ static int hyper_dmabuf_export_fd_ioctl(void *data)
 	if (imported_sgt_info == NULL) /* can't find sgt from the table */
 		return -1;
 
+	/*
+	 * Check if buffer was not unexported by exporter.
+	 * In such exporter is waiting for importer to finish using that buffer,
+	 * so do not allow export fd of such buffer anymore.
+	 */
+	if (imported_sgt_info->flags == HYPER_DMABUF_SGT_INVALID) {
+		return -EINVAL;
+	}
+
 	printk("%s Found buffer gref %d  off %d last len %d nents %d domain %d\n", __func__,
 		imported_sgt_info->gref, imported_sgt_info->frst_ofst,
 		imported_sgt_info->last_len, imported_sgt_info->nents,
@@ -289,9 +299,7 @@ static int hyper_dmabuf_unexport(void *data)
 
 	hyper_dmabuf_create_request(req, HYPER_DMABUF_NOTIFY_UNEXPORT, &unexport_attr->hyper_dmabuf_id);
 
-	/* now send destroy request to remote domain
-	 * currently assuming there's only one importer exist
-	 */
+	/* Now send unexport request to remote domain, marking that buffer should not be used anymore */
 	ret = hyper_dmabuf_send_request(sgt_info->hyper_dmabuf_rdomain, req, true);
 	if (ret < 0) {
 		kfree(req);
@@ -300,11 +308,23 @@ static int hyper_dmabuf_unexport(void *data)
 
 	/* free msg */
 	kfree(req);
-	unexport_attr->status = ret;
 
-	/* Rest of cleanup will follow when importer will free it's buffer,
-	 * current implementation assumes that there is only one importer
-         */
+	/*
+	 * Check if any importer is still using buffer, if not clean it up completly,
+	 * otherwise mark buffer as unexported and postpone its cleanup to time when
+	 * importer will finish using it.
+	 */
+	if (list_empty(&sgt_info->active_sgts->list) &&
+	    list_empty(&sgt_info->active_attached->list)) {
+		hyper_dmabuf_cleanup_sgt_info(sgt_info, false);
+		hyper_dmabuf_remove_exported(unexport_attr->hyper_dmabuf_id);
+		kfree(sgt_info);
+	} else {
+		sgt_info->flags = HYPER_DMABUF_SGT_UNEXPORTED;
+	}
+
+	/* TODO: should we mark here that buffer was destroyed immiedetaly or that was postponed ? */
+	unexport_attr->status = ret;
 
 	return ret;
 }
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
index e7532b5..97b42a4 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
@@ -81,11 +81,10 @@ void hyper_dmabuf_create_request(struct hyper_dmabuf_ring_rq *request,
 void cmd_process_work(struct work_struct *work)
 {
 	struct hyper_dmabuf_imported_sgt_info *imported_sgt_info;
-        struct hyper_dmabuf_sgt_info *sgt_info;
 	struct cmd_process *proc = container_of(work, struct cmd_process, work);
 	struct hyper_dmabuf_ring_rq *req;
 	int domid;
-	int i, ret;
+	int i;
 
 	req = proc->rq;
 	domid = proc->domid;
@@ -118,31 +117,11 @@ void cmd_process_work(struct work_struct *work)
 		for (i=0; i<4; i++)
 			imported_sgt_info->private[i] = req->operands[5+i];
 
+		imported_sgt_info->flags = 0;
+		imported_sgt_info->ref_count = 0;
 		hyper_dmabuf_register_imported(imported_sgt_info);
 		break;
 
-	case HYPER_DMABUF_UNEXPORT_FINISH:
-		/* destroy sg_list for hyper_dmabuf_id on local side */
-		/* command : DMABUF_DESTROY_FINISH,
-		 * operands0 : hyper_dmabuf_id
-		 */
-
-		/* TODO: that should be done on workqueue, when received ack from
-		 * all importers that buffer is no longer used
-		 */
-		sgt_info = hyper_dmabuf_find_exported(req->operands[0]);
-		hyper_dmabuf_remove_exported(req->operands[0]);
-		if (!sgt_info)
-			printk("sgt_info does not exist in the list\n");
-
-		ret = hyper_dmabuf_cleanup_sgt_info(sgt_info, FORCED_UNEXPORTING);
-		if (!ret)
-			kfree(sgt_info);
-		else
-			printk("failed to clean up sgt_info\n");
-
-		break;
-
 	case HYPER_DMABUF_OPS_TO_REMOTE:
 		/* notifying dmabuf map/unmap to importer (probably not needed) */
 		/* for dmabuf synchronization */
@@ -191,16 +170,18 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_ring_rq *req)
 			hyper_dmabuf_find_imported(req->operands[0]);
 
 		if (imported_sgt_info) {
-			hyper_dmabuf_free_sgt(imported_sgt_info->sgt);
-
-			hyper_dmabuf_cleanup_imported_pages(imported_sgt_info);
-			hyper_dmabuf_remove_imported(req->operands[0]);
-
-			/* Notify exporter that buffer is freed and it can
-			 * cleanup it
-			 */
-			req->status = HYPER_DMABUF_REQ_NEEDS_FOLLOW_UP;
-			req->command = HYPER_DMABUF_UNEXPORT_FINISH;
+			/* check if buffer is still mapped and in use */
+			if (imported_sgt_info->sgt) {
+				/*
+				 * Buffer is still in  use, just mark that it should
+				 * not be allowed to export its fd anymore.
+				 */
+				imported_sgt_info->flags = HYPER_DMABUF_SGT_INVALID;
+			} else {
+				/* No one is using buffer, remove it from imported list */
+				hyper_dmabuf_remove_imported(req->operands[0]);
+				kfree(imported_sgt_info);
+			}
 		} else {
 			req->status = HYPER_DMABUF_REQ_ERROR;
 		}
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
index 39a114a..fc1365b 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.h
@@ -4,7 +4,6 @@
 enum hyper_dmabuf_command {
 	HYPER_DMABUF_EXPORT = 0x10,
 	HYPER_DMABUF_NOTIFY_UNEXPORT,
-	HYPER_DMABUF_UNEXPORT_FINISH,
 	HYPER_DMABUF_OPS_TO_REMOTE,
 	HYPER_DMABUF_OPS_TO_SOURCE,
 };
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c
index fa2fa11..61ba4ed 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_remote_sync.c
@@ -8,6 +8,7 @@
 #include "hyper_dmabuf_drv.h"
 #include "xen/hyper_dmabuf_xen_comm.h"
 #include "hyper_dmabuf_msg.h"
+#include "hyper_dmabuf_imp.h"
 
 extern struct hyper_dmabuf_private hyper_dmabuf_private;
 
@@ -114,10 +115,17 @@ int hyper_dmabuf_remote_sync(int id, int ops)
 		break;
 
 	case HYPER_DMABUF_OPS_RELEASE:
-		/* remote importer shouldn't release dma_buf because
-		 * exporter will hold handle to the dma_buf as
-		 * far as dma_buf is shared with other domains.
+		/*
+		 * 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_sgts->list) &&                                                                  	    list_empty(&sgt_info->active_attached->list) &&
+		     sgt_info->flags == HYPER_DMABUF_SGT_UNEXPORTED) {
+			hyper_dmabuf_cleanup_sgt_info(sgt_info, false);
+			hyper_dmabuf_remove_exported(id);
+			kfree(sgt_info);
+		}
+
 		break;
 
 	case HYPER_DMABUF_OPS_BEGIN_CPU_ACCESS:
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h
index bfe80ee..1194cf2 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_struct.h
@@ -18,6 +18,11 @@
  * frame buffer) */
 #define MAX_ALLOWED_NUM_PAGES_FOR_GREF_NUM_ARRAYS 4
 
+enum hyper_dmabuf_sgt_flags {
+        HYPER_DMABUF_SGT_INVALID = 0x10,
+        HYPER_DMABUF_SGT_UNEXPORTED,
+};
+
 /* stack of mapped sgts */
 struct sgt_list {
 	struct sg_table *sgt;
@@ -76,7 +81,7 @@ struct hyper_dmabuf_sgt_info {
 	struct attachment_list *active_attached;
 	struct kmap_vaddr_list *va_kmapped;
 	struct vmap_vaddr_list *va_vmapped;
-
+	int flags;
 	struct hyper_dmabuf_shared_pages_info shared_pages_info;
 	int private[4]; /* device specific info (e.g. image's meta info?) */
 };
@@ -92,6 +97,8 @@ struct hyper_dmabuf_imported_sgt_info {
 	grant_ref_t gref; /* reference number of top level addressing page of shared pages */
 	struct sg_table *sgt; /* sgt pointer after importing buffer */
 	struct hyper_dmabuf_shared_pages_info shared_pages_info;
+	int flags;
+	int ref_count;
 	int private[4]; /* device specific info (e.g. image's meta info?) */
 };
 
-- 
2.7.4