[PATCH v8 0/8] vhost: Add support of kthread API

[PATCH v8 0/8] 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.

Changelog v6:
 1. move the check of VHOST_NEW_WORKER from vhost_scsi to vhost
 2. Change the ioctl name VHOST_SET_INHERIT_FROM_OWNER to VHOST_FORK_FROM_OWNER
 3. reuse the function __vhost_worker_flush
 4. use a ops sturct to support worker relates function
 5. reset the value of inherit_owner in vhost_dev_reset_owner.

Changelog v7:
 1. add a KConfig knob to disable legacy app support
 2. Split the changes into two patches to separately introduce the ops and add kthread support.
 3. Utilized INX_MAX to avoid modifications in __vhost_worker_flush
 4. Rebased on the latest kernel
 5. Address other comments

Changelog v8:
 1. Rebased on the latest kernel
 2. Address some other comments

Tested with QEMU with kthread mode/task mode/kthread+task mode

Cindy Lu (8):
  vhost: Add a new parameter in vhost_dev to allow user select kthread
  vhost: Reintroduce vhost_worker to support kthread
  vhost: Add the cgroup related function
  vhost: Introduce vhost_worker_ops in vhost_worker
  vhost: Reintroduce kthread mode support in vhost
  vhost: uapi to control task mode (owner vs kthread)
  vhost: Add check for inherit_owner status
  vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

 drivers/vhost/Kconfig      |  15 +++
 drivers/vhost/vhost.c      | 219 +++++++++++++++++++++++++++++++++----
 drivers/vhost/vhost.h      |  21 ++++
 include/uapi/linux/vhost.h |  16 +++
 4 files changed, 252 insertions(+), 19 deletions(-)

-- 
2.45.0

[PATCH v8 1/8] 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 app, Therefore, we are reintroducing kthread API support.

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 | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 63612faeab72..250dc43f1786 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..19bb94922a0e 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -176,6 +176,15 @@ struct vhost_dev {
 	int byte_weight;
 	struct xarray worker_xa;
 	bool use_worker;
+	/*
+	 * 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.
+	 * here we use true as default value
+	 */
+	bool inherit_owner;
 	int (*msg_handler)(struct vhost_dev *dev, u32 asid,
 			   struct vhost_iotlb_msg *msg);
 };
-- 
2.45.0

[PATCH v8 2/8] vhost: Reintroduce 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 change to support task in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
change to 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 250dc43f1786..9500e85b42ce 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 v8 3/8] 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_attach_task_to_cgroups().

The old function was remove in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9500e85b42ce..20571bd6f7bd 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,46 @@ 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_attach_cgroups_struct attach;
+	int saved_cnt;
+
+	attach.owner = current;
+
+	vhost_work_init(&attach.work, vhost_attach_cgroups_work);
+	vhost_worker_queue(worker, &attach.work);
+
+	mutex_lock(&worker->mutex);
+
+	/*
+	 * Bypass attachment_cnt check in __vhost_worker_flush:
+	 * Temporarily change it to INT_MAX to bypass the check
+	 */
+	saved_cnt = worker->attachment_cnt;
+	worker->attachment_cnt = INT_MAX;
+	__vhost_worker_flush(worker);
+	worker->attachment_cnt = saved_cnt;
+
+	mutex_unlock(&worker->mutex);
+
+	return attach.ret;
+}
+
 /* Caller should have device mutex */
 bool vhost_dev_has_owner(struct vhost_dev *dev)
 {
-- 
2.45.0

[PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Cindy Lu]

Abstract vhost worker operations (create/stop/wakeup) into an ops
structure to prepare for kthread mode support.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 20571bd6f7bd..c162ad772f8f 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->ops->wakeup(worker);
 	}
 }
 
@@ -706,7 +706,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->ops->stop(worker);
 	kfree(worker);
 }
 
@@ -729,42 +729,69 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	xa_destroy(&dev->worker_xa);
 }
 
+static void vhost_task_wakeup(struct vhost_worker *worker)
+{
+	return vhost_task_wake(worker->vtsk);
+}
+
+static void vhost_task_do_stop(struct vhost_worker *worker)
+{
+	return vhost_task_stop(worker->vtsk);
+}
+
+static int vhost_task_worker_create(struct vhost_worker *worker,
+				    struct vhost_dev *dev, const char *name)
+{
+	struct vhost_task *vtsk;
+	u32 id;
+	int ret;
+
+	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
+				 worker, name);
+	if (IS_ERR(vtsk))
+		return PTR_ERR(vtsk);
+
+	worker->vtsk = vtsk;
+	vhost_task_start(vtsk);
+	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+	if (ret < 0) {
+		vhost_task_do_stop(worker);
+		return ret;
+	}
+	worker->id = id;
+	return 0;
+}
+
+static const struct vhost_worker_ops vhost_task_ops = {
+	.create = vhost_task_worker_create,
+	.stop = vhost_task_do_stop,
+	.wakeup = vhost_task_wakeup,
+};
+
 static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
-	struct vhost_task *vtsk;
 	char name[TASK_COMM_LEN];
 	int ret;
-	u32 id;
+	const struct vhost_worker_ops *ops = &vhost_task_ops;
 
 	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
 	if (!worker)
 		return NULL;
 
 	worker->dev = dev;
+	worker->ops = ops;
 	snprintf(name, sizeof(name), "vhost-%d", current->pid);
 
-	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
-				 worker, name);
-	if (IS_ERR(vtsk))
-		goto free_worker;
-
 	mutex_init(&worker->mutex);
 	init_llist_head(&worker->work_list);
 	worker->kcov_handle = kcov_common_handle();
-	worker->vtsk = vtsk;
-
-	vhost_task_start(vtsk);
-
-	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+	ret = ops->create(worker, dev, name);
 	if (ret < 0)
-		goto stop_worker;
-	worker->id = id;
+		goto free_worker;
 
 	return worker;
 
-stop_worker:
-	vhost_task_stop(vtsk);
 free_worker:
 	kfree(worker);
 	return NULL;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 19bb94922a0e..98895e299efa 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -26,6 +26,16 @@ struct vhost_work {
 	unsigned long		flags;
 };
 
+struct vhost_worker;
+struct vhost_dev;
+
+struct vhost_worker_ops {
+	int (*create)(struct vhost_worker *worker, struct vhost_dev *dev,
+		      const char *name);
+	void (*stop)(struct vhost_worker *worker);
+	void (*wakeup)(struct vhost_worker *worker);
+};
+
 struct vhost_worker {
 	struct vhost_task	*vtsk;
 	struct vhost_dev	*dev;
@@ -36,6 +46,7 @@ struct vhost_worker {
 	u32			id;
 	int			attachment_cnt;
 	bool			killed;
+	const struct vhost_worker_ops *ops;
 };
 
 /* Poll a file (eventfd or socket) */
-- 
2.45.0

[PATCH v8 5/8] vhost: Reintroduce kthread mode support in vhost

[Cindy Lu]

This commit restores the previously removed functions kthread
wake/stop/create, and use ops structure vhost_worker_ops to
manage worker wakeup, stop and creation. The function
vhost_worker_create initializes these ops pointers based on
the value of inherit_owner

The old function was remove in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c162ad772f8f..be97028a8baf 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -734,11 +734,21 @@ static void vhost_task_wakeup(struct vhost_worker *worker)
 	return vhost_task_wake(worker->vtsk);
 }
 
+static void vhost_kthread_wakeup(struct vhost_worker *worker)
+{
+	wake_up_process(worker->kthread_task);
+}
+
 static void vhost_task_do_stop(struct vhost_worker *worker)
 {
 	return vhost_task_stop(worker->vtsk);
 }
 
+static void vhost_kthread_do_stop(struct vhost_worker *worker)
+{
+	kthread_stop(worker->kthread_task);
+}
+
 static int vhost_task_worker_create(struct vhost_worker *worker,
 				    struct vhost_dev *dev, const char *name)
 {
@@ -762,6 +772,41 @@ static int vhost_task_worker_create(struct vhost_worker *worker,
 	return 0;
 }
 
+static int vhost_kthread_worker_create(struct vhost_worker *worker,
+				       struct vhost_dev *dev, const char *name)
+{
+	struct task_struct *task;
+	u32 id;
+	int ret;
+
+	task = kthread_create(vhost_run_work_kthread_list, worker, "%s", name);
+	if (IS_ERR(task))
+		return PTR_ERR(task);
+
+	worker->kthread_task = task;
+	wake_up_process(task);
+	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+	if (ret < 0)
+		goto stop_worker;
+
+	ret = vhost_attach_task_to_cgroups(worker);
+	if (ret)
+		goto stop_worker;
+
+	worker->id = id;
+	return 0;
+
+stop_worker:
+	vhost_kthread_do_stop(worker);
+	return ret;
+}
+
+static const struct vhost_worker_ops kthread_ops = {
+	.create = vhost_kthread_worker_create,
+	.stop = vhost_kthread_do_stop,
+	.wakeup = vhost_kthread_wakeup,
+};
+
 static const struct vhost_worker_ops vhost_task_ops = {
 	.create = vhost_task_worker_create,
 	.stop = vhost_task_do_stop,
@@ -773,7 +818,8 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 	struct vhost_worker *worker;
 	char name[TASK_COMM_LEN];
 	int ret;
-	const struct vhost_worker_ops *ops = &vhost_task_ops;
+	const struct vhost_worker_ops *ops =
+		dev->inherit_owner ? &vhost_task_ops : &kthread_ops;
 
 	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
 	if (!worker)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 98895e299efa..af4b2f7d3b91 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -37,6 +37,7 @@ struct vhost_worker_ops {
 };
 
 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. */
-- 
2.45.0

[PATCH v8 6/8] vhost: uapi to control task mode (owner vs kthread)

[Cindy Lu]

Add a new UAPI to configure the vhost device to use the kthread mode
The userspace application can use IOCTL VHOST_FORK_FROM_OWNER
to choose between owner and kthread 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 | 16 ++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index be97028a8baf..ff930c2e5b78 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1134,7 +1134,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
 	int i;
 
 	vhost_dev_cleanup(dev);
-
+	dev->inherit_owner = true;
 	dev->umem = umem;
 	/* We don't need VQ locks below since vhost_dev_cleanup makes sure
 	 * VQs aren't running.
@@ -2287,7 +2287,25 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 		r = vhost_dev_set_owner(d);
 		goto done;
 	}
-
+	if (ioctl == VHOST_FORK_FROM_OWNER) {
+		u8 inherit_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;
+		}
+		if (inherit_owner > 1) {
+			r = -EINVAL;
+			goto done;
+		}
+		d->inherit_owner = (bool)inherit_owner;
+		r = 0;
+		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..1ae0917bfeca 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,20 @@
  */
 #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
 					      struct vhost_vring_state)
+
+/**
+ * VHOST_FORK_FROM_OWNER - Set the inherit_owner flag for the vhost device,
+ * This ioctl must called before VHOST_SET_OWNER.
+ *
+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
+ *
+ * When inherit_owner is set to 1(default value):
+ *   - Vhost will create tasks similar to processes forked from the owner,
+ *     inheriting all of the owner's attributes.
+ *
+ * When inherit_owner is set to 0:
+ *   - Vhost will create tasks as kernel thread.
+ */
+#define VHOST_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
+
 #endif
-- 
2.45.0

[PATCH v8 7/8] vhost: Add check for inherit_owner status

[Cindy Lu]

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

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ff930c2e5b78..fb0c7fb43f78 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1018,6 +1018,13 @@ long vhost_worker_ioctl(struct vhost_dev *dev, unsigned int ioctl,
 	switch (ioctl) {
 	/* dev worker ioctls */
 	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 (!dev->inherit_owner)
+			return -EFAULT;
 		ret = vhost_new_worker(dev, &state);
 		if (!ret && copy_to_user(argp, &state, sizeof(state)))
 			ret = -EFAULT;
-- 
2.45.0

[PATCH v8 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

[Cindy Lu]

Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
is disabled, and any attempt to use it will result in failure.

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

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index b455d9ab6f3d..e5b9dcbf31b6 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
 	  If unsure, say "N".
 
 endif
+
+config VHOST_ENABLE_FORK_OWNER_IOCTL
+	bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
+	default n
+	help
+	  This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
+	  userspace applications to modify the thread mode for vhost devices.
+
+          By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
+          meaning the ioctl is disabled and any operation using this ioctl
+          will fail.
+          When the configuration is enabled (y), the ioctl becomes
+          available, allowing users to set the mode if needed.
+
+	  If unsure, say "N".
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index fb0c7fb43f78..568e43cb54a9 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 		r = vhost_dev_set_owner(d);
 		goto done;
 	}
+
+#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
 	if (ioctl == VHOST_FORK_FROM_OWNER) {
 		u8 inherit_owner;
 		/*inherit_owner can only be modified before owner is set*/
@@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 		r = 0;
 		goto done;
 	}
+#endif
 	/* You must be the owner to do anything else */
 	r = vhost_dev_check_owner(d);
 	if (r)
-- 
2.45.0

Re: [PATCH v8 0/8] vhost: Add support of kthread API

[Lei Yang]

QE tested this series of v8 with virtio-net regression tests,
everything works fine.

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

On Fri, Mar 28, 2025 at 6:04 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.
>
> Changelog v6:
>  1. move the check of VHOST_NEW_WORKER from vhost_scsi to vhost
>  2. Change the ioctl name VHOST_SET_INHERIT_FROM_OWNER to VHOST_FORK_FROM_OWNER
>  3. reuse the function __vhost_worker_flush
>  4. use a ops sturct to support worker relates function
>  5. reset the value of inherit_owner in vhost_dev_reset_owner.
>
> Changelog v7:
>  1. add a KConfig knob to disable legacy app support
>  2. Split the changes into two patches to separately introduce the ops and add kthread support.
>  3. Utilized INX_MAX to avoid modifications in __vhost_worker_flush
>  4. Rebased on the latest kernel
>  5. Address other comments
>
> Changelog v8:
>  1. Rebased on the latest kernel
>  2. Address some other comments
>
> Tested with QEMU with kthread mode/task mode/kthread+task mode
>
> Cindy Lu (8):
>   vhost: Add a new parameter in vhost_dev to allow user select kthread
>   vhost: Reintroduce vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: Introduce vhost_worker_ops in vhost_worker
>   vhost: Reintroduce kthread mode support in vhost
>   vhost: uapi to control task mode (owner vs kthread)
>   vhost: Add check for inherit_owner status
>   vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
>
>  drivers/vhost/Kconfig      |  15 +++
>  drivers/vhost/vhost.c      | 219 +++++++++++++++++++++++++++++++++----
>  drivers/vhost/vhost.h      |  21 ++++
>  include/uapi/linux/vhost.h |  16 +++
>  4 files changed, 252 insertions(+), 19 deletions(-)
>
> --
> 2.45.0
>
>

Re: [PATCH v8 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:52PM +0800, Cindy Lu wrote:
>Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
>to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
>When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
>is disabled, and any attempt to use it will result in failure.
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/Kconfig | 15 +++++++++++++++
> drivers/vhost/vhost.c |  3 +++
> 2 files changed, 18 insertions(+)

IMHO this patch should be squashed with "[PATCH v8 6/8] vhost: uapi to
control task mode (owner vs kthread)".

It might break the bisection to support an ioctl, and after a few
commits enable or disable it depending on a kernel configuration.

>
>diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
>index b455d9ab6f3d..e5b9dcbf31b6 100644
>--- a/drivers/vhost/Kconfig
>+++ b/drivers/vhost/Kconfig
>@@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> 	  If unsure, say "N".
>
> endif

nit: there is a mix of tabs and spaces in the next block, should we
fix it?

>+
>+config VHOST_ENABLE_FORK_OWNER_IOCTL
>+	bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
>+	default n
>+	help
>+	  This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
>+	  userspace applications to modify the thread mode for vhost devices.
   ^
   tabs

>+
>+          By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
>+          meaning the ioctl is disabled and any operation using this ioctl
>+          will fail.
>+          When the configuration is enabled (y), the ioctl becomes
>+          available, allowing users to set the mode if needed.
   ^
   spaces
>+
>+	  If unsure, say "N".
   ^
   tabs

>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index fb0c7fb43f78..568e43cb54a9 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> 		r = vhost_dev_set_owner(d);
> 		goto done;
> 	}
>+
>+#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> 	if (ioctl == VHOST_FORK_FROM_OWNER) {
> 		u8 inherit_owner;
> 		/*inherit_owner can only be modified before owner is set*/
>@@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> 		r = 0;
> 		goto done;
> 	}
>+#endif
> 	/* You must be the owner to do anything else */
> 	r = vhost_dev_check_owner(d);
> 	if (r)
>-- 
>2.45.0
>

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

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:45PM +0800, Cindy Lu 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

nit: missing space "principles,it"

>userspace app, Therefore, we are reintroducing kthread API support.

nit: "userspace app, Therefore" -> "userspace app. Therefore"

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

I would explain that by default this is true, and so we are not changing 
the behavior with this patch.

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

Anyway, the patch LGTM with or without the commit tweaks:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 63612faeab72..250dc43f1786 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..19bb94922a0e 100644
>--- a/drivers/vhost/vhost.h
>+++ b/drivers/vhost/vhost.h
>@@ -176,6 +176,15 @@ struct vhost_dev {
> 	int byte_weight;
> 	struct xarray worker_xa;
> 	bool use_worker;
>+	/*
>+	 * 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.
>+	 * here we use true as default value
>+	 */
>+	bool inherit_owner;
> 	int (*msg_handler)(struct vhost_dev *dev, u32 asid,
> 			   struct vhost_iotlb_msg *msg);
> };
>-- 
>2.45.0
>

Re: [PATCH v8 2/8] vhost: Reintroduce vhost_worker to support kthread

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:46PM +0800, Cindy Lu 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 change to support task in

nit: s/change/changed

>commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
>change to 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(+)

I tried to build commit by commit (mainly to check bisection), and I
discovered that nowdays unused functions produce an error (yes, we can 
mute it for example by setting CONFIG_WERROR to N), but I wanted to 
point it out:

../drivers/vhost/vhost.c:391:12: error: ‘vhost_run_work_kthread_list’ defined but not used [-Werror=unused-function]
   391 | static int vhost_run_work_kthread_list(void *data)

So not sure if we need to squash this where we use it.

Same issue also for the next 2 patches.

Thanks,
Stefano

>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 250dc43f1786..9500e85b42ce 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 v8 3/8] vhost: Add the cgroup related function

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:47PM +0800, Cindy Lu wrote:
>Add back the previously removed cgroup function to support the kthread

nit: Missing . at the end

>The biggest change for this part is in vhost_attach_cgroups() and
>vhost_attach_task_to_cgroups().
>
>The old function was remove in

nit: s/remove/removed

>commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)

As I mentioned, this patch also has unused functions, but LGTM.

Thanks,
Stefano

>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 9500e85b42ce..20571bd6f7bd 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,46 @@ 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_attach_cgroups_struct attach;
>+	int saved_cnt;
>+
>+	attach.owner = current;
>+
>+	vhost_work_init(&attach.work, vhost_attach_cgroups_work);
>+	vhost_worker_queue(worker, &attach.work);
>+
>+	mutex_lock(&worker->mutex);
>+
>+	/*
>+	 * Bypass attachment_cnt check in __vhost_worker_flush:
>+	 * Temporarily change it to INT_MAX to bypass the check
>+	 */
>+	saved_cnt = worker->attachment_cnt;
>+	worker->attachment_cnt = INT_MAX;
>+	__vhost_worker_flush(worker);
>+	worker->attachment_cnt = saved_cnt;
>+
>+	mutex_unlock(&worker->mutex);
>+
>+	return attach.ret;
>+}
>+
> /* Caller should have device mutex */
> bool vhost_dev_has_owner(struct vhost_dev *dev)
> {
>-- 
>2.45.0
>

Re: [PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:48PM +0800, Cindy Lu wrote:
>Abstract vhost worker operations (create/stop/wakeup) into an ops
>structure to prepare for kthread mode support.
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 63 ++++++++++++++++++++++++++++++-------------
> drivers/vhost/vhost.h | 11 ++++++++
> 2 files changed, 56 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 20571bd6f7bd..c162ad772f8f 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->ops->wakeup(worker);
> 	}
> }
>
>@@ -706,7 +706,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->ops->stop(worker);
> 	kfree(worker);
> }
>
>@@ -729,42 +729,69 @@ static void vhost_workers_free(struct vhost_dev *dev)
> 	xa_destroy(&dev->worker_xa);
> }
>
>+static void vhost_task_wakeup(struct vhost_worker *worker)
>+{
>+	return vhost_task_wake(worker->vtsk);
>+}
>+
>+static void vhost_task_do_stop(struct vhost_worker *worker)
>+{
>+	return vhost_task_stop(worker->vtsk);
>+}
>+
>+static int vhost_task_worker_create(struct vhost_worker *worker,
>+				    struct vhost_dev *dev, const char *name)
>+{
>+	struct vhost_task *vtsk;
>+	u32 id;
>+	int ret;
>+
>+	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
>+				 worker, name);
>+	if (IS_ERR(vtsk))
>+		return PTR_ERR(vtsk);
>+
>+	worker->vtsk = vtsk;
>+	vhost_task_start(vtsk);
>+	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
>+	if (ret < 0) {
>+		vhost_task_do_stop(worker);
>+		return ret;
>+	}

In the final code, xa_alloc() is duplicated among the functions that 
create ktrhead or task, might it make sense to leave it out and do it in 
vhost_worker_create() ?

Thanks,
Stefano

>+	worker->id = id;
>+	return 0;
>+}
>+
>+static const struct vhost_worker_ops vhost_task_ops = {
>+	.create = vhost_task_worker_create,
>+	.stop = vhost_task_do_stop,
>+	.wakeup = vhost_task_wakeup,
>+};
>+
> static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> {
> 	struct vhost_worker *worker;
>-	struct vhost_task *vtsk;
> 	char name[TASK_COMM_LEN];
> 	int ret;
>-	u32 id;
>+	const struct vhost_worker_ops *ops = &vhost_task_ops;
>
> 	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> 	if (!worker)
> 		return NULL;
>
> 	worker->dev = dev;
>+	worker->ops = ops;
> 	snprintf(name, sizeof(name), "vhost-%d", current->pid);
>
>-	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
>-				 worker, name);
>-	if (IS_ERR(vtsk))
>-		goto free_worker;
>-
> 	mutex_init(&worker->mutex);
> 	init_llist_head(&worker->work_list);
> 	worker->kcov_handle = kcov_common_handle();
>-	worker->vtsk = vtsk;
>-
>-	vhost_task_start(vtsk);
>-
>-	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
>+	ret = ops->create(worker, dev, name);
> 	if (ret < 0)
>-		goto stop_worker;
>-	worker->id = id;
>+		goto free_worker;
>
> 	return worker;
>
>-stop_worker:
>-	vhost_task_stop(vtsk);
> free_worker:
> 	kfree(worker);
> 	return NULL;
>diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>index 19bb94922a0e..98895e299efa 100644
>--- a/drivers/vhost/vhost.h
>+++ b/drivers/vhost/vhost.h
>@@ -26,6 +26,16 @@ struct vhost_work {
> 	unsigned long		flags;
> };
>
>+struct vhost_worker;
>+struct vhost_dev;
>+
>+struct vhost_worker_ops {
>+	int (*create)(struct vhost_worker *worker, struct vhost_dev *dev,
>+		      const char *name);
>+	void (*stop)(struct vhost_worker *worker);
>+	void (*wakeup)(struct vhost_worker *worker);
>+};
>+
> struct vhost_worker {
> 	struct vhost_task	*vtsk;
> 	struct vhost_dev	*dev;
>@@ -36,6 +46,7 @@ struct vhost_worker {
> 	u32			id;
> 	int			attachment_cnt;
> 	bool			killed;
>+	const struct vhost_worker_ops *ops;
> };
>
> /* Poll a file (eventfd or socket) */
>-- 
>2.45.0
>

Re: [PATCH v8 5/8] vhost: Reintroduce kthread mode support in vhost

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:49PM +0800, Cindy Lu wrote:
>This commit restores the previously removed functions kthread
>wake/stop/create, and use ops structure vhost_worker_ops to
>manage worker wakeup, stop and creation. The function
>vhost_worker_create initializes these ops pointers based on
>the value of inherit_owner
>
>The old function was remove in

nit: s/remove/removed

>commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
> drivers/vhost/vhost.h |  1 +
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index c162ad772f8f..be97028a8baf 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -734,11 +734,21 @@ static void vhost_task_wakeup(struct vhost_worker *worker)
> 	return vhost_task_wake(worker->vtsk);
> }
>
>+static void vhost_kthread_wakeup(struct vhost_worker *worker)
>+{
>+	wake_up_process(worker->kthread_task);
>+}
>+
> static void vhost_task_do_stop(struct vhost_worker *worker)
> {
> 	return vhost_task_stop(worker->vtsk);
> }
>
>+static void vhost_kthread_do_stop(struct vhost_worker *worker)
>+{
>+	kthread_stop(worker->kthread_task);
>+}
>+
> static int vhost_task_worker_create(struct vhost_worker *worker,
> 				    struct vhost_dev *dev, const char *name)
> {
>@@ -762,6 +772,41 @@ static int vhost_task_worker_create(struct vhost_worker *worker,
> 	return 0;
> }
>
>+static int vhost_kthread_worker_create(struct vhost_worker *worker,
>+				       struct vhost_dev *dev, const char *name)
>+{
>+	struct task_struct *task;
>+	u32 id;
>+	int ret;
>+
>+	task = kthread_create(vhost_run_work_kthread_list, worker, "%s", name);
>+	if (IS_ERR(task))
>+		return PTR_ERR(task);
>+
>+	worker->kthread_task = task;
>+	wake_up_process(task);
>+	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
>+	if (ret < 0)
>+		goto stop_worker;
>+
>+	ret = vhost_attach_task_to_cgroups(worker);
>+	if (ret)
>+		goto stop_worker;
>+
>+	worker->id = id;
>+	return 0;
>+
>+stop_worker:
>+	vhost_kthread_do_stop(worker);
>+	return ret;
>+}
>+
>+static const struct vhost_worker_ops kthread_ops = {
>+	.create = vhost_kthread_worker_create,
>+	.stop = vhost_kthread_do_stop,
>+	.wakeup = vhost_kthread_wakeup,
>+};
>+
> static const struct vhost_worker_ops vhost_task_ops = {
> 	.create = vhost_task_worker_create,
> 	.stop = vhost_task_do_stop,
>@@ -773,7 +818,8 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> 	struct vhost_worker *worker;
> 	char name[TASK_COMM_LEN];
> 	int ret;
>-	const struct vhost_worker_ops *ops = &vhost_task_ops;
>+	const struct vhost_worker_ops *ops =
>+		dev->inherit_owner ? &vhost_task_ops : &kthread_ops;
>
> 	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> 	if (!worker)
>diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>index 98895e299efa..af4b2f7d3b91 100644
>--- a/drivers/vhost/vhost.h
>+++ b/drivers/vhost/vhost.h
>@@ -37,6 +37,7 @@ struct vhost_worker_ops {
> };
>
> struct vhost_worker {
>+	struct task_struct *kthread_task;
                           ^
nit: I'm not a fan of tabs, but here all the other fields have it, so I 
would put it in.

> 	struct vhost_task	*vtsk;
> 	struct vhost_dev	*dev;
> 	/* Used to serialize device wide flushing with worker swapping. */
>-- 
>2.45.0
>

The patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH v8 6/8] vhost: uapi to control task mode (owner vs kthread)

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:50PM +0800, Cindy Lu wrote:
>Add a new UAPI to configure the vhost device to use the kthread mode

nit: missing . at the end

>The userspace application can use IOCTL VHOST_FORK_FROM_OWNER
>to choose between owner and kthread mode if necessary

Ditto

>This setting must be applied before VHOST_SET_OWNER, as the worker
>will be created in the VHOST_SET_OWNER function

Ditto.

>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c      | 22 ++++++++++++++++++++--
> include/uapi/linux/vhost.h | 16 ++++++++++++++++
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index be97028a8baf..ff930c2e5b78 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -1134,7 +1134,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
> 	int i;
>
> 	vhost_dev_cleanup(dev);
>-
>+	dev->inherit_owner = true;
> 	dev->umem = umem;
> 	/* We don't need VQ locks below since vhost_dev_cleanup makes sure
> 	 * VQs aren't running.
>@@ -2287,7 +2287,25 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> 		r = vhost_dev_set_owner(d);
> 		goto done;
> 	}
>-

As I mentioned, I'll add the Kconfig in this patch to avoid bisection 
issues.

The rest LGTM.

Thanks,
Stefano

>+	if (ioctl == VHOST_FORK_FROM_OWNER) {
>+		u8 inherit_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;
>+		}
>+		if (inherit_owner > 1) {
>+			r = -EINVAL;
>+			goto done;
>+		}
>+		d->inherit_owner = (bool)inherit_owner;
>+		r = 0;
>+		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..1ae0917bfeca 100644
>--- a/include/uapi/linux/vhost.h
>+++ b/include/uapi/linux/vhost.h
>@@ -235,4 +235,20 @@
>  */
> #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
> 					      struct vhost_vring_state)
>+
>+/**
>+ * VHOST_FORK_FROM_OWNER - Set the inherit_owner flag for the vhost device,
>+ * This ioctl must called before VHOST_SET_OWNER.
>+ *
>+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
>+ *
>+ * When inherit_owner is set to 1(default value):
>+ *   - Vhost will create tasks similar to processes forked from the owner,
>+ *     inheriting all of the owner's attributes.
>+ *
>+ * When inherit_owner is set to 0:
>+ *   - Vhost will create tasks as kernel thread.
>+ */
>+#define VHOST_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
>+
> #endif
>-- 
>2.45.0
>

Re: [PATCH v8 7/8] vhost: Add check for inherit_owner status

[Stefano Garzarella]

On Fri, Mar 28, 2025 at 06:02:51PM +0800, Cindy Lu wrote:
>The VHOST_NEW_WORKER requires the inherit_owner
>setting to be true. So we need to add a check for this.
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 7 +++++++
> 1 file changed, 7 insertions(+)

IMHO we should squash this patch also with the previous one, or do this 
before allowing the user to change inherit_owner, otherwise bisection 
can be broken.

Thanks,
Stefano

>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index ff930c2e5b78..fb0c7fb43f78 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -1018,6 +1018,13 @@ long vhost_worker_ioctl(struct vhost_dev *dev, unsigned int ioctl,
> 	switch (ioctl) {
> 	/* dev worker ioctls */
> 	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 (!dev->inherit_owner)
>+			return -EFAULT;
> 		ret = vhost_new_worker(dev, &state);
> 		if (!ret && copy_to_user(argp, &state, sizeof(state)))
> 			ret = -EFAULT;
>-- 
>2.45.0
>

Re: [PATCH v8 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

[Cindy Lu]

On Tue, Apr 1, 2025 at 9:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 28, 2025 at 06:02:52PM +0800, Cindy Lu wrote:
> >Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> >to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> >When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> >is disabled, and any attempt to use it will result in failure.
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/Kconfig | 15 +++++++++++++++
> > drivers/vhost/vhost.c |  3 +++
> > 2 files changed, 18 insertions(+)
>
> IMHO this patch should be squashed with "[PATCH v8 6/8] vhost: uapi to
> control task mode (owner vs kthread)".
>
> It might break the bisection to support an ioctl, and after a few
> commits enable or disable it depending on a kernel configuration.
>
> >
> >diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> >index b455d9ab6f3d..e5b9dcbf31b6 100644
> >--- a/drivers/vhost/Kconfig
> >+++ b/drivers/vhost/Kconfig
> >@@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> >         If unsure, say "N".
> >
> > endif
>
> nit: there is a mix of tabs and spaces in the next block, should we
> fix it?
>
sure ,will fix this
Thanks
Cindy
> >+
> >+config VHOST_ENABLE_FORK_OWNER_IOCTL
> >+      bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> >+      default n
> >+      help
> >+        This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> >+        userspace applications to modify the thread mode for vhost devices.
>    ^
>    tabs
>
> >+
> >+          By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> >+          meaning the ioctl is disabled and any operation using this ioctl
> >+          will fail.
> >+          When the configuration is enabled (y), the ioctl becomes
> >+          available, allowing users to set the mode if needed.
>    ^
>    spaces
> >+
> >+        If unsure, say "N".
>    ^
>    tabs
>
will fix this
Thanks
Cindy
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index fb0c7fb43f78..568e43cb54a9 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >               r = vhost_dev_set_owner(d);
> >               goto done;
> >       }
> >+
> >+#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> >       if (ioctl == VHOST_FORK_FROM_OWNER) {
> >               u8 inherit_owner;
> >               /*inherit_owner can only be modified before owner is set*/
> >@@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >               r = 0;
> >               goto done;
> >       }
> >+#endif
> >       /* You must be the owner to do anything else */
> >       r = vhost_dev_check_owner(d);
> >       if (r)
> >--
> >2.45.0
> >
>

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

[Cindy Lu]

On Tue, Apr 1, 2025 at 9:30 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 28, 2025 at 06:02:45PM +0800, Cindy Lu 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
>
> nit: missing space "principles,it"
>
> >userspace app, Therefore, we are reintroducing kthread API support.
>
> nit: "userspace app, Therefore" -> "userspace app. Therefore"
>
> >
> >Introduce a new parameter to enable users to choose between
> >kthread and task mode.
>
> I would explain that by default this is true, and so we are not changing
> the behavior with this patch.
>
Sure,I will change these
Thanks
Cindy
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 1 +
> > drivers/vhost/vhost.h | 9 +++++++++
> > 2 files changed, 10 insertions(+)
>
> Anyway, the patch LGTM with or without the commit tweaks:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index 63612faeab72..250dc43f1786 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..19bb94922a0e 100644
> >--- a/drivers/vhost/vhost.h
> >+++ b/drivers/vhost/vhost.h
> >@@ -176,6 +176,15 @@ struct vhost_dev {
> >       int byte_weight;
> >       struct xarray worker_xa;
> >       bool use_worker;
> >+      /*
> >+       * 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.
> >+       * here we use true as default value
> >+       */
> >+      bool inherit_owner;
> >       int (*msg_handler)(struct vhost_dev *dev, u32 asid,
> >                          struct vhost_iotlb_msg *msg);
> > };
> >--
> >2.45.0
> >
>

Re: [PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Cindy Lu]

On Tue, Apr 1, 2025 at 9:48 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 28, 2025 at 06:02:48PM +0800, Cindy Lu wrote:
> >Abstract vhost worker operations (create/stop/wakeup) into an ops
> >structure to prepare for kthread mode support.
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 63 ++++++++++++++++++++++++++++++-------------
> > drivers/vhost/vhost.h | 11 ++++++++
> > 2 files changed, 56 insertions(+), 18 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index 20571bd6f7bd..c162ad772f8f 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->ops->wakeup(worker);
> >       }
> > }
> >
> >@@ -706,7 +706,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->ops->stop(worker);
> >       kfree(worker);
> > }
> >
> >@@ -729,42 +729,69 @@ static void vhost_workers_free(struct vhost_dev *dev)
> >       xa_destroy(&dev->worker_xa);
> > }
> >
> >+static void vhost_task_wakeup(struct vhost_worker *worker)
> >+{
> >+      return vhost_task_wake(worker->vtsk);
> >+}
> >+
> >+static void vhost_task_do_stop(struct vhost_worker *worker)
> >+{
> >+      return vhost_task_stop(worker->vtsk);
> >+}
> >+
> >+static int vhost_task_worker_create(struct vhost_worker *worker,
> >+                                  struct vhost_dev *dev, const char *name)
> >+{
> >+      struct vhost_task *vtsk;
> >+      u32 id;
> >+      int ret;
> >+
> >+      vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> >+                               worker, name);
> >+      if (IS_ERR(vtsk))
> >+              return PTR_ERR(vtsk);
> >+
> >+      worker->vtsk = vtsk;
> >+      vhost_task_start(vtsk);
> >+      ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> >+      if (ret < 0) {
> >+              vhost_task_do_stop(worker);
> >+              return ret;
> >+      }
>
> In the final code, xa_alloc() is duplicated among the functions that
> create ktrhead or task, might it make sense to leave it out and do it in
> vhost_worker_create() ?
>
> Thanks,
> Stefano
>
Thanks a lot Stefano. I previously tried moving xa_alloc() out, but
that made the code strange.
I think keeping xa_alloc() in the create_ops function completes the
job in  a single function, and maybe it could be used in some other
functions in the future
thanks
cindy

> >+      worker->id = id;
> >+      return 0;
> >+}
> >+
> >+static const struct vhost_worker_ops vhost_task_ops = {
> >+      .create = vhost_task_worker_create,
> >+      .stop = vhost_task_do_stop,
> >+      .wakeup = vhost_task_wakeup,
> >+};
> >+
> > static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> > {
> >       struct vhost_worker *worker;
> >-      struct vhost_task *vtsk;
> >       char name[TASK_COMM_LEN];
> >       int ret;
> >-      u32 id;
> >+      const struct vhost_worker_ops *ops = &vhost_task_ops;
> >
> >       worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> >       if (!worker)
> >               return NULL;
> >
> >       worker->dev = dev;
> >+      worker->ops = ops;
> >       snprintf(name, sizeof(name), "vhost-%d", current->pid);
> >
> >-      vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> >-                               worker, name);
> >-      if (IS_ERR(vtsk))
> >-              goto free_worker;
> >-
> >       mutex_init(&worker->mutex);
> >       init_llist_head(&worker->work_list);
> >       worker->kcov_handle = kcov_common_handle();
> >-      worker->vtsk = vtsk;
> >-
> >-      vhost_task_start(vtsk);
> >-
> >-      ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> >+      ret = ops->create(worker, dev, name);
> >       if (ret < 0)
> >-              goto stop_worker;
> >-      worker->id = id;
> >+              goto free_worker;
> >
> >       return worker;
> >
> >-stop_worker:
> >-      vhost_task_stop(vtsk);
> > free_worker:
> >       kfree(worker);
> >       return NULL;
> >diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> >index 19bb94922a0e..98895e299efa 100644
> >--- a/drivers/vhost/vhost.h
> >+++ b/drivers/vhost/vhost.h
> >@@ -26,6 +26,16 @@ struct vhost_work {
> >       unsigned long           flags;
> > };
> >
> >+struct vhost_worker;
> >+struct vhost_dev;
> >+
> >+struct vhost_worker_ops {
> >+      int (*create)(struct vhost_worker *worker, struct vhost_dev *dev,
> >+                    const char *name);
> >+      void (*stop)(struct vhost_worker *worker);
> >+      void (*wakeup)(struct vhost_worker *worker);
> >+};
> >+
> > struct vhost_worker {
> >       struct vhost_task       *vtsk;
> >       struct vhost_dev        *dev;
> >@@ -36,6 +46,7 @@ struct vhost_worker {
> >       u32                     id;
> >       int                     attachment_cnt;
> >       bool                    killed;
> >+      const struct vhost_worker_ops *ops;
> > };
> >
> > /* Poll a file (eventfd or socket) */
> >--
> >2.45.0
> >
>

Re: [PATCH v8 5/8] vhost: Reintroduce kthread mode support in vhost

[Cindy Lu]

On Tue, Apr 1, 2025 at 9:51 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 28, 2025 at 06:02:49PM +0800, Cindy Lu wrote:
> >This commit restores the previously removed functions kthread
> >wake/stop/create, and use ops structure vhost_worker_ops to
> >manage worker wakeup, stop and creation. The function
> >vhost_worker_create initializes these ops pointers based on
> >the value of inherit_owner
> >
> >The old function was remove in
>
> nit: s/remove/removed
>
> >commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
> > drivers/vhost/vhost.h |  1 +
> > 2 files changed, 48 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index c162ad772f8f..be97028a8baf 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -734,11 +734,21 @@ static void vhost_task_wakeup(struct vhost_worker *worker)
> >       return vhost_task_wake(worker->vtsk);
> > }
> >
> >+static void vhost_kthread_wakeup(struct vhost_worker *worker)
> >+{
> >+      wake_up_process(worker->kthread_task);
> >+}
> >+
> > static void vhost_task_do_stop(struct vhost_worker *worker)
> > {
> >       return vhost_task_stop(worker->vtsk);
> > }
> >
> >+static void vhost_kthread_do_stop(struct vhost_worker *worker)
> >+{
> >+      kthread_stop(worker->kthread_task);
> >+}
> >+
> > static int vhost_task_worker_create(struct vhost_worker *worker,
> >                                   struct vhost_dev *dev, const char *name)
> > {
> >@@ -762,6 +772,41 @@ static int vhost_task_worker_create(struct vhost_worker *worker,
> >       return 0;
> > }
> >
> >+static int vhost_kthread_worker_create(struct vhost_worker *worker,
> >+                                     struct vhost_dev *dev, const char *name)
> >+{
> >+      struct task_struct *task;
> >+      u32 id;
> >+      int ret;
> >+
> >+      task = kthread_create(vhost_run_work_kthread_list, worker, "%s", name);
> >+      if (IS_ERR(task))
> >+              return PTR_ERR(task);
> >+
> >+      worker->kthread_task = task;
> >+      wake_up_process(task);
> >+      ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> >+      if (ret < 0)
> >+              goto stop_worker;
> >+
> >+      ret = vhost_attach_task_to_cgroups(worker);
> >+      if (ret)
> >+              goto stop_worker;
> >+
> >+      worker->id = id;
> >+      return 0;
> >+
> >+stop_worker:
> >+      vhost_kthread_do_stop(worker);
> >+      return ret;
> >+}
> >+
> >+static const struct vhost_worker_ops kthread_ops = {
> >+      .create = vhost_kthread_worker_create,
> >+      .stop = vhost_kthread_do_stop,
> >+      .wakeup = vhost_kthread_wakeup,
> >+};
> >+
> > static const struct vhost_worker_ops vhost_task_ops = {
> >       .create = vhost_task_worker_create,
> >       .stop = vhost_task_do_stop,
> >@@ -773,7 +818,8 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> >       struct vhost_worker *worker;
> >       char name[TASK_COMM_LEN];
> >       int ret;
> >-      const struct vhost_worker_ops *ops = &vhost_task_ops;
> >+      const struct vhost_worker_ops *ops =
> >+              dev->inherit_owner ? &vhost_task_ops : &kthread_ops;
> >
> >       worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> >       if (!worker)
> >diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> >index 98895e299efa..af4b2f7d3b91 100644
> >--- a/drivers/vhost/vhost.h
> >+++ b/drivers/vhost/vhost.h
> >@@ -37,6 +37,7 @@ struct vhost_worker_ops {
> > };
> >
> > struct vhost_worker {
> >+      struct task_struct *kthread_task;
>                            ^
> nit: I'm not a fan of tabs, but here all the other fields have it, so I
> would put it in.
>
Sure,will fix this
Thanks
Cindy
> >       struct vhost_task       *vtsk;
> >       struct vhost_dev        *dev;
> >       /* Used to serialize device wide flushing with worker swapping. */
> >--
> >2.45.0
> >
>
> The patch LGTM:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>

Re: [PATCH v8 7/8] vhost: Add check for inherit_owner status

[Cindy Lu]

On Tue, Apr 1, 2025 at 9:59 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 28, 2025 at 06:02:51PM +0800, Cindy Lu wrote:
> >The VHOST_NEW_WORKER requires the inherit_owner
> >setting to be true. So we need to add a check for this.
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
>
> IMHO we should squash this patch also with the previous one, or do this
> before allowing the user to change inherit_owner, otherwise bisection
> can be broken.
>
> Thanks,
> Stefano
>
Sure, will do
Thanks
Cindy
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index ff930c2e5b78..fb0c7fb43f78 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -1018,6 +1018,13 @@ long vhost_worker_ioctl(struct vhost_dev *dev, unsigned int ioctl,
> >       switch (ioctl) {
> >       /* dev worker ioctls */
> >       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 (!dev->inherit_owner)
> >+                      return -EFAULT;
> >               ret = vhost_new_worker(dev, &state);
> >               if (!ret && copy_to_user(argp, &state, sizeof(state)))
> >                       ret = -EFAULT;
> >--
> >2.45.0
> >
>

Re: [PATCH v8 6/8] vhost: uapi to control task mode (owner vs kthread)

[Cindy Lu]

On Tue, Apr 1, 2025 at 9:57 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 28, 2025 at 06:02:50PM +0800, Cindy Lu wrote:
> >Add a new UAPI to configure the vhost device to use the kthread mode
>
> nit: missing . at the end
>
> >The userspace application can use IOCTL VHOST_FORK_FROM_OWNER
> >to choose between owner and kthread mode if necessary
>
> Ditto
>
> >This setting must be applied before VHOST_SET_OWNER, as the worker
> >will be created in the VHOST_SET_OWNER function
>
> Ditto.
>
Sure, will fix this

> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c      | 22 ++++++++++++++++++++--
> > include/uapi/linux/vhost.h | 16 ++++++++++++++++
> > 2 files changed, 36 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index be97028a8baf..ff930c2e5b78 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -1134,7 +1134,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
> >       int i;
> >
> >       vhost_dev_cleanup(dev);
> >-
> >+      dev->inherit_owner = true;
> >       dev->umem = umem;
> >       /* We don't need VQ locks below since vhost_dev_cleanup makes sure
> >        * VQs aren't running.
> >@@ -2287,7 +2287,25 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >               r = vhost_dev_set_owner(d);
> >               goto done;
> >       }
> >-
>
> As I mentioned, I'll add the Kconfig in this patch to avoid bisection
> issues.
>
> The rest LGTM.
>
sure, will fix this
Thanks
cindy
> Thanks,
> Stefano
>
> >+      if (ioctl == VHOST_FORK_FROM_OWNER) {
> >+              u8 inherit_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;
> >+              }
> >+              if (inherit_owner > 1) {
> >+                      r = -EINVAL;
> >+                      goto done;
> >+              }
> >+              d->inherit_owner = (bool)inherit_owner;
> >+              r = 0;
> >+              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..1ae0917bfeca 100644
> >--- a/include/uapi/linux/vhost.h
> >+++ b/include/uapi/linux/vhost.h
> >@@ -235,4 +235,20 @@
> >  */
> > #define VHOST_VDPA_GET_VRING_SIZE     _IOWR(VHOST_VIRTIO, 0x82,       \
> >                                             struct vhost_vring_state)
> >+
> >+/**
> >+ * VHOST_FORK_FROM_OWNER - Set the inherit_owner flag for the vhost device,
> >+ * This ioctl must called before VHOST_SET_OWNER.
> >+ *
> >+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> >+ *
> >+ * When inherit_owner is set to 1(default value):
> >+ *   - Vhost will create tasks similar to processes forked from the owner,
> >+ *     inheriting all of the owner's attributes.
> >+ *
> >+ * When inherit_owner is set to 0:
> >+ *   - Vhost will create tasks as kernel thread.
> >+ */
> >+#define VHOST_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> >+
> > #endif
> >--
> >2.45.0
> >
>

Re: [PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Stefano Garzarella]

On Mon, 7 Apr 2025 at 05:14, Cindy Lu <lulu@redhat.com> wrote:
>
> On Tue, Apr 1, 2025 at 9:48 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Fri, Mar 28, 2025 at 06:02:48PM +0800, Cindy Lu wrote:
> > >Abstract vhost worker operations (create/stop/wakeup) into an ops
> > >structure to prepare for kthread mode support.
> > >
> > >Signed-off-by: Cindy Lu <lulu@redhat.com>
> > >---
> > > drivers/vhost/vhost.c | 63 ++++++++++++++++++++++++++++++-------------
> > > drivers/vhost/vhost.h | 11 ++++++++
> > > 2 files changed, 56 insertions(+), 18 deletions(-)
> > >
> > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > >index 20571bd6f7bd..c162ad772f8f 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->ops->wakeup(worker);
> > >       }
> > > }
> > >
> > >@@ -706,7 +706,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->ops->stop(worker);
> > >       kfree(worker);
> > > }
> > >
> > >@@ -729,42 +729,69 @@ static void vhost_workers_free(struct vhost_dev *dev)
> > >       xa_destroy(&dev->worker_xa);
> > > }
> > >
> > >+static void vhost_task_wakeup(struct vhost_worker *worker)
> > >+{
> > >+      return vhost_task_wake(worker->vtsk);
> > >+}
> > >+
> > >+static void vhost_task_do_stop(struct vhost_worker *worker)
> > >+{
> > >+      return vhost_task_stop(worker->vtsk);
> > >+}
> > >+
> > >+static int vhost_task_worker_create(struct vhost_worker *worker,
> > >+                                  struct vhost_dev *dev, const char *name)
> > >+{
> > >+      struct vhost_task *vtsk;
> > >+      u32 id;
> > >+      int ret;
> > >+
> > >+      vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> > >+                               worker, name);
> > >+      if (IS_ERR(vtsk))
> > >+              return PTR_ERR(vtsk);
> > >+
> > >+      worker->vtsk = vtsk;
> > >+      vhost_task_start(vtsk);
> > >+      ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> > >+      if (ret < 0) {
> > >+              vhost_task_do_stop(worker);
> > >+              return ret;
> > >+      }
> >
> > In the final code, xa_alloc() is duplicated among the functions that
> > create ktrhead or task, might it make sense to leave it out and do it in
> > vhost_worker_create() ?
> >
> > Thanks,
> > Stefano
> >
> Thanks a lot Stefano. I previously tried moving xa_alloc() out, but
> that made the code strange.
> I think keeping xa_alloc() in the create_ops function completes the
> job in  a single function, and maybe it could be used in some other
> functions in the future

Sure, if you tried, and it doesn't add benefits, that's perfectly fine
to ignore this suggestion! ;-)

Thanks,
Stefano

> thanks
> cindy
>
> > >+      worker->id = id;
> > >+      return 0;
> > >+}
> > >+
> > >+static const struct vhost_worker_ops vhost_task_ops = {
> > >+      .create = vhost_task_worker_create,
> > >+      .stop = vhost_task_do_stop,
> > >+      .wakeup = vhost_task_wakeup,
> > >+};
> > >+
> > > static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> > > {
> > >       struct vhost_worker *worker;
> > >-      struct vhost_task *vtsk;
> > >       char name[TASK_COMM_LEN];
> > >       int ret;
> > >-      u32 id;
> > >+      const struct vhost_worker_ops *ops = &vhost_task_ops;
> > >
> > >       worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> > >       if (!worker)
> > >               return NULL;
> > >
> > >       worker->dev = dev;
> > >+      worker->ops = ops;
> > >       snprintf(name, sizeof(name), "vhost-%d", current->pid);
> > >
> > >-      vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> > >-                               worker, name);
> > >-      if (IS_ERR(vtsk))
> > >-              goto free_worker;
> > >-
> > >       mutex_init(&worker->mutex);
> > >       init_llist_head(&worker->work_list);
> > >       worker->kcov_handle = kcov_common_handle();
> > >-      worker->vtsk = vtsk;
> > >-
> > >-      vhost_task_start(vtsk);
> > >-
> > >-      ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> > >+      ret = ops->create(worker, dev, name);
> > >       if (ret < 0)
> > >-              goto stop_worker;
> > >-      worker->id = id;
> > >+              goto free_worker;
> > >
> > >       return worker;
> > >
> > >-stop_worker:
> > >-      vhost_task_stop(vtsk);
> > > free_worker:
> > >       kfree(worker);
> > >       return NULL;
> > >diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > >index 19bb94922a0e..98895e299efa 100644
> > >--- a/drivers/vhost/vhost.h
> > >+++ b/drivers/vhost/vhost.h
> > >@@ -26,6 +26,16 @@ struct vhost_work {
> > >       unsigned long           flags;
> > > };
> > >
> > >+struct vhost_worker;
> > >+struct vhost_dev;
> > >+
> > >+struct vhost_worker_ops {
> > >+      int (*create)(struct vhost_worker *worker, struct vhost_dev *dev,
> > >+                    const char *name);
> > >+      void (*stop)(struct vhost_worker *worker);
> > >+      void (*wakeup)(struct vhost_worker *worker);
> > >+};
> > >+
> > > struct vhost_worker {
> > >       struct vhost_task       *vtsk;
> > >       struct vhost_dev        *dev;
> > >@@ -36,6 +46,7 @@ struct vhost_worker {
> > >       u32                     id;
> > >       int                     attachment_cnt;
> > >       bool                    killed;
> > >+      const struct vhost_worker_ops *ops;
> > > };
> > >
> > > /* Poll a file (eventfd or socket) */
> > >--
> > >2.45.0
> > >
> >
>

Re: [PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Michael S. Tsirkin]

On Fri, Mar 28, 2025 at 06:02:48PM +0800, Cindy Lu wrote:
> Abstract vhost worker operations (create/stop/wakeup) into an ops
> structure to prepare for kthread mode support.
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>

I worry about the overhead of indirect calls here.

We have the wrappers, and only two options,
why did you decide to add it like this,
with ops?



> ---
>  drivers/vhost/vhost.c | 63 ++++++++++++++++++++++++++++++-------------
>  drivers/vhost/vhost.h | 11 ++++++++
>  2 files changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 20571bd6f7bd..c162ad772f8f 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->ops->wakeup(worker);
>  	}
>  }
>  
> @@ -706,7 +706,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->ops->stop(worker);
>  	kfree(worker);
>  }
>  
> @@ -729,42 +729,69 @@ static void vhost_workers_free(struct vhost_dev *dev)
>  	xa_destroy(&dev->worker_xa);
>  }
>  
> +static void vhost_task_wakeup(struct vhost_worker *worker)
> +{
> +	return vhost_task_wake(worker->vtsk);
> +}
> +
> +static void vhost_task_do_stop(struct vhost_worker *worker)
> +{
> +	return vhost_task_stop(worker->vtsk);
> +}
> +
> +static int vhost_task_worker_create(struct vhost_worker *worker,
> +				    struct vhost_dev *dev, const char *name)
> +{
> +	struct vhost_task *vtsk;
> +	u32 id;
> +	int ret;
> +
> +	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> +				 worker, name);
> +	if (IS_ERR(vtsk))
> +		return PTR_ERR(vtsk);
> +
> +	worker->vtsk = vtsk;
> +	vhost_task_start(vtsk);
> +	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> +	if (ret < 0) {
> +		vhost_task_do_stop(worker);
> +		return ret;
> +	}
> +	worker->id = id;
> +	return 0;
> +}
> +
> +static const struct vhost_worker_ops vhost_task_ops = {
> +	.create = vhost_task_worker_create,
> +	.stop = vhost_task_do_stop,
> +	.wakeup = vhost_task_wakeup,
> +};
> +
>  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
>  {
>  	struct vhost_worker *worker;
> -	struct vhost_task *vtsk;
>  	char name[TASK_COMM_LEN];
>  	int ret;
> -	u32 id;
> +	const struct vhost_worker_ops *ops = &vhost_task_ops;
>  
>  	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
>  	if (!worker)
>  		return NULL;
>  
>  	worker->dev = dev;
> +	worker->ops = ops;
>  	snprintf(name, sizeof(name), "vhost-%d", current->pid);
>  
> -	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> -				 worker, name);
> -	if (IS_ERR(vtsk))
> -		goto free_worker;
> -
>  	mutex_init(&worker->mutex);
>  	init_llist_head(&worker->work_list);
>  	worker->kcov_handle = kcov_common_handle();
> -	worker->vtsk = vtsk;
> -
> -	vhost_task_start(vtsk);
> -
> -	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> +	ret = ops->create(worker, dev, name);
>  	if (ret < 0)
> -		goto stop_worker;
> -	worker->id = id;
> +		goto free_worker;
>  
>  	return worker;
>  
> -stop_worker:
> -	vhost_task_stop(vtsk);
>  free_worker:
>  	kfree(worker);
>  	return NULL;
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 19bb94922a0e..98895e299efa 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -26,6 +26,16 @@ struct vhost_work {
>  	unsigned long		flags;
>  };
>  
> +struct vhost_worker;
> +struct vhost_dev;
> +
> +struct vhost_worker_ops {
> +	int (*create)(struct vhost_worker *worker, struct vhost_dev *dev,
> +		      const char *name);
> +	void (*stop)(struct vhost_worker *worker);
> +	void (*wakeup)(struct vhost_worker *worker);
> +};
> +
>  struct vhost_worker {
>  	struct vhost_task	*vtsk;
>  	struct vhost_dev	*dev;
> @@ -36,6 +46,7 @@ struct vhost_worker {
>  	u32			id;
>  	int			attachment_cnt;
>  	bool			killed;
> +	const struct vhost_worker_ops *ops;
>  };
>  
>  /* Poll a file (eventfd or socket) */
> -- 
> 2.45.0

Re: [PATCH v8 5/8] vhost: Reintroduce kthread mode support in vhost

[Mike Christie]

On 3/28/25 5:02 AM, Cindy Lu wrote:
> +static int vhost_kthread_worker_create(struct vhost_worker *worker,
> +				       struct vhost_dev *dev, const char *name)
> +{
> +	struct task_struct *task;
> +	u32 id;
> +	int ret;
> +
> +	task = kthread_create(vhost_run_work_kthread_list, worker, "%s", name);
> +	if (IS_ERR(task))
> +		return PTR_ERR(task);
> +
> +	worker->kthread_task = task;
> +	wake_up_process(task);
> +	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> +	if (ret < 0)
> +		goto stop_worker;
> +
> +	ret = vhost_attach_task_to_cgroups(worker);
> +	if (ret)

If you go to stop_worker here, it will leave the worker in the xa above. I
think you need another goto to unwind that.

> +		goto stop_worker;
> +
> +	worker->id = id;
> +	return 0;
> +
> +stop_worker:
> +	vhost_kthread_do_stop(worker);
> +	return ret;
> +}
> +

Re: [PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Mike Christie]

On 4/7/25 3:17 AM, Michael S. Tsirkin wrote:
> On Fri, Mar 28, 2025 at 06:02:48PM +0800, Cindy Lu wrote:
>> Abstract vhost worker operations (create/stop/wakeup) into an ops
>> structure to prepare for kthread mode support.
>>
>> Signed-off-by: Cindy Lu <lulu@redhat.com>
> 
> I worry about the overhead of indirect calls here.
> 
> We have the wrappers, and only two options,
> why did you decide to add it like this,
> with ops?
> 
That was from my review comment. Originally, I thought we
could share more code. For example I thought
vhost_run_work_kthread_list from patch 2 in this thread and
kernel/vhost_task.c:vhost_task_fn could be merged.

Re: [PATCH v8 5/8] vhost: Reintroduce kthread mode support in vhost

[Cindy Lu]

On Tue, Apr 8, 2025 at 12:04 AM Mike Christie
<michael.christie@oracle.com> wrote:
>
> On 3/28/25 5:02 AM, Cindy Lu wrote:
> > +static int vhost_kthread_worker_create(struct vhost_worker *worker,
> > +                                    struct vhost_dev *dev, const char *name)
> > +{
> > +     struct task_struct *task;
> > +     u32 id;
> > +     int ret;
> > +
> > +     task = kthread_create(vhost_run_work_kthread_list, worker, "%s", name);
> > +     if (IS_ERR(task))
> > +             return PTR_ERR(task);
> > +
> > +     worker->kthread_task = task;
> > +     wake_up_process(task);
> > +     ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> > +     if (ret < 0)
> > +             goto stop_worker;
> > +
> > +     ret = vhost_attach_task_to_cgroups(worker);
> > +     if (ret)
>
> If you go to stop_worker here, it will leave the worker in the xa above. I
> think you need another goto to unwind that.
>
sure, will fix this
Thanks
Cindy
> > +             goto stop_worker;
> > +
> > +     worker->id = id;
> > +     return 0;
> > +
> > +stop_worker:
> > +     vhost_kthread_do_stop(worker);
> > +     return ret;
> > +}
> > +
>

Re: [PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Cindy Lu]

On Tue, Apr 8, 2025 at 12:06 AM Mike Christie
<michael.christie@oracle.com> wrote:
>
> On 4/7/25 3:17 AM, Michael S. Tsirkin wrote:
> > On Fri, Mar 28, 2025 at 06:02:48PM +0800, Cindy Lu wrote:
> >> Abstract vhost worker operations (create/stop/wakeup) into an ops
> >> structure to prepare for kthread mode support.
> >>
> >> Signed-off-by: Cindy Lu <lulu@redhat.com>
> >
> > I worry about the overhead of indirect calls here.
> >
> > We have the wrappers, and only two options,
> > why did you decide to add it like this,
> > with ops?
> >
> That was from my review comment. Originally, I thought we
> could share more code. For example I thought
> vhost_run_work_kthread_list from patch 2 in this thread and
> kernel/vhost_task.c:vhost_task_fn could be merged.
>
Hi Mike
I guess you mean function vhost_run_work_list and vhost_run_work_kthread_list?
sure, I will try to merge these two functions in next version
Thanks
Cindy

Re: [PATCH v8 3/8] vhost: Add the cgroup related function

[Markus Elfring]

…
> +++ b/drivers/vhost/vhost.c
…
> +static int vhost_attach_task_to_cgroups(struct vhost_worker *worker)
> +{
…
> +	vhost_worker_queue(worker, &attach.work);
> +
> +	mutex_lock(&worker->mutex);
…
> +	mutex_unlock(&worker->mutex);
> +
> +	return attach.ret;
> +}
…

Under which circumstances would you become interested to apply a statement
like “guard(mutex)(&worker->mutex);”?
https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/mutex.h#L201

Regards,
Markus

Re: [PATCH v8 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

[Michael S. Tsirkin]

On Fri, Mar 28, 2025 at 06:02:52PM +0800, Cindy Lu wrote:
> Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> is disabled, and any attempt to use it will result in failure.
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/Kconfig | 15 +++++++++++++++
>  drivers/vhost/vhost.c |  3 +++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index b455d9ab6f3d..e5b9dcbf31b6 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
>  	  If unsure, say "N".
>  
>  endif
> +
> +config VHOST_ENABLE_FORK_OWNER_IOCTL
> +	bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> +	default n
> +	help
> +	  This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> +	  userspace applications to modify the thread mode for vhost devices.

ok

> +          By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> +          meaning the ioctl is disabled and any operation using this ioctl
> +          will fail.
> +          When the configuration is enabled (y), the ioctl becomes
> +          available, allowing users to set the mode if needed.

no need to be so verbose - the disabled beavious belongs in commit log
not here.

Also either ioctl or IOCTL but not both.

> +
> +	  If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fb0c7fb43f78..568e43cb54a9 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  		r = vhost_dev_set_owner(d);
>  		goto done;
>  	}
> +
> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
>  	if (ioctl == VHOST_FORK_FROM_OWNER) {
>  		u8 inherit_owner;
>  		/*inherit_owner can only be modified before owner is set*/
> @@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  		r = 0;
>  		goto done;
>  	}
> +#endif
>  	/* You must be the owner to do anything else */
>  	r = vhost_dev_check_owner(d);
>  	if (r)
> -- 
> 2.45.0

Re: [PATCH v8 4/8] vhost: Introduce vhost_worker_ops in vhost_worker

[Mike Christie]

On 4/8/25 4:45 AM, Cindy Lu wrote:
> On Tue, Apr 8, 2025 at 12:06 AM Mike Christie
> <michael.christie@oracle.com> wrote:
>>
>> On 4/7/25 3:17 AM, Michael S. Tsirkin wrote:
>>> On Fri, Mar 28, 2025 at 06:02:48PM +0800, Cindy Lu wrote:
>>>> Abstract vhost worker operations (create/stop/wakeup) into an ops
>>>> structure to prepare for kthread mode support.
>>>>
>>>> Signed-off-by: Cindy Lu <lulu@redhat.com>
>>>
>>> I worry about the overhead of indirect calls here.
>>>
>>> We have the wrappers, and only two options,
>>> why did you decide to add it like this,
>>> with ops?
>>>
>> That was from my review comment. Originally, I thought we
>> could share more code. For example I thought
>> vhost_run_work_kthread_list from patch 2 in this thread and
>> kernel/vhost_task.c:vhost_task_fn could be merged.
>>
> Hi Mike
> I guess you mean function vhost_run_work_list and vhost_run_work_kthread_list?
> sure, I will try to merge these two functions in next version

Oh no, I meant the opposite. I don't think it will work out
like how I thought it would originally.

I think Michael's concern about the extra indirect pointer
access in the IO path may cause issues with net. For scsi I
didn't see any issue but that's probably because we have
other perf issues.

So if Michael is saying to not do the ops then that's fine
with me.

Re: [PATCH v8 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER

[Cindy Lu]

On Tue, Apr 8, 2025 at 7:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Mar 28, 2025 at 06:02:52PM +0800, Cindy Lu wrote:
> > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > is disabled, and any attempt to use it will result in failure.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/Kconfig | 15 +++++++++++++++
> >  drivers/vhost/vhost.c |  3 +++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> >         If unsure, say "N".
> >
> >  endif
> > +
> > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > +     bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > +     default n
> > +     help
> > +       This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > +       userspace applications to modify the thread mode for vhost devices.
>
> ok
>
> > +          By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > +          meaning the ioctl is disabled and any operation using this ioctl
> > +          will fail.
> > +          When the configuration is enabled (y), the ioctl becomes
> > +          available, allowing users to set the mode if needed.
>
> no need to be so verbose - the disabled beavious belongs in commit log
> not here.
>
> Also either ioctl or IOCTL but not both.
>
sure, will change this
Thanks
cindy
> > +
> > +       If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index fb0c7fb43f78..568e43cb54a9 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >               r = vhost_dev_set_owner(d);
> >               goto done;
> >       }
> > +
> > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> >       if (ioctl == VHOST_FORK_FROM_OWNER) {
> >               u8 inherit_owner;
> >               /*inherit_owner can only be modified before owner is set*/
> > @@ -2313,6 +2315,7 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >               r = 0;
> >               goto done;
> >       }
> > +#endif
> >       /* You must be the owner to do anything else */
> >       r = vhost_dev_check_owner(d);
> >       if (r)
> > --
> > 2.45.0
>