[RFC PATCH 07/60] hyper_dmabuf: message parsing done via workqueue

From: Dongwon Kim
Date: Tue Dec 19 2017 - 14:51:17 EST

Use workqueue mechanism to delay message parsing done
after exiting from ISR to reduce ISR execution time.

Signed-off-by: Dongwon Kim <dongwon.kim@xxxxxxxxx>
---
drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c | 13 ++
drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.h | 5 +
drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c | 4 +-
drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c | 155 ++++++++++++++-------
.../xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c | 75 ++++------
.../xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.h | 7 -
6 files changed, 152 insertions(+), 107 deletions(-)

diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
index 0698327..70b4878 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.c
@@ -1,5 +1,8 @@
#include <linux/init.h> /* module_init, module_exit */
#include <linux/module.h> /* version info, MODULE_LICENSE, MODULE_AUTHOR, printk() */
+#include <linux/workqueue.h>
+#include <xen/grant_table.h>
+#include "hyper_dmabuf_drv.h"
#include "hyper_dmabuf_conf.h"
#include "hyper_dmabuf_list.h"
#include "xen/hyper_dmabuf_xen_comm_list.h"
@@ -10,6 +13,8 @@ MODULE_AUTHOR("IOTG-PED, INTEL");
int register_device(void);
int unregister_device(void);

+struct hyper_dmabuf_private hyper_dmabuf_private;
+
/*===============================================================================================*/
static int hyper_dmabuf_drv_init(void)
{
@@ -24,6 +29,10 @@ static int hyper_dmabuf_drv_init(void)

printk( KERN_NOTICE "initializing database for imported/exported dmabufs\n");

+ /* device structure initialization */
+ /* currently only does work-queue initialization */
+ hyper_dmabuf_private.work_queue = create_workqueue("hyper_dmabuf_wqueue");
+
ret = hyper_dmabuf_table_init();
if (ret < 0) {
return -EINVAL;
@@ -45,6 +54,10 @@ static void hyper_dmabuf_drv_exit(void)
hyper_dmabuf_table_destroy();
hyper_dmabuf_ring_table_init();

+ /* destroy workqueue */
+ if (hyper_dmabuf_private.work_queue)
+ destroy_workqueue(hyper_dmabuf_private.work_queue);
+
printk( KERN_NOTICE "dma_buf-src_sink model: Exiting" );
unregister_device();
}
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.h b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.h
index 2dad9a6..6145d29 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.h
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_drv.h
@@ -1,6 +1,11 @@
#ifndef __LINUX_PUBLIC_HYPER_DMABUF_DRV_H__
#define __LINUX_PUBLIC_HYPER_DMABUF_DRV_H__

+struct hyper_dmabuf_private {
+ struct device *device;
+ struct workqueue_struct *work_queue;
+};
+
typedef int (*hyper_dmabuf_ioctl_t)(void *data);

struct hyper_dmabuf_ioctl_desc {
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
index af94359..e4d8316 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_ioctl.c
@@ -15,9 +15,7 @@
#include "xen/hyper_dmabuf_xen_comm_list.h"
#include "hyper_dmabuf_msg.h"

-struct hyper_dmabuf_private {
- struct device *device;
-} hyper_dmabuf_private;
+extern struct hyper_dmabuf_private hyper_dmabuf_private;

static uint32_t hyper_dmabuf_id_gen(void) {
/* TODO: add proper implementation */
diff --git a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
index 3237e50..0166e61 100644
--- a/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
+++ b/drivers/xen/hyper_dmabuf/hyper_dmabuf_msg.c
@@ -3,12 +3,23 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/dma-buf.h>
+#include <xen/grant_table.h>
+#include <linux/workqueue.h>
+#include "hyper_dmabuf_drv.h"
#include "hyper_dmabuf_imp.h"
//#include "hyper_dmabuf_remote_sync.h"
#include "xen/hyper_dmabuf_xen_comm.h"
#include "hyper_dmabuf_msg.h"
#include "hyper_dmabuf_list.h"

+extern struct hyper_dmabuf_private hyper_dmabuf_private;
+
+struct cmd_process {
+ struct work_struct work;
+ struct hyper_dmabuf_ring_rq *rq;
+ int domid;
+};
+
void hyper_dmabuf_create_request(struct hyper_dmabuf_ring_rq *request,
enum hyper_dmabuf_command command, int *operands)
{
@@ -71,18 +82,17 @@ void hyper_dmabuf_create_request(struct hyper_dmabuf_ring_rq *request,
}
}

-int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_ring_rq *req)
+void cmd_process_work(struct work_struct *work)
{
- uint32_t i, ret;
struct hyper_dmabuf_imported_sgt_info *imported_sgt_info;
- struct hyper_dmabuf_sgt_info *sgt_info;
-
- /* make sure req is not NULL (may not be needed) */
- if (!req) {
- return -EINVAL;
- }
+ 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;

- req->status = HYPER_DMABUF_REQ_PROCESSED;
+ req = proc->rq;
+ domid = proc->domid;

switch (req->command) {
case HYPER_DMABUF_EXPORT:
@@ -115,33 +125,6 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_ring_rq *req)
hyper_dmabuf_register_imported(imported_sgt_info);
break;

- case HYPER_DMABUF_DESTROY:
- /* destroy sg_list for hyper_dmabuf_id on remote side */
- /* command : DMABUF_DESTROY,
- * operands0 : hyper_dmabuf_id
- */
-
- imported_sgt_info =
- hyper_dmabuf_find_imported(req->operands[0]);
-
- if (imported_sgt_info) {
- hyper_dmabuf_cleanup_imported_pages(imported_sgt_info);
-
- hyper_dmabuf_remove_imported(req->operands[0]);
-
- /* TODO: cleanup sgt on importer side etc */
- }
-
- /* Notify exporter that buffer is freed and it can cleanup it */
- req->status = HYPER_DMABUF_REQ_NEEDS_FOLLOW_UP;
- req->command = HYPER_DMABUF_DESTROY_FINISH;
-
-#if 0 /* function is not implemented yet */
-
- ret = hyper_dmabuf_destroy_sgt(req->hyper_dmabuf_id);
-#endif
- break;
-
case HYPER_DMABUF_DESTROY_FINISH:
/* destroy sg_list for hyper_dmabuf_id on local side */
/* command : DMABUF_DESTROY_FINISH,
@@ -180,33 +163,101 @@ int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_ring_rq *req)
*/
break;

- /* requesting the other side to setup another ring channel for reverse direction */
- case HYPER_DMABUF_EXPORTER_RING_SETUP:
- /* command: HYPER_DMABUF_EXPORTER_RING_SETUP
- * no operands needed */
+ case HYPER_DMABUF_IMPORTER_RING_SETUP:
+ /* command: HYPER_DMABUF_IMPORTER_RING_SETUP */
+ /* no operands needed */
+ hyper_dmabuf_importer_ringbuf_init(domid, req->operands[0], req->operands[1]);
+
+ break;
+
+ default:
+ /* shouldn't get here */
+ /* no matched command, nothing to do.. just return error */
+ break;
+ }
+
+ kfree(req);
+ kfree(proc);
+}
+
+int hyper_dmabuf_msg_parse(int domid, struct hyper_dmabuf_ring_rq *req)
+{
+ struct cmd_process *proc;
+ struct hyper_dmabuf_ring_rq *temp_req;
+ struct hyper_dmabuf_imported_sgt_info *imported_sgt_info;
+ int ret;
+
+ if (!req) {
+ printk("request is NULL\n");
+ return -EINVAL;
+ }
+
+ if ((req->command < HYPER_DMABUF_EXPORT) ||
+ (req->command > HYPER_DMABUF_IMPORTER_RING_SETUP)) {
+ printk("invalid command\n");
+ return -EINVAL;
+ }
+
+ req->status = HYPER_DMABUF_REQ_PROCESSED;
+
+ /* HYPER_DMABUF_EXPORTER_RING_SETUP requires immediate
+ * follow up so can't be processed in workqueue
+ */
+ if (req->command == HYPER_DMABUF_EXPORTER_RING_SETUP) {
ret = hyper_dmabuf_exporter_ringbuf_init(domid, &req->operands[0], &req->operands[1]);
if (ret < 0) {
req->status = HYPER_DMABUF_REQ_ERROR;
- return -EINVAL;
}

req->status = HYPER_DMABUF_REQ_NEEDS_FOLLOW_UP;
req->command = HYPER_DMABUF_IMPORTER_RING_SETUP;
- break;

- case HYPER_DMABUF_IMPORTER_RING_SETUP:
- /* command: HYPER_DMABUF_IMPORTER_RING_SETUP */
- /* no operands needed */
- ret = hyper_dmabuf_importer_ringbuf_init(domid, req->operands[0], req->operands[1]);
- if (ret < 0)
- return -EINVAL;
+ return req->command;
+ }

- break;
+ /* HYPER_DMABUF_DESTROY requires immediate
+ * follow up so can't be processed in workqueue
+ */
+ if (req->command == HYPER_DMABUF_DESTROY) {
+ /* destroy sg_list for hyper_dmabuf_id on remote side */
+ /* command : DMABUF_DESTROY,
+ * operands0 : hyper_dmabuf_id
+ */
+ imported_sgt_info =
+ hyper_dmabuf_find_imported(req->operands[0]);

- default:
- /* no matched command, nothing to do.. just return error */
- return -EINVAL;
+ if (imported_sgt_info) {
+ hyper_dmabuf_cleanup_imported_pages(imported_sgt_info);
+
+ hyper_dmabuf_remove_imported(req->operands[0]);
+
+ /* TODO: cleanup sgt on importer side etc */
+ }
+
+ /* Notify exporter that buffer is freed and it can cleanup it */
+ req->status = HYPER_DMABUF_REQ_NEEDS_FOLLOW_UP;
+ req->command = HYPER_DMABUF_DESTROY_FINISH;
+
+#if 0 /* function is not implemented yet */
+
+ ret = hyper_dmabuf_destroy_sgt(req->hyper_dmabuf_id);
+#endif
+ return req->command;
}

+ temp_req = (struct hyper_dmabuf_ring_rq *)kmalloc(sizeof(*temp_req), GFP_KERNEL);
+
+ memcpy(temp_req, req, sizeof(*temp_req));
+
+ proc = (struct cmd_process *) kcalloc(1, sizeof(struct cmd_process),
+ GFP_KERNEL);
+
+ proc->rq = temp_req;
+ proc->domid = domid;
+
+ INIT_WORK(&(proc->work), cmd_process_work);
+
+ queue_work(hyper_dmabuf_private.work_queue, &(proc->work));
+
return req->command;
}
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 22f2ef0..05855ba1 100644
--- a/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c
+++ b/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.c
@@ -14,7 +14,6 @@
#include "../hyper_dmabuf_msg.h"

static int export_req_id = 0;
-static int import_req_id = 0;

int32_t hyper_dmabuf_get_domid(void)
{
@@ -37,12 +36,6 @@ int hyper_dmabuf_next_req_id_export(void)
return export_req_id;
}

-int hyper_dmabuf_next_req_id_import(void)
-{
- import_req_id++;
- return import_req_id;
-}
-
/* For now cache latast rings as global variables TODO: keep them in list*/
static irqreturn_t hyper_dmabuf_front_ring_isr(int irq, void *dev_id);
static irqreturn_t hyper_dmabuf_back_ring_isr(int irq, void *dev_id);
@@ -81,7 +74,8 @@ int hyper_dmabuf_exporter_ringbuf_init(int rdomain, grant_ref_t *refid, int *por

alloc_unbound.dom = DOMID_SELF;
alloc_unbound.remote_dom = rdomain;
- ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, &alloc_unbound);
+ ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
+ &alloc_unbound);
if (ret != 0) {
printk("Cannot allocate event channel\n");
return -EINVAL;
@@ -96,7 +90,8 @@ int hyper_dmabuf_exporter_ringbuf_init(int rdomain, grant_ref_t *refid, int *por
printk("Failed to setup event channel\n");
close.port = alloc_unbound.port;
HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
- gnttab_end_foreign_access(ring_info->gref_ring, 0, virt_to_mfn(shared_ring));
+ gnttab_end_foreign_access(ring_info->gref_ring, 0,
+ virt_to_mfn(shared_ring));
return -EINVAL;
}

@@ -108,7 +103,8 @@ int hyper_dmabuf_exporter_ringbuf_init(int rdomain, grant_ref_t *refid, int *por
*refid = ring_info->gref_ring;
*port = ring_info->port;

- printk("%s: allocated eventchannel gref %d port: %d irq: %d\n", __func__,
+ printk("%s: allocated eventchannel gref %d port: %d irq: %d\n",
+ __func__,
ring_info->gref_ring,
ring_info->port,
ring_info->irq);
@@ -162,8 +158,9 @@ int hyper_dmabuf_importer_ringbuf_init(int sdomain, grant_ref_t gref, int port)

BACK_RING_INIT(&ring_info->ring_back, sring, PAGE_SIZE);

- ret = bind_interdomain_evtchn_to_irqhandler(sdomain, port, hyper_dmabuf_back_ring_isr, 0,
- NULL, (void*)ring_info);
+ ret = bind_interdomain_evtchn_to_irqhandler(sdomain, port,
+ hyper_dmabuf_back_ring_isr, 0,
+ NULL, (void*)ring_info);
if (ret < 0) {
return -EINVAL;
}
@@ -216,26 +213,20 @@ int hyper_dmabuf_send_request(int domain, struct hyper_dmabuf_ring_rq *req)
return 0;
}

-/* called by interrupt (WORKQUEUE) */
-int hyper_dmabuf_send_response(struct hyper_dmabuf_ring_rp* response, int domain)
-{
- /* as a importer and as a exporter */
- return 0;
-}
-
/* ISR for request from exporter (as an importer) */
-static irqreturn_t hyper_dmabuf_back_ring_isr(int irq, void *dev_id)
+static irqreturn_t hyper_dmabuf_back_ring_isr(int irq, void *info)
{
RING_IDX rc, rp;
- struct hyper_dmabuf_ring_rq request;
- struct hyper_dmabuf_ring_rp response;
+ struct hyper_dmabuf_ring_rq req;
+ struct hyper_dmabuf_ring_rp resp;
+
int notify, more_to_do;
int ret;
-// struct hyper_dmabuf_work *work;

- struct hyper_dmabuf_ring_info_import *ring_info = (struct hyper_dmabuf_ring_info_import *)dev_id;
+ struct hyper_dmabuf_ring_info_import *ring_info;
struct hyper_dmabuf_back_ring *ring;

+ ring_info = (struct hyper_dmabuf_ring_info_import *)info;
ring = &ring_info->ring_back;

do {
@@ -246,22 +237,16 @@ static irqreturn_t hyper_dmabuf_back_ring_isr(int irq, void *dev_id)
if (RING_REQUEST_CONS_OVERFLOW(ring, rc))
break;

- memcpy(&request, RING_GET_REQUEST(ring, rc), sizeof(request));
+ memcpy(&req, RING_GET_REQUEST(ring, rc), sizeof(req));
printk("Got request\n");
ring->req_cons = ++rc;

- /* TODO: probably using linked list for multiple requests then let
- * a task in a workqueue to process those is better idea becuase
- * we do not want to stay in ISR for long.
- */
- ret = hyper_dmabuf_msg_parse(ring_info->sdomain, &request);
+ ret = hyper_dmabuf_msg_parse(ring_info->sdomain, &req);

if (ret > 0) {
- /* build response */
- memcpy(&response, &request, sizeof(response));
-
- /* we sent back modified request as a response.. we might just need to have request only..*/
- memcpy(RING_GET_RESPONSE(ring, ring->rsp_prod_pvt), &response, sizeof(response));
+ memcpy(&resp, &req, sizeof(resp));
+ memcpy(RING_GET_RESPONSE(ring, ring->rsp_prod_pvt), &resp,
+ sizeof(resp));
ring->rsp_prod_pvt++;

RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(ring, notify);
@@ -281,15 +266,17 @@ static irqreturn_t hyper_dmabuf_back_ring_isr(int irq, void *dev_id)
}

/* ISR for responses from importer */
-static irqreturn_t hyper_dmabuf_front_ring_isr(int irq, void *dev_id)
+static irqreturn_t hyper_dmabuf_front_ring_isr(int irq, void *info)
{
/* front ring only care about response from back */
- struct hyper_dmabuf_ring_rp *response;
+ struct hyper_dmabuf_ring_rp *resp;
RING_IDX i, rp;
int more_to_do, ret;

- struct hyper_dmabuf_ring_info_export *ring_info = (struct hyper_dmabuf_ring_info_export *)dev_id;
+ struct hyper_dmabuf_ring_info_export *ring_info;
struct hyper_dmabuf_front_ring *ring;
+
+ ring_info = (struct hyper_dmabuf_ring_info_export *)info;
ring = &ring_info->ring_front;

do {
@@ -298,20 +285,18 @@ static irqreturn_t hyper_dmabuf_front_ring_isr(int irq, void *dev_id)
for (i = ring->rsp_cons; i != rp; i++) {
unsigned long id;

- response = RING_GET_RESPONSE(ring, i);
- id = response->response_id;
+ resp = RING_GET_RESPONSE(ring, i);
+ id = resp->response_id;

- if (response->status == HYPER_DMABUF_REQ_NEEDS_FOLLOW_UP) {
+ if (resp->status == HYPER_DMABUF_REQ_NEEDS_FOLLOW_UP) {
/* parsing response */
- ret = hyper_dmabuf_msg_parse(ring_info->rdomain, (struct hyper_dmabuf_ring_rq*)response);
+ ret = hyper_dmabuf_msg_parse(ring_info->rdomain,
+ (struct hyper_dmabuf_ring_rq *)resp);

if (ret < 0) {
printk("getting error while parsing response\n");
}
- } else if (response->status == HYPER_DMABUF_REQ_ERROR) {
- printk("remote domain %d couldn't process request %d\n", ring_info->rdomain, response->command);
}
-
}

ring->rsp_cons = i;
diff --git a/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.h b/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.h
index 2754917..4ad0529 100644
--- a/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.h
+++ b/drivers/xen/hyper_dmabuf/xen/hyper_dmabuf_xen_comm.h
@@ -36,17 +36,10 @@ struct hyper_dmabuf_ring_info_import {
struct hyper_dmabuf_back_ring ring_back;
};

-//struct hyper_dmabuf_work {
-// hyper_dmabuf_ring_rq requrest;
-// struct work_struct msg_parse;
-//};
-
int32_t hyper_dmabuf_get_domid(void);

int hyper_dmabuf_next_req_id_export(void);

-int hyper_dmabuf_next_req_id_import(void);
-
/* exporter needs to generated info for page sharing */
int hyper_dmabuf_exporter_ringbuf_init(int rdomain, grant_ref_t *gref, int *port);

--
2.7.4