[RFC 0/7] vhost: Add support of kthread API

[RFC 0/7] vhost: Add support of kthread API

[Cindy Lu]

In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
vhost removed the support for the kthread API. However, there are
still situations where there is a request to use kthread.
In this RFC, the support of kthread is added back. Additionally,
a module_param is added to identify which mode we are using, and
a new UAPI is introduced to allow the userspace app to set the
mode they want to use.

Cindy Lu (7):
  vhost: Add a new module_param for enable kthread
  vhost: Add kthread support in function vhost_worker_queue()
  vhost: Add kthread support in function vhost_workers_free()
  vhost: Add the vhost_worker to support kthread
  vhost: Add the cgroup related function
  vhost: Add kthread support in function vhost_worker_create
  vhost: Add new UAPI to support changing Kthread mode

 drivers/vhost/vhost.c      | 241 +++++++++++++++++++++++++++++++++++--
 drivers/vhost/vhost.h      |   1 +
 include/uapi/linux/vhost.h |   2 +
 3 files changed, 235 insertions(+), 9 deletions(-)

-- 
2.45.0

[RFC 1/7] vhost: Add a new module_param for enable kthread

[Cindy Lu]

Add a new module parameter to enable kthread while loading modules.
This parameter will identify if the vhost will use kthread or a task.
The default value will be true.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9ac25d08f473..da701f6b3f2c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -41,6 +41,9 @@ static int max_iotlb_entries = 2048;
 module_param(max_iotlb_entries, int, 0444);
 MODULE_PARM_DESC(max_iotlb_entries,
 	"Maximum number of iotlb entries. (default: 2048)");
+bool use_kthread = true;
+module_param(use_kthread, bool, 0444);
+MODULE_PARM_DESC(use_kthread, "enable vhost to use kthread (default: Y)");
 
 enum {
 	VHOST_MEMORY_F_LOG = 0x1,
-- 
2.45.0

[RFC 2/7] vhost: Add kthread support in function vhost_worker_queue()

[Cindy Lu]

Added back the previously removed function vhost_worker_queue() and
renamed it to vhost_worker_queue_khtread(). The new vhost_worker_queue()
will select the different mode based on the value of the parameter.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 30 ++++++++++++++++++++++++++++--
 drivers/vhost/vhost.h |  1 +
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index da701f6b3f2c..6261c9df468e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -236,8 +236,8 @@ void vhost_poll_stop(struct vhost_poll *poll)
 }
 EXPORT_SYMBOL_GPL(vhost_poll_stop);
 
-static void vhost_worker_queue(struct vhost_worker *worker,
-			       struct vhost_work *work)
+static void vhost_worker_queue_task(struct vhost_worker *worker,
+				    struct vhost_work *work)
 {
 	if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
 		/* We can only add the work to the list after we're
@@ -249,6 +249,32 @@ static void vhost_worker_queue(struct vhost_worker *worker,
 	}
 }
 
+static void vhost_work_queue_kthread(struct vhost_worker *worker,
+				     struct vhost_work *work)
+{
+	if (!worker)
+		return;
+
+	if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
+		/* We can only add the work to the list after we're
+		 * sure it was not in the list.
+		 * test_and_set_bit() implies a memory barrier.
+		 */
+		llist_add(&work->node, &worker->work_list);
+
+		wake_up_process(worker->task);
+	}
+}
+
+static void vhost_worker_queue(struct vhost_worker *worker,
+			       struct vhost_work *work)
+{
+	if (use_kthread) {
+		vhost_work_queue_kthread(worker, work);
+	} else {
+		vhost_worker_queue_task(worker, work);
+	}
+}
 bool vhost_vq_work_queue(struct vhost_virtqueue *vq, struct vhost_work *work)
 {
 	struct vhost_worker *worker;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..c7f126fd09e8 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -27,6 +27,7 @@ struct vhost_work {
 };
 
 struct vhost_worker {
+	struct task_struct	*task;
 	struct vhost_task	*vtsk;
 	struct vhost_dev	*dev;
 	/* Used to serialize device wide flushing with worker swapping. */
-- 
2.45.0

Re: [RFC 2/7] vhost: Add kthread support in function vhost_worker_queue()

[Jason Wang]

On Mon, Aug 19, 2024 at 5:29 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Added back the previously removed function vhost_worker_queue() and
> renamed it to vhost_worker_queue_khtread(). The new vhost_worker_queue()
> will select the different mode based on the value of the parameter.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

It looks like a partial revert of some previous commit, please mention
the commit id in the change log as least.

So did for the following 3 or 4 patches.

Thanks

> ---
>  drivers/vhost/vhost.c | 30 ++++++++++++++++++++++++++++--
>  drivers/vhost/vhost.h |  1 +
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index da701f6b3f2c..6261c9df468e 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -236,8 +236,8 @@ void vhost_poll_stop(struct vhost_poll *poll)
>  }
>  EXPORT_SYMBOL_GPL(vhost_poll_stop);
>
> -static void vhost_worker_queue(struct vhost_worker *worker,
> -                              struct vhost_work *work)
> +static void vhost_worker_queue_task(struct vhost_worker *worker,
> +                                   struct vhost_work *work)
>  {
>         if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
>                 /* We can only add the work to the list after we're
> @@ -249,6 +249,32 @@ static void vhost_worker_queue(struct vhost_worker *worker,
>         }
>  }
>
> +static void vhost_work_queue_kthread(struct vhost_worker *worker,
> +                                    struct vhost_work *work)
> +{
> +       if (!worker)
> +               return;
> +
> +       if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
> +               /* We can only add the work to the list after we're
> +                * sure it was not in the list.
> +                * test_and_set_bit() implies a memory barrier.
> +                */
> +               llist_add(&work->node, &worker->work_list);
> +
> +               wake_up_process(worker->task);
> +       }
> +}
> +
> +static void vhost_worker_queue(struct vhost_worker *worker,
> +                              struct vhost_work *work)
> +{
> +       if (use_kthread) {
> +               vhost_work_queue_kthread(worker, work);
> +       } else {
> +               vhost_worker_queue_task(worker, work);
> +       }
> +}
>  bool vhost_vq_work_queue(struct vhost_virtqueue *vq, struct vhost_work *work)
>  {
>         struct vhost_worker *worker;
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index bb75a292d50c..c7f126fd09e8 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -27,6 +27,7 @@ struct vhost_work {
>  };
>
>  struct vhost_worker {
> +       struct task_struct      *task;
>         struct vhost_task       *vtsk;
>         struct vhost_dev        *dev;
>         /* Used to serialize device wide flushing with worker swapping. */
> --
> 2.45.0
>

Re: [RFC 2/7] vhost: Add kthread support in function vhost_worker_queue()

[Cindy Lu]

On Tue, 27 Aug 2024 at 10:36, Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Aug 19, 2024 at 5:29 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Added back the previously removed function vhost_worker_queue() and
> > renamed it to vhost_worker_queue_khtread(). The new vhost_worker_queue()
> > will select the different mode based on the value of the parameter.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
>
> It looks like a partial revert of some previous commit, please mention
> the commit id in the change log as least.
>
> So did for the following 3 or 4 patches.
>
> Thanks
>
sure, will add this
Thanks
cindy
> > ---
> >  drivers/vhost/vhost.c | 30 ++++++++++++++++++++++++++++--
> >  drivers/vhost/vhost.h |  1 +
> >  2 files changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index da701f6b3f2c..6261c9df468e 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -236,8 +236,8 @@ void vhost_poll_stop(struct vhost_poll *poll)
> >  }
> >  EXPORT_SYMBOL_GPL(vhost_poll_stop);
> >
> > -static void vhost_worker_queue(struct vhost_worker *worker,
> > -                              struct vhost_work *work)
> > +static void vhost_worker_queue_task(struct vhost_worker *worker,
> > +                                   struct vhost_work *work)
> >  {
> >         if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
> >                 /* We can only add the work to the list after we're
> > @@ -249,6 +249,32 @@ static void vhost_worker_queue(struct vhost_worker *worker,
> >         }
> >  }
> >
> > +static void vhost_work_queue_kthread(struct vhost_worker *worker,
> > +                                    struct vhost_work *work)
> > +{
> > +       if (!worker)
> > +               return;
> > +
> > +       if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
> > +               /* We can only add the work to the list after we're
> > +                * sure it was not in the list.
> > +                * test_and_set_bit() implies a memory barrier.
> > +                */
> > +               llist_add(&work->node, &worker->work_list);
> > +
> > +               wake_up_process(worker->task);
> > +       }
> > +}
> > +
> > +static void vhost_worker_queue(struct vhost_worker *worker,
> > +                              struct vhost_work *work)
> > +{
> > +       if (use_kthread) {
> > +               vhost_work_queue_kthread(worker, work);
> > +       } else {
> > +               vhost_worker_queue_task(worker, work);
> > +       }
> > +}
> >  bool vhost_vq_work_queue(struct vhost_virtqueue *vq, struct vhost_work *work)
> >  {
> >         struct vhost_worker *worker;
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index bb75a292d50c..c7f126fd09e8 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -27,6 +27,7 @@ struct vhost_work {
> >  };
> >
> >  struct vhost_worker {
> > +       struct task_struct      *task;
> >         struct vhost_task       *vtsk;
> >         struct vhost_dev        *dev;
> >         /* Used to serialize device wide flushing with worker swapping. */
> > --
> > 2.45.0
> >
>

[RFC 3/7] vhost: Add kthread support in function vhost_workers_free()

[Cindy Lu]

Added back the previously removed function vhost_workers_free() and
renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
will select the different mode based on the value of the parameter.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 52 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6261c9df468e..77071def355c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -647,8 +647,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
 	dev->mm = NULL;
 }
 
-static void vhost_worker_destroy(struct vhost_dev *dev,
-				 struct vhost_worker *worker)
+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
+					 struct vhost_worker *worker)
+{
+	if (!worker)
+		return;
+
+	WARN_ON(!llist_empty(&worker->work_list));
+
+	xa_erase(&dev->worker_xa, worker->id);
+	kthread_stop(worker->task);
+	kfree(worker);
+}
+
+static void vhost_worker_destroy_task(struct vhost_dev *dev,
+				      struct vhost_worker *worker)
 {
 	if (!worker)
 		return;
@@ -659,7 +672,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
 	kfree(worker);
 }
 
-static void vhost_workers_free(struct vhost_dev *dev)
+static void vhost_workers_free_task(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
 	unsigned long i;
@@ -674,10 +687,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	 * created but couldn't clean up (it forgot or crashed).
 	 */
 	xa_for_each(&dev->worker_xa, i, worker)
-		vhost_worker_destroy(dev, worker);
+		vhost_worker_destroy_task(dev, worker);
 	xa_destroy(&dev->worker_xa);
 }
 
+static void vhost_workers_free_kthread(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	unsigned long i;
+
+	if (!dev->use_worker)
+		return;
+
+	for (i = 0; i < dev->nvqs; i++)
+		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
+	/*
+	 * Free the default worker we created and cleanup workers userspace
+	 * created but couldn't clean up (it forgot or crashed).
+	 */
+	xa_for_each(&dev->worker_xa, i, worker)
+		vhost_worker_destroy_kthread(dev, worker);
+	xa_destroy(&dev->worker_xa);
+}
+
+static void vhost_workers_free(struct vhost_dev *dev)
+{
+	if (use_kthread)
+		vhost_workers_free_kthread(dev);
+	else
+		vhost_workers_free_task(dev);
+}
 static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
@@ -845,7 +884,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
 	__vhost_worker_flush(worker);
 	mutex_unlock(&worker->mutex);
 
-	vhost_worker_destroy(dev, worker);
+	if (use_kthread)
+		vhost_worker_destroy_kthread(dev, worker);
+	else
+		vhost_worker_destroy_task(dev, worker);
 	return 0;
 }
 
-- 
2.45.0

[RFC 4/7] vhost: Add the vhost_worker to support kthread

[Cindy Lu]

Add back the previously removed vhost_worker function to support the kthread
and rename it vhost_run_work_kthread_list.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 77071def355c..593f9521f51d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -417,6 +417,44 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	__vhost_vq_meta_reset(vq);
 }
 
+static int vhost_run_work_kthread_list(void *data)
+{
+	struct vhost_worker *worker = data;
+	struct vhost_work *work, *work_next;
+	struct vhost_dev *dev = worker->dev;
+	struct llist_node *node;
+
+	kthread_use_mm(dev->mm);
+
+	for (;;) {
+		/* mb paired w/ kthread_stop */
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		if (kthread_should_stop()) {
+			__set_current_state(TASK_RUNNING);
+			break;
+		}
+		node = llist_del_all(&worker->work_list);
+		if (!node)
+			schedule();
+
+		node = llist_reverse_order(node);
+		/* make sure flag is seen after deletion */
+		smp_wmb();
+		llist_for_each_entry_safe(work, work_next, node, node) {
+			clear_bit(VHOST_WORK_QUEUED, &work->flags);
+			__set_current_state(TASK_RUNNING);
+			kcov_remote_start_common(worker->kcov_handle);
+			work->fn(work);
+			kcov_remote_stop();
+			cond_resched();
+		}
+	}
+	kthread_unuse_mm(dev->mm);
+
+	return 0;
+}
+
 static bool vhost_run_work_list(void *data)
 {
 	struct vhost_worker *worker = data;
-- 
2.45.0

[RFC 5/7] vhost: Add the cgroup related function

[Cindy Lu]

Add back the previously removed cgroup function to support the kthread
The biggest change for this part is in vhost_attach_cgroups() and
vhost_worker_cgroups_kthread(). This is because of the change in
struct dev->worker_xa.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 593f9521f51d..66ccda81f073 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/kthread.h>
+#include <linux/cgroup.h>
 #include <linux/module.h>
 #include <linux/sort.h>
 #include <linux/sched/mm.h>
@@ -648,6 +649,57 @@ 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_worker_cgroups_kthread(struct vhost_worker *worker)
+{
+	struct vhost_flush_struct flush;
+	struct vhost_attach_cgroups_struct attach;
+
+	attach.owner = current;
+
+	vhost_work_init(&attach.work, vhost_attach_cgroups_work);
+	vhost_worker_queue(worker, &attach.work);
+
+	init_completion(&flush.wait_event);
+	vhost_work_init(&flush.work, vhost_flush_work);
+	vhost_worker_queue(worker, &flush.work);
+	wait_for_completion(&flush.wait_event);
+
+	return attach.ret;
+}
+
+static int vhost_attach_cgroups(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	unsigned long i;
+	int ret;
+
+	/*
+	 * Free the default worker we created and cleanup workers userspace
+	 * created but couldn't clean up (it forgot or crashed).
+	 */
+
+	xa_for_each(&dev->worker_xa, i, worker) {
+		ret = vhost_worker_cgroups_kthread(worker);
+		if (ret)
+			return ret;
+	}
+	return ret;
+}
+
 /* Caller should have device mutex */
 bool vhost_dev_has_owner(struct vhost_dev *dev)
 {
-- 
2.45.0

[RFC 6/7] vhost: Add kthread support in function vhost_worker_create

[Cindy Lu]

Split the function vhost_worker_create to support both task and kthread
       
Added back the previous old function vhost_worker_create and rename it to
vhost_worker_create_khtread to support the khtread.

The new vhost_worker_queue will be selected which to use based on the
value of the parameter.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 55 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 66ccda81f073..0a7b2999100f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -807,7 +807,8 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	else
 		vhost_workers_free_task(dev);
 }
-static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
+
+static struct vhost_worker *vhost_worker_create_task(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
 	struct vhost_task *vtsk;
@@ -848,6 +849,50 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 	return NULL;
 }
 
+static struct vhost_worker *vhost_worker_create_kthread(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	struct task_struct *task;
+	int ret;
+	u32 id;
+
+	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
+	if (!worker)
+		return NULL;
+
+	worker->dev = dev;
+	worker->kcov_handle = kcov_common_handle();
+
+	mutex_init(&worker->mutex);
+	init_llist_head(&worker->work_list);
+
+	task = kthread_create(vhost_run_work_kthread_list, worker, "vhost-%d",
+			      current->pid);
+	if (IS_ERR(task)) {
+		ret = PTR_ERR(task);
+		goto free_worker;
+	}
+
+	worker->task = task;
+	wake_up_process(task); /* avoid contributing to loadavg */
+	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+	if (ret < 0)
+		goto stop_worker;
+	worker->id = id;
+
+	ret = vhost_attach_cgroups(dev);
+	if (ret)
+		goto stop_worker;
+
+	return worker;
+
+stop_worker:
+	kthread_stop(worker->task);
+free_worker:
+	kfree(worker);
+	return NULL;
+}
+
 /* Caller must have device mutex */
 static void __vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 				     struct vhost_worker *worker)
@@ -936,6 +981,14 @@ static int vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 	return 0;
 }
 
+static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
+{
+	if (use_kthread)
+		return vhost_worker_create_kthread(dev);
+	else
+		return vhost_worker_create_task(dev);
+}
+
 /* Caller must have device mutex */
 static int vhost_new_worker(struct vhost_dev *dev,
 			    struct vhost_worker_state *info)
-- 
2.45.0

[RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Cindy Lu]

Add a new UAPI to support setting the vhost device to
use kthread mode. The user space application needs to use
VHOST_SET_USE_KTHREAD to set the mode. This setting must
be set before VHOST_SET_OWNER is set.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c      | 11 ++++++++++-
 include/uapi/linux/vhost.h |  2 ++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 0a7b2999100f..d6b71bddc272 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2340,14 +2340,23 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 {
 	struct eventfd_ctx *ctx;
 	u64 p;
-	long r;
+	long r = 0;
 	int i, fd;
+	bool kthread;
 
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
 		r = vhost_dev_set_owner(d);
 		goto done;
 	}
+	if (ioctl == VHOST_SET_USE_KTHREAD) {
+		if (copy_from_user(&kthread, argp, sizeof(kthread))) {
+			r = -EFAULT;
+			goto done;
+		}
+		use_kthread = kthread;
+		goto done;
+	}
 
 	/* You must be the owner to do anything else */
 	r = vhost_dev_check_owner(d);
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index b95dd84eef2d..386fe735da63 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,6 @@
  */
 #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
 					      struct vhost_vring_state)
+
+#define VHOST_SET_USE_KTHREAD _IOW(VHOST_VIRTIO, 0x83, bool)
 #endif
-- 
2.45.0

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Christoph Hellwig]

On Mon, Aug 19, 2024 at 05:27:33PM +0800, Cindy Lu wrote:
> Add a new UAPI to support setting the vhost device to
> use kthread mode. The user space application needs to use
> VHOST_SET_USE_KTHREAD to set the mode. This setting must
> be set before VHOST_SET_OWNER is set.

Usage of an API is a complete kernel internal detail that has absolutely
no business being exposed to applications.

What is the application visible behavior that the API use is the proxy
for?

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Cindy Lu]

On Wed, 21 Aug 2024 at 13:07, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Aug 19, 2024 at 05:27:33PM +0800, Cindy Lu wrote:
> > Add a new UAPI to support setting the vhost device to
> > use kthread mode. The user space application needs to use
> > VHOST_SET_USE_KTHREAD to set the mode. This setting must
> > be set before VHOST_SET_OWNER is set.
>
> Usage of an API is a complete kernel internal detail that has absolutely
> no business being exposed to applications.
>
> What is the application visible behavior that the API use is the proxy
> for?
>
The userspace application may need to know the details of the kernel.
For example, some userspaces may use a script to track threads. If
they use task mode, the script may not work properly. In that case,
they can choose Kthread mode.
Thanks
Cindy
>

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Jason Wang]

On Wed, Aug 21, 2024 at 1:07 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Aug 19, 2024 at 05:27:33PM +0800, Cindy Lu wrote:
> > Add a new UAPI to support setting the vhost device to
> > use kthread mode. The user space application needs to use
> > VHOST_SET_USE_KTHREAD to set the mode.

I think we need a better name. Probably something like
VHOST_INHERIT_OWNER or others (not a native speaker)

> > This setting must
> > be set before VHOST_SET_OWNER is set.
>
> Usage of an API is a complete kernel internal detail that has absolutely
> no business being exposed to applications.
>
> What is the application visible behavior that the API use is the proxy
> for?

Vhost used to be created by kthreadd but some recent patches change it
to behave like being froked by the owner. So the various attributes
that interhit from the parent has been changed (scheduling and
namespace etc).

Thanks

>
>

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Christoph Hellwig]

On Mon, Aug 26, 2024 at 02:21:52PM +0800, Jason Wang wrote:
> > What is the application visible behavior that the API use is the proxy
> > for?
> 
> Vhost used to be created by kthreadd but some recent patches change it
> to behave like being froked by the owner. So the various attributes
> that interhit from the parent has been changed (scheduling and
> namespace etc).

Well, if that breaks userspace it needs to be changed to opt into the
new behavior rather than requiring a flag to not break the existing
applications.  Assuming the change is intentional to start with.

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Jason Wang]

On Mon, Aug 26, 2024 at 2:31 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Aug 26, 2024 at 02:21:52PM +0800, Jason Wang wrote:
> > > What is the application visible behavior that the API use is the proxy
> > > for?
> >
> > Vhost used to be created by kthreadd but some recent patches change it
> > to behave like being froked by the owner. So the various attributes
> > that interhit from the parent has been changed (scheduling and
> > namespace etc).
>
> Well, if that breaks userspace it needs to be changed to opt into the
> new behavior rather than requiring a flag to not break the existing
> applications.

Yes, if I was not wrong, this is something this series tries to reach.

No flag means old behaviour, new flag means new behaviour.

> Assuming the change is intentional to start with.
>
>

Thanks

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Jason Wang]

On Tue, Aug 27, 2024 at 10:09 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Aug 26, 2024 at 2:31 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Aug 26, 2024 at 02:21:52PM +0800, Jason Wang wrote:
> > > > What is the application visible behavior that the API use is the proxy
> > > > for?
> > >
> > > Vhost used to be created by kthreadd but some recent patches change it
> > > to behave like being froked by the owner. So the various attributes
> > > that interhit from the parent has been changed (scheduling and
> > > namespace etc).
> >
> > Well, if that breaks userspace it needs to be changed to opt into the
> > new behavior rather than requiring a flag to not break the existing
> > applications.
>
> Yes, if I was not wrong, this is something this series tries to reach.
>
> No flag means old behaviour, new flag means new behaviour.

Ok, I see. The series is trying to do the reverse.

We need to fix that.

Thanks

>
> > Assuming the change is intentional to start with.
> >
> >
>
> Thanks

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Jason Wang]

On Mon, Aug 19, 2024 at 5:29 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add a new UAPI to support setting the vhost device to
> use kthread mode. The user space application needs to use
> VHOST_SET_USE_KTHREAD to set the mode. This setting must
> be set before VHOST_SET_OWNER is set.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c      | 11 ++++++++++-
>  include/uapi/linux/vhost.h |  2 ++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 0a7b2999100f..d6b71bddc272 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2340,14 +2340,23 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  {
>         struct eventfd_ctx *ctx;
>         u64 p;
> -       long r;
> +       long r = 0;
>         int i, fd;
> +       bool kthread;
>
>         /* If you are not the owner, you can become one */
>         if (ioctl == VHOST_SET_OWNER) {
>                 r = vhost_dev_set_owner(d);
>                 goto done;
>         }
> +       if (ioctl == VHOST_SET_USE_KTHREAD) {
> +               if (copy_from_user(&kthread, argp, sizeof(kthread))) {
> +                       r = -EFAULT;
> +                       goto done;
> +               }
> +               use_kthread = kthread;
> +               goto done;
> +       }
>
>         /* You must be the owner to do anything else */
>         r = vhost_dev_check_owner(d);
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index b95dd84eef2d..386fe735da63 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,6 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE      _IOWR(VHOST_VIRTIO, 0x82,       \
>                                               struct vhost_vring_state)
> +

Unnecessary changes.

> +#define VHOST_SET_USE_KTHREAD _IOW(VHOST_VIRTIO, 0x83, bool)

So I think we need to do something the opposite. New flag for new
behaviour instead of new flag for the old one ...

By using this we can unbreak the old applications.

Btw, I think this needs to come before/along with the introduction of
the module parameter that enforce old beahviour.

Thanks


>  #endif

> --
> 2.45.0
>

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Cindy Lu]

On Tue, 27 Aug 2024 at 10:32, Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Aug 19, 2024 at 5:29 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Add a new UAPI to support setting the vhost device to
> > use kthread mode. The user space application needs to use
> > VHOST_SET_USE_KTHREAD to set the mode. This setting must
> > be set before VHOST_SET_OWNER is set.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      | 11 ++++++++++-
> >  include/uapi/linux/vhost.h |  2 ++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 0a7b2999100f..d6b71bddc272 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2340,14 +2340,23 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >  {
> >         struct eventfd_ctx *ctx;
> >         u64 p;
> > -       long r;
> > +       long r = 0;
> >         int i, fd;
> > +       bool kthread;
> >
> >         /* If you are not the owner, you can become one */
> >         if (ioctl == VHOST_SET_OWNER) {
> >                 r = vhost_dev_set_owner(d);
> >                 goto done;
> >         }
> > +       if (ioctl == VHOST_SET_USE_KTHREAD) {
> > +               if (copy_from_user(&kthread, argp, sizeof(kthread))) {
> > +                       r = -EFAULT;
> > +                       goto done;
> > +               }
> > +               use_kthread = kthread;
> > +               goto done;
> > +       }
> >
> >         /* You must be the owner to do anything else */
> >         r = vhost_dev_check_owner(d);
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index b95dd84eef2d..386fe735da63 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -235,4 +235,6 @@
> >   */
> >  #define VHOST_VDPA_GET_VRING_SIZE      _IOWR(VHOST_VIRTIO, 0x82,       \
> >                                               struct vhost_vring_state)
> > +
>
> Unnecessary changes.
>
> > +#define VHOST_SET_USE_KTHREAD _IOW(VHOST_VIRTIO, 0x83, bool)
>
> So I think we need to do something the opposite. New flag for new
> behaviour instead of new flag for the old one ...
>
> By using this we can unbreak the old applications.
>
> Btw, I think this needs to come before/along with the introduction of
> the module parameter that enforce old behavior.
>
I think the old behavior your mentioned here is use Kthread?
So If I understand correctly, the NEW uapi is something like
VHOST_SET_ENFORCE_TASK

The module parameter can also be changed to.
bool enforce_kthread = true;
module_param(enforce_true, bool, 0444);

thanks
Cindy
> Thanks
>
>
> >  #endif
>
> > --
> > 2.45.0
> >
>

Re: [RFC 7/7] vhost: Add new UAPI to support changing Kthread mode

[Jason Wang]

On Mon, Aug 19, 2024 at 5:29 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add a new UAPI to support setting the vhost device to
> use kthread mode. The user space application needs to use
> VHOST_SET_USE_KTHREAD to set the mode. This setting must
> be set before VHOST_SET_OWNER is set.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c      | 11 ++++++++++-
>  include/uapi/linux/vhost.h |  2 ++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 0a7b2999100f..d6b71bddc272 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2340,14 +2340,23 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  {
>         struct eventfd_ctx *ctx;
>         u64 p;
> -       long r;
> +       long r = 0;
>         int i, fd;
> +       bool kthread;
>
>         /* If you are not the owner, you can become one */
>         if (ioctl == VHOST_SET_OWNER) {
>                 r = vhost_dev_set_owner(d);
>                 goto done;
>         }
> +       if (ioctl == VHOST_SET_USE_KTHREAD) {

Btw, should we enforce this to be called when there's no owner?

> +               if (copy_from_user(&kthread, argp, sizeof(kthread))) {
> +                       r = -EFAULT;
> +                       goto done;
> +               }
> +               use_kthread = kthread;
> +               goto done;
> +       }
>
>         /* You must be the owner to do anything else */
>         r = vhost_dev_check_owner(d);
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index b95dd84eef2d..386fe735da63 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,6 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE      _IOWR(VHOST_VIRTIO, 0x82,       \
>                                               struct vhost_vring_state)
> +
> +#define VHOST_SET_USE_KTHREAD _IOW(VHOST_VIRTIO, 0x83, bool)
>  #endif
> --
> 2.45.0
>

Thanks

Re: [RFC 0/7] vhost: Add support of kthread API

[Jason Wang]

On Mon, Aug 19, 2024 at 5:29 PM Cindy Lu <lulu@redhat.com> wrote:
>
> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> vhost removed the support for the kthread API. However, there are
> still situations where there is a request to use kthread.

I think we need some tweak here. For example, we need to mention that
the introduction of the vhost_taks introduce userspace noticeable
changes as the worker inherit attributes from the owner instead of the
kthreadd etc.

> In this RFC, the support of kthread is added back. Additionally,
> a module_param is added to identify which mode we are using,

It's probably not identified, it's more about if we need to "enforce"
the old behaviour.

> and
> a new UAPI is introduced to allow the userspace app to set the
> mode they want to use.
>
> Cindy Lu (7):
>   vhost: Add a new module_param for enable kthread
>   vhost: Add kthread support in function vhost_worker_queue()
>   vhost: Add kthread support in function vhost_workers_free()
>   vhost: Add the vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: Add kthread support in function vhost_worker_create
>   vhost: Add new UAPI to support changing Kthread mode
>
>  drivers/vhost/vhost.c      | 241 +++++++++++++++++++++++++++++++++++--
>  drivers/vhost/vhost.h      |   1 +
>  include/uapi/linux/vhost.h |   2 +
>  3 files changed, 235 insertions(+), 9 deletions(-)
>
> --
> 2.45.0
>

Re: [RFC 0/7] vhost: Add support of kthread API

[Cindy Lu]

On Tue, 27 Aug 2024 at 10:35, Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Aug 19, 2024 at 5:29 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > vhost removed the support for the kthread API. However, there are
> > still situations where there is a request to use kthread.
>
> I think we need some tweak here. For example, we need to mention that
> the introduction of the vhost_taks introduce userspace noticeable
> changes as the worker inherit attributes from the owner instead of the
> kthreadd etc.
>
> > In this RFC, the support of kthread is added back. Additionally,
> > a module_param is added to identify which mode we are using,
>
> It's probably not identified, it's more about if we need to "enforce"
> the old behaviour.
>
sure, will fix this
Thanks
cindy
> > and
> > a new UAPI is introduced to allow the userspace app to set the
> > mode they want to use.
> >
> > Cindy Lu (7):
> >   vhost: Add a new module_param for enable kthread
> >   vhost: Add kthread support in function vhost_worker_queue()
> >   vhost: Add kthread support in function vhost_workers_free()
> >   vhost: Add the vhost_worker to support kthread
> >   vhost: Add the cgroup related function
> >   vhost: Add kthread support in function vhost_worker_create
> >   vhost: Add new UAPI to support changing Kthread mode
> >
> >  drivers/vhost/vhost.c      | 241 +++++++++++++++++++++++++++++++++++--
> >  drivers/vhost/vhost.h      |   1 +
> >  include/uapi/linux/vhost.h |   2 +
> >  3 files changed, 235 insertions(+), 9 deletions(-)
> >
> > --
> > 2.45.0
> >
>