[PATCH v5 0/6] vhost: Add support of kthread API

[PATCH v5 0/6] vhost: Add support of kthread API

[Cindy Lu]

In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),   
the vhost now uses vhost_task and operates as a child of the   
owner thread. This aligns with containerization principles.   
However, this change has caused confusion for some legacy   
userspace applications. Therefore, we are reintroducing   
support for the kthread API.  

In this series, a new UAPI is implemented to allow   
userspace applications to configure their thread mode. 

Changelog v2:
 1. Change the module_param's name to enforce_inherit_owner, and the default value is true.
 2. Change the UAPI's name to VHOST_SET_INHERIT_FROM_OWNER.
 
Changelog v3:
 1. Change the module_param's name to inherit_owner_default, and the default value is true.
 2. Add a structure for task function; the worker will select a different mode based on the value inherit_owner.
 3. device will have their own inherit_owner in struct vhost_dev
 4. Address other comments

Changelog v4: 
 1. remove the module_param, only keep the UAPI
 2. remove the structure for task function; change to use the function pointer in vhost_worker
 3. fix the issue in vhost_worker_create and vhost_dev_ioctl
 4. Address other comments

Changelog v5: 
 1. Change wakeup and stop function pointers in struct vhost_worker to void.
 2. merging patches 4, 5, 6 in a single patch
 3. Fix spelling issues and address other comments.
 
Tested with QEMU with kthread mode/task mode/kthread+task mode

Cindy Lu (6):
  vhost: Add a new parameter in vhost_dev to allow user select kthread
  vhost: Add the vhost_worker to support kthread
  vhost: Add the cgroup related function
  vhost: Add worker related functions to support kthread
  vhost: Add new UAPI to support change to task mode
  vhost_scsi: Add check for inherit_owner status

 drivers/vhost/scsi.c       |   8 ++
 drivers/vhost/vhost.c      | 178 +++++++++++++++++++++++++++++++++----
 drivers/vhost/vhost.h      |   4 +
 include/uapi/linux/vhost.h |  19 ++++
 4 files changed, 192 insertions(+), 17 deletions(-)

-- 
2.45.0

[PATCH v5 1/6] vhost: Add a new parameter in vhost_dev to allow user select kthread

[Cindy Lu]

The vhost now uses vhost_task and workers as a child of the owner
thread. While this aligns with containerization principles, it confuses
some legacy userspace applications. Therefore, we are reintroducing
support for the kthread API.

Introduce a new parameter to enable users to choose between kthread and
task mode.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9ac25d08f473..eaddbd39c29b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -552,6 +552,7 @@ void vhost_dev_init(struct vhost_dev *dev,
 	dev->byte_weight = byte_weight;
 	dev->use_worker = use_worker;
 	dev->msg_handler = msg_handler;
+	dev->inherit_owner = true;
 	init_waitqueue_head(&dev->wait);
 	INIT_LIST_HEAD(&dev->read_list);
 	INIT_LIST_HEAD(&dev->pending_list);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..c650c4506c70 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -176,6 +176,7 @@ struct vhost_dev {
 	int byte_weight;
 	struct xarray worker_xa;
 	bool use_worker;
+	bool inherit_owner;
 	int (*msg_handler)(struct vhost_dev *dev, u32 asid,
 			   struct vhost_iotlb_msg *msg);
 };
-- 
2.45.0

[PATCH v5 2/6] vhost: Add the vhost_worker to support kthread

[Cindy Lu]

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

The old function vhost_worker() was changed to support tasks in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
and to support multiple workers per device using xarray in
commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray").

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 eaddbd39c29b..1feba29abf95 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -388,6 +388,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

[PATCH v5 3/6] vhost: Add the cgroup related function

[Cindy Lu]

Reintroduce the previously removed functions vhost_attach_cgroups_work()
and vhost_attach_cgroups() to support kthread mode. Rename
vhost_attach_cgroups() to vhost_attach_task_to_cgroups(), and include
the implementation of the old function vhost_dev_flush() in this
new function.

These function was removed in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1feba29abf95..812dfd218bc2 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>
@@ -620,6 +621,38 @@ 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_task_to_cgroups(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;
+}
+
 /* Caller should have device mutex */
 bool vhost_dev_has_owner(struct vhost_dev *dev)
 {
-- 
2.45.0

[PATCH v5 4/6] vhost: Add worker related functions to support kthread

[Cindy Lu]

Restore the previously removed functions kthread_wakeup and
kthread_stop, and add two new function pointers to wake up and stop
the workers. The function vhost_worker_create will initialize these
pointers based on the value of inherit_owner.

The functions vhost_worker_queue() and vhost_worker_destroy() will
use the function pointer in vhost_worker, which is initialized
according to the inherit_owner value.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 812dfd218bc2..ff17c42e2d1a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -243,7 +243,7 @@ static void vhost_worker_queue(struct vhost_worker *worker,
 		 * test_and_set_bit() implies a memory barrier.
 		 */
 		llist_add(&work->node, &worker->work_list);
-		vhost_task_wake(worker->vtsk);
+		worker->worker_wakeup(worker);
 	}
 }
 
@@ -698,7 +698,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
 
 	WARN_ON(!llist_empty(&worker->work_list));
 	xa_erase(&dev->worker_xa, worker->id);
-	vhost_task_stop(worker->vtsk);
+	worker->worker_stop(worker);
 	kfree(worker);
 }
 
@@ -721,14 +721,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	xa_destroy(&dev->worker_xa);
 }
 
+static void vhost_task_wakeup_fn(struct vhost_worker *worker)
+{
+	return vhost_task_wake(worker->vtsk);
+}
+
+static void vhost_kthread_wakeup_fn(struct vhost_worker *worker)
+{
+	wake_up_process(worker->kthread_task);
+}
+
+static void vhost_task_stop_fn(struct vhost_worker *worker)
+{
+	return vhost_task_stop(worker->vtsk);
+}
+
+static void vhost_kthread_stop_fn(struct vhost_worker *worker)
+{
+	kthread_stop(worker->kthread_task);
+}
+
 static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
-	struct vhost_task *vtsk;
+	struct vhost_task *vtsk = NULL;
+	struct task_struct *task = NULL;
 	char name[TASK_COMM_LEN];
 	int ret;
 	u32 id;
 
+	/* Allocate resources for the worker */
 	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
 	if (!worker)
 		return NULL;
@@ -736,27 +758,57 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 	worker->dev = dev;
 	snprintf(name, sizeof(name), "vhost-%d", current->pid);
 
-	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
-				 worker, name);
-	if (!vtsk)
-		goto free_worker;
-
 	mutex_init(&worker->mutex);
 	init_llist_head(&worker->work_list);
 	worker->kcov_handle = kcov_common_handle();
-	worker->vtsk = vtsk;
+    /*
+     * If inherit_owner is true we use vhost_tasks to create
+     * the worker so all settings/limits like cgroups, NPROC,
+     * scheduler, etc are inherited from the owner.
+     * If false,we use kthreads and only attach to the same
+     * cgroups as the owner for compat with older kernels.
+     */
+	if (dev->inherit_owner) {
+		vtsk = vhost_task_create(vhost_run_work_list,
+					 vhost_worker_killed, worker, name);
+		if (!vtsk)
+			goto free_worker;
+
+		worker->vtsk = vtsk;
+		worker->worker_wakeup = vhost_task_wakeup_fn;
+		worker->worker_stop = vhost_task_stop_fn;
+
+		vhost_task_start(vtsk);
+		ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
+			       GFP_KERNEL);
+		if (ret < 0)
+			goto stop_worker;
+	} else {
+		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->kthread_task = task;
+		worker->worker_wakeup = vhost_kthread_wakeup_fn;
+		worker->worker_stop = vhost_kthread_stop_fn;
 
-	vhost_task_start(vtsk);
+		wake_up_process(task);
+		ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
+			       GFP_KERNEL);
+		if (ret < 0)
+			goto stop_worker;
 
-	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_task_to_cgroups(worker);
+		if (ret)
+			goto stop_worker;
+	}
 
+	worker->id = id;
 	return worker;
-
 stop_worker:
-	vhost_task_stop(vtsk);
+	worker->worker_stop(worker);
 free_worker:
 	kfree(worker);
 	return NULL;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index c650c4506c70..63b1da08a2b0 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -27,6 +27,7 @@ struct vhost_work {
 };
 
 struct vhost_worker {
+	struct task_struct *kthread_task;
 	struct vhost_task	*vtsk;
 	struct vhost_dev	*dev;
 	/* Used to serialize device wide flushing with worker swapping. */
@@ -36,6 +37,8 @@ struct vhost_worker {
 	u32			id;
 	int			attachment_cnt;
 	bool			killed;
+	void (*worker_wakeup)(struct vhost_worker *worker);
+	void (*worker_stop)(struct vhost_worker *worker);
 };
 
 /* Poll a file (eventfd or socket) */
-- 
2.45.0

[PATCH v5 5/6] vhost: Add new UAPI to support change to task mode

[Cindy Lu]

Add a new UAPI to enable setting the vhost device to task mode.
The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
to configure the mode if necessary.
This setting must be applied before VHOST_SET_OWNER, as the worker
will be created in the VHOST_SET_OWNER function

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ff17c42e2d1a..47c1329360ac 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2250,15 +2250,35 @@ 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;
+	u8 inherit_owner;
 
 	/* 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_INHERIT_FROM_OWNER) {
+		/*inherit_owner can only be modified before owner is set*/
+		if (vhost_dev_has_owner(d)) {
+			r = -EBUSY;
+			goto done;
+		}
+		if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
+			r = -EFAULT;
+			goto done;
+		}
+		/* Validate the inherit_owner value, ensuring it is either 0 or 1 */
+		if (inherit_owner > 1) {
+			r = -EINVAL;
+			goto done;
+		}
+
+		d->inherit_owner = (bool)inherit_owner;
 
+		goto done;
+	}
 	/* You must be the owner to do anything else */
 	r = vhost_dev_check_owner(d);
 	if (r)
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index b95dd84eef2d..f5fcf0b25736 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,23 @@
  */
 #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
 					      struct vhost_vring_state)
+
+/**
+ * VHOST_SET_INHERIT_FROM_OWNER - Set the inherit_owner flag for the vhost device
+ *
+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
+ *
+ * When inherit_owner is set to 1 (default behavior):
+ *   - The VHOST worker threads inherit their values/checks from
+ *     the thread that owns the VHOST device. The vhost threads will
+ *     be counted in the nproc rlimits.
+ *
+ * When inherit_owner is set to 0:
+ *   - The VHOST worker threads will use the traditional kernel thread (kthread)
+ *     implementation, which may be preferred by older userspace applications that
+ *     do not utilize the newer vhost_task concept.
+ */
+
+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
+
 #endif
-- 
2.45.0

[PATCH v5 6/6] vhost_scsi: Add check for inherit_owner status

[Cindy Lu]

The vhost_scsi VHOST_NEW_WORKER requires the inherit_owner
setting to be true. So we need to implement a check for this.

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

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 718fa4e0b31e..0d63b6b5c852 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -2086,6 +2086,14 @@ vhost_scsi_ioctl(struct file *f,
 			return -EFAULT;
 		return vhost_scsi_set_features(vs, features);
 	case VHOST_NEW_WORKER:
+		/*
+		 * vhost_tasks will account for worker threads under the parent's
+		 * NPROC value but kthreads do not. To avoid userspace overflowing
+		 * the system with worker threads inherit_owner must be true.
+		 */
+		if (!vs->dev.inherit_owner)
+			return -EFAULT;
+		fallthrough;
 	case VHOST_FREE_WORKER:
 	case VHOST_ATTACH_VRING_WORKER:
 	case VHOST_GET_VRING_WORKER:
-- 
2.45.0

Re: [PATCH v5 1/6] vhost: Add a new parameter in vhost_dev to allow user select kthread

[Jason Wang]

On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
>
> The vhost now uses vhost_task and workers as a child of the owner
> thread. While this aligns with containerization principles, it confuses
> some legacy userspace applications. Therefore, we are reintroducing
> support for the kthread API.
>
> Introduce a new parameter to enable users to choose between kthread and
> task mode.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH v5 2/6] vhost: Add the vhost_worker to support kthread

[Jason Wang]

On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add the previously removed function vhost_worker() back to support the
> kthread and rename it to vhost_run_work_kthread_list.
>
> The old function vhost_worker() was changed to support tasks in
> commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> and to support multiple workers per device using xarray in
> commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray").
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

I think we need to tweak the title as this patch just brings back the
kthread worker?

Other than that,

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  drivers/vhost/vhost.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index eaddbd39c29b..1feba29abf95 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -388,6 +388,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
>

Re: [PATCH v5 3/6] vhost: Add the cgroup related function

[Jason Wang]

On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Reintroduce the previously removed functions vhost_attach_cgroups_work()
> and vhost_attach_cgroups() to support kthread mode. Rename
> vhost_attach_cgroups() to vhost_attach_task_to_cgroups(), and include
> the implementation of the old function vhost_dev_flush() in this
> new function.
>
> These function was removed in
> commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 1feba29abf95..812dfd218bc2 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>
> @@ -620,6 +621,38 @@ 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_task_to_cgroups(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;
> +}

This seems to be inconsistent with what you said above which is

"""
> These function was removed in
> commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
"""

As 6e890c5d5021 had:

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_dev_flush(dev);
        return attach.ret;
}

It seems current vhost_dev_flush() will still work or the open coding
of the flush logic needs to be explained.

Thanks

> +
>  /* Caller should have device mutex */
>  bool vhost_dev_has_owner(struct vhost_dev *dev)
>  {
> --
> 2.45.0
>

Re: [PATCH v5 4/6] vhost: Add worker related functions to support kthread

[Jason Wang]

On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Restore the previously removed functions kthread_wakeup and
> kthread_stop, and add two new function pointers to wake up and stop
> the workers. The function vhost_worker_create will initialize these
> pointers based on the value of inherit_owner.
>
> The functions vhost_worker_queue() and vhost_worker_destroy() will
> use the function pointer in vhost_worker, which is initialized
> according to the inherit_owner value.

I'd suggest using "vhost: introduce worker ops to support multiple
thread models" as the title.

>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c | 84 ++++++++++++++++++++++++++++++++++---------
>  drivers/vhost/vhost.h |  3 ++
>  2 files changed, 71 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 812dfd218bc2..ff17c42e2d1a 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -243,7 +243,7 @@ static void vhost_worker_queue(struct vhost_worker *worker,
>                  * test_and_set_bit() implies a memory barrier.
>                  */
>                 llist_add(&work->node, &worker->work_list);
> -               vhost_task_wake(worker->vtsk);
> +               worker->worker_wakeup(worker);
>         }
>  }
>
> @@ -698,7 +698,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
>
>         WARN_ON(!llist_empty(&worker->work_list));
>         xa_erase(&dev->worker_xa, worker->id);
> -       vhost_task_stop(worker->vtsk);
> +       worker->worker_stop(worker);
>         kfree(worker);
>  }
>
> @@ -721,14 +721,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
>         xa_destroy(&dev->worker_xa);
>  }
>
> +static void vhost_task_wakeup_fn(struct vhost_worker *worker)
> +{
> +       return vhost_task_wake(worker->vtsk);
> +}
> +
> +static void vhost_kthread_wakeup_fn(struct vhost_worker *worker)
> +{
> +       wake_up_process(worker->kthread_task);
> +}
> +
> +static void vhost_task_stop_fn(struct vhost_worker *worker)
> +{
> +       return vhost_task_stop(worker->vtsk);
> +}
> +
> +static void vhost_kthread_stop_fn(struct vhost_worker *worker)
> +{
> +       kthread_stop(worker->kthread_task);
> +}
> +
>  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
>  {
>         struct vhost_worker *worker;
> -       struct vhost_task *vtsk;
> +       struct vhost_task *vtsk = NULL;
> +       struct task_struct *task = NULL;
>         char name[TASK_COMM_LEN];
>         int ret;
>         u32 id;
>
> +       /* Allocate resources for the worker */
>         worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
>         if (!worker)
>                 return NULL;
> @@ -736,27 +758,57 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
>         worker->dev = dev;
>         snprintf(name, sizeof(name), "vhost-%d", current->pid);
>
> -       vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> -                                worker, name);
> -       if (!vtsk)
> -               goto free_worker;
> -
>         mutex_init(&worker->mutex);
>         init_llist_head(&worker->work_list);
>         worker->kcov_handle = kcov_common_handle();
> -       worker->vtsk = vtsk;
> +    /*
> +     * If inherit_owner is true we use vhost_tasks to create
> +     * the worker so all settings/limits like cgroups, NPROC,
> +     * scheduler, etc are inherited from the owner.
> +     * If false,we use kthreads and only attach to the same
> +     * cgroups as the owner for compat with older kernels.
> +     */
> +       if (dev->inherit_owner) {
> +               vtsk = vhost_task_create(vhost_run_work_list,
> +                                        vhost_worker_killed, worker, name);
> +               if (!vtsk)
> +                       goto free_worker;
> +
> +               worker->vtsk = vtsk;
> +               worker->worker_wakeup = vhost_task_wakeup_fn;
> +               worker->worker_stop = vhost_task_stop_fn;
> +
> +               vhost_task_start(vtsk);
> +               ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
> +                              GFP_KERNEL);
> +               if (ret < 0)
> +                       goto stop_worker;

Let's simply have a new ops like worker_create to avoid the if/else here.

> +       } else {
> +               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->kthread_task = task;
> +               worker->worker_wakeup = vhost_kthread_wakeup_fn;
> +               worker->worker_stop = vhost_kthread_stop_fn;
>
> -       vhost_task_start(vtsk);
> +               wake_up_process(task);
> +               ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
> +                              GFP_KERNEL);
> +               if (ret < 0)
> +                       goto stop_worker;
>
> -       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_task_to_cgroups(worker);
> +               if (ret)
> +                       goto stop_worker;
> +       }
>
> +       worker->id = id;
>         return worker;
> -
>  stop_worker:
> -       vhost_task_stop(vtsk);
> +       worker->worker_stop(worker);
>  free_worker:
>         kfree(worker);
>         return NULL;
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index c650c4506c70..63b1da08a2b0 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -27,6 +27,7 @@ struct vhost_work {
>  };
>
>  struct vhost_worker {
> +       struct task_struct *kthread_task;
>         struct vhost_task       *vtsk;
>         struct vhost_dev        *dev;
>         /* Used to serialize device wide flushing with worker swapping. */
> @@ -36,6 +37,8 @@ struct vhost_worker {
>         u32                     id;
>         int                     attachment_cnt;
>         bool                    killed;
> +       void (*worker_wakeup)(struct vhost_worker *worker);
> +       void (*worker_stop)(struct vhost_worker *worker);

Let's use a dedicated ops structure for this.

Thanks

>  };
>
>  /* Poll a file (eventfd or socket) */
> --
> 2.45.0
>

Re: [PATCH v5 5/6] vhost: Add new UAPI to support change to task mode

[Jason Wang]

On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add a new UAPI to enable setting the vhost device to task mode.
> The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
> to configure the mode if necessary.
> This setting must be applied before VHOST_SET_OWNER, as the worker
> will be created in the VHOST_SET_OWNER function
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c      | 22 +++++++++++++++++++++-
>  include/uapi/linux/vhost.h | 19 +++++++++++++++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index ff17c42e2d1a..47c1329360ac 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2250,15 +2250,35 @@ 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;
> +       u8 inherit_owner;
>
>         /* 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_INHERIT_FROM_OWNER) {
> +               /*inherit_owner can only be modified before owner is set*/
> +               if (vhost_dev_has_owner(d)) {
> +                       r = -EBUSY;
> +                       goto done;
> +               }
> +               if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
> +                       r = -EFAULT;
> +                       goto done;
> +               }

Not a native speaker but I wonder if "VHOST_FORK_FROM_OWNER" is better or not.

> +               /* Validate the inherit_owner value, ensuring it is either 0 or 1 */
> +               if (inherit_owner > 1) {
> +                       r = -EINVAL;
> +                       goto done;
> +               }
> +
> +               d->inherit_owner = (bool)inherit_owner;

So this allows userspace to reset the owner and toggle the value. This
seems to be fine, but I wonder if we need to some cleanup in
vhost_dev_reset_owner() or not. Let's explain this somewhere (probably
in the commit log).

>
> +               goto done;
> +       }
>         /* You must be the owner to do anything else */
>         r = vhost_dev_check_owner(d);
>         if (r)
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index b95dd84eef2d..f5fcf0b25736 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,23 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE      _IOWR(VHOST_VIRTIO, 0x82,       \
>                                               struct vhost_vring_state)
> +
> +/**
> + * VHOST_SET_INHERIT_FROM_OWNER - Set the inherit_owner flag for the vhost device
> + *
> + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> + *
> + * When inherit_owner is set to 1 (default behavior):
> + *   - The VHOST worker threads inherit their values/checks from
> + *     the thread that owns the VHOST device. The vhost threads will
> + *     be counted in the nproc rlimits.
> + *
> + * When inherit_owner is set to 0:
> + *   - The VHOST worker threads will use the traditional kernel thread (kthread)
> + *     implementation, which may be preferred by older userspace applications that
> + *     do not utilize the newer vhost_task concept.
> + */
> +
> +#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> +
>  #endif
> --
> 2.45.0

Thanks

Re: [PATCH v5 0/6] vhost: Add support of kthread API

[Lei Yang]

I tested this series v5 with virtio-net regression tests, everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>


On Mon, Dec 30, 2024 at 8:46 PM Cindy Lu <lulu@redhat.com> wrote:
>
> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> the vhost now uses vhost_task and operates as a child of the
> owner thread. This aligns with containerization principles.
> However, this change has caused confusion for some legacy
> userspace applications. Therefore, we are reintroducing
> support for the kthread API.
>
> In this series, a new UAPI is implemented to allow
> userspace applications to configure their thread mode.
>
> Changelog v2:
>  1. Change the module_param's name to enforce_inherit_owner, and the default value is true.
>  2. Change the UAPI's name to VHOST_SET_INHERIT_FROM_OWNER.
>
> Changelog v3:
>  1. Change the module_param's name to inherit_owner_default, and the default value is true.
>  2. Add a structure for task function; the worker will select a different mode based on the value inherit_owner.
>  3. device will have their own inherit_owner in struct vhost_dev
>  4. Address other comments
>
> Changelog v4:
>  1. remove the module_param, only keep the UAPI
>  2. remove the structure for task function; change to use the function pointer in vhost_worker
>  3. fix the issue in vhost_worker_create and vhost_dev_ioctl
>  4. Address other comments
>
> Changelog v5:
>  1. Change wakeup and stop function pointers in struct vhost_worker to void.
>  2. merging patches 4, 5, 6 in a single patch
>  3. Fix spelling issues and address other comments.
>
> Tested with QEMU with kthread mode/task mode/kthread+task mode
>
> Cindy Lu (6):
>   vhost: Add a new parameter in vhost_dev to allow user select kthread
>   vhost: Add the vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: Add worker related functions to support kthread
>   vhost: Add new UAPI to support change to task mode
>   vhost_scsi: Add check for inherit_owner status
>
>  drivers/vhost/scsi.c       |   8 ++
>  drivers/vhost/vhost.c      | 178 +++++++++++++++++++++++++++++++++----
>  drivers/vhost/vhost.h      |   4 +
>  include/uapi/linux/vhost.h |  19 ++++
>  4 files changed, 192 insertions(+), 17 deletions(-)
>
> --
> 2.45.0
>
>

Re: [PATCH v5 5/6] vhost: Add new UAPI to support change to task mode

[Cindy Lu]

On Thu, Jan 2, 2025 at 11:37 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Add a new UAPI to enable setting the vhost device to task mode.
> > The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
> > to configure the mode if necessary.
> > This setting must be applied before VHOST_SET_OWNER, as the worker
> > will be created in the VHOST_SET_OWNER function
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      | 22 +++++++++++++++++++++-
> >  include/uapi/linux/vhost.h | 19 +++++++++++++++++++
> >  2 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index ff17c42e2d1a..47c1329360ac 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2250,15 +2250,35 @@ 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;
> > +       u8 inherit_owner;
> >
> >         /* 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_INHERIT_FROM_OWNER) {
> > +               /*inherit_owner can only be modified before owner is set*/
> > +               if (vhost_dev_has_owner(d)) {
> > +                       r = -EBUSY;
> > +                       goto done;
> > +               }
> > +               if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
> > +                       r = -EFAULT;
> > +                       goto done;
> > +               }
>
> Not a native speaker but I wonder if "VHOST_FORK_FROM_OWNER" is better or not.
>
Sure will do
> > +               /* Validate the inherit_owner value, ensuring it is either 0 or 1 */
> > +               if (inherit_owner > 1) {
> > +                       r = -EINVAL;
> > +                       goto done;
> > +               }
> > +
> > +               d->inherit_owner = (bool)inherit_owner;
>
> So this allows userspace to reset the owner and toggle the value. This
> seems to be fine, but I wonder if we need to some cleanup in
> vhost_dev_reset_owner() or not. Let's explain this somewhere (probably
> in the commit log).
>
sure will add these information
Thanks
Cindy
> >
> > +               goto done;
> > +       }
> >         /* You must be the owner to do anything else */
> >         r = vhost_dev_check_owner(d);
> >         if (r)
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index b95dd84eef2d..f5fcf0b25736 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -235,4 +235,23 @@
> >   */
> >  #define VHOST_VDPA_GET_VRING_SIZE      _IOWR(VHOST_VIRTIO, 0x82,       \
> >                                               struct vhost_vring_state)
> > +
> > +/**
> > + * VHOST_SET_INHERIT_FROM_OWNER - Set the inherit_owner flag for the vhost device
> > + *
> > + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> > + *
> > + * When inherit_owner is set to 1 (default behavior):
> > + *   - The VHOST worker threads inherit their values/checks from
> > + *     the thread that owns the VHOST device. The vhost threads will
> > + *     be counted in the nproc rlimits.
> > + *
> > + * When inherit_owner is set to 0:
> > + *   - The VHOST worker threads will use the traditional kernel thread (kthread)
> > + *     implementation, which may be preferred by older userspace applications that
> > + *     do not utilize the newer vhost_task concept.
> > + */
> > +
> > +#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> > +
> >  #endif
> > --
> > 2.45.0
>
> Thanks
>

Re: [PATCH v5 4/6] vhost: Add worker related functions to support kthread

[Cindy Lu]

On Thu, Jan 2, 2025 at 11:33 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Restore the previously removed functions kthread_wakeup and
> > kthread_stop, and add two new function pointers to wake up and stop
> > the workers. The function vhost_worker_create will initialize these
> > pointers based on the value of inherit_owner.
> >
> > The functions vhost_worker_queue() and vhost_worker_destroy() will
> > use the function pointer in vhost_worker, which is initialized
> > according to the inherit_owner value.
>
> I'd suggest using "vhost: introduce worker ops to support multiple
> thread models" as the title.
>
sure will change this
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c | 84 ++++++++++++++++++++++++++++++++++---------
> >  drivers/vhost/vhost.h |  3 ++
> >  2 files changed, 71 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 812dfd218bc2..ff17c42e2d1a 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -243,7 +243,7 @@ static void vhost_worker_queue(struct vhost_worker *worker,
> >                  * test_and_set_bit() implies a memory barrier.
> >                  */
> >                 llist_add(&work->node, &worker->work_list);
> > -               vhost_task_wake(worker->vtsk);
> > +               worker->worker_wakeup(worker);
> >         }
> >  }
> >
> > @@ -698,7 +698,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
> >
> >         WARN_ON(!llist_empty(&worker->work_list));
> >         xa_erase(&dev->worker_xa, worker->id);
> > -       vhost_task_stop(worker->vtsk);
> > +       worker->worker_stop(worker);
> >         kfree(worker);
> >  }
> >
> > @@ -721,14 +721,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
> >         xa_destroy(&dev->worker_xa);
> >  }
> >
> > +static void vhost_task_wakeup_fn(struct vhost_worker *worker)
> > +{
> > +       return vhost_task_wake(worker->vtsk);
> > +}
> > +
> > +static void vhost_kthread_wakeup_fn(struct vhost_worker *worker)
> > +{
> > +       wake_up_process(worker->kthread_task);
> > +}
> > +
> > +static void vhost_task_stop_fn(struct vhost_worker *worker)
> > +{
> > +       return vhost_task_stop(worker->vtsk);
> > +}
> > +
> > +static void vhost_kthread_stop_fn(struct vhost_worker *worker)
> > +{
> > +       kthread_stop(worker->kthread_task);
> > +}
> > +
> >  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> >  {
> >         struct vhost_worker *worker;
> > -       struct vhost_task *vtsk;
> > +       struct vhost_task *vtsk = NULL;
> > +       struct task_struct *task = NULL;
> >         char name[TASK_COMM_LEN];
> >         int ret;
> >         u32 id;
> >
> > +       /* Allocate resources for the worker */
> >         worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> >         if (!worker)
> >                 return NULL;
> > @@ -736,27 +758,57 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> >         worker->dev = dev;
> >         snprintf(name, sizeof(name), "vhost-%d", current->pid);
> >
> > -       vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> > -                                worker, name);
> > -       if (!vtsk)
> > -               goto free_worker;
> > -
> >         mutex_init(&worker->mutex);
> >         init_llist_head(&worker->work_list);
> >         worker->kcov_handle = kcov_common_handle();
> > -       worker->vtsk = vtsk;
> > +    /*
> > +     * If inherit_owner is true we use vhost_tasks to create
> > +     * the worker so all settings/limits like cgroups, NPROC,
> > +     * scheduler, etc are inherited from the owner.
> > +     * If false,we use kthreads and only attach to the same
> > +     * cgroups as the owner for compat with older kernels.
> > +     */
> > +       if (dev->inherit_owner) {
> > +               vtsk = vhost_task_create(vhost_run_work_list,
> > +                                        vhost_worker_killed, worker, name);
> > +               if (!vtsk)
> > +                       goto free_worker;
> > +
> > +               worker->vtsk = vtsk;
> > +               worker->worker_wakeup = vhost_task_wakeup_fn;
> > +               worker->worker_stop = vhost_task_stop_fn;
> > +
> > +               vhost_task_start(vtsk);
> > +               ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
> > +                              GFP_KERNEL);
> > +               if (ret < 0)
> > +                       goto stop_worker;
>
> Let's simply have a new ops like worker_create to avoid the if/else here.
>
will change this
> > +       } else {
> > +               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->kthread_task = task;
> > +               worker->worker_wakeup = vhost_kthread_wakeup_fn;
> > +               worker->worker_stop = vhost_kthread_stop_fn;
> >
> > -       vhost_task_start(vtsk);
> > +               wake_up_process(task);
> > +               ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
> > +                              GFP_KERNEL);
> > +               if (ret < 0)
> > +                       goto stop_worker;
> >
> > -       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_task_to_cgroups(worker);
> > +               if (ret)
> > +                       goto stop_worker;
> > +       }
> >
> > +       worker->id = id;
> >         return worker;
> > -
> >  stop_worker:
> > -       vhost_task_stop(vtsk);
> > +       worker->worker_stop(worker);
> >  free_worker:
> >         kfree(worker);
> >         return NULL;
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index c650c4506c70..63b1da08a2b0 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -27,6 +27,7 @@ struct vhost_work {
> >  };
> >
> >  struct vhost_worker {
> > +       struct task_struct *kthread_task;
> >         struct vhost_task       *vtsk;
> >         struct vhost_dev        *dev;
> >         /* Used to serialize device wide flushing with worker swapping. */
> > @@ -36,6 +37,8 @@ struct vhost_worker {
> >         u32                     id;
> >         int                     attachment_cnt;
> >         bool                    killed;
> > +       void (*worker_wakeup)(struct vhost_worker *worker);
> > +       void (*worker_stop)(struct vhost_worker *worker);
>
> Let's use a dedicated ops structure for this.
>
> Thanks
>
sure will do
Thanks
cindy
> >  };
> >
> >  /* Poll a file (eventfd or socket) */
> > --
> > 2.45.0
> >
>

Re: [PATCH v5 3/6] vhost: Add the cgroup related function

[Cindy Lu]

On Thu, Jan 2, 2025 at 11:29 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Reintroduce the previously removed functions vhost_attach_cgroups_work()
> > and vhost_attach_cgroups() to support kthread mode. Rename
> > vhost_attach_cgroups() to vhost_attach_task_to_cgroups(), and include
> > the implementation of the old function vhost_dev_flush() in this
> > new function.
> >
> > These function was removed in
> > commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 1feba29abf95..812dfd218bc2 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>
> > @@ -620,6 +621,38 @@ 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_task_to_cgroups(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;
> > +}
>
> This seems to be inconsistent with what you said above which is
>
> """
> > These function was removed in
> > commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> """
>
> As 6e890c5d5021 had:
>
> 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_dev_flush(dev);
>         return attach.ret;
> }
>
> It seems current vhost_dev_flush() will still work or the open coding
> of the flush logic needs to be explained.
>
> Thanks
>
the current vhost_dev_flush was changed to support xarray, So it does
not work here , I will add more information here
Thanks
cindy
> > +
> >  /* Caller should have device mutex */
> >  bool vhost_dev_has_owner(struct vhost_dev *dev)
> >  {
> > --
> > 2.45.0
> >
>

Re: [PATCH v5 2/6] vhost: Add the vhost_worker to support kthread

[Cindy Lu]

On Thu, Jan 2, 2025 at 11:19 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Add the previously removed function vhost_worker() back to support the
> > kthread and rename it to vhost_run_work_kthread_list.
> >
> > The old function vhost_worker() was changed to support tasks in
> > commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> > and to support multiple workers per device using xarray in
> > commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray").
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
>
> I think we need to tweak the title as this patch just brings back the
> kthread worker?
>
> Other than that,
>
sure will do
Thanks
cindy
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Thanks
>
> > ---
> >  drivers/vhost/vhost.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index eaddbd39c29b..1feba29abf95 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -388,6 +388,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
> >
>

Re: [PATCH v5 3/6] vhost: Add the cgroup related function

[Jason Wang]

On Wed, Jan 8, 2025 at 10:57 AM Cindy Lu <lulu@redhat.com> wrote:
>
> On Thu, Jan 2, 2025 at 11:29 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > Reintroduce the previously removed functions vhost_attach_cgroups_work()
> > > and vhost_attach_cgroups() to support kthread mode. Rename
> > > vhost_attach_cgroups() to vhost_attach_task_to_cgroups(), and include
> > > the implementation of the old function vhost_dev_flush() in this
> > > new function.
> > >
> > > These function was removed in
> > > commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  drivers/vhost/vhost.c | 33 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 1feba29abf95..812dfd218bc2 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>
> > > @@ -620,6 +621,38 @@ 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_task_to_cgroups(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;
> > > +}
> >
> > This seems to be inconsistent with what you said above which is
> >
> > """
> > > These function was removed in
> > > commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> > """
> >
> > As 6e890c5d5021 had:
> >
> > 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_dev_flush(dev);
> >         return attach.ret;
> > }
> >
> > It seems current vhost_dev_flush() will still work or the open coding
> > of the flush logic needs to be explained.
> >
> > Thanks
> >
> the current vhost_dev_flush was changed to support xarray, So it does
> not work here , I will add more information here

Any reason it can't work here?

Thanks

> Thanks
> cindy
> > > +
> > >  /* Caller should have device mutex */
> > >  bool vhost_dev_has_owner(struct vhost_dev *dev)
> > >  {
> > > --
> > > 2.45.0
> > >
> >
>

Re: [PATCH v5 5/6] vhost: Add new UAPI to support change to task mode

[Michael S. Tsirkin]

On Mon, Dec 30, 2024 at 08:43:52PM +0800, Cindy Lu wrote:
> Add a new UAPI to enable setting the vhost device to task mode.
> The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
> to configure the mode if necessary.
> This setting must be applied before VHOST_SET_OWNER, as the worker
> will be created in the VHOST_SET_OWNER function
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>

I'd like a Kconfig option to control enabling/blocking this
ioctl. Make it a patch on top pls.


> ---
>  drivers/vhost/vhost.c      | 22 +++++++++++++++++++++-
>  include/uapi/linux/vhost.h | 19 +++++++++++++++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index ff17c42e2d1a..47c1329360ac 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2250,15 +2250,35 @@ 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;
> +	u8 inherit_owner;
>  
>  	/* 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_INHERIT_FROM_OWNER) {
> +		/*inherit_owner can only be modified before owner is set*/
> +		if (vhost_dev_has_owner(d)) {
> +			r = -EBUSY;
> +			goto done;
> +		}
> +		if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
> +			r = -EFAULT;
> +			goto done;
> +		}
> +		/* Validate the inherit_owner value, ensuring it is either 0 or 1 */
> +		if (inherit_owner > 1) {
> +			r = -EINVAL;
> +			goto done;
> +		}
> +
> +		d->inherit_owner = (bool)inherit_owner;
>  
> +		goto done;
> +	}
>  	/* You must be the owner to do anything else */
>  	r = vhost_dev_check_owner(d);
>  	if (r)
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index b95dd84eef2d..f5fcf0b25736 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,23 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
>  					      struct vhost_vring_state)
> +
> +/**
> + * VHOST_SET_INHERIT_FROM_OWNER - Set the inherit_owner flag for the vhost device
> + *
> + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> + *
> + * When inherit_owner is set to 1 (default behavior):
> + *   - The VHOST worker threads inherit their values/checks from
> + *     the thread that owns the VHOST device. The vhost threads will
> + *     be counted in the nproc rlimits.
> + *
> + * When inherit_owner is set to 0:
> + *   - The VHOST worker threads will use the traditional kernel thread (kthread)
> + *     implementation, which may be preferred by older userspace applications that
> + *     do not utilize the newer vhost_task concept.
> + */
> +
> +#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> +
>  #endif
> -- 
> 2.45.0

Re: [PATCH v5 5/6] vhost: Add new UAPI to support change to task mode

[Michael S. Tsirkin]

On Thu, Jan 02, 2025 at 11:36:58AM +0800, Jason Wang wrote:
> On Mon, Dec 30, 2024 at 8:45 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Add a new UAPI to enable setting the vhost device to task mode.
> > The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
> > to configure the mode if necessary.
> > This setting must be applied before VHOST_SET_OWNER, as the worker
> > will be created in the VHOST_SET_OWNER function
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      | 22 +++++++++++++++++++++-
> >  include/uapi/linux/vhost.h | 19 +++++++++++++++++++
> >  2 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index ff17c42e2d1a..47c1329360ac 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2250,15 +2250,35 @@ 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;
> > +       u8 inherit_owner;
> >
> >         /* 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_INHERIT_FROM_OWNER) {
> > +               /*inherit_owner can only be modified before owner is set*/
> > +               if (vhost_dev_has_owner(d)) {
> > +                       r = -EBUSY;
> > +                       goto done;
> > +               }
> > +               if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
> > +                       r = -EFAULT;
> > +                       goto done;
> > +               }
> 
> Not a native speaker but I wonder if "VHOST_FORK_FROM_OWNER" is better or not.
> 
> > +               /* Validate the inherit_owner value, ensuring it is either 0 or 1 */
> > +               if (inherit_owner > 1) {
> > +                       r = -EINVAL;
> > +                       goto done;
> > +               }
> > +
> > +               d->inherit_owner = (bool)inherit_owner;
> 
> So this allows userspace to reset the owner and toggle the value. This
> seems to be fine, but I wonder if we need to some cleanup in
> vhost_dev_reset_owner() or not. Let's explain this somewhere (probably
> in the commit log).
> 
> >
> > +               goto done;
> > +       }
> >         /* You must be the owner to do anything else */
> >         r = vhost_dev_check_owner(d);
> >         if (r)
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index b95dd84eef2d..f5fcf0b25736 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -235,4 +235,23 @@
> >   */
> >  #define VHOST_VDPA_GET_VRING_SIZE      _IOWR(VHOST_VIRTIO, 0x82,       \
> >                                               struct vhost_vring_state)
> > +
> > +/**
> > + * VHOST_SET_INHERIT_FROM_OWNER - Set the inherit_owner flag for the vhost device
> > + *
> > + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> > + *
> > + * When inherit_owner is set to 1 (default behavior):
> > + *   - The VHOST worker threads inherit their values/checks from
> > + *     the thread that owns the VHOST device. The vhost threads will
> > + *     be counted in the nproc rlimits.
> > + *
> > + * When inherit_owner is set to 0:
> > + *   - The VHOST worker threads will use the traditional kernel thread (kthread)
> > + *     implementation, which may be preferred by older userspace applications that
> > + *     do not utilize the newer vhost_task concept.
> > + */
> > +
> > +#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> > +
> >  #endif
> > --
> > 2.45.0
> 
> Thanks


At this point, make these changes with patches on top pls.

Re: [PATCH v5 0/6] vhost: Add support of kthread API

[Michael S. Tsirkin]

On Mon, Dec 30, 2024 at 08:43:47PM +0800, Cindy Lu wrote:
> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),   
> the vhost now uses vhost_task and operates as a child of the   
> owner thread. This aligns with containerization principles.   
> However, this change has caused confusion for some legacy   
> userspace applications. Therefore, we are reintroducing   
> support for the kthread API.  


I briefly applied this, but there seem to be a bit too
many nits. So I will wait for v6 with everything addressed.

Thanks!

> In this series, a new UAPI is implemented to allow   
> userspace applications to configure their thread mode. 
> 
> Changelog v2:
>  1. Change the module_param's name to enforce_inherit_owner, and the default value is true.
>  2. Change the UAPI's name to VHOST_SET_INHERIT_FROM_OWNER.
>  
> Changelog v3:
>  1. Change the module_param's name to inherit_owner_default, and the default value is true.
>  2. Add a structure for task function; the worker will select a different mode based on the value inherit_owner.
>  3. device will have their own inherit_owner in struct vhost_dev
>  4. Address other comments
> 
> Changelog v4: 
>  1. remove the module_param, only keep the UAPI
>  2. remove the structure for task function; change to use the function pointer in vhost_worker
>  3. fix the issue in vhost_worker_create and vhost_dev_ioctl
>  4. Address other comments
> 
> Changelog v5: 
>  1. Change wakeup and stop function pointers in struct vhost_worker to void.
>  2. merging patches 4, 5, 6 in a single patch
>  3. Fix spelling issues and address other comments.
>  
> Tested with QEMU with kthread mode/task mode/kthread+task mode
> 
> Cindy Lu (6):
>   vhost: Add a new parameter in vhost_dev to allow user select kthread
>   vhost: Add the vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: Add worker related functions to support kthread
>   vhost: Add new UAPI to support change to task mode
>   vhost_scsi: Add check for inherit_owner status
> 
>  drivers/vhost/scsi.c       |   8 ++
>  drivers/vhost/vhost.c      | 178 +++++++++++++++++++++++++++++++++----
>  drivers/vhost/vhost.h      |   4 +
>  include/uapi/linux/vhost.h |  19 ++++
>  4 files changed, 192 insertions(+), 17 deletions(-)
> 
> -- 
> 2.45.0

Re: [PATCH v5 4/6] vhost: Add worker related functions to support kthread

[Stefano Garzarella]

On Mon, Dec 30, 2024 at 08:43:51PM +0800, Cindy Lu wrote:
>Restore the previously removed functions kthread_wakeup and
>kthread_stop, and add two new function pointers to wake up and stop
>the workers. The function vhost_worker_create will initialize these
>pointers based on the value of inherit_owner.
>
>The functions vhost_worker_queue() and vhost_worker_destroy() will
>use the function pointer in vhost_worker, which is initialized
>according to the inherit_owner value.
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 84 ++++++++++++++++++++++++++++++++++---------
> drivers/vhost/vhost.h |  3 ++
> 2 files changed, 71 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 812dfd218bc2..ff17c42e2d1a 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c

[...]

>@@ -736,27 +758,57 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> 	worker->dev = dev;
> 	snprintf(name, sizeof(name), "vhost-%d", current->pid);
>
>-	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
>-				 worker, name);
>-	if (!vtsk)
>-		goto free_worker;
>-
> 	mutex_init(&worker->mutex);
> 	init_llist_head(&worker->work_list);
> 	worker->kcov_handle = kcov_common_handle();
>-	worker->vtsk = vtsk;
>+    /*
>+     * If inherit_owner is true we use vhost_tasks to create
>+     * the worker so all settings/limits like cgroups, NPROC,
>+     * scheduler, etc are inherited from the owner.
>+     * If false,we use kthreads and only attach to the same
>+     * cgroups as the owner for compat with older kernels.
>+     */

This comment block seems to have incorrect indentation, perhaps you need 
to replace the spaces with at least one tab.

>+	if (dev->inherit_owner) {
>+		vtsk = vhost_task_create(vhost_run_work_list,
>+					 vhost_worker_killed, worker, name);
>+		if (!vtsk)
>+			goto free_worker;
>+
>+		worker->vtsk = vtsk;
>+		worker->worker_wakeup = vhost_task_wakeup_fn;
>+		worker->worker_stop = vhost_task_stop_fn;
>+
>+		vhost_task_start(vtsk);
>+		ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
>+			       GFP_KERNEL);
>+		if (ret < 0)
>+			goto stop_worker;
>+	} else {
>+		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->kthread_task = task;
>+		worker->worker_wakeup = vhost_kthread_wakeup_fn;
>+		worker->worker_stop = vhost_kthread_stop_fn;
>
>-	vhost_task_start(vtsk);
>+		wake_up_process(task);
>+		ret = xa_alloc(&dev->worker_xa, &id, worker, 
>xa_limit_32b,
>+			       GFP_KERNEL);
>+		if (ret < 0)
>+			goto stop_worker;
>
>-	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_task_to_cgroups(worker);
>+		if (ret)
>+			goto stop_worker;
>+	}
>
>+	worker->id = id;
> 	return worker;
>-
> stop_worker:
>-	vhost_task_stop(vtsk);
>+	worker->worker_stop(worker);
> free_worker:
> 	kfree(worker);
> 	return NULL;
>diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>index c650c4506c70..63b1da08a2b0 100644
>--- a/drivers/vhost/vhost.h
>+++ b/drivers/vhost/vhost.h
>@@ -27,6 +27,7 @@ struct vhost_work {
> };
>
> struct vhost_worker {
>+	struct task_struct *kthread_task;
> 	struct vhost_task	*vtsk;
> 	struct vhost_dev	*dev;
> 	/* Used to serialize device wide flushing with worker swapping. */
>@@ -36,6 +37,8 @@ struct vhost_worker {
> 	u32			id;
> 	int			attachment_cnt;
> 	bool			killed;
>+	void (*worker_wakeup)(struct vhost_worker *worker);
>+	void (*worker_stop)(struct vhost_worker *worker);
> };
>
> /* Poll a file (eventfd or socket) */
>-- 
>2.45.0
>

Re: [PATCH v5 4/6] vhost: Add worker related functions to support kthread

[Cindy Lu]

On Wed, Jan 8, 2025 at 10:35 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, Dec 30, 2024 at 08:43:51PM +0800, Cindy Lu wrote:
> >Restore the previously removed functions kthread_wakeup and
> >kthread_stop, and add two new function pointers to wake up and stop
> >the workers. The function vhost_worker_create will initialize these
> >pointers based on the value of inherit_owner.
> >
> >The functions vhost_worker_queue() and vhost_worker_destroy() will
> >use the function pointer in vhost_worker, which is initialized
> >according to the inherit_owner value.
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 84 ++++++++++++++++++++++++++++++++++---------
> > drivers/vhost/vhost.h |  3 ++
> > 2 files changed, 71 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index 812dfd218bc2..ff17c42e2d1a 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
>
> [...]
>
> >@@ -736,27 +758,57 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> >       worker->dev = dev;
> >       snprintf(name, sizeof(name), "vhost-%d", current->pid);
> >
> >-      vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> >-                               worker, name);
> >-      if (!vtsk)
> >-              goto free_worker;
> >-
> >       mutex_init(&worker->mutex);
> >       init_llist_head(&worker->work_list);
> >       worker->kcov_handle = kcov_common_handle();
> >-      worker->vtsk = vtsk;
> >+    /*
> >+     * If inherit_owner is true we use vhost_tasks to create
> >+     * the worker so all settings/limits like cgroups, NPROC,
> >+     * scheduler, etc are inherited from the owner.
> >+     * If false,we use kthreads and only attach to the same
> >+     * cgroups as the owner for compat with older kernels.
> >+     */
>
> This comment block seems to have incorrect indentation, perhaps you need
> to replace the spaces with at least one tab.
>
sure will do
Thanks
cindy
> >+      if (dev->inherit_owner) {
> >+              vtsk = vhost_task_create(vhost_run_work_list,
> >+                                       vhost_worker_killed, worker, name);
> >+              if (!vtsk)
> >+                      goto free_worker;
> >+
> >+              worker->vtsk = vtsk;
> >+              worker->worker_wakeup = vhost_task_wakeup_fn;
> >+              worker->worker_stop = vhost_task_stop_fn;
> >+
> >+              vhost_task_start(vtsk);
> >+              ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
> >+                             GFP_KERNEL);
> >+              if (ret < 0)
> >+                      goto stop_worker;
> >+      } else {
> >+              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->kthread_task = task;
> >+              worker->worker_wakeup = vhost_kthread_wakeup_fn;
> >+              worker->worker_stop = vhost_kthread_stop_fn;
> >
> >-      vhost_task_start(vtsk);
> >+              wake_up_process(task);
> >+              ret = xa_alloc(&dev->worker_xa, &id, worker,
> >xa_limit_32b,
> >+                             GFP_KERNEL);
> >+              if (ret < 0)
> >+                      goto stop_worker;
> >
> >-      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_task_to_cgroups(worker);
> >+              if (ret)
> >+                      goto stop_worker;
> >+      }
> >
> >+      worker->id = id;
> >       return worker;
> >-
> > stop_worker:
> >-      vhost_task_stop(vtsk);
> >+      worker->worker_stop(worker);
> > free_worker:
> >       kfree(worker);
> >       return NULL;
> >diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> >index c650c4506c70..63b1da08a2b0 100644
> >--- a/drivers/vhost/vhost.h
> >+++ b/drivers/vhost/vhost.h
> >@@ -27,6 +27,7 @@ struct vhost_work {
> > };
> >
> > struct vhost_worker {
> >+      struct task_struct *kthread_task;
> >       struct vhost_task       *vtsk;
> >       struct vhost_dev        *dev;
> >       /* Used to serialize device wide flushing with worker swapping. */
> >@@ -36,6 +37,8 @@ struct vhost_worker {
> >       u32                     id;
> >       int                     attachment_cnt;
> >       bool                    killed;
> >+      void (*worker_wakeup)(struct vhost_worker *worker);
> >+      void (*worker_stop)(struct vhost_worker *worker);
> > };
> >
> > /* Poll a file (eventfd or socket) */
> >--
> >2.45.0
> >
>

Re: [PATCH v5 0/6] vhost: Add support of kthread API

[Cindy Lu]

On Wed, Jan 8, 2025 at 8:23 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Dec 30, 2024 at 08:43:47PM +0800, Cindy Lu wrote:
> > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > the vhost now uses vhost_task and operates as a child of the
> > owner thread. This aligns with containerization principles.
> > However, this change has caused confusion for some legacy
> > userspace applications. Therefore, we are reintroducing
> > support for the kthread API.
>
>
> I briefly applied this, but there seem to be a bit too
> many nits. So I will wait for v6 with everything addressed.
>
> Thanks!
>
sure, I will rebase this to the latest kernel, Thanks
Thanks
Cindy
> > In this series, a new UAPI is implemented to allow
> > userspace applications to configure their thread mode.
> >
> > Changelog v2:
> >  1. Change the module_param's name to enforce_inherit_owner, and the default value is true.
> >  2. Change the UAPI's name to VHOST_SET_INHERIT_FROM_OWNER.
> >
> > Changelog v3:
> >  1. Change the module_param's name to inherit_owner_default, and the default value is true.
> >  2. Add a structure for task function; the worker will select a different mode based on the value inherit_owner.
> >  3. device will have their own inherit_owner in struct vhost_dev
> >  4. Address other comments
> >
> > Changelog v4:
> >  1. remove the module_param, only keep the UAPI
> >  2. remove the structure for task function; change to use the function pointer in vhost_worker
> >  3. fix the issue in vhost_worker_create and vhost_dev_ioctl
> >  4. Address other comments
> >
> > Changelog v5:
> >  1. Change wakeup and stop function pointers in struct vhost_worker to void.
> >  2. merging patches 4, 5, 6 in a single patch
> >  3. Fix spelling issues and address other comments.
> >
> > Tested with QEMU with kthread mode/task mode/kthread+task mode
> >
> > Cindy Lu (6):
> >   vhost: Add a new parameter in vhost_dev to allow user select kthread
> >   vhost: Add the vhost_worker to support kthread
> >   vhost: Add the cgroup related function
> >   vhost: Add worker related functions to support kthread
> >   vhost: Add new UAPI to support change to task mode
> >   vhost_scsi: Add check for inherit_owner status
> >
> >  drivers/vhost/scsi.c       |   8 ++
> >  drivers/vhost/vhost.c      | 178 +++++++++++++++++++++++++++++++++----
> >  drivers/vhost/vhost.h      |   4 +
> >  include/uapi/linux/vhost.h |  19 ++++
> >  4 files changed, 192 insertions(+), 17 deletions(-)
> >
> > --
> > 2.45.0
>

Re: [PATCH v5 6/6] vhost_scsi: Add check for inherit_owner status

[Mike Christie]

On 12/30/24 6:43 AM, Cindy Lu wrote:
> The vhost_scsi VHOST_NEW_WORKER requires the inherit_owner
> setting to be true. So we need to implement a check for this.
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/scsi.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 718fa4e0b31e..0d63b6b5c852 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -2086,6 +2086,14 @@ vhost_scsi_ioctl(struct file *f,
>  			return -EFAULT;
>  		return vhost_scsi_set_features(vs, features);
>  	case VHOST_NEW_WORKER:
> +		/*
> +		 * vhost_tasks will account for worker threads under the parent's
> +		 * NPROC value but kthreads do not. To avoid userspace overflowing
> +		 * the system with worker threads inherit_owner must be true.
> +		 */
> +		if (!vs->dev.inherit_owner)> +			return -EFAULT;

Why did you keep this here? I had mentioned it belonged in the common code:

https://lore.kernel.org/virtualization/3864ae3b-d6cd-4227-b4bb-56e014c71667@oracle.com/T/#u

because the limitation applies to all drivers. I didn't see a reply about it
but saw you did fix up the comment. Not sure if I missed it or there was
a technical issue you hit.


> +		fallthrough;
>  	case VHOST_FREE_WORKER:
>  	case VHOST_ATTACH_VRING_WORKER:
>  	case VHOST_GET_VRING_WORKER:

Re: [PATCH v5 0/6] vhost: Add support of kthread API

[Mike Christie]

On 12/30/24 6:43 AM, Cindy Lu wrote:
> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),   
> the vhost now uses vhost_task and operates as a child of the   
> owner thread. This aligns with containerization principles.   
> However, this change has caused confusion for some legacy   
> userspace applications. Therefore, we are reintroducing   
> support for the kthread API.  
> 
> In this series, a new UAPI is implemented to allow   
> userspace applications to configure their thread mode. 
> 
Tested the patches with a hacked up QEMU that can toggle the
inherit owner bit so I could test both modes.

Tested-by: Mike Christie <michael.christie@oracle.com>