* [RFC PATCH 1/4] vhost: Introduce a universal thread to serve all users
2015-07-13 4:07 [RFC PATCH 0/4] Shared vhost design Bandan Das
@ 2015-07-13 4:07 ` Bandan Das
[not found] ` <OF8AF3E3F8.F0120188-ONC2257E8E.00740E46-C2257E90.0035BD30@il.ibm.com>
2015-08-10 9:27 ` Michael S. Tsirkin
2015-07-13 4:07 ` [RFC PATCH 2/4] vhost: Limit the number of devices served by a single worker thread Bandan Das
` (4 subsequent siblings)
5 siblings, 2 replies; 18+ messages in thread
From: Bandan Das @ 2015-07-13 4:07 UTC (permalink / raw)
To: kvm
Cc: netdev, linux-kernel, mst, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang
vhost threads are per-device, but in most cases a single thread
is enough. This change creates a single thread that is used to
serve all guests.
However, this complicates cgroups associations. The current policy
is to attach the per-device thread to all cgroups of the parent process
that the device is associated it. This is no longer possible if we
have a single thread. So, we end up moving the thread around to
cgroups of whichever device that needs servicing. This is a very
inefficient protocol but seems to be the only way to integrate
cgroups support.
Signed-off-by: Razya Ladelsky <razya@il.ibm.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
drivers/vhost/scsi.c | 15 +++--
drivers/vhost/vhost.c | 150 ++++++++++++++++++++++++--------------------------
drivers/vhost/vhost.h | 19 +++++--
3 files changed, 97 insertions(+), 87 deletions(-)
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index ea32b38..6c42936 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -535,7 +535,7 @@ static void vhost_scsi_complete_cmd(struct vhost_scsi_cmd *cmd)
llist_add(&cmd->tvc_completion_list, &vs->vs_completion_list);
- vhost_work_queue(&vs->dev, &vs->vs_completion_work);
+ vhost_work_queue(vs->dev.worker, &vs->vs_completion_work);
}
static int vhost_scsi_queue_data_in(struct se_cmd *se_cmd)
@@ -1282,7 +1282,7 @@ vhost_scsi_send_evt(struct vhost_scsi *vs,
}
llist_add(&evt->list, &vs->vs_event_list);
- vhost_work_queue(&vs->dev, &vs->vs_event_work);
+ vhost_work_queue(vs->dev.worker, &vs->vs_event_work);
}
static void vhost_scsi_evt_handle_kick(struct vhost_work *work)
@@ -1335,8 +1335,8 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
/* Flush both the vhost poll and vhost work */
for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
vhost_scsi_flush_vq(vs, i);
- vhost_work_flush(&vs->dev, &vs->vs_completion_work);
- vhost_work_flush(&vs->dev, &vs->vs_event_work);
+ vhost_work_flush(vs->dev.worker, &vs->vs_completion_work);
+ vhost_work_flush(vs->dev.worker, &vs->vs_event_work);
/* Wait for all reqs issued before the flush to be finished */
for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
@@ -1584,8 +1584,11 @@ static int vhost_scsi_open(struct inode *inode, struct file *f)
if (!vqs)
goto err_vqs;
- vhost_work_init(&vs->vs_completion_work, vhost_scsi_complete_cmd_work);
- vhost_work_init(&vs->vs_event_work, vhost_scsi_evt_work);
+ vhost_work_init(&vs->dev, &vs->vs_completion_work,
+ vhost_scsi_complete_cmd_work);
+
+ vhost_work_init(&vs->dev, &vs->vs_event_work,
+ vhost_scsi_evt_work);
vs->vs_events_nr = 0;
vs->vs_events_missed = false;
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2ee2826..951c96b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -11,6 +11,8 @@
* Generic code for virtio server in host kernel.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/eventfd.h>
#include <linux/vhost.h>
#include <linux/uio.h>
@@ -28,6 +30,9 @@
#include "vhost.h"
+/* Just one worker thread to service all devices */
+static struct vhost_worker *worker;
+
enum {
VHOST_MEMORY_MAX_NREGIONS = 64,
VHOST_MEMORY_F_LOG = 0x1,
@@ -58,13 +63,15 @@ static int vhost_poll_wakeup(wait_queue_t *wait, unsigned mode, int sync,
return 0;
}
-void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn)
+void vhost_work_init(struct vhost_dev *dev,
+ struct vhost_work *work, vhost_work_fn_t fn)
{
INIT_LIST_HEAD(&work->node);
work->fn = fn;
init_waitqueue_head(&work->done);
work->flushing = 0;
work->queue_seq = work->done_seq = 0;
+ work->dev = dev;
}
EXPORT_SYMBOL_GPL(vhost_work_init);
@@ -78,7 +85,7 @@ void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
poll->dev = dev;
poll->wqh = NULL;
- vhost_work_init(&poll->work, fn);
+ vhost_work_init(dev, &poll->work, fn);
}
EXPORT_SYMBOL_GPL(vhost_poll_init);
@@ -116,30 +123,30 @@ void vhost_poll_stop(struct vhost_poll *poll)
}
EXPORT_SYMBOL_GPL(vhost_poll_stop);
-static bool vhost_work_seq_done(struct vhost_dev *dev, struct vhost_work *work,
- unsigned seq)
+static bool vhost_work_seq_done(struct vhost_worker *worker,
+ struct vhost_work *work, unsigned seq)
{
int left;
- spin_lock_irq(&dev->work_lock);
+ spin_lock_irq(&worker->work_lock);
left = seq - work->done_seq;
- spin_unlock_irq(&dev->work_lock);
+ spin_unlock_irq(&worker->work_lock);
return left <= 0;
}
-void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work)
+void vhost_work_flush(struct vhost_worker *worker, struct vhost_work *work)
{
unsigned seq;
int flushing;
- spin_lock_irq(&dev->work_lock);
+ spin_lock_irq(&worker->work_lock);
seq = work->queue_seq;
work->flushing++;
- spin_unlock_irq(&dev->work_lock);
- wait_event(work->done, vhost_work_seq_done(dev, work, seq));
- spin_lock_irq(&dev->work_lock);
+ spin_unlock_irq(&worker->work_lock);
+ wait_event(work->done, vhost_work_seq_done(worker, work, seq));
+ spin_lock_irq(&worker->work_lock);
flushing = --work->flushing;
- spin_unlock_irq(&dev->work_lock);
+ spin_unlock_irq(&worker->work_lock);
BUG_ON(flushing < 0);
}
EXPORT_SYMBOL_GPL(vhost_work_flush);
@@ -148,29 +155,30 @@ EXPORT_SYMBOL_GPL(vhost_work_flush);
* locks that are also used by the callback. */
void vhost_poll_flush(struct vhost_poll *poll)
{
- vhost_work_flush(poll->dev, &poll->work);
+ vhost_work_flush(poll->dev->worker, &poll->work);
}
EXPORT_SYMBOL_GPL(vhost_poll_flush);
-void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work)
+void vhost_work_queue(struct vhost_worker *worker,
+ struct vhost_work *work)
{
unsigned long flags;
- spin_lock_irqsave(&dev->work_lock, flags);
+ spin_lock_irqsave(&worker->work_lock, flags);
if (list_empty(&work->node)) {
- list_add_tail(&work->node, &dev->work_list);
+ list_add_tail(&work->node, &worker->work_list);
work->queue_seq++;
- spin_unlock_irqrestore(&dev->work_lock, flags);
- wake_up_process(dev->worker);
+ spin_unlock_irqrestore(&worker->work_lock, flags);
+ wake_up_process(worker->thread);
} else {
- spin_unlock_irqrestore(&dev->work_lock, flags);
+ spin_unlock_irqrestore(&worker->work_lock, flags);
}
}
EXPORT_SYMBOL_GPL(vhost_work_queue);
void vhost_poll_queue(struct vhost_poll *poll)
{
- vhost_work_queue(poll->dev, &poll->work);
+ vhost_work_queue(poll->dev->worker, &poll->work);
}
EXPORT_SYMBOL_GPL(vhost_poll_queue);
@@ -203,19 +211,18 @@ static void vhost_vq_reset(struct vhost_dev *dev,
static int vhost_worker(void *data)
{
- struct vhost_dev *dev = data;
+ struct vhost_worker *worker = data;
struct vhost_work *work = NULL;
unsigned uninitialized_var(seq);
mm_segment_t oldfs = get_fs();
set_fs(USER_DS);
- use_mm(dev->mm);
for (;;) {
/* mb paired w/ kthread_stop */
set_current_state(TASK_INTERRUPTIBLE);
- spin_lock_irq(&dev->work_lock);
+ spin_lock_irq(&worker->work_lock);
if (work) {
work->done_seq = seq;
if (work->flushing)
@@ -223,21 +230,35 @@ static int vhost_worker(void *data)
}
if (kthread_should_stop()) {
- spin_unlock_irq(&dev->work_lock);
+ spin_unlock_irq(&worker->work_lock);
__set_current_state(TASK_RUNNING);
break;
}
- if (!list_empty(&dev->work_list)) {
- work = list_first_entry(&dev->work_list,
+ if (!list_empty(&worker->work_list)) {
+ work = list_first_entry(&worker->work_list,
struct vhost_work, node);
list_del_init(&work->node);
seq = work->queue_seq;
} else
work = NULL;
- spin_unlock_irq(&dev->work_lock);
+ spin_unlock_irq(&worker->work_lock);
if (work) {
+ struct vhost_dev *dev = work->dev;
+
__set_current_state(TASK_RUNNING);
+
+ if (current->mm != dev->mm) {
+ unuse_mm(current->mm);
+ use_mm(dev->mm);
+ }
+
+ /* TODO: Consider a more elegant solution */
+ if (worker->owner != dev->owner) {
+ /* Should check for return value */
+ cgroup_attach_task_all(dev->owner, current);
+ worker->owner = dev->owner;
+ }
work->fn(work);
if (need_resched())
schedule();
@@ -245,7 +266,6 @@ static int vhost_worker(void *data)
schedule();
}
- unuse_mm(dev->mm);
set_fs(oldfs);
return 0;
}
@@ -304,9 +324,8 @@ void vhost_dev_init(struct vhost_dev *dev,
dev->log_file = NULL;
dev->memory = NULL;
dev->mm = NULL;
- spin_lock_init(&dev->work_lock);
- INIT_LIST_HEAD(&dev->work_list);
- dev->worker = NULL;
+ dev->worker = worker;
+ dev->owner = current;
for (i = 0; i < dev->nvqs; ++i) {
vq = dev->vqs[i];
@@ -331,31 +350,6 @@ long vhost_dev_check_owner(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
-struct vhost_attach_cgroups_struct {
- struct vhost_work work;
- struct task_struct *owner;
- int ret;
-};
-
-static void vhost_attach_cgroups_work(struct vhost_work *work)
-{
- struct vhost_attach_cgroups_struct *s;
-
- s = container_of(work, struct vhost_attach_cgroups_struct, work);
- s->ret = cgroup_attach_task_all(s->owner, current);
-}
-
-static int vhost_attach_cgroups(struct vhost_dev *dev)
-{
- struct vhost_attach_cgroups_struct attach;
-
- attach.owner = current;
- vhost_work_init(&attach.work, vhost_attach_cgroups_work);
- vhost_work_queue(dev, &attach.work);
- vhost_work_flush(dev, &attach.work);
- return attach.ret;
-}
-
/* Caller should have device mutex */
bool vhost_dev_has_owner(struct vhost_dev *dev)
{
@@ -366,7 +360,6 @@ EXPORT_SYMBOL_GPL(vhost_dev_has_owner);
/* Caller should have device mutex */
long vhost_dev_set_owner(struct vhost_dev *dev)
{
- struct task_struct *worker;
int err;
/* Is there an owner already? */
@@ -377,28 +370,15 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
/* No owner, become one */
dev->mm = get_task_mm(current);
- worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
- if (IS_ERR(worker)) {
- err = PTR_ERR(worker);
- goto err_worker;
- }
-
dev->worker = worker;
- wake_up_process(worker); /* avoid contributing to loadavg */
-
- err = vhost_attach_cgroups(dev);
- if (err)
- goto err_cgroup;
err = vhost_dev_alloc_iovecs(dev);
if (err)
- goto err_cgroup;
+ goto err_alloc;
return 0;
-err_cgroup:
- kthread_stop(worker);
+err_alloc:
dev->worker = NULL;
-err_worker:
if (dev->mm)
mmput(dev->mm);
dev->mm = NULL;
@@ -472,11 +452,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
/* No one will access memory at this point */
kfree(dev->memory);
dev->memory = NULL;
- WARN_ON(!list_empty(&dev->work_list));
- if (dev->worker) {
- kthread_stop(dev->worker);
- dev->worker = NULL;
- }
if (dev->mm)
mmput(dev->mm);
dev->mm = NULL;
@@ -1567,11 +1542,32 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify);
static int __init vhost_init(void)
{
+ struct vhost_worker *w =
+ kzalloc(sizeof(*w), GFP_KERNEL);
+ if (!w)
+ return -ENOMEM;
+
+ w->thread = kthread_create(vhost_worker,
+ w, "vhost-worker");
+ if (IS_ERR(w->thread))
+ return PTR_ERR(w->thread);
+
+ worker = w;
+ spin_lock_init(&worker->work_lock);
+ INIT_LIST_HEAD(&worker->work_list);
+ wake_up_process(worker->thread);
+ pr_info("Created universal thread to service requests\n");
+
return 0;
}
static void __exit vhost_exit(void)
{
+ if (worker) {
+ kthread_stop(worker->thread);
+ WARN_ON(!list_empty(&worker->work_list));
+ kfree(worker);
+ }
}
module_init(vhost_init);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 8c1c792..2f204ce 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -22,6 +22,7 @@ struct vhost_work {
int flushing;
unsigned queue_seq;
unsigned done_seq;
+ struct vhost_dev *dev;
};
/* Poll a file (eventfd or socket) */
@@ -35,8 +36,8 @@ struct vhost_poll {
struct vhost_dev *dev;
};
-void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn);
-void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work);
+void vhost_work_init(struct vhost_dev *dev,
+ struct vhost_work *work, vhost_work_fn_t fn);
void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
unsigned long mask, struct vhost_dev *dev);
@@ -44,7 +45,6 @@ int 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);
void vhost_poll_queue(struct vhost_poll *poll);
-void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work);
long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp);
struct vhost_log {
@@ -116,11 +116,22 @@ struct vhost_dev {
int nvqs;
struct file *log_file;
struct eventfd_ctx *log_ctx;
+ /* vhost shared worker */
+ struct vhost_worker *worker;
+ /* for cgroup support */
+ struct task_struct *owner;
+};
+
+struct vhost_worker {
spinlock_t work_lock;
struct list_head work_list;
- struct task_struct *worker;
+ struct task_struct *thread;
+ struct task_struct *owner;
};
+void vhost_work_queue(struct vhost_worker *worker,
+ struct vhost_work *work);
+void vhost_work_flush(struct vhost_worker *worker, struct vhost_work *work);
void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int nvqs);
long vhost_dev_set_owner(struct vhost_dev *dev);
bool vhost_dev_has_owner(struct vhost_dev *dev);
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread[parent not found: <OF8AF3E3F8.F0120188-ONC2257E8E.00740E46-C2257E90.0035BD30@il.ibm.com>]
* Re: [RFC PATCH 1/4] vhost: Introduce a universal thread to serve all users
[not found] ` <OF8AF3E3F8.F0120188-ONC2257E8E.00740E46-C2257E90.0035BD30@il.ibm.com>
@ 2015-08-08 22:40 ` Bandan Das
0 siblings, 0 replies; 18+ messages in thread
From: Bandan Das @ 2015-08-08 22:40 UTC (permalink / raw)
To: Eyal Moscovici
Cc: cgroups, jasowang, kvm, linux-kernel, mst, netdev, Razya Ladelsky
Eyal Moscovici <EYALMO@il.ibm.com> writes:
> Hi,
>
> Do you know what is the overhead of switching the vhost thread from one
> cgroup to another?
I misinterpreted this question earlier. I think what you are asking here is
that when the vm process is moved from one cgroup to another, what is the
overhead of moving the vhost thread to the new cgroup.
This design does not provide any hooks for the vhost thread to move to
a new cgroup. Rather, I think a better approach is to create a new vhost
thread and bind the process to it if the process is migrated to a new
cgroup. This is much less complicated, and there's a good chance that
it's impossible to migrate the vhost thread since it's serving other guests.
I will address this in v2.
> Eyal Moscovici
> HL-Cloud Infrastructure Solutions
> IBM Haifa Research Lab
>
>
>
> From: Bandan Das <bsd@redhat.com>
> To: kvm@vger.kernel.org
> Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
> mst@redhat.com, Eyal Moscovici/Haifa/IBM@IBMIL, Razya
> Ladelsky/Haifa/IBM@IBMIL, cgroups@vger.kernel.org, jasowang@redhat.com
> Date: 07/13/2015 07:08 AM
> Subject: [RFC PATCH 1/4] vhost: Introduce a universal thread to
> serve all users
>
>
>
> vhost threads are per-device, but in most cases a single thread
> is enough. This change creates a single thread that is used to
> serve all guests.
>
> However, this complicates cgroups associations. The current policy
> is to attach the per-device thread to all cgroups of the parent process
> that the device is associated it. This is no longer possible if we
> have a single thread. So, we end up moving the thread around to
> cgroups of whichever device that needs servicing. This is a very
> inefficient protocol but seems to be the only way to integrate
> cgroups support.
>
> Signed-off-by: Razya Ladelsky <razya@il.ibm.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
> drivers/vhost/scsi.c | 15 +++--
> drivers/vhost/vhost.c | 150
> ++++++++++++++++++++++++--------------------------
> drivers/vhost/vhost.h | 19 +++++--
> 3 files changed, 97 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index ea32b38..6c42936 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -535,7 +535,7 @@ static void vhost_scsi_complete_cmd(struct
> vhost_scsi_cmd *cmd)
>
> llist_add(&cmd->tvc_completion_list,
> &vs->vs_completion_list);
>
> - vhost_work_queue(&vs->dev, &vs->vs_completion_work);
> + vhost_work_queue(vs->dev.worker,
> &vs->vs_completion_work);
> }
>
> static int vhost_scsi_queue_data_in(struct se_cmd *se_cmd)
> @@ -1282,7 +1282,7 @@ vhost_scsi_send_evt(struct vhost_scsi *vs,
> }
>
> llist_add(&evt->list, &vs->vs_event_list);
> - vhost_work_queue(&vs->dev, &vs->vs_event_work);
> + vhost_work_queue(vs->dev.worker, &vs->vs_event_work);
> }
>
> static void vhost_scsi_evt_handle_kick(struct vhost_work *work)
> @@ -1335,8 +1335,8 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
> /* Flush both the vhost poll and vhost work */
> for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
> vhost_scsi_flush_vq(vs, i);
> - vhost_work_flush(&vs->dev, &vs->vs_completion_work);
> - vhost_work_flush(&vs->dev, &vs->vs_event_work);
> + vhost_work_flush(vs->dev.worker,
> &vs->vs_completion_work);
> + vhost_work_flush(vs->dev.worker, &vs->vs_event_work);
>
> /* Wait for all reqs issued before the flush to be
> finished */
> for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
> @@ -1584,8 +1584,11 @@ static int vhost_scsi_open(struct inode *inode,
> struct file *f)
> if (!vqs)
> goto err_vqs;
>
> - vhost_work_init(&vs->vs_completion_work,
> vhost_scsi_complete_cmd_work);
> - vhost_work_init(&vs->vs_event_work, vhost_scsi_evt_work);
> + vhost_work_init(&vs->dev, &vs->vs_completion_work,
> + vhost_scsi_complete_cmd_work);
> +
> + vhost_work_init(&vs->dev, &vs->vs_event_work,
> + vhost_scsi_evt_work);
>
> vs->vs_events_nr = 0;
> vs->vs_events_missed = false;
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ee2826..951c96b 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -11,6 +11,8 @@
> * Generic code for virtio server in host kernel.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/eventfd.h>
> #include <linux/vhost.h>
> #include <linux/uio.h>
> @@ -28,6 +30,9 @@
>
> #include "vhost.h"
>
> +/* Just one worker thread to service all devices */
> +static struct vhost_worker *worker;
> +
> enum {
> VHOST_MEMORY_MAX_NREGIONS = 64,
> VHOST_MEMORY_F_LOG = 0x1,
> @@ -58,13 +63,15 @@ static int vhost_poll_wakeup(wait_queue_t *wait,
> unsigned mode, int sync,
> return 0;
> }
>
> -void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn)
> +void vhost_work_init(struct vhost_dev *dev,
> + struct vhost_work *work,
> vhost_work_fn_t fn)
> {
> INIT_LIST_HEAD(&work->node);
> work->fn = fn;
> init_waitqueue_head(&work->done);
> work->flushing = 0;
> work->queue_seq = work->done_seq = 0;
> + work->dev = dev;
> }
> EXPORT_SYMBOL_GPL(vhost_work_init);
>
> @@ -78,7 +85,7 @@ void vhost_poll_init(struct vhost_poll *poll,
> vhost_work_fn_t fn,
> poll->dev = dev;
> poll->wqh = NULL;
>
> - vhost_work_init(&poll->work, fn);
> + vhost_work_init(dev, &poll->work, fn);
> }
> EXPORT_SYMBOL_GPL(vhost_poll_init);
>
> @@ -116,30 +123,30 @@ void vhost_poll_stop(struct vhost_poll *poll)
> }
> EXPORT_SYMBOL_GPL(vhost_poll_stop);
>
> -static bool vhost_work_seq_done(struct vhost_dev *dev, struct vhost_work
> *work,
> - unsigned
> seq)
> +static bool vhost_work_seq_done(struct vhost_worker *worker,
> + struct
> vhost_work *work, unsigned seq)
> {
> int left;
>
> - spin_lock_irq(&dev->work_lock);
> + spin_lock_irq(&worker->work_lock);
> left = seq - work->done_seq;
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> return left <= 0;
> }
>
> -void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work)
> +void vhost_work_flush(struct vhost_worker *worker, struct vhost_work
> *work)
> {
> unsigned seq;
> int flushing;
>
> - spin_lock_irq(&dev->work_lock);
> + spin_lock_irq(&worker->work_lock);
> seq = work->queue_seq;
> work->flushing++;
> - spin_unlock_irq(&dev->work_lock);
> - wait_event(work->done, vhost_work_seq_done(dev, work,
> seq));
> - spin_lock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> + wait_event(work->done, vhost_work_seq_done(worker, work,
> seq));
> + spin_lock_irq(&worker->work_lock);
> flushing = --work->flushing;
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> BUG_ON(flushing < 0);
> }
> EXPORT_SYMBOL_GPL(vhost_work_flush);
> @@ -148,29 +155,30 @@ EXPORT_SYMBOL_GPL(vhost_work_flush);
> * locks that are also used by the callback. */
> void vhost_poll_flush(struct vhost_poll *poll)
> {
> - vhost_work_flush(poll->dev, &poll->work);
> + vhost_work_flush(poll->dev->worker, &poll->work);
> }
> EXPORT_SYMBOL_GPL(vhost_poll_flush);
>
> -void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work)
> +void vhost_work_queue(struct vhost_worker *worker,
> + struct vhost_work *work)
> {
> unsigned long flags;
>
> - spin_lock_irqsave(&dev->work_lock, flags);
> + spin_lock_irqsave(&worker->work_lock, flags);
> if (list_empty(&work->node)) {
> - list_add_tail(&work->node,
> &dev->work_list);
> + list_add_tail(&work->node,
> &worker->work_list);
> work->queue_seq++;
> - spin_unlock_irqrestore(&dev->work_lock,
> flags);
> - wake_up_process(dev->worker);
> + spin_unlock_irqrestore(&worker->work_lock, flags);
> + wake_up_process(worker->thread);
> } else {
> - spin_unlock_irqrestore(&dev->work_lock,
> flags);
> + spin_unlock_irqrestore(&worker->work_lock, flags);
> }
> }
> EXPORT_SYMBOL_GPL(vhost_work_queue);
>
> void vhost_poll_queue(struct vhost_poll *poll)
> {
> - vhost_work_queue(poll->dev, &poll->work);
> + vhost_work_queue(poll->dev->worker, &poll->work);
> }
> EXPORT_SYMBOL_GPL(vhost_poll_queue);
>
> @@ -203,19 +211,18 @@ static void vhost_vq_reset(struct vhost_dev *dev,
>
> static int vhost_worker(void *data)
> {
> - struct vhost_dev *dev = data;
> + struct vhost_worker *worker = data;
> struct vhost_work *work = NULL;
> unsigned uninitialized_var(seq);
> mm_segment_t oldfs = get_fs();
>
> set_fs(USER_DS);
> - use_mm(dev->mm);
>
> for (;;) {
> /* mb paired w/ kthread_stop */
> set_current_state(TASK_INTERRUPTIBLE);
>
> - spin_lock_irq(&dev->work_lock);
> + spin_lock_irq(&worker->work_lock);
> if (work) {
> work->done_seq = seq;
> if (work->flushing)
> @@ -223,21 +230,35 @@ static int vhost_worker(void *data)
> }
>
> if (kthread_should_stop()) {
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> __set_current_state(TASK_RUNNING);
> break;
> }
> - if (!list_empty(&dev->work_list)) {
> - work =
> list_first_entry(&dev->work_list,
> + if (!list_empty(&worker->work_list)) {
> + work =
> list_first_entry(&worker->work_list,
> struct vhost_work, node);
> list_del_init(&work->node);
> seq = work->queue_seq;
> } else
> work = NULL;
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
>
> if (work) {
> + struct vhost_dev *dev =
> work->dev;
> +
> __set_current_state(TASK_RUNNING);
> +
> + if (current->mm !=
> dev->mm) {
> + unuse_mm(current->mm);
> + use_mm(dev->mm);
> + }
> +
> + /* TODO: Consider a more
> elegant solution */
> + if (worker->owner !=
> dev->owner) {
> + /* Should
> check for return value */
> + cgroup_attach_task_all(dev->owner, current);
> + worker->owner = dev->owner;
> + }
> work->fn(work);
> if (need_resched())
> schedule();
> @@ -245,7 +266,6 @@ static int vhost_worker(void *data)
> schedule();
>
> }
> - unuse_mm(dev->mm);
> set_fs(oldfs);
> return 0;
> }
> @@ -304,9 +324,8 @@ void vhost_dev_init(struct vhost_dev *dev,
> dev->log_file = NULL;
> dev->memory = NULL;
> dev->mm = NULL;
> - spin_lock_init(&dev->work_lock);
> - INIT_LIST_HEAD(&dev->work_list);
> - dev->worker = NULL;
> + dev->worker = worker;
> + dev->owner = current;
>
> for (i = 0; i < dev->nvqs; ++i) {
> vq = dev->vqs[i];
> @@ -331,31 +350,6 @@ long vhost_dev_check_owner(struct vhost_dev *dev)
> }
> EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
>
> -struct vhost_attach_cgroups_struct {
> - struct vhost_work work;
> - struct task_struct *owner;
> - int ret;
> -};
> -
> -static void vhost_attach_cgroups_work(struct vhost_work *work)
> -{
> - struct vhost_attach_cgroups_struct *s;
> -
> - s = container_of(work, struct
> vhost_attach_cgroups_struct, work);
> - s->ret = cgroup_attach_task_all(s->owner, current);
> -}
> -
> -static int vhost_attach_cgroups(struct vhost_dev *dev)
> -{
> - struct vhost_attach_cgroups_struct attach;
> -
> - attach.owner = current;
> - vhost_work_init(&attach.work, vhost_attach_cgroups_work);
> - vhost_work_queue(dev, &attach.work);
> - vhost_work_flush(dev, &attach.work);
> - return attach.ret;
> -}
> -
> /* Caller should have device mutex */
> bool vhost_dev_has_owner(struct vhost_dev *dev)
> {
> @@ -366,7 +360,6 @@ EXPORT_SYMBOL_GPL(vhost_dev_has_owner);
> /* Caller should have device mutex */
> long vhost_dev_set_owner(struct vhost_dev *dev)
> {
> - struct task_struct *worker;
> int err;
>
> /* Is there an owner already? */
> @@ -377,28 +370,15 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
>
> /* No owner, become one */
> dev->mm = get_task_mm(current);
> - worker = kthread_create(vhost_worker, dev, "vhost-%d",
> current->pid);
> - if (IS_ERR(worker)) {
> - err = PTR_ERR(worker);
> - goto err_worker;
> - }
> -
> dev->worker = worker;
> - wake_up_process(worker); /* avoid
> contributing to loadavg */
> -
> - err = vhost_attach_cgroups(dev);
> - if (err)
> - goto err_cgroup;
>
> err = vhost_dev_alloc_iovecs(dev);
> if (err)
> - goto err_cgroup;
> + goto err_alloc;
>
> return 0;
> -err_cgroup:
> - kthread_stop(worker);
> +err_alloc:
> dev->worker = NULL;
> -err_worker:
> if (dev->mm)
> mmput(dev->mm);
> dev->mm = NULL;
> @@ -472,11 +452,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool
> locked)
> /* No one will access memory at this point */
> kfree(dev->memory);
> dev->memory = NULL;
> - WARN_ON(!list_empty(&dev->work_list));
> - if (dev->worker) {
> - kthread_stop(dev->worker);
> - dev->worker = NULL;
> - }
> if (dev->mm)
> mmput(dev->mm);
> dev->mm = NULL;
> @@ -1567,11 +1542,32 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify);
>
> static int __init vhost_init(void)
> {
> + struct vhost_worker *w =
> + kzalloc(sizeof(*w), GFP_KERNEL);
> + if (!w)
> + return -ENOMEM;
> +
> + w->thread = kthread_create(vhost_worker,
> + w,
> "vhost-worker");
> + if (IS_ERR(w->thread))
> + return PTR_ERR(w->thread);
> +
> + worker = w;
> + spin_lock_init(&worker->work_lock);
> + INIT_LIST_HEAD(&worker->work_list);
> + wake_up_process(worker->thread);
> + pr_info("Created universal thread to service
> requests\n");
> +
> return 0;
> }
>
> static void __exit vhost_exit(void)
> {
> + if (worker) {
> + kthread_stop(worker->thread);
> + WARN_ON(!list_empty(&worker->work_list));
> + kfree(worker);
> + }
> }
>
> module_init(vhost_init);
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 8c1c792..2f204ce 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -22,6 +22,7 @@ struct vhost_work {
> int flushing;
> unsigned queue_seq;
> unsigned done_seq;
> + struct vhost_dev *dev;
> };
>
> /* Poll a file (eventfd or socket) */
> @@ -35,8 +36,8 @@ struct vhost_poll {
> struct vhost_dev *dev;
> };
>
> -void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn);
> -void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work);
> +void vhost_work_init(struct vhost_dev *dev,
> + struct vhost_work *work,
> vhost_work_fn_t fn);
>
> void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
> unsigned long mask, struct vhost_dev
> *dev);
> @@ -44,7 +45,6 @@ int 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);
> void vhost_poll_queue(struct vhost_poll *poll);
> -void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work);
> long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user
> *argp);
>
> struct vhost_log {
> @@ -116,11 +116,22 @@ struct vhost_dev {
> int nvqs;
> struct file *log_file;
> struct eventfd_ctx *log_ctx;
> + /* vhost shared worker */
> + struct vhost_worker *worker;
> + /* for cgroup support */
> + struct task_struct *owner;
> +};
> +
> +struct vhost_worker {
> spinlock_t work_lock;
> struct list_head work_list;
> - struct task_struct *worker;
> + struct task_struct *thread;
> + struct task_struct *owner;
> };
>
> +void vhost_work_queue(struct vhost_worker *worker,
> + struct vhost_work *work);
> +void vhost_work_flush(struct vhost_worker *worker, struct vhost_work
> *work);
> void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int
> nvqs);
> long vhost_dev_set_owner(struct vhost_dev *dev);
> bool vhost_dev_has_owner(struct vhost_dev *dev);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 1/4] vhost: Introduce a universal thread to serve all users
2015-07-13 4:07 ` [RFC PATCH 1/4] vhost: Introduce a universal thread to serve all users Bandan Das
[not found] ` <OF8AF3E3F8.F0120188-ONC2257E8E.00740E46-C2257E90.0035BD30@il.ibm.com>
@ 2015-08-10 9:27 ` Michael S. Tsirkin
2015-08-10 20:09 ` Bandan Das
1 sibling, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-08-10 9:27 UTC (permalink / raw)
To: Bandan Das
Cc: kvm, netdev, linux-kernel, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang
On Mon, Jul 13, 2015 at 12:07:32AM -0400, Bandan Das wrote:
> vhost threads are per-device, but in most cases a single thread
> is enough. This change creates a single thread that is used to
> serve all guests.
>
> However, this complicates cgroups associations. The current policy
> is to attach the per-device thread to all cgroups of the parent process
> that the device is associated it. This is no longer possible if we
> have a single thread. So, we end up moving the thread around to
> cgroups of whichever device that needs servicing. This is a very
> inefficient protocol but seems to be the only way to integrate
> cgroups support.
>
> Signed-off-by: Razya Ladelsky <razya@il.ibm.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
BTW, how does this interact with virtio net MQ?
It would seem that MQ gains from more parallelism and
CPU locality.
> ---
> drivers/vhost/scsi.c | 15 +++--
> drivers/vhost/vhost.c | 150 ++++++++++++++++++++++++--------------------------
> drivers/vhost/vhost.h | 19 +++++--
> 3 files changed, 97 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index ea32b38..6c42936 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -535,7 +535,7 @@ static void vhost_scsi_complete_cmd(struct vhost_scsi_cmd *cmd)
>
> llist_add(&cmd->tvc_completion_list, &vs->vs_completion_list);
>
> - vhost_work_queue(&vs->dev, &vs->vs_completion_work);
> + vhost_work_queue(vs->dev.worker, &vs->vs_completion_work);
> }
>
> static int vhost_scsi_queue_data_in(struct se_cmd *se_cmd)
> @@ -1282,7 +1282,7 @@ vhost_scsi_send_evt(struct vhost_scsi *vs,
> }
>
> llist_add(&evt->list, &vs->vs_event_list);
> - vhost_work_queue(&vs->dev, &vs->vs_event_work);
> + vhost_work_queue(vs->dev.worker, &vs->vs_event_work);
> }
>
> static void vhost_scsi_evt_handle_kick(struct vhost_work *work)
> @@ -1335,8 +1335,8 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
> /* Flush both the vhost poll and vhost work */
> for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
> vhost_scsi_flush_vq(vs, i);
> - vhost_work_flush(&vs->dev, &vs->vs_completion_work);
> - vhost_work_flush(&vs->dev, &vs->vs_event_work);
> + vhost_work_flush(vs->dev.worker, &vs->vs_completion_work);
> + vhost_work_flush(vs->dev.worker, &vs->vs_event_work);
>
> /* Wait for all reqs issued before the flush to be finished */
> for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
> @@ -1584,8 +1584,11 @@ static int vhost_scsi_open(struct inode *inode, struct file *f)
> if (!vqs)
> goto err_vqs;
>
> - vhost_work_init(&vs->vs_completion_work, vhost_scsi_complete_cmd_work);
> - vhost_work_init(&vs->vs_event_work, vhost_scsi_evt_work);
> + vhost_work_init(&vs->dev, &vs->vs_completion_work,
> + vhost_scsi_complete_cmd_work);
> +
> + vhost_work_init(&vs->dev, &vs->vs_event_work,
> + vhost_scsi_evt_work);
>
> vs->vs_events_nr = 0;
> vs->vs_events_missed = false;
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ee2826..951c96b 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -11,6 +11,8 @@
> * Generic code for virtio server in host kernel.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/eventfd.h>
> #include <linux/vhost.h>
> #include <linux/uio.h>
> @@ -28,6 +30,9 @@
>
> #include "vhost.h"
>
> +/* Just one worker thread to service all devices */
> +static struct vhost_worker *worker;
> +
> enum {
> VHOST_MEMORY_MAX_NREGIONS = 64,
> VHOST_MEMORY_F_LOG = 0x1,
> @@ -58,13 +63,15 @@ static int vhost_poll_wakeup(wait_queue_t *wait, unsigned mode, int sync,
> return 0;
> }
>
> -void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn)
> +void vhost_work_init(struct vhost_dev *dev,
> + struct vhost_work *work, vhost_work_fn_t fn)
> {
> INIT_LIST_HEAD(&work->node);
> work->fn = fn;
> init_waitqueue_head(&work->done);
> work->flushing = 0;
> work->queue_seq = work->done_seq = 0;
> + work->dev = dev;
> }
> EXPORT_SYMBOL_GPL(vhost_work_init);
>
> @@ -78,7 +85,7 @@ void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
> poll->dev = dev;
> poll->wqh = NULL;
>
> - vhost_work_init(&poll->work, fn);
> + vhost_work_init(dev, &poll->work, fn);
> }
> EXPORT_SYMBOL_GPL(vhost_poll_init);
>
> @@ -116,30 +123,30 @@ void vhost_poll_stop(struct vhost_poll *poll)
> }
> EXPORT_SYMBOL_GPL(vhost_poll_stop);
>
> -static bool vhost_work_seq_done(struct vhost_dev *dev, struct vhost_work *work,
> - unsigned seq)
> +static bool vhost_work_seq_done(struct vhost_worker *worker,
> + struct vhost_work *work, unsigned seq)
> {
> int left;
>
> - spin_lock_irq(&dev->work_lock);
> + spin_lock_irq(&worker->work_lock);
> left = seq - work->done_seq;
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> return left <= 0;
> }
>
> -void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work)
> +void vhost_work_flush(struct vhost_worker *worker, struct vhost_work *work)
> {
> unsigned seq;
> int flushing;
>
> - spin_lock_irq(&dev->work_lock);
> + spin_lock_irq(&worker->work_lock);
> seq = work->queue_seq;
> work->flushing++;
> - spin_unlock_irq(&dev->work_lock);
> - wait_event(work->done, vhost_work_seq_done(dev, work, seq));
> - spin_lock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> + wait_event(work->done, vhost_work_seq_done(worker, work, seq));
> + spin_lock_irq(&worker->work_lock);
> flushing = --work->flushing;
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> BUG_ON(flushing < 0);
> }
> EXPORT_SYMBOL_GPL(vhost_work_flush);
> @@ -148,29 +155,30 @@ EXPORT_SYMBOL_GPL(vhost_work_flush);
> * locks that are also used by the callback. */
> void vhost_poll_flush(struct vhost_poll *poll)
> {
> - vhost_work_flush(poll->dev, &poll->work);
> + vhost_work_flush(poll->dev->worker, &poll->work);
> }
> EXPORT_SYMBOL_GPL(vhost_poll_flush);
>
> -void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work)
> +void vhost_work_queue(struct vhost_worker *worker,
> + struct vhost_work *work)
> {
> unsigned long flags;
>
> - spin_lock_irqsave(&dev->work_lock, flags);
> + spin_lock_irqsave(&worker->work_lock, flags);
> if (list_empty(&work->node)) {
> - list_add_tail(&work->node, &dev->work_list);
> + list_add_tail(&work->node, &worker->work_list);
> work->queue_seq++;
> - spin_unlock_irqrestore(&dev->work_lock, flags);
> - wake_up_process(dev->worker);
> + spin_unlock_irqrestore(&worker->work_lock, flags);
> + wake_up_process(worker->thread);
> } else {
> - spin_unlock_irqrestore(&dev->work_lock, flags);
> + spin_unlock_irqrestore(&worker->work_lock, flags);
> }
> }
> EXPORT_SYMBOL_GPL(vhost_work_queue);
>
> void vhost_poll_queue(struct vhost_poll *poll)
> {
> - vhost_work_queue(poll->dev, &poll->work);
> + vhost_work_queue(poll->dev->worker, &poll->work);
> }
> EXPORT_SYMBOL_GPL(vhost_poll_queue);
>
> @@ -203,19 +211,18 @@ static void vhost_vq_reset(struct vhost_dev *dev,
>
> static int vhost_worker(void *data)
> {
> - struct vhost_dev *dev = data;
> + struct vhost_worker *worker = data;
> struct vhost_work *work = NULL;
> unsigned uninitialized_var(seq);
> mm_segment_t oldfs = get_fs();
>
> set_fs(USER_DS);
> - use_mm(dev->mm);
>
> for (;;) {
> /* mb paired w/ kthread_stop */
> set_current_state(TASK_INTERRUPTIBLE);
>
> - spin_lock_irq(&dev->work_lock);
> + spin_lock_irq(&worker->work_lock);
> if (work) {
> work->done_seq = seq;
> if (work->flushing)
> @@ -223,21 +230,35 @@ static int vhost_worker(void *data)
> }
>
> if (kthread_should_stop()) {
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
> __set_current_state(TASK_RUNNING);
> break;
> }
> - if (!list_empty(&dev->work_list)) {
> - work = list_first_entry(&dev->work_list,
> + if (!list_empty(&worker->work_list)) {
> + work = list_first_entry(&worker->work_list,
> struct vhost_work, node);
> list_del_init(&work->node);
> seq = work->queue_seq;
> } else
> work = NULL;
> - spin_unlock_irq(&dev->work_lock);
> + spin_unlock_irq(&worker->work_lock);
>
> if (work) {
> + struct vhost_dev *dev = work->dev;
> +
> __set_current_state(TASK_RUNNING);
> +
> + if (current->mm != dev->mm) {
> + unuse_mm(current->mm);
> + use_mm(dev->mm);
> + }
> +
> + /* TODO: Consider a more elegant solution */
> + if (worker->owner != dev->owner) {
> + /* Should check for return value */
> + cgroup_attach_task_all(dev->owner, current);
> + worker->owner = dev->owner;
> + }
> work->fn(work);
> if (need_resched())
> schedule();
> @@ -245,7 +266,6 @@ static int vhost_worker(void *data)
> schedule();
>
> }
> - unuse_mm(dev->mm);
> set_fs(oldfs);
> return 0;
> }
> @@ -304,9 +324,8 @@ void vhost_dev_init(struct vhost_dev *dev,
> dev->log_file = NULL;
> dev->memory = NULL;
> dev->mm = NULL;
> - spin_lock_init(&dev->work_lock);
> - INIT_LIST_HEAD(&dev->work_list);
> - dev->worker = NULL;
> + dev->worker = worker;
> + dev->owner = current;
>
> for (i = 0; i < dev->nvqs; ++i) {
> vq = dev->vqs[i];
> @@ -331,31 +350,6 @@ long vhost_dev_check_owner(struct vhost_dev *dev)
> }
> EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
>
> -struct vhost_attach_cgroups_struct {
> - struct vhost_work work;
> - struct task_struct *owner;
> - int ret;
> -};
> -
> -static void vhost_attach_cgroups_work(struct vhost_work *work)
> -{
> - struct vhost_attach_cgroups_struct *s;
> -
> - s = container_of(work, struct vhost_attach_cgroups_struct, work);
> - s->ret = cgroup_attach_task_all(s->owner, current);
> -}
> -
> -static int vhost_attach_cgroups(struct vhost_dev *dev)
> -{
> - struct vhost_attach_cgroups_struct attach;
> -
> - attach.owner = current;
> - vhost_work_init(&attach.work, vhost_attach_cgroups_work);
> - vhost_work_queue(dev, &attach.work);
> - vhost_work_flush(dev, &attach.work);
> - return attach.ret;
> -}
> -
> /* Caller should have device mutex */
> bool vhost_dev_has_owner(struct vhost_dev *dev)
> {
> @@ -366,7 +360,6 @@ EXPORT_SYMBOL_GPL(vhost_dev_has_owner);
> /* Caller should have device mutex */
> long vhost_dev_set_owner(struct vhost_dev *dev)
> {
> - struct task_struct *worker;
> int err;
>
> /* Is there an owner already? */
> @@ -377,28 +370,15 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
>
> /* No owner, become one */
> dev->mm = get_task_mm(current);
> - worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
> - if (IS_ERR(worker)) {
> - err = PTR_ERR(worker);
> - goto err_worker;
> - }
> -
> dev->worker = worker;
> - wake_up_process(worker); /* avoid contributing to loadavg */
> -
> - err = vhost_attach_cgroups(dev);
> - if (err)
> - goto err_cgroup;
>
> err = vhost_dev_alloc_iovecs(dev);
> if (err)
> - goto err_cgroup;
> + goto err_alloc;
>
> return 0;
> -err_cgroup:
> - kthread_stop(worker);
> +err_alloc:
> dev->worker = NULL;
> -err_worker:
> if (dev->mm)
> mmput(dev->mm);
> dev->mm = NULL;
> @@ -472,11 +452,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
> /* No one will access memory at this point */
> kfree(dev->memory);
> dev->memory = NULL;
> - WARN_ON(!list_empty(&dev->work_list));
> - if (dev->worker) {
> - kthread_stop(dev->worker);
> - dev->worker = NULL;
> - }
> if (dev->mm)
> mmput(dev->mm);
> dev->mm = NULL;
> @@ -1567,11 +1542,32 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify);
>
> static int __init vhost_init(void)
> {
> + struct vhost_worker *w =
> + kzalloc(sizeof(*w), GFP_KERNEL);
> + if (!w)
> + return -ENOMEM;
> +
> + w->thread = kthread_create(vhost_worker,
> + w, "vhost-worker");
> + if (IS_ERR(w->thread))
> + return PTR_ERR(w->thread);
> +
> + worker = w;
> + spin_lock_init(&worker->work_lock);
> + INIT_LIST_HEAD(&worker->work_list);
> + wake_up_process(worker->thread);
> + pr_info("Created universal thread to service requests\n");
> +
> return 0;
> }
>
> static void __exit vhost_exit(void)
> {
> + if (worker) {
> + kthread_stop(worker->thread);
> + WARN_ON(!list_empty(&worker->work_list));
> + kfree(worker);
> + }
> }
>
> module_init(vhost_init);
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 8c1c792..2f204ce 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -22,6 +22,7 @@ struct vhost_work {
> int flushing;
> unsigned queue_seq;
> unsigned done_seq;
> + struct vhost_dev *dev;
> };
>
> /* Poll a file (eventfd or socket) */
> @@ -35,8 +36,8 @@ struct vhost_poll {
> struct vhost_dev *dev;
> };
>
> -void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn);
> -void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work);
> +void vhost_work_init(struct vhost_dev *dev,
> + struct vhost_work *work, vhost_work_fn_t fn);
>
> void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
> unsigned long mask, struct vhost_dev *dev);
> @@ -44,7 +45,6 @@ int 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);
> void vhost_poll_queue(struct vhost_poll *poll);
> -void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work);
> long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp);
>
> struct vhost_log {
> @@ -116,11 +116,22 @@ struct vhost_dev {
> int nvqs;
> struct file *log_file;
> struct eventfd_ctx *log_ctx;
> + /* vhost shared worker */
> + struct vhost_worker *worker;
> + /* for cgroup support */
> + struct task_struct *owner;
> +};
> +
> +struct vhost_worker {
> spinlock_t work_lock;
> struct list_head work_list;
> - struct task_struct *worker;
> + struct task_struct *thread;
> + struct task_struct *owner;
> };
>
> +void vhost_work_queue(struct vhost_worker *worker,
> + struct vhost_work *work);
> +void vhost_work_flush(struct vhost_worker *worker, struct vhost_work *work);
> void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int nvqs);
> long vhost_dev_set_owner(struct vhost_dev *dev);
> bool vhost_dev_has_owner(struct vhost_dev *dev);
> --
> 2.4.3
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [RFC PATCH 1/4] vhost: Introduce a universal thread to serve all users
2015-08-10 9:27 ` Michael S. Tsirkin
@ 2015-08-10 20:09 ` Bandan Das
[not found] ` <jpg1tfarjly.fsf-oDDOE2N8RG3XLSnhx7PemevR1TjyzBtM@public.gmane.org>
0 siblings, 1 reply; 18+ messages in thread
From: Bandan Das @ 2015-08-10 20:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Jul 13, 2015 at 12:07:32AM -0400, Bandan Das wrote:
>> vhost threads are per-device, but in most cases a single thread
>> is enough. This change creates a single thread that is used to
>> serve all guests.
>>
>> However, this complicates cgroups associations. The current policy
>> is to attach the per-device thread to all cgroups of the parent process
>> that the device is associated it. This is no longer possible if we
>> have a single thread. So, we end up moving the thread around to
>> cgroups of whichever device that needs servicing. This is a very
>> inefficient protocol but seems to be the only way to integrate
>> cgroups support.
>>
>> Signed-off-by: Razya Ladelsky <razya@il.ibm.com>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>
> BTW, how does this interact with virtio net MQ?
> It would seem that MQ gains from more parallelism and
> CPU locality.
Hm.. Good point. As of this version, this design will always have
one worker thread servicing a guest. Now suppose we have 10 virtio
queues for a guest, surely, we could benefit from spawning off another
worker just like we are doing in case of a new guest/device with
the devs_per_worker parameter.
>> ---
>> drivers/vhost/scsi.c | 15 +++--
>> drivers/vhost/vhost.c | 150 ++++++++++++++++++++++++--------------------------
>> drivers/vhost/vhost.h | 19 +++++--
>> 3 files changed, 97 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
>> index ea32b38..6c42936 100644
>> --- a/drivers/vhost/scsi.c
>> +++ b/drivers/vhost/scsi.c
>> @@ -535,7 +535,7 @@ static void vhost_scsi_complete_cmd(struct vhost_scsi_cmd *cmd)
>>
>> llist_add(&cmd->tvc_completion_list, &vs->vs_completion_list);
>>
>> - vhost_work_queue(&vs->dev, &vs->vs_completion_work);
>> + vhost_work_queue(vs->dev.worker, &vs->vs_completion_work);
>> }
>>
>> static int vhost_scsi_queue_data_in(struct se_cmd *se_cmd)
>> @@ -1282,7 +1282,7 @@ vhost_scsi_send_evt(struct vhost_scsi *vs,
>> }
>>
>> llist_add(&evt->list, &vs->vs_event_list);
>> - vhost_work_queue(&vs->dev, &vs->vs_event_work);
>> + vhost_work_queue(vs->dev.worker, &vs->vs_event_work);
>> }
>>
>> static void vhost_scsi_evt_handle_kick(struct vhost_work *work)
>> @@ -1335,8 +1335,8 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
>> /* Flush both the vhost poll and vhost work */
>> for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
>> vhost_scsi_flush_vq(vs, i);
>> - vhost_work_flush(&vs->dev, &vs->vs_completion_work);
>> - vhost_work_flush(&vs->dev, &vs->vs_event_work);
>> + vhost_work_flush(vs->dev.worker, &vs->vs_completion_work);
>> + vhost_work_flush(vs->dev.worker, &vs->vs_event_work);
>>
>> /* Wait for all reqs issued before the flush to be finished */
>> for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
>> @@ -1584,8 +1584,11 @@ static int vhost_scsi_open(struct inode *inode, struct file *f)
>> if (!vqs)
>> goto err_vqs;
>>
>> - vhost_work_init(&vs->vs_completion_work, vhost_scsi_complete_cmd_work);
>> - vhost_work_init(&vs->vs_event_work, vhost_scsi_evt_work);
>> + vhost_work_init(&vs->dev, &vs->vs_completion_work,
>> + vhost_scsi_complete_cmd_work);
>> +
>> + vhost_work_init(&vs->dev, &vs->vs_event_work,
>> + vhost_scsi_evt_work);
>>
>> vs->vs_events_nr = 0;
>> vs->vs_events_missed = false;
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 2ee2826..951c96b 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -11,6 +11,8 @@
>> * Generic code for virtio server in host kernel.
>> */
>>
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> #include <linux/eventfd.h>
>> #include <linux/vhost.h>
>> #include <linux/uio.h>
>> @@ -28,6 +30,9 @@
>>
>> #include "vhost.h"
>>
>> +/* Just one worker thread to service all devices */
>> +static struct vhost_worker *worker;
>> +
>> enum {
>> VHOST_MEMORY_MAX_NREGIONS = 64,
>> VHOST_MEMORY_F_LOG = 0x1,
>> @@ -58,13 +63,15 @@ static int vhost_poll_wakeup(wait_queue_t *wait, unsigned mode, int sync,
>> return 0;
>> }
>>
>> -void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn)
>> +void vhost_work_init(struct vhost_dev *dev,
>> + struct vhost_work *work, vhost_work_fn_t fn)
>> {
>> INIT_LIST_HEAD(&work->node);
>> work->fn = fn;
>> init_waitqueue_head(&work->done);
>> work->flushing = 0;
>> work->queue_seq = work->done_seq = 0;
>> + work->dev = dev;
>> }
>> EXPORT_SYMBOL_GPL(vhost_work_init);
>>
>> @@ -78,7 +85,7 @@ void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
>> poll->dev = dev;
>> poll->wqh = NULL;
>>
>> - vhost_work_init(&poll->work, fn);
>> + vhost_work_init(dev, &poll->work, fn);
>> }
>> EXPORT_SYMBOL_GPL(vhost_poll_init);
>>
>> @@ -116,30 +123,30 @@ void vhost_poll_stop(struct vhost_poll *poll)
>> }
>> EXPORT_SYMBOL_GPL(vhost_poll_stop);
>>
>> -static bool vhost_work_seq_done(struct vhost_dev *dev, struct vhost_work *work,
>> - unsigned seq)
>> +static bool vhost_work_seq_done(struct vhost_worker *worker,
>> + struct vhost_work *work, unsigned seq)
>> {
>> int left;
>>
>> - spin_lock_irq(&dev->work_lock);
>> + spin_lock_irq(&worker->work_lock);
>> left = seq - work->done_seq;
>> - spin_unlock_irq(&dev->work_lock);
>> + spin_unlock_irq(&worker->work_lock);
>> return left <= 0;
>> }
>>
>> -void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work)
>> +void vhost_work_flush(struct vhost_worker *worker, struct vhost_work *work)
>> {
>> unsigned seq;
>> int flushing;
>>
>> - spin_lock_irq(&dev->work_lock);
>> + spin_lock_irq(&worker->work_lock);
>> seq = work->queue_seq;
>> work->flushing++;
>> - spin_unlock_irq(&dev->work_lock);
>> - wait_event(work->done, vhost_work_seq_done(dev, work, seq));
>> - spin_lock_irq(&dev->work_lock);
>> + spin_unlock_irq(&worker->work_lock);
>> + wait_event(work->done, vhost_work_seq_done(worker, work, seq));
>> + spin_lock_irq(&worker->work_lock);
>> flushing = --work->flushing;
>> - spin_unlock_irq(&dev->work_lock);
>> + spin_unlock_irq(&worker->work_lock);
>> BUG_ON(flushing < 0);
>> }
>> EXPORT_SYMBOL_GPL(vhost_work_flush);
>> @@ -148,29 +155,30 @@ EXPORT_SYMBOL_GPL(vhost_work_flush);
>> * locks that are also used by the callback. */
>> void vhost_poll_flush(struct vhost_poll *poll)
>> {
>> - vhost_work_flush(poll->dev, &poll->work);
>> + vhost_work_flush(poll->dev->worker, &poll->work);
>> }
>> EXPORT_SYMBOL_GPL(vhost_poll_flush);
>>
>> -void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work)
>> +void vhost_work_queue(struct vhost_worker *worker,
>> + struct vhost_work *work)
>> {
>> unsigned long flags;
>>
>> - spin_lock_irqsave(&dev->work_lock, flags);
>> + spin_lock_irqsave(&worker->work_lock, flags);
>> if (list_empty(&work->node)) {
>> - list_add_tail(&work->node, &dev->work_list);
>> + list_add_tail(&work->node, &worker->work_list);
>> work->queue_seq++;
>> - spin_unlock_irqrestore(&dev->work_lock, flags);
>> - wake_up_process(dev->worker);
>> + spin_unlock_irqrestore(&worker->work_lock, flags);
>> + wake_up_process(worker->thread);
>> } else {
>> - spin_unlock_irqrestore(&dev->work_lock, flags);
>> + spin_unlock_irqrestore(&worker->work_lock, flags);
>> }
>> }
>> EXPORT_SYMBOL_GPL(vhost_work_queue);
>>
>> void vhost_poll_queue(struct vhost_poll *poll)
>> {
>> - vhost_work_queue(poll->dev, &poll->work);
>> + vhost_work_queue(poll->dev->worker, &poll->work);
>> }
>> EXPORT_SYMBOL_GPL(vhost_poll_queue);
>>
>> @@ -203,19 +211,18 @@ static void vhost_vq_reset(struct vhost_dev *dev,
>>
>> static int vhost_worker(void *data)
>> {
>> - struct vhost_dev *dev = data;
>> + struct vhost_worker *worker = data;
>> struct vhost_work *work = NULL;
>> unsigned uninitialized_var(seq);
>> mm_segment_t oldfs = get_fs();
>>
>> set_fs(USER_DS);
>> - use_mm(dev->mm);
>>
>> for (;;) {
>> /* mb paired w/ kthread_stop */
>> set_current_state(TASK_INTERRUPTIBLE);
>>
>> - spin_lock_irq(&dev->work_lock);
>> + spin_lock_irq(&worker->work_lock);
>> if (work) {
>> work->done_seq = seq;
>> if (work->flushing)
>> @@ -223,21 +230,35 @@ static int vhost_worker(void *data)
>> }
>>
>> if (kthread_should_stop()) {
>> - spin_unlock_irq(&dev->work_lock);
>> + spin_unlock_irq(&worker->work_lock);
>> __set_current_state(TASK_RUNNING);
>> break;
>> }
>> - if (!list_empty(&dev->work_list)) {
>> - work = list_first_entry(&dev->work_list,
>> + if (!list_empty(&worker->work_list)) {
>> + work = list_first_entry(&worker->work_list,
>> struct vhost_work, node);
>> list_del_init(&work->node);
>> seq = work->queue_seq;
>> } else
>> work = NULL;
>> - spin_unlock_irq(&dev->work_lock);
>> + spin_unlock_irq(&worker->work_lock);
>>
>> if (work) {
>> + struct vhost_dev *dev = work->dev;
>> +
>> __set_current_state(TASK_RUNNING);
>> +
>> + if (current->mm != dev->mm) {
>> + unuse_mm(current->mm);
>> + use_mm(dev->mm);
>> + }
>> +
>> + /* TODO: Consider a more elegant solution */
>> + if (worker->owner != dev->owner) {
>> + /* Should check for return value */
>> + cgroup_attach_task_all(dev->owner, current);
>> + worker->owner = dev->owner;
>> + }
>> work->fn(work);
>> if (need_resched())
>> schedule();
>> @@ -245,7 +266,6 @@ static int vhost_worker(void *data)
>> schedule();
>>
>> }
>> - unuse_mm(dev->mm);
>> set_fs(oldfs);
>> return 0;
>> }
>> @@ -304,9 +324,8 @@ void vhost_dev_init(struct vhost_dev *dev,
>> dev->log_file = NULL;
>> dev->memory = NULL;
>> dev->mm = NULL;
>> - spin_lock_init(&dev->work_lock);
>> - INIT_LIST_HEAD(&dev->work_list);
>> - dev->worker = NULL;
>> + dev->worker = worker;
>> + dev->owner = current;
>>
>> for (i = 0; i < dev->nvqs; ++i) {
>> vq = dev->vqs[i];
>> @@ -331,31 +350,6 @@ long vhost_dev_check_owner(struct vhost_dev *dev)
>> }
>> EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
>>
>> -struct vhost_attach_cgroups_struct {
>> - struct vhost_work work;
>> - struct task_struct *owner;
>> - int ret;
>> -};
>> -
>> -static void vhost_attach_cgroups_work(struct vhost_work *work)
>> -{
>> - struct vhost_attach_cgroups_struct *s;
>> -
>> - s = container_of(work, struct vhost_attach_cgroups_struct, work);
>> - s->ret = cgroup_attach_task_all(s->owner, current);
>> -}
>> -
>> -static int vhost_attach_cgroups(struct vhost_dev *dev)
>> -{
>> - struct vhost_attach_cgroups_struct attach;
>> -
>> - attach.owner = current;
>> - vhost_work_init(&attach.work, vhost_attach_cgroups_work);
>> - vhost_work_queue(dev, &attach.work);
>> - vhost_work_flush(dev, &attach.work);
>> - return attach.ret;
>> -}
>> -
>> /* Caller should have device mutex */
>> bool vhost_dev_has_owner(struct vhost_dev *dev)
>> {
>> @@ -366,7 +360,6 @@ EXPORT_SYMBOL_GPL(vhost_dev_has_owner);
>> /* Caller should have device mutex */
>> long vhost_dev_set_owner(struct vhost_dev *dev)
>> {
>> - struct task_struct *worker;
>> int err;
>>
>> /* Is there an owner already? */
>> @@ -377,28 +370,15 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
>>
>> /* No owner, become one */
>> dev->mm = get_task_mm(current);
>> - worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
>> - if (IS_ERR(worker)) {
>> - err = PTR_ERR(worker);
>> - goto err_worker;
>> - }
>> -
>> dev->worker = worker;
>> - wake_up_process(worker); /* avoid contributing to loadavg */
>> -
>> - err = vhost_attach_cgroups(dev);
>> - if (err)
>> - goto err_cgroup;
>>
>> err = vhost_dev_alloc_iovecs(dev);
>> if (err)
>> - goto err_cgroup;
>> + goto err_alloc;
>>
>> return 0;
>> -err_cgroup:
>> - kthread_stop(worker);
>> +err_alloc:
>> dev->worker = NULL;
>> -err_worker:
>> if (dev->mm)
>> mmput(dev->mm);
>> dev->mm = NULL;
>> @@ -472,11 +452,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
>> /* No one will access memory at this point */
>> kfree(dev->memory);
>> dev->memory = NULL;
>> - WARN_ON(!list_empty(&dev->work_list));
>> - if (dev->worker) {
>> - kthread_stop(dev->worker);
>> - dev->worker = NULL;
>> - }
>> if (dev->mm)
>> mmput(dev->mm);
>> dev->mm = NULL;
>> @@ -1567,11 +1542,32 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify);
>>
>> static int __init vhost_init(void)
>> {
>> + struct vhost_worker *w =
>> + kzalloc(sizeof(*w), GFP_KERNEL);
>> + if (!w)
>> + return -ENOMEM;
>> +
>> + w->thread = kthread_create(vhost_worker,
>> + w, "vhost-worker");
>> + if (IS_ERR(w->thread))
>> + return PTR_ERR(w->thread);
>> +
>> + worker = w;
>> + spin_lock_init(&worker->work_lock);
>> + INIT_LIST_HEAD(&worker->work_list);
>> + wake_up_process(worker->thread);
>> + pr_info("Created universal thread to service requests\n");
>> +
>> return 0;
>> }
>>
>> static void __exit vhost_exit(void)
>> {
>> + if (worker) {
>> + kthread_stop(worker->thread);
>> + WARN_ON(!list_empty(&worker->work_list));
>> + kfree(worker);
>> + }
>> }
>>
>> module_init(vhost_init);
>> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>> index 8c1c792..2f204ce 100644
>> --- a/drivers/vhost/vhost.h
>> +++ b/drivers/vhost/vhost.h
>> @@ -22,6 +22,7 @@ struct vhost_work {
>> int flushing;
>> unsigned queue_seq;
>> unsigned done_seq;
>> + struct vhost_dev *dev;
>> };
>>
>> /* Poll a file (eventfd or socket) */
>> @@ -35,8 +36,8 @@ struct vhost_poll {
>> struct vhost_dev *dev;
>> };
>>
>> -void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn);
>> -void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work);
>> +void vhost_work_init(struct vhost_dev *dev,
>> + struct vhost_work *work, vhost_work_fn_t fn);
>>
>> void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
>> unsigned long mask, struct vhost_dev *dev);
>> @@ -44,7 +45,6 @@ int 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);
>> void vhost_poll_queue(struct vhost_poll *poll);
>> -void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work);
>> long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp);
>>
>> struct vhost_log {
>> @@ -116,11 +116,22 @@ struct vhost_dev {
>> int nvqs;
>> struct file *log_file;
>> struct eventfd_ctx *log_ctx;
>> + /* vhost shared worker */
>> + struct vhost_worker *worker;
>> + /* for cgroup support */
>> + struct task_struct *owner;
>> +};
>> +
>> +struct vhost_worker {
>> spinlock_t work_lock;
>> struct list_head work_list;
>> - struct task_struct *worker;
>> + struct task_struct *thread;
>> + struct task_struct *owner;
>> };
>>
>> +void vhost_work_queue(struct vhost_worker *worker,
>> + struct vhost_work *work);
>> +void vhost_work_flush(struct vhost_worker *worker, struct vhost_work *work);
>> void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int nvqs);
>> long vhost_dev_set_owner(struct vhost_dev *dev);
>> bool vhost_dev_has_owner(struct vhost_dev *dev);
>> --
>> 2.4.3
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC PATCH 2/4] vhost: Limit the number of devices served by a single worker thread
2015-07-13 4:07 [RFC PATCH 0/4] Shared vhost design Bandan Das
2015-07-13 4:07 ` [RFC PATCH 1/4] vhost: Introduce a universal thread to serve all users Bandan Das
@ 2015-07-13 4:07 ` Bandan Das
2015-07-13 4:07 ` [RFC PATCH 3/4] cgroup: Introduce a function to compare cgroups Bandan Das
` (3 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Bandan Das @ 2015-07-13 4:07 UTC (permalink / raw)
To: kvm
Cc: netdev, linux-kernel, mst, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang
When the number of devices increase, the universal thread model
(introduced in the preceding patch) may end up being the bottleneck.
Moreover, a single worker thread also forces us to change cgroups
based on the device we are serving.
We introduce a worker pool struct that starts with one worker
thread and we keep adding more threads when the numbers of devs
reaches a certain threshold. The default value is set at 7 but is
not based on any empirical data. The value can also be changed by
the user with the devs_per_worker module parameter.
Note that this patch doesn't change how cgroups work. We still
keep moving around the worker thread to the cgroups of the
device we are serving at the moment.
Signed-off-by: Razya Ladelsky <razya@il.ibm.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
drivers/vhost/net.c | 6 +--
drivers/vhost/scsi.c | 3 +-
drivers/vhost/vhost.c | 135 +++++++++++++++++++++++++++++++++++++++++---------
drivers/vhost/vhost.h | 13 ++++-
4 files changed, 128 insertions(+), 29 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 7d137a4..7bfa019 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -705,7 +705,8 @@ static int vhost_net_open(struct inode *inode, struct file *f)
n->vqs[i].vhost_hlen = 0;
n->vqs[i].sock_hlen = 0;
}
- vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
+ if (vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX))
+ return dev->err;
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);
@@ -801,9 +802,6 @@ static int vhost_net_release(struct inode *inode, struct file *f)
sockfd_put(rx_sock);
/* Make sure no callbacks are outstanding */
synchronize_rcu_bh();
- /* We do an extra flush before freeing memory,
- * since jobs can re-queue themselves. */
- vhost_net_flush(n);
kfree(n->dev.vqs);
kvfree(n);
return 0;
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 6c42936..97de2db 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1601,7 +1601,8 @@ static int vhost_scsi_open(struct inode *inode, struct file *f)
vqs[i] = &vs->vqs[i].vq;
vs->vqs[i].vq.handle_kick = vhost_scsi_handle_kick;
}
- vhost_dev_init(&vs->dev, vqs, VHOST_SCSI_MAX_VQ);
+ if (vhost_dev_init(&vs->dev, vqs, VHOST_SCSI_MAX_VQ))
+ return vs->dev.err;
vhost_scsi_init_inflight(vs, NULL);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 951c96b..6a5d4c0 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -27,11 +27,19 @@
#include <linux/kthread.h>
#include <linux/cgroup.h>
#include <linux/module.h>
+#include <linux/moduleparam.h>
#include "vhost.h"
-/* Just one worker thread to service all devices */
-static struct vhost_worker *worker;
+static int __read_mostly devs_per_worker = 7;
+module_param(devs_per_worker, int, S_IRUGO);
+MODULE_PARM_DESC(devs_per_worker, "Setup the number of devices being served by a worker thread");
+
+/* Only used to give a unique id to a vhost thread at the moment */
+static unsigned int total_vhost_workers;
+
+/* Pool of vhost threads */
+static struct vhost_pool *vhost_pool;
enum {
VHOST_MEMORY_MAX_NREGIONS = 64,
@@ -270,6 +278,63 @@ static int vhost_worker(void *data)
return 0;
}
+static void vhost_create_worker(struct vhost_dev *dev)
+{
+ struct vhost_worker *worker;
+ struct vhost_pool *pool = vhost_pool;
+
+ worker = kzalloc(sizeof(*worker), GFP_KERNEL);
+ if (!worker) {
+ dev->err = -ENOMEM;
+ return;
+ }
+
+ worker->thread = kthread_create(vhost_worker,
+ worker,
+ "vhost-%d",
+ total_vhost_workers);
+ if (IS_ERR(worker->thread)) {
+ dev->err = PTR_ERR(worker->thread);
+ goto therror;
+ }
+
+ spin_lock_init(&worker->work_lock);
+ INIT_LIST_HEAD(&worker->work_list);
+ list_add(&worker->node, &pool->workers);
+ worker->owner = NULL;
+ worker->num_devices++;
+ total_vhost_workers++;
+ dev->worker = worker;
+ dev->worker_assigned = true;
+ return;
+
+therror:
+ if (worker->thread)
+ kthread_stop(worker->thread);
+ kfree(worker);
+}
+
+static int vhost_dev_assign_worker(struct vhost_dev *dev)
+{
+ struct vhost_worker *worker;
+
+ mutex_lock(&vhost_pool->pool_lock);
+ list_for_each_entry(worker, &vhost_pool->workers, node) {
+ if (worker->num_devices < devs_per_worker) {
+ dev->worker = worker;
+ dev->worker_assigned = true;
+ worker->num_devices++;
+ break;
+ }
+ }
+ if (!dev->worker_assigned)
+ /* create a new worker */
+ vhost_create_worker(dev);
+ mutex_unlock(&vhost_pool->pool_lock);
+
+ return dev->err;
+}
+
static void vhost_vq_free_iovecs(struct vhost_virtqueue *vq)
{
kfree(vq->indirect);
@@ -311,7 +376,7 @@ static void vhost_dev_free_iovecs(struct vhost_dev *dev)
vhost_vq_free_iovecs(dev->vqs[i]);
}
-void vhost_dev_init(struct vhost_dev *dev,
+int vhost_dev_init(struct vhost_dev *dev,
struct vhost_virtqueue **vqs, int nvqs)
{
struct vhost_virtqueue *vq;
@@ -324,9 +389,14 @@ void vhost_dev_init(struct vhost_dev *dev,
dev->log_file = NULL;
dev->memory = NULL;
dev->mm = NULL;
- dev->worker = worker;
+ dev->worker = NULL;
+ dev->err = 0;
+ dev->worker_assigned = false;
dev->owner = current;
+ if (vhost_dev_assign_worker(dev))
+ goto done;
+
for (i = 0; i < dev->nvqs; ++i) {
vq = dev->vqs[i];
vq->log = NULL;
@@ -339,6 +409,9 @@ void vhost_dev_init(struct vhost_dev *dev,
vhost_poll_init(&vq->poll, vq->handle_kick,
POLLIN, dev);
}
+
+done:
+ return dev->err;
}
EXPORT_SYMBOL_GPL(vhost_dev_init);
@@ -370,7 +443,6 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
/* No owner, become one */
dev->mm = get_task_mm(current);
- dev->worker = worker;
err = vhost_dev_alloc_iovecs(dev);
if (err)
@@ -424,6 +496,24 @@ void vhost_dev_stop(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_dev_stop);
+static void vhost_deassign_worker(struct vhost_dev *dev)
+{
+ if (dev->worker) {
+ mutex_lock(&vhost_pool->pool_lock);
+ WARN_ON(dev->worker->num_devices <= 0);
+ if (!--dev->worker->num_devices) {
+ WARN_ON(!list_empty(&dev->worker->work_list));
+ list_del(&dev->worker->node);
+ kthread_stop(dev->worker->thread);
+ dev->worker->thread = NULL;
+ kfree(dev->worker);
+ }
+ mutex_unlock(&vhost_pool->pool_lock);
+ }
+
+ dev->worker = NULL;
+}
+
/* Caller should have device mutex if and only if locked is set */
void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
{
@@ -452,6 +542,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
/* No one will access memory at this point */
kfree(dev->memory);
dev->memory = NULL;
+ vhost_deassign_worker(dev);
if (dev->mm)
mmput(dev->mm);
dev->mm = NULL;
@@ -1542,31 +1633,29 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify);
static int __init vhost_init(void)
{
- struct vhost_worker *w =
- kzalloc(sizeof(*w), GFP_KERNEL);
- if (!w)
+ struct vhost_pool *pool =
+ kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool)
return -ENOMEM;
-
- w->thread = kthread_create(vhost_worker,
- w, "vhost-worker");
- if (IS_ERR(w->thread))
- return PTR_ERR(w->thread);
-
- worker = w;
- spin_lock_init(&worker->work_lock);
- INIT_LIST_HEAD(&worker->work_list);
- wake_up_process(worker->thread);
- pr_info("Created universal thread to service requests\n");
+ mutex_init(&pool->pool_lock);
+ INIT_LIST_HEAD(&pool->workers);
+ vhost_pool = pool;
return 0;
}
static void __exit vhost_exit(void)
{
- if (worker) {
- kthread_stop(worker->thread);
- WARN_ON(!list_empty(&worker->work_list));
- kfree(worker);
+ struct vhost_worker *worker;
+
+ if (vhost_pool) {
+ list_for_each_entry(worker, &vhost_pool->workers, node) {
+ kthread_stop(worker->thread);
+ WARN_ON(!list_empty(&worker->work_list));
+ list_del(&worker->node);
+ kfree(worker);
+ }
+ kfree(vhost_pool);
}
}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 2f204ce..a45193b 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -120,19 +120,30 @@ struct vhost_dev {
struct vhost_worker *worker;
/* for cgroup support */
struct task_struct *owner;
+ bool worker_assigned;
+ int err;
};
struct vhost_worker {
spinlock_t work_lock;
+ unsigned id;
struct list_head work_list;
struct task_struct *thread;
struct task_struct *owner;
+ int num_devices;
+ struct list_head node;
+};
+
+struct vhost_pool {
+ struct work_struct work;
+ struct mutex pool_lock;
+ struct list_head workers;
};
void vhost_work_queue(struct vhost_worker *worker,
struct vhost_work *work);
void vhost_work_flush(struct vhost_worker *worker, struct vhost_work *work);
-void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int nvqs);
+int vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs, int nvqs);
long vhost_dev_set_owner(struct vhost_dev *dev);
bool vhost_dev_has_owner(struct vhost_dev *dev);
long vhost_dev_check_owner(struct vhost_dev *);
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [RFC PATCH 3/4] cgroup: Introduce a function to compare cgroups
2015-07-13 4:07 [RFC PATCH 0/4] Shared vhost design Bandan Das
2015-07-13 4:07 ` [RFC PATCH 1/4] vhost: Introduce a universal thread to serve all users Bandan Das
2015-07-13 4:07 ` [RFC PATCH 2/4] vhost: Limit the number of devices served by a single worker thread Bandan Das
@ 2015-07-13 4:07 ` Bandan Das
2015-07-13 4:07 ` [RFC PATCH 4/4] vhost: Add cgroup-aware creation of worker threads Bandan Das
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Bandan Das @ 2015-07-13 4:07 UTC (permalink / raw)
To: kvm
Cc: netdev, linux-kernel, mst, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang
This function takes two tasks and iterates through all
hierarchies to check if they belong to the same cgroups.
It ignores the check for default hierarchies or for
hierarchies with no subsystems attached. This function
will be used by the next patch to add rudimentary cgroup support
with vhost workers.
Signed-off-by: Bandan Das <bsd@redhat.com>
---
include/linux/cgroup.h | 1 +
kernel/cgroup.c | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index b9cb94c..606fb5b 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -933,6 +933,7 @@ void css_task_iter_start(struct cgroup_subsys_state *css,
struct task_struct *css_task_iter_next(struct css_task_iter *it);
void css_task_iter_end(struct css_task_iter *it);
+int cgroup_match_groups(struct task_struct *tsk1, struct task_struct *tsk2);
int cgroup_attach_task_all(struct task_struct *from, struct task_struct *);
int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 469dd54..ba4121e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2465,6 +2465,46 @@ out_unlock_cgroup:
}
/**
+ * cgroup_match_groups - check if tsk1 and tsk2 belong to
+ * same cgroups in all hierarchies
+ * Returns 0 on success
+ */
+int cgroup_match_groups(struct task_struct *tsk1, struct task_struct *tsk2)
+{
+ struct cgroup_root *root;
+ int retval = 0;
+
+ WARN_ON(!tsk1 || !tsk2);
+
+ mutex_lock(&cgroup_mutex);
+ for_each_root(root) {
+ struct cgroup *cg_tsk1;
+ struct cgroup *cg_tsk2;
+
+ /* Default hierarchy */
+ if (root == &cgrp_dfl_root)
+ continue;
+ /* No subsystems attached */
+ if (!root->subsys_mask)
+ continue;
+
+ down_read(&css_set_rwsem);
+ cg_tsk1 = task_cgroup_from_root(tsk1, root);
+ cg_tsk2 = task_cgroup_from_root(tsk2, root);
+ up_read(&css_set_rwsem);
+
+ if (cg_tsk1 != cg_tsk2) {
+ retval = 1;
+ break;
+ }
+ }
+ mutex_unlock(&cgroup_mutex);
+
+ return retval;
+}
+EXPORT_SYMBOL_GPL(cgroup_match_groups);
+
+/**
* cgroup_attach_task_all - attach task 'tsk' to all cgroups of task 'from'
* @from: attach to all cgroups of a given task
* @tsk: the task to be attached
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [RFC PATCH 4/4] vhost: Add cgroup-aware creation of worker threads
2015-07-13 4:07 [RFC PATCH 0/4] Shared vhost design Bandan Das
` (2 preceding siblings ...)
2015-07-13 4:07 ` [RFC PATCH 3/4] cgroup: Introduce a function to compare cgroups Bandan Das
@ 2015-07-13 4:07 ` Bandan Das
2015-07-27 21:12 ` Michael S. Tsirkin
[not found] ` <OF451FED84.3040AFD2-ONC2257E8C.0043F908-C2257E8C.00446592@il.ibm.com>
2015-07-27 21:02 ` Michael S. Tsirkin
5 siblings, 1 reply; 18+ messages in thread
From: Bandan Das @ 2015-07-13 4:07 UTC (permalink / raw)
To: kvm
Cc: netdev, linux-kernel, mst, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang
With the help of the cgroup function to compare groups introduced
in the previous patch, this changes worker creation policy.
If the new device belongs to different cgroups than any of the
devices we are currently serving, we end up creating a new worker
thread even if we haven't reached the devs_per_worker threshold
Signed-off-by: Bandan Das <bsd@redhat.com>
---
drivers/vhost/vhost.c | 47 +++++++++++++++++++++++++++++++++++++++--------
1 file changed, 39 insertions(+), 8 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6a5d4c0..dc0fa37 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -261,12 +261,6 @@ static int vhost_worker(void *data)
use_mm(dev->mm);
}
- /* TODO: Consider a more elegant solution */
- if (worker->owner != dev->owner) {
- /* Should check for return value */
- cgroup_attach_task_all(dev->owner, current);
- worker->owner = dev->owner;
- }
work->fn(work);
if (need_resched())
schedule();
@@ -278,6 +272,36 @@ static int vhost_worker(void *data)
return 0;
}
+struct vhost_attach_cgroups_struct {
+ struct vhost_work work;
+ struct task_struct *owner;
+ int ret;
+};
+
+static void vhost_attach_cgroups_work(struct vhost_work *work)
+{
+ struct vhost_attach_cgroups_struct *s;
+
+ s = container_of(work, struct vhost_attach_cgroups_struct, work);
+ s->ret = cgroup_attach_task_all(s->owner, current);
+}
+
+static void vhost_attach_cgroups(struct vhost_dev *dev,
+ struct vhost_worker *worker)
+{
+ struct vhost_attach_cgroups_struct attach;
+
+ attach.owner = dev->owner;
+ vhost_work_init(dev, &attach.work, vhost_attach_cgroups_work);
+ vhost_work_queue(worker, &attach.work);
+ vhost_work_flush(worker, &attach.work);
+
+ if (!attach.ret)
+ worker->owner = dev->owner;
+
+ dev->err = attach.ret;
+}
+
static void vhost_create_worker(struct vhost_dev *dev)
{
struct vhost_worker *worker;
@@ -300,8 +324,14 @@ static void vhost_create_worker(struct vhost_dev *dev)
spin_lock_init(&worker->work_lock);
INIT_LIST_HEAD(&worker->work_list);
+
+ /* attach to the cgroups of the process that created us */
+ vhost_attach_cgroups(dev, worker);
+ if (dev->err)
+ goto therror;
+ worker->owner = dev->owner;
+
list_add(&worker->node, &pool->workers);
- worker->owner = NULL;
worker->num_devices++;
total_vhost_workers++;
dev->worker = worker;
@@ -320,7 +350,8 @@ static int vhost_dev_assign_worker(struct vhost_dev *dev)
mutex_lock(&vhost_pool->pool_lock);
list_for_each_entry(worker, &vhost_pool->workers, node) {
- if (worker->num_devices < devs_per_worker) {
+ if (worker->num_devices < devs_per_worker &&
+ (!cgroup_match_groups(dev->owner, worker->owner))) {
dev->worker = worker;
dev->worker_assigned = true;
worker->num_devices++;
--
2.4.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [RFC PATCH 4/4] vhost: Add cgroup-aware creation of worker threads
2015-07-13 4:07 ` [RFC PATCH 4/4] vhost: Add cgroup-aware creation of worker threads Bandan Das
@ 2015-07-27 21:12 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-07-27 21:12 UTC (permalink / raw)
To: Bandan Das
Cc: kvm, netdev, linux-kernel, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang, Tejun Heo
On Mon, Jul 13, 2015 at 12:07:35AM -0400, Bandan Das wrote:
> With the help of the cgroup function to compare groups introduced
> in the previous patch, this changes worker creation policy.
> If the new device belongs to different cgroups than any of the
> devices we are currently serving, we end up creating a new worker
> thread even if we haven't reached the devs_per_worker threshold
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
Would it make sense to integrate this in the work-queue mechanism somehow?
Just a thought - correctly accounting kernel's work
on behalf of specific userspace groups might have value generally.
Or is the usecase too special?
Cc Tejun for comments.
> ---
> drivers/vhost/vhost.c | 47 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 6a5d4c0..dc0fa37 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -261,12 +261,6 @@ static int vhost_worker(void *data)
> use_mm(dev->mm);
> }
>
> - /* TODO: Consider a more elegant solution */
> - if (worker->owner != dev->owner) {
> - /* Should check for return value */
> - cgroup_attach_task_all(dev->owner, current);
> - worker->owner = dev->owner;
> - }
> work->fn(work);
> if (need_resched())
> schedule();
> @@ -278,6 +272,36 @@ static int vhost_worker(void *data)
> return 0;
> }
>
> +struct vhost_attach_cgroups_struct {
> + struct vhost_work work;
> + struct task_struct *owner;
> + int ret;
> +};
> +
> +static void vhost_attach_cgroups_work(struct vhost_work *work)
> +{
> + struct vhost_attach_cgroups_struct *s;
> +
> + s = container_of(work, struct vhost_attach_cgroups_struct, work);
> + s->ret = cgroup_attach_task_all(s->owner, current);
> +}
> +
> +static void vhost_attach_cgroups(struct vhost_dev *dev,
> + struct vhost_worker *worker)
> +{
> + struct vhost_attach_cgroups_struct attach;
> +
> + attach.owner = dev->owner;
> + vhost_work_init(dev, &attach.work, vhost_attach_cgroups_work);
> + vhost_work_queue(worker, &attach.work);
> + vhost_work_flush(worker, &attach.work);
> +
> + if (!attach.ret)
> + worker->owner = dev->owner;
> +
> + dev->err = attach.ret;
> +}
> +
> static void vhost_create_worker(struct vhost_dev *dev)
> {
> struct vhost_worker *worker;
> @@ -300,8 +324,14 @@ static void vhost_create_worker(struct vhost_dev *dev)
>
> spin_lock_init(&worker->work_lock);
> INIT_LIST_HEAD(&worker->work_list);
> +
> + /* attach to the cgroups of the process that created us */
> + vhost_attach_cgroups(dev, worker);
> + if (dev->err)
> + goto therror;
> + worker->owner = dev->owner;
> +
> list_add(&worker->node, &pool->workers);
> - worker->owner = NULL;
> worker->num_devices++;
> total_vhost_workers++;
> dev->worker = worker;
> @@ -320,7 +350,8 @@ static int vhost_dev_assign_worker(struct vhost_dev *dev)
>
> mutex_lock(&vhost_pool->pool_lock);
> list_for_each_entry(worker, &vhost_pool->workers, node) {
> - if (worker->num_devices < devs_per_worker) {
> + if (worker->num_devices < devs_per_worker &&
> + (!cgroup_match_groups(dev->owner, worker->owner))) {
> dev->worker = worker;
> dev->worker_assigned = true;
> worker->num_devices++;
> --
> 2.4.3
^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <OF451FED84.3040AFD2-ONC2257E8C.0043F908-C2257E8C.00446592@il.ibm.com>]
* Re: [RFC PATCH 0/4] Shared vhost design
[not found] ` <OF451FED84.3040AFD2-ONC2257E8C.0043F908-C2257E8C.00446592@il.ibm.com>
@ 2015-07-27 19:48 ` Bandan Das
2015-07-27 21:07 ` Michael S. Tsirkin
0 siblings, 1 reply; 18+ messages in thread
From: Bandan Das @ 2015-07-27 19:48 UTC (permalink / raw)
To: Eyal Moscovici
Cc: cgroups, jasowang, kvm, linux-kernel, mst, netdev, Razya Ladelsky
Eyal Moscovici <EYALMO@il.ibm.com> writes:
> Hi,
>
> The test showed the same relative numbers as we got in our internal
> testing. I was wondering about the configuration in regards to NUMA. From
Thanks for confirming.
> our testing we saw that if the VMs are spread across 2 NUMA nodes then
> having a shared vhost thread per node performs better then having the two
> threads in the same core.
IIUC, this is similar to my test setup and observations i.e
> 14* 1173.8 1216.9
In this case, there's a shared vhost thread on CPU 14 for numa node 0
and another on CPU 15 for numa node 1. Guests running on CPUs 0,2,4,6,8,10,12
are serviced by vhost-0 that runs on CPU 14 and guests running on CPUs 1,3,5,7,9,11,13
get serviced by vhost-1 (Numa node 1). I tried some other configurations but
this configuration gave me the best results.
Eyal, I think it makes sense to add polling on top of these patches and
get numbers for them too. Thoughts ?
Bandan
> Eyal Moscovici
> HL-Cloud Infrastructure Solutions
> IBM Haifa Research Lab
>
>
>
> From: Bandan Das <bsd@redhat.com>
> To: kvm@vger.kernel.org
> Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
> mst@redhat.com, Eyal Moscovici/Haifa/IBM@IBMIL, Razya
> Ladelsky/Haifa/IBM@IBMIL, cgroups@vger.kernel.org, jasowang@redhat.com
> Date: 07/13/2015 07:08 AM
> Subject: [RFC PATCH 0/4] Shared vhost design
>
>
>
> Hello,
>
> There have been discussions on improving the current vhost design. The
> first
> attempt, to my knowledge was Shirley Ma's patch to create a dedicated
> vhost
> worker per cgroup.
>
> http://comments.gmane.org/gmane.linux.network/224730
>
> Later, I posted a cmwq based approach for performance comparisions
> http://comments.gmane.org/gmane.linux.network/286858
>
> More recently was the Elvis work that was presented in KVM Forum 2013
> http://www.linux-kvm.org/images/a/a3/Kvm-forum-2013-elvis.pdf
>
> The Elvis patches rely on common vhost thread design for scalability
> along with polling for performance. Since there are two major changes
> being proposed, we decided to split up the work. The first (this RFC),
> proposing a re-design of the vhost threading model and the second part
> (not posted yet) to focus more on improving performance.
>
> I am posting this with the hope that we can have a meaningful discussion
> on the proposed new architecture. We have run some tests to show that the
> new
> design is scalable and in terms of performance, is comparable to the
> current
> stable design.
>
> Test Setup:
> The testing is based on the setup described in the Elvis proposal.
> The initial tests are just an aggregate of Netperf STREAM and MAERTS but
> as we progress, I am happy to run more tests. The hosts are two identical
> 16 core Haswell systems with point to point network links. For the first
> 10 runs,
> with n=1 upto n=10 guests running in parallel, I booted the target system
> with nr_cpus=8
> and mem=12G. The purpose was to do a comparision of resource utilization
> and how it affects performance. Finally, with the number of guests set at
> 14,
> I didn't limit the number of CPUs booted on the host or limit memory seen
> by
> the kernel but boot the kernel with isolcpus=14,15 that will be used to
> run
> the vhost threads. The guests are pinned to cpus 0-13 and based on which
> cpu the guest is running on, the corresponding I/O thread is either pinned
> to cpu 14 or 15.
>
> Results
> # X axis is number of guests
> # Y axis is netperf number
> # nr_cpus=8 and mem=12G
> #Number of Guests #Baseline #ELVIS
> 1 1119.3 1111.0
> 2 1135.6 1130.2
> 3 1135.5 1131.6
> 4 1136.0 1127.1
> 5 1118.6 1129.3
> 6 1123.4 1129.8
> 7 1128.7 1135.4
> 8 1129.9 1137.5
> 9 1130.6 1135.1
> 10 1129.3 1138.9
> 14* 1173.8 1216.9
>
> #* Last run with the vCPU and I/O thread(s) pinned, no CPU/memory limit
> imposed.
> # I/O thread runs on CPU 14 or 15 depending on which guest it's serving
>
> There's a simple graph at
> http://people.redhat.com/~bdas/elvis/data/results.png
> that shows how task affinity results in a jump and even without it,
> as the number of guests increase, the shared vhost design performs
> slightly better.
>
> Observations:
> 1. In terms of "stock" performance, the results are comparable.
> 2. However, with a tuned setup, even without polling, we see an
> improvement
> with the new design.
> 3. Making the new design simulate old behavior would be a matter of
> setting
> the number of guests per vhost threads to 1.
> 4. Maybe, setting a per guest limit on the work being done by a specific
> vhost
> thread is needed for it to be fair.
> 5. cgroup associations needs to be figured out. I just slightly hacked the
> current cgroup association mechanism to work with the new model. Ccing
> cgroups
> for input/comments.
>
> Many thanks to Razya Ladelsky and Eyal Moscovici, IBM for the initial
> patches, the helpful testing suggestions and discussions.
>
> Bandan Das (4):
> vhost: Introduce a universal thread to serve all users
> vhost: Limit the number of devices served by a single worker thread
> cgroup: Introduce a function to compare cgroups
> vhost: Add cgroup-aware creation of worker threads
>
> drivers/vhost/net.c | 6 +-
> drivers/vhost/scsi.c | 18 ++--
> drivers/vhost/vhost.c | 272
> +++++++++++++++++++++++++++++++++++--------------
> drivers/vhost/vhost.h | 32 +++++-
> include/linux/cgroup.h | 1 +
> kernel/cgroup.c | 40 ++++++++
> 6 files changed, 275 insertions(+), 94 deletions(-)
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [RFC PATCH 0/4] Shared vhost design
2015-07-27 19:48 ` [RFC PATCH 0/4] Shared vhost design Bandan Das
@ 2015-07-27 21:07 ` Michael S. Tsirkin
[not found] ` <OFFB2CB583.341B00EF-ONC2257E94.002FF06E-C2257E94.0032BC0A@il.ibm.com>
0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-07-27 21:07 UTC (permalink / raw)
To: Bandan Das
Cc: Eyal Moscovici, cgroups, jasowang, kvm, linux-kernel, netdev,
Razya Ladelsky
On Mon, Jul 27, 2015 at 03:48:19PM -0400, Bandan Das wrote:
> Eyal Moscovici <EYALMO@il.ibm.com> writes:
>
> > Hi,
> >
> > The test showed the same relative numbers as we got in our internal
> > testing. I was wondering about the configuration in regards to NUMA. From
> Thanks for confirming.
>
> > our testing we saw that if the VMs are spread across 2 NUMA nodes then
> > having a shared vhost thread per node performs better then having the two
> > threads in the same core.
>
> IIUC, this is similar to my test setup and observations i.e
> > 14* 1173.8 1216.9
>
> In this case, there's a shared vhost thread on CPU 14 for numa node 0
> and another on CPU 15 for numa node 1. Guests running on CPUs 0,2,4,6,8,10,12
> are serviced by vhost-0 that runs on CPU 14 and guests running on CPUs 1,3,5,7,9,11,13
> get serviced by vhost-1 (Numa node 1). I tried some other configurations but
> this configuration gave me the best results.
>
>
> Eyal, I think it makes sense to add polling on top of these patches and
> get numbers for them too. Thoughts ?
>
> Bandan
So simple polling by vhost is kind of ok for some guests, but I think to
really make it work for a reasonably wide selection of guests/workloads
you need to combine it with 1. polling the NIC - it makes no sense to me
to only poll one side of the equation; and probably 2. - polling in
guest.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC PATCH 0/4] Shared vhost design
2015-07-13 4:07 [RFC PATCH 0/4] Shared vhost design Bandan Das
` (4 preceding siblings ...)
[not found] ` <OF451FED84.3040AFD2-ONC2257E8C.0043F908-C2257E8C.00446592@il.ibm.com>
@ 2015-07-27 21:02 ` Michael S. Tsirkin
[not found] ` <20150727235818-mutt-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
5 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-07-27 21:02 UTC (permalink / raw)
To: Bandan Das
Cc: kvm, netdev, linux-kernel, Eyal Moscovici, Razya Ladelsky,
cgroups, jasowang
On Mon, Jul 13, 2015 at 12:07:31AM -0400, Bandan Das wrote:
> Hello,
>
> There have been discussions on improving the current vhost design. The first
> attempt, to my knowledge was Shirley Ma's patch to create a dedicated vhost
> worker per cgroup.
>
> http://comments.gmane.org/gmane.linux.network/224730
>
> Later, I posted a cmwq based approach for performance comparisions
> http://comments.gmane.org/gmane.linux.network/286858
>
> More recently was the Elvis work that was presented in KVM Forum 2013
> http://www.linux-kvm.org/images/a/a3/Kvm-forum-2013-elvis.pdf
>
> The Elvis patches rely on common vhost thread design for scalability
> along with polling for performance. Since there are two major changes
> being proposed, we decided to split up the work. The first (this RFC),
> proposing a re-design of the vhost threading model and the second part
> (not posted yet) to focus more on improving performance.
>
> I am posting this with the hope that we can have a meaningful discussion
> on the proposed new architecture. We have run some tests to show that the new
> design is scalable and in terms of performance, is comparable to the current
> stable design.
>
> Test Setup:
> The testing is based on the setup described in the Elvis proposal.
> The initial tests are just an aggregate of Netperf STREAM and MAERTS but
> as we progress, I am happy to run more tests. The hosts are two identical
> 16 core Haswell systems with point to point network links. For the first 10 runs,
> with n=1 upto n=10 guests running in parallel, I booted the target system with nr_cpus=8
> and mem=12G. The purpose was to do a comparision of resource utilization
> and how it affects performance. Finally, with the number of guests set at 14,
> I didn't limit the number of CPUs booted on the host or limit memory seen by
> the kernel but boot the kernel with isolcpus=14,15 that will be used to run
> the vhost threads. The guests are pinned to cpus 0-13 and based on which
> cpu the guest is running on, the corresponding I/O thread is either pinned
> to cpu 14 or 15.
> Results
> # X axis is number of guests
> # Y axis is netperf number
> # nr_cpus=8 and mem=12G
> #Number of Guests #Baseline #ELVIS
> 1 1119.3 1111.0
> 2 1135.6 1130.2
> 3 1135.5 1131.6
> 4 1136.0 1127.1
> 5 1118.6 1129.3
> 6 1123.4 1129.8
> 7 1128.7 1135.4
> 8 1129.9 1137.5
> 9 1130.6 1135.1
> 10 1129.3 1138.9
> 14* 1173.8 1216.9
I'm a bit too busy now, with 2.4 and related stuff, will review once we
finish 2.4. But I'd like to ask two things:
- did you actually test a config where cgroups were used?
- does the design address the issue of VM 1 being blocked
(e.g. because it hits swap) and blocking VM 2?
>
> #* Last run with the vCPU and I/O thread(s) pinned, no CPU/memory limit imposed.
> # I/O thread runs on CPU 14 or 15 depending on which guest it's serving
>
> There's a simple graph at
> http://people.redhat.com/~bdas/elvis/data/results.png
> that shows how task affinity results in a jump and even without it,
> as the number of guests increase, the shared vhost design performs
> slightly better.
>
> Observations:
> 1. In terms of "stock" performance, the results are comparable.
> 2. However, with a tuned setup, even without polling, we see an improvement
> with the new design.
> 3. Making the new design simulate old behavior would be a matter of setting
> the number of guests per vhost threads to 1.
> 4. Maybe, setting a per guest limit on the work being done by a specific vhost
> thread is needed for it to be fair.
> 5. cgroup associations needs to be figured out. I just slightly hacked the
> current cgroup association mechanism to work with the new model. Ccing cgroups
> for input/comments.
>
> Many thanks to Razya Ladelsky and Eyal Moscovici, IBM for the initial
> patches, the helpful testing suggestions and discussions.
>
> Bandan Das (4):
> vhost: Introduce a universal thread to serve all users
> vhost: Limit the number of devices served by a single worker thread
> cgroup: Introduce a function to compare cgroups
> vhost: Add cgroup-aware creation of worker threads
>
> drivers/vhost/net.c | 6 +-
> drivers/vhost/scsi.c | 18 ++--
> drivers/vhost/vhost.c | 272 +++++++++++++++++++++++++++++++++++--------------
> drivers/vhost/vhost.h | 32 +++++-
> include/linux/cgroup.h | 1 +
> kernel/cgroup.c | 40 ++++++++
> 6 files changed, 275 insertions(+), 94 deletions(-)
>
> --
> 2.4.3
^ permalink raw reply [flat|nested] 18+ messages in thread
