workqueue: Add an API to create a singlethread workqueue attached to the current task's cgroup

Add a new kernel API to create a singlethread workqueue and attach it's task to current task's cgroup and cpumask.

Signed-off-by Sridhar Samudrala <[email protected]>

include/linux/workqueue.h +1 -0

index 9466e86..6d6f301 100644
@@ -211,6 +211,7 @@ __create_workqueue_key(const char *name, int singlethread,
 #define create_freezeable_workqueue(name) __create_workqueue((name), 1, 1, 0)
 #define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0, 0)
 
+extern struct workqueue_struct *create_singlethread_workqueue_in_current_cg(char *name); 
 extern void destroy_workqueue(struct workqueue_struct *wq);
 
 extern int queue_work(struct workqueue_struct *wq, struct work_struct *work);

kernel/workqueue.c +40 -0

index 5bfb213..6ba226e 100644
@@ -35,6 +35,7 @@
 #include <linux/lockdep.h>
 #define CREATE_TRACE_POINTS
 #include <trace/events/workqueue.h>
+#include <linux/cgroup.h>
 
 /*
  * The per-CPU workqueue (if single thread, we always use the first

@@ -193,6 +194,45 @@ static const struct cpumask *cpu_singlethread_map __read_mostly;
  */
 static cpumask_var_t cpu_populated_map __read_mostly;
 
+static struct task_struct *get_singlethread_wq_task(struct workqueue_struct *wq)
+{
+	return (per_cpu_ptr(wq->cpu_wq, singlethread_cpu))->thread;
+}
+
+/* Create a singlethread workqueue and attach it's task to the current task's
+ * cgroup and set it's cpumask to the current task's cpumask.
+ */
+struct workqueue_struct *create_singlethread_workqueue_in_current_cg(char *name)
+{
+	struct workqueue_struct *wq;
+	struct task_struct *task;
+	cpumask_var_t mask;
+
+	wq = create_singlethread_workqueue(name);
+	if (!wq)
+		goto out;
+
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+		goto err;
+			
+	if (sched_getaffinity(current->pid, mask))
+		goto err;
+
+	task = get_singlethread_wq_task(wq);
+	if (sched_setaffinity(task->pid, mask))
+		goto err;
+
+	if (cgroup_attach_task_current_cg(task))
+		goto err;
+out:	
+	return wq;
+err:
+	destroy_workqueue(wq);
+	wq = NULL;
+	goto out;
+}
+EXPORT_SYMBOL_GPL(create_singlethread_workqueue_in_current_cg);
+
 /* If it's single threaded, it isn't in the list of workqueues. */
 static inline int is_wq_single_threaded(struct workqueue_struct *wq)
 {

Quoted 4 lines · Sridhar Samudrala

Add a new kernel API to create a singlethread workqueue and attach it's task to current task's cgroup and cpumask.

Signed-off-by Sridhar Samudrala <[email protected]>

Could someone familiar with workqueue code please comment on whether this patch is suitable for 2.6.35?

It is needed to fix the case where vhost user might cause a kernel thread to consume more CPU than allowed by the cgroup. Should I merge it through the vhost tree?
Ack for this?

kernel/workqueue.c static const struct cpumask *cpu_singlethread_map __read_mostly; · line 200

Quoted 4 lines

kernel/workqueue.c +4 -0

+static struct task_struct *get_singlethread_wq_task(struct workqueue_struct *wq)
+{
+	return (per_cpu_ptr(wq->cpu_wq, singlethread_cpu))->thread;
+}

Quoted 24 lines

kernel/workqueue.c +24 -0

+/* Create a singlethread workqueue and attach it's task to the current task's
+ * cgroup and set it's cpumask to the current task's cpumask.
+ */
+struct workqueue_struct *create_singlethread_workqueue_in_current_cg(char *name)
+{
+	struct workqueue_struct *wq;
+	struct task_struct *task;
+	cpumask_var_t mask;
+
+	wq = create_singlethread_workqueue(name);
+	if (!wq)
+		goto out;
+
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+		goto err;
+			
+	if (sched_getaffinity(current->pid, mask))
+		goto err;
+
+	task = get_singlethread_wq_task(wq);
+	if (sched_setaffinity(task->pid, mask))
+		goto err;
+
+	if (cgroup_attach_task_current_cg(task))

kernel/workqueue.c static const struct cpumask *cpu_singlethread_map __read_mostly; · line 233

Quoted 8 lines

kernel/workqueue.c +8 -0

+		goto err;
+out:	
+	return wq;
+err:
+	destroy_workqueue(wq);
+	wq = NULL;
+	goto out;
+}

Quoted 10 lines · Michael S. Tsirkin

Quoted 4 lines · Sridhar Samudrala

Add a new kernel API to create a singlethread workqueue and attach it's task to current task's cgroup and cpumask.

Signed-off-by Sridhar Samudrala <[email protected]>

Could someone familiar with workqueue code please comment on whether this patch is suitable for 2.6.35?

Quoted 4 lines

It is needed to fix the case where vhost user might cause a kernel thread to consume more CPU than allowed by the cgroup. Should I merge it through the vhost tree?
Ack for this?

I don't understand the reasons for this patch, but this doesn't matter.

Quoted 16 lines · Oleg Nesterov

Quoted 14 lines · Michael S. Tsirkin

Quoted 4 lines · Sridhar Samudrala

Add a new kernel API to create a singlethread workqueue and attach it's task to current task's cgroup and cpumask.

Signed-off-by Sridhar Samudrala <[email protected]>

Could someone familiar with workqueue code please comment on whether this patch is suitable for 2.6.35?

It is needed to fix the case where vhost user might cause a kernel thread to consume more CPU than allowed by the cgroup. Should I merge it through the vhost tree?
Ack for this?

Quoted 1 line

I don't understand the reasons for this patch, but this doesn't matter.

Depending on userspace application, driver can create a lot of work for a workqueue to handle. By making the workqueue thread belong in a cgroup, we make it possible to the CPU and other resources thus consumed.

Quoted 1 line

I don't understand the reasons for this patch, but this doesn't matter.

Depending on userspace application, driver can create a lot of work for a workqueue to handle. By making the workqueue thread belong in a cgroup, we make it possible to the CPU and other resources thus consumed.

Hello,

Quoted 2 lines · Michael S. Tsirkin

Quoted 1 line

I don't understand the reasons for this patch, but this doesn't matter.

Quoted 4 lines

Depending on userspace application, driver can create a lot of work for a workqueue to handle. By making the workqueue thread belong in a cgroup, we make it possible to the CPU and other resources thus consumed.

Hmmm.... I don't really get it. The unit of scheduling in workqueue is a work.

Unless you're gonna convert every driver to use this special kind of workqueue (and what happens when multiple tasks from different cgroups share the driver?),

I can't see how this is gonna be
useful. If you really wanna impose cgroup control on workqueue items, you'll have to do it per work item which might lead to the problem of priority inversion.

Can you please describe what you're trying to do
in more detail?

Thank you.

Signature · 1 line

tejun

Quoted 3 lines

Unless you're gonna convert every driver to use this special kind of workqueue (and what happens when multiple tasks from different cgroups share the driver?),

We'll then create a workqueue per task. Each workqueue will have the right cgroup. But we are not trying to selve the problem for every driver.

Hello,

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

Unless you're gonna convert every driver to use this special kind of workqueue (and what happens when multiple tasks from different cgroups share the driver?),

Quoted 3 lines

We'll then create a workqueue per task. Each workqueue will have the right cgroup. But we are not trying to selve the problem for every driver.

Ah... I see. You're gonna use multiple workqueues. Once concern that I have is that this is abuse of workqueue interface to certain level and depends on the implementation detail of workqueue rather than its intended usage model.

stop_machine() was a similar case and in the
end it was better served by a different mechanism built on kthread directly (cpu_stop). Wouldn't it be cleaner to use kthread directly for your case too? You're basically trying to use workqueue as a frontend to kthread, so...

Thanks.

Signature · 1 line

tejun

Well, this is why I proposed adding a new API for creating workqueue within workqueue.c, rather than exposing the task and attaching it to cgroups in our driver: so that workqueue maintainers can fix the implementation if it ever changes.

And after all, it's an internal API, we can always change it later if we need.

Well, yes but we are using APIs like flush_work etc. These are very handy. It seems much easier than rolling our own queue on top of kthread.

Hello, Michael.

Quoted 7 lines · Michael S. Tsirkin

Well, this is why I proposed adding a new API for creating workqueue within workqueue.c, rather than exposing the task and attaching it to cgroups in our driver: so that workqueue maintainers can fix the implementation if it ever changes.

And after all, it's an internal API, we can always change it later if we need.

...

Quoted 2 lines

Well, yes but we are using APIs like flush_work etc. These are very handy. It seems much easier than rolling our own queue on top of kthread.

The thing is that this kind of one-off usage becomes problemetic when you're trying to change the implementation detail. All current workqueue users don't care which thread they run on and they shouldn't as each work owns the context only for the duration the work is executing. If this sort of fundamental guidelines are followed, the implementation can be improved in pretty much transparent way but when you start depending on specific implementation details, things become messy pretty quickly.

If this type of usage were more common, adding proper way to account work usage according to cgroups would make sense but that's not the case here and I removed the only exception case recently while trying to implement cmwq and if this is added. So, this would be the only one which makes such extra assumptions in the whole kernel. One way or the other, workqueue needs to be improved and I don't really think adding the single exception at this point is a good idea.

The thing I realized after stop_machine conversion was that there was no reason to use workqueue there at all. There already are more than enough not-too-difficult synchronization constructs and if you're using a thread for dedicated purposes, code complexity isn't that different either way. Plus, it would also be clearer that dedicated threads are required there for what reason. So, I strongly suggest using a kthread. If there are issues which are noticeably difficult to solve with kthread, we can definitely talk about that and think about things again.

Thank you.

Signature · 1 line

tejun

Well, we have create_singlethread_workqueue, right? This is not very different ... is it?

Just copying structures and code from workqueue.c, adding vhost_ in front of it will definitely work:

there is nothing magic about the workqueue library.
But this just involves cut and paste which might be best avoided.

One final idea before we go the cut and paste way: how about 'create_workqueue_from_task' that would get a thread and have workqueue run there?

Hello,

Quoted 5 lines · Michael S. Tsirkin

Well, we have create_singlethread_workqueue, right? This is not very different ... is it?

Just copying structures and code from workqueue.c, adding vhost_ in front of it will definitely work:

Sure it will, but you'll probably be able to get away with much less.

Quoted 2 lines

there is nothing magic about the workqueue library.
But this just involves cut and paste which might be best avoided.

What I'm saying is that some magic needs to be added to workqueue and if you add this single(!) exception, it will have to be backed out pretty soon, so it would be better to do it properly now.

Quoted 3 lines

One final idea before we go the cut and paste way: how about 'create_workqueue_from_task' that would get a thread and have workqueue run there?

You can currently depend on that implementation detail but it's not the workqueue interface is meant to do. The single threadedness is there as execution ordering and concurrency specification and it doesn't (or rather won't) necessarily mean that a specific single thread is bound to certain workqueue.

Can you please direct me to have a look at the code. I'll be happy to do the conversion for you.

Thanks.

Signature · 1 line

tejun

This conversion is to make each vhost use a dedicated kthread so that resource control via cgroup can be applied.

drivers/vhost/vhost.c +15 -0

+	if (kthread_should_stop()) {
+		__set_current_state(TASK_RUNNING);
+		return 0;
+	}
+
+	poll = NULL;
+	spin_lock(&dev->poller_lock);
+	if (!list_empty(&dev->poll_list)) {
+		poll = list_first_entry(&dev->poll_list,
+					struct vhost_poll, node);
+		list_del_init(&poll->node);
+	}
+	spin_unlock(&dev->poller_lock);
+
+	if (poll) {

Quoted 3 lines · Tejun Heo

This conversion is to make each vhost use a dedicated kthread so that resource control via cgroup can be applied.

Personally, I agree. I think This is better than play with workqueue thread.

A couple of simple questions after the quick glance at the unapplied patch...

drivers/vhost/vhost.c void vhost_poll_stop(struct vhost_poll * · line 100

Quoted 7 lines

void vhost_poll_flush(struct vhost_poll *poll)
{

drivers/vhost/vhost.c +4 -1

-	flush_work(&poll->work);
+	int seq = poll->queue_seq;
+
+	if (seq - poll->done_seq > 0)
+		wait_event(poll->done, seq - poll->done_seq <= 0);

The check before wait_event() is not needed, please note that wait_event() checks the condition before __wait_event().

What I can't understand is why we do have ->queue_seq and ->done_seq.

Isn't the single "bool poll->active" enough? vhost_poll_queue() sets ->active == T, vhost_poller() clears it before wake_up_all(poll->done).

drivers/vhost/vhost.c static void vhost_vq_reset(struct vhost_ · line 148

Quoted 7 lines

drivers/vhost/vhost.c +7 -0

+static int vhost_poller(void *data)
+{
+	struct vhost_dev *dev = data;
+	struct vhost_poll *poll;
+
+repeat:
+	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */

I don't understand the comment... why do we need this barrier?

Quoted 15 lines

drivers/vhost/vhost.c +15 -0

+	if (kthread_should_stop()) {
+		__set_current_state(TASK_RUNNING);
+		return 0;
+	}
+
+	poll = NULL;
+	spin_lock(&dev->poller_lock);
+	if (!list_empty(&dev->poll_list)) {
+		poll = list_first_entry(&dev->poll_list,
+					struct vhost_poll, node);
+		list_del_init(&poll->node);
+	}
+	spin_unlock(&dev->poller_lock);
+
+	if (poll) {

drivers/vhost/vhost.c static void vhost_vq_reset(struct vhost_ · line 174

Quoted 10 lines

drivers/vhost/vhost.c +10 -0

+		__set_current_state(TASK_RUNNING);
+		poll->fn(poll);
+		smp_wmb();	/* paired with rmb in vhost_poll_flush() */
+		poll->done_seq = poll->queue_seq;
+		wake_up_all(&poll->done);
+	} else
+		schedule();
+
+	goto repeat;
+}

Given that vhost_poll_queue() does list_add() and wake_up_process() under ->poller_lock, I don't think we need any barriers to change ->state.

IOW, can't vhost_poller() simply do

	while(!kthread_should_stop()) {

		poll = NULL;
		spin_lock(&dev->poller_lock);
		if (!list_empty(&dev->poll_list)) {
			...
		} else
			 __set_current_state(TASK_INTERRUPTIBLE);
		spin_unlock(&dev->poller_lock);

		if (poll) {
			...
		} else
			schedule();
	}

?

Quoted 4 lines · Oleg Nesterov

What I can't understand is why we do have ->queue_seq and ->done_seq.

Isn't the single "bool poll->active" enough? vhost_poll_queue() sets ->active == T, vhost_poller() clears it before wake_up_all(poll->done).

I might have slightly over engineered this part not knowing the expected workload. ->queue_seq/->done_seq pair is to guarantee that flushers never get starved.

Quoted 9 lines

drivers/vhost/vhost.c static void vhost_vq_reset(struct vhost_ · line 148

Quoted 7 lines

drivers/vhost/vhost.c +7 -0

+static int vhost_poller(void *data)
+{
+	struct vhost_dev *dev = data;
+	struct vhost_poll *poll;
+
+repeat:
+	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */

I don't understand the comment... why do we need this barrier?

So that either kthread_stop()'s should_stop = 1 in kthread_stop() is visible to kthread_should_stop() or task state is set to RUNNING.

Quoted 3 lines

I might have slightly over engineered this part not knowing the expected workload. ->queue_seq/->done_seq pair is to guarantee that flushers never get starved.

Ah, indeed.

Well, afaics we do not need 2 counters anyway, both vhost_poll_queue() and vhost_poller() could increment the single counter and the flusher can take bit 0 into account. But I agree 2 counters are much more clean.

Quoted 10 lines

Quoted 9 lines

drivers/vhost/vhost.c static void vhost_vq_reset(struct vhost_ · line 148

Quoted 7 lines

drivers/vhost/vhost.c +7 -0

+static int vhost_poller(void *data)
+{
+	struct vhost_dev *dev = data;
+	struct vhost_poll *poll;
+
+repeat:
+	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */

I don't understand the comment... why do we need this barrier?

Quoted 2 lines

So that either kthread_stop()'s should_stop = 1 in kthread_stop() is visible to kthread_should_stop() or task state is set to RUNNING.

Of course, you are right. I am really surprized I asked this question ;)

Replace vhost_workqueue with per-vhost kthread. Other than callback argument change from struct work_struct * to struct vhost_poll *, there's no visible change to vhost_poll_*() interface.

This conversion is to make each vhost use a dedicated kthread so that resource control via cgroup can be applied.

Partially based on Sridhar Samudrala's patch.

Cc Michael S. Tsirkin <[email protected]>

Cc Sridhar Samudrala <[email protected]>

---
Okay, here is three patch series to convert vhost to use per-vhost kthread, add cgroup_attach_task_current_cg() and apply it to the vhost kthreads. The conversion is mostly straight forward although flush is slightly tricky.

The problem is that I have no idea how to test this.

Index: work/drivers/vhost/vhost.c
===================================================================

drivers/vhost/vhost.c +0 -0

drivers/vhost/vhost.c +18 -0

@@ -125,10 +139,50 @@ static void vhost_vq_reset(struct vhost_
 	vq->log_ctx = NULL;
 }
 
+static int vhost_poller(void *data)
+{
+	struct vhost_dev *dev = data;
+	struct vhost_poll *poll;
+
+repeat:
+	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
+
+	if (kthread_should_stop()) {
+		__set_current_state(TASK_RUNNING);
+		return 0;
+	}
+
+	poll = NULL;
+	spin_lock(&dev->poller_lock);
+	if (!list_empty(&dev->poll_list)) {
+		poll = list_first_entry(&dev->poll_list,
+					struct vhost_poll, node);

drivers/vhost/vhost.c +18 -4

+	} else
+		schedule();
+
+	goto repeat;
+}
+
 long vhost_dev_init(struct vhost_dev *dev,
 		    struct vhost_virtqueue *vqs, int nvqs)
 {
+	struct task_struct *poller;
 	int i;
+
+	poller = kthread_create(vhost_poller, dev, "vhost-%d", current->pid);
+	if (IS_ERR(poller))
+		return PTR_ERR(poller);
+
 	dev->vqs = vqs;
 	dev->nvqs = nvqs;
 	mutex_init(&dev->mutex);

@@ -136,6 +190,9 @@ long vhost_dev_init(struct vhost_dev *de
 	dev->log_file = NULL;
 	dev->memory = NULL;
 	dev->mm = NULL;
+	spin_lock_init(&dev->poller_lock);
+	INIT_LIST_HEAD(&dev->poll_list);
+	dev->poller = poller;
 
 	for (i = 0; i < dev->nvqs; ++i) {
 		dev->vqs[i].dev = dev;

@@ -143,8 +200,7 @@ long vhost_dev_init(struct vhost_dev *de
 		vhost_vq_reset(dev, dev->vqs + i);
 		if (dev->vqs[i].handle_kick)
 			vhost_poll_init(&dev->vqs[i].poll,
-					dev->vqs[i].handle_kick,
-					POLLIN);
+					dev->vqs[i].handle_kick, POLLIN, dev);
 	}
 	return 0;
 }

@@ -217,6 +273,8 @@ void vhost_dev_cleanup(struct vhost_dev
 	if (dev->mm)
 		mmput(dev->mm);
 	dev->mm = NULL;
+
+	kthread_stop(dev->poller);
 }
 
 static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)

@@ -1113,16 +1171,3 @@ void vhost_disable_notify(struct vhost_v
 		vq_err(vq, "Failed to enable notification at %p: %d\n",
 		       &vq->used->flags, r);
 }
-
-int vhost_init(void)

drivers/vhost/vhost.c +0 -1

-}

Index: work/drivers/vhost/vhost.h
===================================================================

drivers/vhost/vhost.h +19 -13

@@ -5,7 +5,6 @@
 #include <linux/vhost.h>
 #include <linux/mm.h>
 #include <linux/mutex.h>
-#include <linux/workqueue.h>
 #include <linux/poll.h>
 #include <linux/file.h>
 #include <linux/skbuff.h>

@@ -20,19 +19,26 @@ enum {
 	VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
 };
 
+struct vhost_poll;
+typedef void (*vhost_poll_fn_t)(struct vhost_poll *poll);
+
 /* Poll a file (eventfd or socket) */
 /* Note: there's nothing vhost specific about this structure. */
 struct vhost_poll {
+	vhost_poll_fn_t		  fn;
 	poll_table                table;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
-	/* struct which will handle all actual work. */
-	struct work_struct        work;
+	struct list_head	  node;
+	wait_queue_head_t	  done;
 	unsigned long		  mask;
+	struct vhost_dev	 *dev;
+	int			  queue_seq;
+	int			  done_seq;
 };
 
-void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
-		     unsigned long mask);
+void vhost_poll_init(struct vhost_poll *poll, vhost_poll_fn_t fn,
+		     unsigned long mask, struct vhost_dev *dev);
 void vhost_poll_start(struct vhost_poll *poll, struct file *file);
 void vhost_poll_stop(struct vhost_poll *poll);
 void vhost_poll_flush(struct vhost_poll *poll);

@@ -63,7 +69,7 @@ struct vhost_virtqueue {
 	struct vhost_poll poll;
 
 	/* The routine to call when the Guest pings us, or timeout. */
-	work_func_t handle_kick;
+	vhost_poll_fn_t handle_kick;
 
 	/* Last available index we saw. */
 	u16 last_avail_idx;

@@ -86,11 +92,11 @@ struct vhost_virtqueue {
 	struct iovec hdr[VHOST_NET_MAX_SG];
 	size_t hdr_size;
 	/* We use a kind of RCU to access private pointer.
-	 * All readers access it from workqueue, which makes it possible to
-	 * flush the workqueue instead of synchronize_rcu. Therefore readers do
+	 * All readers access it from poller, which makes it possible to
+	 * flush the vhost_poll instead of synchronize_rcu. Therefore readers do
 	 * not need to call rcu_read_lock/rcu_read_unlock: the beginning of
-	 * work item execution acts instead of rcu_read_lock() and the end of
-	 * work item execution acts instead of rcu_read_lock().
+	 * vhost_poll execution acts instead of rcu_read_lock() and the end of
+	 * vhost_poll execution acts instead of rcu_read_lock().
 	 * Writers use virtqueue mutex. */
 	void *private_data;
 	/* Log write descriptors */

@@ -110,6 +116,9 @@ struct vhost_dev {
 	int nvqs;
 	struct file *log_file;
 	struct eventfd_ctx *log_ctx;
+	spinlock_t poller_lock;
+	struct list_head poll_list;
+	struct task_struct *poller;
 };
 
 long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue *vqs, int nvqs);

@@ -136,9 +145,6 @@ bool vhost_enable_notify(struct vhost_vi
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
 		    unsigned int log_num, u64 len);
 
-int vhost_init(void);
-void vhost_cleanup(void);
-
 #define vq_err(vq, fmt, ...) do {                                  \
 		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
 		if ((vq)->error_ctx)                               \

Quoted 3 lines · Tejun Heo

Replace vhost_workqueue with per-vhost kthread. Other than callback argument change from struct work_struct * to struct vhost_poll *, there's no visible change to vhost_poll_*() interface.

I would prefer a substructure vhost_work, even just to make the code easier to review and compare to workqueue.c.

Quoted 1 line

The problem is that I have no idea how to test this.

It's a 3 step process:

You should now be able to ping guest to host and back. Use something like netperf to stress the connection. Close qemu with kill -9 and unload module to test flushing code.

drivers/vhost/vhost.c static void vhost_vq_reset(struct vhost_ · line 169

Quoted 6 lines

drivers/vhost/vhost.c +6 -0

+	if (poll) {
+		__set_current_state(TASK_RUNNING);
+		poll->fn(poll);
+		smp_wmb();	/* paired with rmb in vhost_poll_flush() */
+		poll->done_seq = poll->queue_seq;
+		wake_up_all(&poll->done);

This seems to add wakeups on data path, which uses spinlocks etc. OTOH workqueue.c adds a special barrier entry which only does a wakeup when needed. Right?

drivers/vhost/vhost.c void vhost_disable_notify(struct vhost_v · line 1127

Quoted 3 lines

drivers/vhost/vhost.c +0 -3

-void vhost_cleanup(void)
-{
-	destroy_workqueue(vhost_workqueue);

I note that destroy_workqueue does a flush, kthread_stop doesn't. Right? Sure we don't need to check nothing is in one of the lists? Maybe add a BUG_ON?

Hello,

Quoted 7 lines · Michael S. Tsirkin

Quoted 3 lines · Tejun Heo

Replace vhost_workqueue with per-vhost kthread. Other than callback argument change from struct work_struct * to struct vhost_poll *, there's no visible change to vhost_poll_*() interface.

I would prefer a substructure vhost_work, even just to make the code easier to review and compare to workqueue.c.

Yeap, sure.

Quoted 2 lines

Quoted 1 line

The problem is that I have no idea how to test this.

Quoted 1 line

It's a 3 step process:

...

Quoted 3 lines

You should now be able to ping guest to host and back. Use something like netperf to stress the connection. Close qemu with kill -9 and unload module to test flushing code.

Thanks for the instruction. I'll see if there's a way to do it without building qemu myself on opensuse.

But please feel free to go
ahead and test it. It might just work! :-)

Quoted 7 lines

Quoted 6 lines

drivers/vhost/vhost.c +6 -0

+	if (poll) {
+		__set_current_state(TASK_RUNNING);
+		poll->fn(poll);
+		smp_wmb();	/* paired with rmb in vhost_poll_flush() */
+		poll->done_seq = poll->queue_seq;
+		wake_up_all(&poll->done);

Quoted 3 lines

This seems to add wakeups on data path, which uses spinlocks etc. OTOH workqueue.c adds a special barrier entry which only does a wakeup when needed. Right?

Yeah, well, if it's a really hot path sure we can avoid wake_up_all() in most cases. Do you think that would be necessary?

Quoted 7 lines

drivers/vhost/vhost.c void vhost_disable_notify(struct vhost_v · line 1127

Quoted 3 lines

drivers/vhost/vhost.c +0 -3

-void vhost_cleanup(void)
-{
-	destroy_workqueue(vhost_workqueue);

I note that destroy_workqueue does a flush, kthread_stop doesn't. Right? Sure we don't need to check nothing is in one of the lists? Maybe add a BUG_ON?

There were a bunch of flushes before kthread_stop() and they seemed to stop and flush everything. Aren't they enough?

We can definitely add
BUG_ON() after kthread_should_stop() check succeeds either way tho.

Thanks.

Signature · 1 line

tejun

Replace vhost_workqueue with per-vhost kthread. Other than callback argument change from struct work_struct * to struct vhost_work *, there's no visible change to vhost_poll_*() interface.

This conversion is to make each vhost use a dedicated kthread so that resource control via cgroup can be applied.

Partially based on Sridhar Samudrala's patch.

* Updated to use sub structure vhost_work instead of directly using vhost_poll at Michael's suggestion.

* Added flusher wake_up() optimization at Michael's suggestion.

Signed-off-by Tejun Heo<[email protected]>

Cc Michael S. Tsirkin<[email protected]>

Cc Sridhar Samudrala<[email protected]>

---
Okay, just tested it. dev->work_lock had to be updated to use irq operations but other than that it worked just fine. Copied a large file using scp and it seems to perform pretty well although I don't have any reference of comparison. So, here's the updated version with the sign off.

Thanks.

  drivers/vhost/net.c   |   56 ++++++++++---------------
  drivers/vhost/vhost.c |  111 ++++++++++++++++++++++++++++++++++++++------------
  drivers/vhost/vhost.h |   38 +++++++++++------
  3 files changed, 134 insertions(+), 71 deletions(-)

Index: work/drivers/vhost/net.c
===================================================================

drivers/vhost/net.c +24 -32

@@ -294,54 +294,58 @@ static void handle_rx(struct vhost_net *
  	unuse_mm(net->dev.mm);
  }
 
-static void handle_tx_kick(struct work_struct *work)
+static void handle_tx_kick(struct vhost_work *work)
  {
-	struct vhost_virtqueue *vq;
-	struct vhost_net *net;
-	vq = container_of(work, struct vhost_virtqueue, poll.work);
-	net = container_of(vq->dev, struct vhost_net, dev);
+	struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
+						  poll.work);
+	struct vhost_net *net = container_of(vq->dev, struct vhost_net, dev);
+
  	handle_tx(net);
  }
 
-static void handle_rx_kick(struct work_struct *work)
+static void handle_rx_kick(struct vhost_work *work)
  {
-	struct vhost_virtqueue *vq;
-	struct vhost_net *net;
-	vq = container_of(work, struct vhost_virtqueue, poll.work);
-	net = container_of(vq->dev, struct vhost_net, dev);
+	struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
+						  poll.work);
+	struct vhost_net *net = container_of(vq->dev, struct vhost_net, dev);
+
  	handle_rx(net);
  }
 
-static void handle_tx_net(struct work_struct *work)
+static void handle_tx_net(struct vhost_work *work)
  {
-	struct vhost_net *net;
-	net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_TX].work);
+	struct vhost_net *net = container_of(work, struct vhost_net,
+					     poll[VHOST_NET_VQ_TX].work);
  	handle_tx(net);
  }
 
-static void handle_rx_net(struct work_struct *work)
+static void handle_rx_net(struct vhost_work *work)
  {
-	struct vhost_net *net;
-	net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_RX].work);
+	struct vhost_net *net = container_of(work, struct vhost_net,
+					     poll[VHOST_NET_VQ_RX].work);
  	handle_rx(net);
  }
 
  static int vhost_net_open(struct inode *inode, struct file *f)
  {
  	struct vhost_net *n = kmalloc(sizeof *n, GFP_KERNEL);
+	struct vhost_dev *dev;
  	int r;
+
  	if (!n)
  		return -ENOMEM;
+
+	dev =&n->dev;
  	n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
  	n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;
-	r = vhost_dev_init(&n->dev, n->vqs, VHOST_NET_VQ_MAX);
+	r = vhost_dev_init(dev, n->vqs, VHOST_NET_VQ_MAX);
  	if (r<  0) {
  		kfree(n);
  		return r;
  	}
 
-	vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT);
-	vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN);
+	vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
+	vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
  	n->tx_poll_state = VHOST_NET_POLL_DISABLED;
 
  	f->private_data = n;

@@ -644,25 +648,13 @@ static struct miscdevice vhost_net_misc
 
  static int vhost_net_init(void)
  {
-	int r = vhost_init();
-	if (r)
-		goto err_init;
-	r = misc_register(&vhost_net_misc);
-	if (r)
-		goto err_reg;
-	return 0;
-err_reg:
-	vhost_cleanup();
-err_init:
-	return r;
-
+	return misc_register(&vhost_net_misc);
  }
  module_init(vhost_net_init);
 
  static void vhost_net_exit(void)
  {
  	misc_deregister(&vhost_net_misc);
-	vhost_cleanup();
  }
  module_exit(vhost_net_exit);
 

Index: work/drivers/vhost/vhost.c
===================================================================

drivers/vhost/vhost.c +85 -26

@@ -17,12 +17,12 @@
  #include<linux/mm.h>
  #include<linux/miscdevice.h>
  #include<linux/mutex.h>
-#include<linux/workqueue.h>
  #include<linux/rcupdate.h>
  #include<linux/poll.h>
  #include<linux/file.h>
  #include<linux/highmem.h>
  #include<linux/slab.h>
+#include<linux/kthread.h>
 
  #include<linux/net.h>
  #include<linux/if_packet.h>

@@ -37,8 +37,6 @@ enum {
  	VHOST_MEMORY_F_LOG = 0x1,
  };
 
-static struct workqueue_struct *vhost_workqueue;
-
  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
  			    poll_table *pt)
  {

@@ -52,23 +50,31 @@ static void vhost_poll_func(struct file
  static int vhost_poll_wakeup(wait_queue_t *wait, unsigned mode, int sync,
  			     void *key)
  {
-	struct vhost_poll *poll;
-	poll = container_of(wait, struct vhost_poll, wait);
+	struct vhost_poll *poll = container_of(wait, struct vhost_poll, wait);
+
  	if (!((unsigned long)key&  poll->mask))
  		return 0;
 
-	queue_work(vhost_workqueue,&poll->work);
+	vhost_poll_queue(poll);
  	return 0;
  }
 
  /* Init poll structure */
-void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
-		     unsigned long mask)
+void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
+		     unsigned long mask, struct vhost_dev *dev)
  {
-	INIT_WORK(&poll->work, func);
+	struct vhost_work *work =&poll->work;
+
  	init_waitqueue_func_entry(&poll->wait, vhost_poll_wakeup);
  	init_poll_funcptr(&poll->table, vhost_poll_func);
  	poll->mask = mask;
+	poll->dev = dev;
+
+	INIT_LIST_HEAD(&work->node);
+	work->fn = fn;
+	init_waitqueue_head(&work->done);
+	atomic_set(&work->flushing, 0);
+	work->queue_seq = work->done_seq = 0;
  }
 
  /* Start polling a file. We add ourselves to file's wait queue. The caller must

@@ -92,12 +98,29 @@ void vhost_poll_stop(struct vhost_poll *
   * locks that are also used by the callback. */
  void vhost_poll_flush(struct vhost_poll *poll)
  {
-	flush_work(&poll->work);
+	struct vhost_work *work =&poll->work;
+	int seq = work->queue_seq;
+
+	atomic_inc(&work->flushing);
+	smp_mb__after_atomic_inc();	/* mb flush-b0 paired with worker-b1 */
+	wait_event(work->done, seq - work->done_seq<= 0);
+	atomic_dec(&work->flushing);
+	smp_mb__after_atomic_dec();	/* rmb flush-b1 paired with worker-b0 */
  }
 
  void vhost_poll_queue(struct vhost_poll *poll)
  {
-	queue_work(vhost_workqueue,&poll->work);
+	struct vhost_dev *dev = poll->dev;
+	struct vhost_work *work =&poll->work;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->work_lock, flags);
+	if (list_empty(&work->node)) {
+		list_add_tail(&work->node,&dev->work_list);
+		work->queue_seq++;
+		wake_up_process(dev->worker);
+	}
+	spin_unlock_irqrestore(&dev->work_lock, flags);
  }
 
  static void vhost_vq_reset(struct vhost_dev *dev,

@@ -125,10 +148,52 @@ static void vhost_vq_reset(struct vhost_
  	vq->log_ctx = NULL;
  }
 
+static int vhost_worker(void *data)
+{
+	struct vhost_dev *dev = data;
+	struct vhost_work *work;
+
+repeat:
+	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
+
+	if (kthread_should_stop()) {
+		__set_current_state(TASK_RUNNING);
+		return 0;
+	}
+
+	work = NULL;
+	spin_lock_irq(&dev->work_lock);
+	if (!list_empty(&dev->work_list)) {
+		work = list_first_entry(&dev->work_list,
+					struct vhost_work, node);
+		list_del_init(&work->node);
+	}
+	spin_unlock_irq(&dev->work_lock);
+
+	if (work) {
+		__set_current_state(TASK_RUNNING);
+		work->fn(work);
+		smp_wmb();	/* wmb worker-b0 paired with flush-b1 */
+		work->done_seq = work->queue_seq;
+		smp_mb();	/* mb worker-b1 paired with flush-b0 */
+		if (atomic_read(&work->flushing))
+			wake_up_all(&work->done);
+	} else
+		schedule();
+
+	goto repeat;
+}
+
  long vhost_dev_init(struct vhost_dev *dev,
  		    struct vhost_virtqueue *vqs, int nvqs)
  {
+	struct task_struct *worker;
  	int i;
+
+	worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
+	if (IS_ERR(worker))
+		return PTR_ERR(worker);
+
  	dev->vqs = vqs;
  	dev->nvqs = nvqs;
  	mutex_init(&dev->mutex);

@@ -136,6 +201,9 @@ long vhost_dev_init(struct vhost_dev *de
  	dev->log_file = NULL;
  	dev->memory = NULL;
  	dev->mm = NULL;
+	spin_lock_init(&dev->work_lock);
+	INIT_LIST_HEAD(&dev->work_list);
+	dev->worker = worker;
 
  	for (i = 0; i<  dev->nvqs; ++i) {
  		dev->vqs[i].dev = dev;

@@ -143,9 +211,10 @@ long vhost_dev_init(struct vhost_dev *de
  		vhost_vq_reset(dev, dev->vqs + i);
  		if (dev->vqs[i].handle_kick)
  			vhost_poll_init(&dev->vqs[i].poll,
-					dev->vqs[i].handle_kick,
-					POLLIN);
+					dev->vqs[i].handle_kick, POLLIN, dev);
  	}
+
+	wake_up_process(worker);	/* avoid contributing to loadavg */
  	return 0;
  }
 

@@ -217,6 +286,9 @@ void vhost_dev_cleanup(struct vhost_dev
  	if (dev->mm)
  		mmput(dev->mm);
  	dev->mm = NULL;
+
+	WARN_ON(!list_empty(&dev->work_list));
+	kthread_stop(dev->worker);
  }
 
  static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)

@@ -1113,16 +1185,3 @@ void vhost_disable_notify(struct vhost_v
  		vq_err(vq, "Failed to enable notification at %p: %d\n",
  		&vq->used->flags, r);
  }
-
-int vhost_init(void)
-{
-	vhost_workqueue = create_singlethread_workqueue("vhost");
-	if (!vhost_workqueue)
-		return -ENOMEM;
-	return 0;
-}
-
-void vhost_cleanup(void)
-{
-	destroy_workqueue(vhost_workqueue);
-}

Index: work/drivers/vhost/vhost.h
===================================================================

drivers/vhost/vhost.h +25 -13

@@ -5,13 +5,13 @@
  #include<linux/vhost.h>
  #include<linux/mm.h>
  #include<linux/mutex.h>
-#include<linux/workqueue.h>
  #include<linux/poll.h>
  #include<linux/file.h>
  #include<linux/skbuff.h>
  #include<linux/uio.h>
  #include<linux/virtio_config.h>
  #include<linux/virtio_ring.h>
+#include<asm/atomic.h>
 
  struct vhost_device;
 

@@ -20,19 +20,31 @@ enum {
  	VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
  };
 
+struct vhost_work;
+typedef void (*vhost_work_fn_t)(struct vhost_work *work);
+
+struct vhost_work {
+	struct list_head	  node;
+	vhost_work_fn_t		  fn;
+	wait_queue_head_t	  done;
+	atomic_t		  flushing;
+	int			  queue_seq;
+	int			  done_seq;
+};
+
  /* Poll a file (eventfd or socket) */
  /* Note: there's nothing vhost specific about this structure. */
  struct vhost_poll {
  	poll_table                table;
  	wait_queue_head_t        *wqh;
  	wait_queue_t              wait;
-	/* struct which will handle all actual work. */
-	struct work_struct        work;
+	struct vhost_work	  work;
  	unsigned long		  mask;
+	struct vhost_dev	 *dev;
  };
 
-void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
-		     unsigned long mask);
+void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
+		     unsigned long mask, struct vhost_dev *dev);
  void vhost_poll_start(struct vhost_poll *poll, struct file *file);
  void vhost_poll_stop(struct vhost_poll *poll);
  void vhost_poll_flush(struct vhost_poll *poll);

@@ -63,7 +75,7 @@ struct vhost_virtqueue {
  	struct vhost_poll poll;
 
  	/* The routine to call when the Guest pings us, or timeout. */
-	work_func_t handle_kick;
+	vhost_work_fn_t handle_kick;
 
  	/* Last available index we saw. */
  	u16 last_avail_idx;

@@ -86,11 +98,11 @@ struct vhost_virtqueue {
  	struct iovec hdr[VHOST_NET_MAX_SG];
  	size_t hdr_size;
  	/* We use a kind of RCU to access private pointer.
-	 * All readers access it from workqueue, which makes it possible to
-	 * flush the workqueue instead of synchronize_rcu. Therefore readers do
+	 * All readers access it from worker, which makes it possible to
+	 * flush the vhost_work instead of synchronize_rcu. Therefore readers do
  	 * not need to call rcu_read_lock/rcu_read_unlock: the beginning of
-	 * work item execution acts instead of rcu_read_lock() and the end of
-	 * work item execution acts instead of rcu_read_lock().
+	 * vhost_work execution acts instead of rcu_read_lock() and the end of
+	 * vhost_work execution acts instead of rcu_read_lock().
  	 * Writers use virtqueue mutex. */
  	void *private_data;
  	/* Log write descriptors */

@@ -110,6 +122,9 @@ struct vhost_dev {
  	int nvqs;
  	struct file *log_file;
  	struct eventfd_ctx *log_ctx;
+	spinlock_t work_lock;
+	struct list_head work_list;
+	struct task_struct *worker;
  };
 
  long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue *vqs, int nvqs);

@@ -136,9 +151,6 @@ bool vhost_enable_notify(struct vhost_vi
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
  		    unsigned int log_num, u64 len);
 
-int vhost_init(void);
-void vhost_cleanup(void);
-
  #define vq_err(vq, fmt, ...) do {                                  \
  		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
  		if ((vq)->error_ctx)                               \

Replace vhost_workqueue with per-vhost kthread. Other than callback argument change from struct work_struct * to struct vhost_work *, there's no visible change to vhost_poll_*() interface.

This conversion is to make each vhost use a dedicated kthread so that resource control via cgroup can be applied.

Partially based on Sridhar Samudrala's patch.

* Updated to use sub structure vhost_work instead of directly using vhost_poll at Michael's suggestion.

* Added flusher wake_up() optimization at Michael's suggestion.

Signed-off-by Tejun Heo <[email protected]>

Cc Michael S. Tsirkin <[email protected]>

Cc Sridhar Samudrala <[email protected]>

All the tricky barrier pairing made me uncomfortable. So I came up with this on top (untested): if we do all operations under the spinlock, we can get by without barriers and atomics. And since we need the lock for list operations anyway, this should have no paerformance impact.

What do you think?

drivers/vhost/vhost.c +7 -10

@@ -151,37 +161,37 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 static int vhost_worker(void *data)
 {
 	struct vhost_dev *dev = data;
-	struct vhost_work *work;
+	struct vhost_work *work = NULL;
 
-repeat:
-	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
 
-	if (kthread_should_stop()) {
-		__set_current_state(TASK_RUNNING);
-		return 0;
-	}
+		if (kthread_should_stop()) {
+			__set_current_state(TASK_RUNNING);
+			return 0;
+		}
 
-	work = NULL;
-	spin_lock_irq(&dev->work_lock);
-	if (!list_empty(&dev->work_list)) {

Hello,

Quoted 6 lines · Michael S. Tsirkin

All the tricky barrier pairing made me uncomfortable. So I came up with this on top (untested): if we do all operations under the spinlock, we can get by without barriers and atomics. And since we need the lock for list operations anyway, this should have no paerformance impact.

What do you think?

I've created kthread_worker in wq#for-next tree and already converted ivtv to use it. Once this lands in mainline, I think converting vhost to use it would be better choice. kthread worker code uses basically the same logic used in the vhost_workqueue code but is better organized and documented. So, I think it would be better to stick with the original implementation, as otherwise we're likely to just decrease test coverage without much gain.

http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=b56c0d8937e665a27d90517ee7a746d0aa05af46;hp=53c5f5ba42c194cb13dd3083ed425f2c5b1ec439

Quoted 21 lines

drivers/vhost/vhost.c +7 -7

@@ -151,37 +161,37 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 static int vhost_worker(void *data)
 {
 	struct vhost_dev *dev = data;
-	struct vhost_work *work;
+	struct vhost_work *work = NULL;
 
-repeat:
-	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
 
-	if (kthread_should_stop()) {
-		__set_current_state(TASK_RUNNING);
-		return 0;
-	}
+		if (kthread_should_stop()) {
+			__set_current_state(TASK_RUNNING);
+			return 0;
+		}
 

drivers/vhost/vhost.c static void vhost_vq_reset(struct vhost_dev *dev, · line 178

Quoted 13 lines

drivers/vhost/vhost.c +5 -8

-	work = NULL;
-	spin_lock_irq(&dev->work_lock);
-	if (!list_empty(&dev->work_list)) {
-		work = list_first_entry(&dev->work_list,
-					struct vhost_work, node);
-		list_del_init(&work->node);
-	}
-	spin_unlock_irq(&dev->work_lock);
+		spin_lock_irq(&dev->work_lock);
+		if (work) {
+			work->done_seq = work->queue_seq;
+			if (work->flushing)
+				wake_up_all(&work->done);

I don't think doing this before executing the function is correct,

so
you'll have to release the lock, execute the function, regrab the lock and then do the flush processing.

Thanks.

Signature · 1 line

tejun

Quoted 8 lines

I've created kthread_worker in wq#for-next tree and already converted ivtv to use it. Once this lands in mainline, I think converting vhost to use it would be better choice. kthread worker code uses basically the same logic used in the vhost_workqueue code but is better organized and documented. So, I think it would be better to stick with the original implementation, as otherwise we're likely to just decrease test coverage without much gain.

Quoted 1 line

http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=b56c0d8937e665a27d90517ee7a746d0aa05af46;hp=53c5f5ba42c194cb13dd3083ed425f2c5b1ec439

Sure, if we keep using workqueue. But I'd like to investigate this direction a bit more because there's discussion to switching from kthread to regular threads altogether.

Quoted 1 line

I don't think doing this before executing the function is correct,

Well, before I execute the function work is NULL, so this is skipped. Correct?

Quoted 4 lines

so
you'll have to release the lock, execute the function, regrab the lock and then do the flush processing.

Quoted 1 line

Thanks.

It's done in the loop, so I thought we can reuse the locking done for the sake of processing the next work item. Makes sense?

Hello,

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines

I've created kthread_worker in wq#for-next tree and already converted ivtv to use it. Once this lands in mainline, I think converting vhost to use it would be better choice. kthread worker code uses basically the same logic used in the vhost_workqueue code but is better organized and documented. So, I think it would be better to stick with the original implementation, as otherwise we're likely to just decrease test coverage without much gain.

http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=b56c0d8937e665a27d90517ee7a746d0aa05af46;hp=53c5f5ba42c194cb13dd3083ed425f2c5b1ec439

Quoted 3 lines

Sure, if we keep using workqueue. But I'd like to investigate this direction a bit more because there's discussion to switching from kthread to regular threads altogether.

Hmmm? It doesn't have much to do with workqueue. kthread_worker is a simple wrapper around kthread. It now assumes kthread but changing it to be useable with any thread shouldn't be too hard. Wouldn't that be better?

Quoted 9 lines

Quoted 1 line

I don't think doing this before executing the function is correct,

Well, before I execute the function work is NULL, so this is skipped. Correct?

Quoted 4 lines

so
you'll have to release the lock, execute the function, regrab the lock and then do the flush processing.

Quoted 5 lines

Quoted 1 line

Thanks.

It's done in the loop, so I thought we can reuse the locking done for the sake of processing the next work item. Makes sense?

Yeap, right. I think it would make much more sense to use common code when it becomes available but if you think the posted change is necessary till then, please feel free to go ahead.

Thanks.

Signature · 1 line

tejun

Hello,

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines

I've created kthread_worker in wq#for-next tree and already converted ivtv to use it. Once this lands in mainline, I think converting vhost to use it would be better choice. kthread worker code uses basically the same logic used in the vhost_workqueue code but is better organized and documented. So, I think it would be better to stick with the original implementation, as otherwise we're likely to just decrease test coverage without much gain.

http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=b56c0d8937e665a27d90517ee7a746d0aa05af46;hp=53c5f5ba42c194cb13dd3083ed425f2c5b1ec439

Quoted 3 lines

Sure, if we keep using workqueue. But I'd like to investigate this direction a bit more because there's discussion to switching from kthread to regular threads altogether.

Hmmm? It doesn't have much to do with workqueue. kthread_worker is a simple wrapper around kthread. It now assumes kthread but changing it to be useable with any thread shouldn't be too hard. Wouldn't that be better?

BTW, kthread_worker would benefit from the optimization I implemented here as well.

Hello,

Quoted 2 lines · Michael S. Tsirkin

BTW, kthread_worker would benefit from the optimization I implemented here as well.

Hmmm... I'm not quite sure whether it's an optimization. I thought the patch was due to feeling uncomfortable about using barriers? Is it an optimization?

Quoted 9 lines · Tejun Heo

Hello,

Quoted 2 lines · Michael S. Tsirkin

BTW, kthread_worker would benefit from the optimization I implemented here as well.

Hmmm... I'm not quite sure whether it's an optimization. I thought the patch was due to feeling uncomfortable about using barriers? Is it an optimization?

Yeah, one less smp_mb() in execution path. The lock dancing in flush() is ugly but then again mucking with barriers could be harder to understand. Care to send a patch against wq#for-next tree?

Thanks.

Signature · 1 line

tejun

Hello,

Quoted 2 lines · Michael S. Tsirkin

BTW, kthread_worker would benefit from the optimization I implemented here as well.

Hmmm... I'm not quite sure whether it's an optimization. I thought the patch was due to feeling uncomfortable about using barriers?

Is it an optimization?

Thanks.

Signature · 1 line

tejun

Quoted 2 lines

Hmmm... I'm not quite sure whether it's an optimization. I thought the patch was due to feeling uncomfortable about using barriers?

Oh yes. But getting rid of barriers is what motivated me originally.

Is there a git tree with kthread_worker applied?
I might do this just for fun ...

* Placing try_to_freeze() could be a bit annoying. It shouldn't be executed when there's a work to flush.

Just one more thing.

Quoted 2 lines · Tejun Heo

* Placing try_to_freeze() could be a bit annoying. It shouldn't be executed when there's a work to flush.

* Similar issue exists for kthread_stop(). The kthread shouldn't exit while there's a work to flush (please note that kthread_worker interface allows detaching / attaching worker kthread during operation, so it should remain in consistent state with regard to flushing).

Thanks.

Signature · 1 line

tejun

Quoted 3 lines · Tejun Heo

Quoted 2 lines

* Placing try_to_freeze() could be a bit annoying. It shouldn't be executed when there's a work to flush.

BTW why is this important?
We could always get another work and flush right after try_to_freeze, and then flush would block for a long time.

BTW the vhost patch you sent does not do this at all. I am guessing it is because our thread is not freezable?

Quoted 5 lines

* Similar issue exists for kthread_stop(). The kthread shouldn't exit while there's a work to flush (please note that kthread_worker interface allows detaching / attaching worker kthread during operation, so it should remain in consistent state with regard to flushing).

Not sure I agree here. Users must synchronise flush and stop calls. Otherwise a work might get queued after stop is called, and you won't be able to flush it.

Hello,

Quoted 10 lines · Michael S. Tsirkin

Quoted 3 lines · Tejun Heo

Quoted 2 lines

* Placing try_to_freeze() could be a bit annoying. It shouldn't be executed when there's a work to flush.

BTW why is this important?
We could always get another work and flush right after try_to_freeze, and then flush would block for a long time.

BTW the vhost patch you sent does not do this at all. I am guessing it is because our thread is not freezable?

Yeap, I think so.

Quoted 6 lines

Quoted 5 lines

* Similar issue exists for kthread_stop(). The kthread shouldn't exit while there's a work to flush (please note that kthread_worker interface allows detaching / attaching worker kthread during operation, so it should remain in consistent state with regard to flushing).

Quoted 3 lines

Not sure I agree here. Users must synchronise flush and stop calls. Otherwise a work might get queued after stop is called, and you won't be able to flush it.

For freeze, it probably is okay but for stop, I think it's better to keep the semantics straight forward.

It may be okay to do otherwise
but having such oddity in generic interface is nasty and may lead to surprises which can be pretty difficult to track down later on. It's just a bit more of annoyance while writing the generic code, so...

Thanks.

Signature · 1 line

tejun

Quoted 2 lines

For freeze, it probably is okay but for stop, I think it's better to keep the semantics straight forward.

What are the semantics then? What do we want stop followed by queue and flush to do?

Just one more thing.

Quoted 1 line · Tejun Heo

Just one more thing.

I noticed that with vhost, flush_work was getting the worker pointer as well. Can we live with this API change?

Quoted 2 lines · Michael S. Tsirkin

I noticed that with vhost, flush_work was getting the worker pointer as well. Can we live with this API change?

Yeah, the flushing mechanism wouldn't work reliably if the work is queued to a different worker without flushing, so yeah passing in @worker might actually be better.

Hello,

Quoted 7 lines · Tejun Heo

Quoted 2 lines · Michael S. Tsirkin

I noticed that with vhost, flush_work was getting the worker pointer as well. Can we live with this API change?

Yeah, the flushing mechanism wouldn't work reliably if the work is queued to a different worker without flushing, so yeah passing in @worker might actually be better.

Thinking a bit more about it, it kind of sucks that queueing to another worker from worker->func() breaks flush. Maybe the right thing to do there is using atomic_t for done_seq? It pays a bit more overhead but maybe that's justifiable to keep the API saner? It would be great if it can be fixed somehow even if it means that the work has to be separately flushed for each worker it has been on before being destroyed.

Or, if flushing has to be associated with a specific worker anyway, maybe it would be better to move the sequence counter to kthread_worker and do it similarly with the original workqueue so that work can be destroyed once execution starts? Then, it can at least remain semantically identical to the original workqueue.

Thanks.

Signature · 1 line

tejun

Hello,

Quoted 4 lines · Tejun Heo

Quoted 2 lines · Michael S. Tsirkin

I noticed that with vhost, flush_work was getting the worker pointer as well. Can we live with this API change?

Quoted 3 lines

Yeah, the flushing mechanism wouldn't work reliably if the work is queued to a different worker without flushing, so yeah passing in @worker might actually be better.

Thinking a bit more about it, it kind of sucks that queueing to another worker from worker->func() breaks flush. Maybe the right thing to do there is using atomic_t for done_seq?

It pays a bit more
overhead but maybe that's justifiable to keep the API saner? It would be great if it can be fixed somehow even if it means that the work has to be separately flushed for each worker it has been on before being destroyed.

Or, if flushing has to be associated with a specific worker anyway, maybe it would be better to move the sequence counter to kthread_worker and do it similarly with the original workqueue so that work can be destroyed once execution starts? Then, it can at least remain semantically identical to the original workqueue.

Thanks.

Signature · 1 line

tejun

Quoted 3 lines

Thinking a bit more about it, it kind of sucks that queueing to another worker from worker->func() breaks flush. Maybe the right thing to do there is using atomic_t for done_seq?

I don't believe it will help: we might have:

worker1 runs work
work requeues itself queued index = 1 worker1 reads queued index = 1 worker2 runs work
work requeues itself queued index = 2 worker2 runs work
worker2 reads queued index = 2 worker2 writes done index = 2 worker1 writes done index = 1

As you see, done index got moved back.

Quoted 16 lines · Michael S. Tsirkin

Quoted 3 lines

Thinking a bit more about it, it kind of sucks that queueing to another worker from worker->func() breaks flush. Maybe the right thing to do there is using atomic_t for done_seq?

I don't believe it will help: we might have:

worker1 runs work
work requeues itself queued index = 1 worker1 reads queued index = 1 worker2 runs work
work requeues itself queued index = 2 worker2 runs work
worker2 reads queued index = 2 worker2 writes done index = 2 worker1 writes done index = 1

Quoted 1 line

As you see, done index got moved back.

Yeah, I think the flushing logic should be moved to the worker. Are you interested in doing it w/ your change?

Thanks.

Signature · 1 line

tejun

I'm unsure how flush_work operates under these conditions. E.g. in workqueue.c, this seems to work by keeping a pointer to current workqueue in the work. But what prevents us from destroying the workqueue when work might not be running?

Is this currently broken if you use multiple workqueues for the same work? If yes, I propose we do as I did, making flush_work get worker pointer, and only flushing on that worker.

Hello,

Quoted 4 lines · Michael S. Tsirkin

Quoted 2 lines

Hmmm... I'm not quite sure whether it's an optimization. I thought the patch was due to feeling uncomfortable about using barriers?

Oh yes. But getting rid of barriers is what motivated me originally.

Yeah, getting rid of barriers is always good. :-)

Quoted 2 lines

Is there a git tree with kthread_worker applied?
I might do this just for fun ...

git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-next

For the original implementaiton, please take a look at commit b56c0d8937e665a27d90517ee7a746d0aa05af46.

* Can you please keep the outer goto repeat loop? I just don't like outermost for (;;).

* Placing try_to_freeze() could be a bit annoying. It shouldn't be executed when there's a work to flush.

* I think A - B <= 0 test would be more familiar. At least time_before/after() are implemented that way.

Thanks.

Signature · 1 line

tejun

Quoted 2 lines

* Can you please keep the outer goto repeat loop? I just don't like outermost for (;;).

Okay ... can we put the code in a {} scope to make it clear where does the loop starts and ends?

Quoted 2 lines

* Placing try_to_freeze() could be a bit annoying. It shouldn't be executed when there's a work to flush.

It currently seems to be executed when there is work to flush. Is this wrong?

Quoted 2 lines

* I think A - B <= 0 test would be more familiar. At least time_before/after() are implemented that way.

I am concerned that this overflows a signed integer - which I seem to remeber that C99 disallows.

timer macros are on data path so might be worth the risk there, but flush is slow path so better be safe?

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

* Can you please keep the outer goto repeat loop? I just don't like outermost for (;;).

Okay ... can we put the code in a {} scope to make it clear where does the loop starts and ends?

If we're gonna do that, it would be better to put it inside a loop construct. The reason why I don't like it is that loops like that don't really help read/writeability much while indenting the whole logic unnecessarily and look more like a result of obsession against goto rather than any practical reason. It's just a cosmetic preference and I might as well be the weirdo here, so if you feel strong about it, please feel free to put everything in a loop.

Quoted 3 lines

Quoted 2 lines

* Placing try_to_freeze() could be a bit annoying. It shouldn't be executed when there's a work to flush.

Quoted 2 lines

It currently seems to be executed when there is work to flush. Is this wrong?

Oh, does it? As I wrote in the other mail, things like that wouldn't necessarily break correctness but I think it would be better to avoid surprises in the generic code if not too difficult.

Quoted 5 lines

Quoted 2 lines

* I think A - B <= 0 test would be more familiar. At least time_before/after() are implemented that way.

I am concerned that this overflows a signed integer - which I seem to remeber that C99 disallows.

Really? Overflows of pointer isn't expected and that's why we have weird RELOC_HIDE() macro for such calculations but integers not expected to overflow is a news to me. Are you sure? That basically means time_before/after() aren't safe either.

Quoted 2 lines

timer macros are on data path so might be worth the risk there, but flush is slow path so better be safe?

I don't think performance matters much here. I just think the sign test is clearer / more familiar for the logic.

Thanks.

Signature · 1 line

tejun

Let's try to define what do we want to achieve then. Do you want code that flushes workers not to block when workers are frozen? How will we handle work submitted when worker is frozen?

Apply the cpumask and cgroup of the initializing task to the created vhost worker.

Based on Sridhar Samudrala's patch. Li Zefan spotted a bug in error path (twice), fixed (twice).

Signed-off-by Tejun Heo <[email protected]>

Cc Michael S. Tsirkin <[email protected]>

Cc Sridhar Samudrala <[email protected]>

Cc Li Zefan <[email protected]>

---
 drivers/vhost/vhost.c |   34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

Index: work/drivers/vhost/vhost.c
===================================================================

drivers/vhost/vhost.c +30 -4

@@ -23,6 +23,7 @@
 #include <linux/highmem.h>
 #include <linux/slab.h>
 #include <linux/kthread.h>
+#include <linux/cgroup.h>
 
 #include <linux/net.h>
 #include <linux/if_packet.h>

@@ -187,11 +188,29 @@ long vhost_dev_init(struct vhost_dev *de
 		    struct vhost_virtqueue *vqs, int nvqs)
 {
 	struct task_struct *worker;
-	int i;
+	cpumask_var_t mask;
+	int i, ret = -ENOMEM;
+
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+		goto out_free_mask;
 
 	worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
-	if (IS_ERR(worker))
-		return PTR_ERR(worker);
+	if (IS_ERR(worker)) {
+		ret = PTR_ERR(worker);
+		goto out_free_mask;
+	}
+
+	ret = sched_getaffinity(current->pid, mask);
+	if (ret)
+		goto out_stop_worker;
+
+	ret = sched_setaffinity(worker->pid, mask);
+	if (ret)
+		goto out_stop_worker;
+
+	ret = cgroup_attach_task_current_cg(worker);
+	if (ret)
+		goto out_stop_worker;
 
 	dev->vqs = vqs;
 	dev->nvqs = nvqs;

@@ -214,7 +233,14 @@ long vhost_dev_init(struct vhost_dev *de
 	}
 
 	wake_up_process(worker);	/* avoid contributing to loadavg */
-	return 0;
+	ret = 0;
+	goto out_free_mask;
+
+out_stop_worker:
+	kthread_stop(worker);
+out_free_mask:
+	free_cpumask_var(mask);
+	return ret;
 }
 
 /* Caller should have device mutex */

Something that I wanted to figure out - what happens if the CPU mask limits us to a certain CPU that subsequently goes offline?

Will e.g. flush block forever or until that CPU comes back? Also, does singlethreaded workqueue behave in the same way?

Apply the cpumask and cgroup of the initializing task to the created vhost worker.

Based on Sridhar Samudrala's patch. Li Zefan spotted a bug in error path (twice), fixed (twice).

Signed-off-by Tejun Heo <[email protected]>

Cc Michael S. Tsirkin <[email protected]>

Cc Sridhar Samudrala <[email protected]>

Cc Li Zefan <[email protected]>

---
 drivers/vhost/vhost.c |   34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

Index: work/drivers/vhost/vhost.c
===================================================================

drivers/vhost/vhost.c +1 -0

@@ -23,6 +23,7 @@
 #include <linux/highmem.h>
 #include <linux/slab.h>
 #include <linux/kthread.h>
+#include <linux/cgroup.h>
 
 #include <linux/net.h>
 #include <linux/if_packet.h>

drivers/vhost/vhost.c +24 -4

 	worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
-	if (IS_ERR(worker))
-		return PTR_ERR(worker);
+	if (IS_ERR(worker)) {
+		ret = PTR_ERR(worker);
+		goto out_free_mask;
+	}
+
+	ret = sched_getaffinity(current->pid, mask);
+	if (ret)
+		goto out_stop_worker;
+
+	ret = sched_setaffinity(worker->pid, mask);
+	if (ret)
+		goto out_stop_worker;
+
+	ret = cgroup_attach_task_current_cg(worker);
+	if (ret)
+		goto out_stop_worker;
 
 	dev->vqs = vqs;
 	dev->nvqs = nvqs;

@@ -214,7 +233,14 @@ long vhost_dev_init(struct vhost_dev *de
 	}
 
 	wake_up_process(worker);	/* avoid contributing to loadavg */
-	return 0;
+	ret = 0;
+	goto out_free_mask;
+
+out_stop_worker:
+	kthread_stop(worker);
+out_free_mask:
+	free_cpumask_var(mask);
+	return ret;
 }
 
 /* Caller should have device mutex */
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

drivers/vhost/vhost.c long vhost_dev_init(struct vhost_dev *de · line 195

Quoted 6 lines

drivers/vhost/vhost.c +5 -1

-	int i;
+	cpumask_var_t mask;
+	int i, ret = -ENOMEM;
+
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+		goto out_free_mask;

I think this is another bug in the error path. You should simply do a return instead of a goto here when aloc_cpu_mask fails.

Quoted 3 lines · Tejun Heo

Replace vhost_workqueue with per-vhost kthread. Other than callback argument change from struct work_struct * to struct vhost_poll *, there's no visible change to vhost_poll_*() interface.

I would prefer a substructure vhost_work, even just to make the code easier to review and compare to workqueue.c.

Quoted 7 lines

This conversion is to make each vhost use a dedicated kthread so that resource control via cgroup can be applied.

Partially based on Sridhar Samudrala's patch.

Cc Michael S. Tsirkin <[email protected]>

Cc Sridhar Samudrala <[email protected]>

Quoted 7 lines

---
Okay, here is three patch series to convert vhost to use per-vhost kthread, add cgroup_attach_task_current_cg() and apply it to the vhost kthreads. The conversion is mostly straight forward although flush is slightly tricky.

The problem is that I have no idea how to test this.

It's a 3 step process:

1.
Install qemu-kvm under fc13, or build recent one from source, get it from here:
git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git

2. install guest under it:
qemu-img create -f qcow2 disk.qcow2 100G
Now get some image (e.g. fedora 13 in fc13.iso)
and install guest:
qemu-kvm -enable-kvm -m 1G -cdrom fc13.iso -drive file=disk.qcow2

3. set up networking. I usually simply do host to guest on a special subnet for testing purposes:

Set up a bridge named mstbr0:

ifconfig mstbr0 down
brctl delbr mstbr0
brctl addbr mstbr0
brctl setfd mstbr0 0
ifconfig mstbr0 11.0.0.1

cat > ifup << EOF
#!/bin/sh -x
/sbin/ifconfig msttap0 0.0.0.0 up
brctl addif mstbr0 msttap0
EOF

qemu-kvm -enable-kvm -m 1G -cdrom fc13.iso -drive file=disk.qcow2
-net nic,model=virtio,netdev=foo -netdev
tap,id=foo,ifname=msttap0,script=/home/mst/ifup,downscript=no,vhost=on

after you set up the guest, log into it and
ifconfig eth0 11.0.0.2

You should now be able to ping guest to host and back. Use something like netperf to stress the connection. Close qemu with kill -9 and unload module to test flushing code.

Quoted 4 lines

Index: work/drivers/vhost/vhost.c
===================================================================

drivers/vhost/vhost.c +0 -0

...

Quoted 22 lines

drivers/vhost/vhost.c +18 -0

@@ -125,10 +139,50 @@ static void vhost_vq_reset(struct vhost_
 	vq->log_ctx = NULL;
 }
 
+static int vhost_poller(void *data)
+{
+	struct vhost_dev *dev = data;
+	struct vhost_poll *poll;
+
+repeat:
+	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
+
+	if (kthread_should_stop()) {
+		__set_current_state(TASK_RUNNING);
+		return 0;
+	}
+
+	poll = NULL;
+	spin_lock(&dev->poller_lock);
+	if (!list_empty(&dev->poll_list)) {
+		poll = list_first_entry(&dev->poll_list,
+					struct vhost_poll, node);

drivers/vhost/vhost.c static void vhost_vq_reset(struct vhost_ · line 169

Quoted 10 lines

drivers/vhost/vhost.c +10 -0

+		list_del_init(&poll->node);
+	}
+	spin_unlock(&dev->poller_lock);
+
+	if (poll) {
+		__set_current_state(TASK_RUNNING);
+		poll->fn(poll);
+		smp_wmb();	/* paired with rmb in vhost_poll_flush() */
+		poll->done_seq = poll->queue_seq;
+		wake_up_all(&poll->done);

This seems to add wakeups on data path, which uses spinlocks etc. OTOH workqueue.c adds a special barrier
entry which only does a wakeup when needed.
Right?

Quoted 54 lines

drivers/vhost/vhost.c +18 -4

+	} else
+		schedule();
+
+	goto repeat;
+}
+
 long vhost_dev_init(struct vhost_dev *dev,
 		    struct vhost_virtqueue *vqs, int nvqs)
 {
+	struct task_struct *poller;
 	int i;
+
+	poller = kthread_create(vhost_poller, dev, "vhost-%d", current->pid);
+	if (IS_ERR(poller))
+		return PTR_ERR(poller);
+
 	dev->vqs = vqs;
 	dev->nvqs = nvqs;
 	mutex_init(&dev->mutex);

@@ -136,6 +190,9 @@ long vhost_dev_init(struct vhost_dev *de
 	dev->log_file = NULL;
 	dev->memory = NULL;
 	dev->mm = NULL;
+	spin_lock_init(&dev->poller_lock);
+	INIT_LIST_HEAD(&dev->poll_list);
+	dev->poller = poller;
 
 	for (i = 0; i < dev->nvqs; ++i) {
 		dev->vqs[i].dev = dev;

@@ -143,8 +200,7 @@ long vhost_dev_init(struct vhost_dev *de
 		vhost_vq_reset(dev, dev->vqs + i);
 		if (dev->vqs[i].handle_kick)
 			vhost_poll_init(&dev->vqs[i].poll,
-					dev->vqs[i].handle_kick,
-					POLLIN);
+					dev->vqs[i].handle_kick, POLLIN, dev);
 	}
 	return 0;
 }

@@ -217,6 +273,8 @@ void vhost_dev_cleanup(struct vhost_dev
 	if (dev->mm)
 		mmput(dev->mm);
 	dev->mm = NULL;
+
+	kthread_stop(dev->poller);
 }
 
 static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)

@@ -1113,16 +1171,3 @@ void vhost_disable_notify(struct vhost_v
 		vq_err(vq, "Failed to enable notification at %p: %d\n",
 		       &vq->used->flags, r);
 }
-
-int vhost_init(void)

drivers/vhost/vhost.c void vhost_disable_notify(struct vhost_v · line 1127

Quoted 10 lines

drivers/vhost/vhost.c +0 -10

-{
-	vhost_workqueue = create_singlethread_workqueue("vhost");
-	if (!vhost_workqueue)
-		return -ENOMEM;
-	return 0;
-}
-
-void vhost_cleanup(void)
-{
-	destroy_workqueue(vhost_workqueue);

I note that destroy_workqueue does a flush, kthread_stop doesn't. Right? Sure we don't need to check nothing is in one of the lists? Maybe add a BUG_ON?

Quoted 80 lines

drivers/vhost/vhost.c +0 -1

-}

Index: work/drivers/vhost/vhost.h
===================================================================

drivers/vhost/vhost.h +19 -10

@@ -5,7 +5,6 @@
 #include <linux/vhost.h>
 #include <linux/mm.h>
 #include <linux/mutex.h>
-#include <linux/workqueue.h>
 #include <linux/poll.h>
 #include <linux/file.h>
 #include <linux/skbuff.h>

@@ -20,19 +19,26 @@ enum {
 	VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
 };
 
+struct vhost_poll;
+typedef void (*vhost_poll_fn_t)(struct vhost_poll *poll);
+
 /* Poll a file (eventfd or socket) */
 /* Note: there's nothing vhost specific about this structure. */
 struct vhost_poll {
+	vhost_poll_fn_t		  fn;
 	poll_table                table;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
-	/* struct which will handle all actual work. */
-	struct work_struct        work;
+	struct list_head	  node;
+	wait_queue_head_t	  done;
 	unsigned long		  mask;
+	struct vhost_dev	 *dev;
+	int			  queue_seq;
+	int			  done_seq;
 };
 
-void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
-		     unsigned long mask);
+void vhost_poll_init(struct vhost_poll *poll, vhost_poll_fn_t fn,
+		     unsigned long mask, struct vhost_dev *dev);
 void vhost_poll_start(struct vhost_poll *poll, struct file *file);
 void vhost_poll_stop(struct vhost_poll *poll);
 void vhost_poll_flush(struct vhost_poll *poll);

@@ -63,7 +69,7 @@ struct vhost_virtqueue {
 	struct vhost_poll poll;
 
 	/* The routine to call when the Guest pings us, or timeout. */
-	work_func_t handle_kick;
+	vhost_poll_fn_t handle_kick;
 
 	/* Last available index we saw. */
 	u16 last_avail_idx;

@@ -86,11 +92,11 @@ struct vhost_virtqueue {
 	struct iovec hdr[VHOST_NET_MAX_SG];
 	size_t hdr_size;
 	/* We use a kind of RCU to access private pointer.
-	 * All readers access it from workqueue, which makes it possible to
-	 * flush the workqueue instead of synchronize_rcu. Therefore readers do
+	 * All readers access it from poller, which makes it possible to
+	 * flush the vhost_poll instead of synchronize_rcu. Therefore readers do
 	 * not need to call rcu_read_lock/rcu_read_unlock: the beginning of
-	 * work item execution acts instead of rcu_read_lock() and the end of
-	 * work item execution acts instead of rcu_read_lock().
+	 * vhost_poll execution acts instead of rcu_read_lock() and the end of
+	 * vhost_poll execution acts instead of rcu_read_lock().
 	 * Writers use virtqueue mutex. */
 	void *private_data;
 	/* Log write descriptors */

@@ -110,6 +116,9 @@ struct vhost_dev {
 	int nvqs;
 	struct file *log_file;
 	struct eventfd_ctx *log_ctx;
+	spinlock_t poller_lock;
+	struct list_head poll_list;
+	struct task_struct *poller;
 };
 
 long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue *vqs, int nvqs);

drivers/vhost/vhost.h bool vhost_enable_notify(struct vhost_vi · line 150

Quoted 9 lines

drivers/vhost/vhost.h +0 -3

@@ -136,9 +145,6 @@ bool vhost_enable_notify(struct vhost_vi
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
 		    unsigned int log_num, u64 len);
 
-int vhost_init(void);
-void vhost_cleanup(void);
-
 #define vq_err(vq, fmt, ...) do {                                  \
 		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
 		if ((vq)->error_ctx)                               \

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/