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

[PATCH v6 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.
 
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  
 
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: Reintroduce vhost_worker to support kthread
  vhost: Add the cgroup related function
  vhost: introduce worker ops to support multiple thread models
  vhost: Add new UAPI to support change to task mode
  vhost: Add check for inherit_owner status

 drivers/vhost/vhost.c      | 227 +++++++++++++++++++++++++++++++++----
 drivers/vhost/vhost.h      |  13 +++
 include/uapi/linux/vhost.h |  18 +++
 3 files changed, 234 insertions(+), 24 deletions(-)

-- 
2.45.0

[PATCH v6 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 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 | 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

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

[Jason Wang]

On Sun, Feb 23, 2025 at 11:40 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 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>

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

Thanks

[PATCH v6 2/6] 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 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 v6 2/6] vhost: Reintroduce vhost_worker to support kthread

[Jason Wang]

On Sun, Feb 23, 2025 at 11:41 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 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>

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

Thanks

[PATCH v6 3/6] 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().

Reuse the function __vhost_worker_flush, but in this situation, the
attachment_cnt is 0. Therefore, add a boolean to disable this check.

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 | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1feba29abf95..adbb957c8b5f 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>
@@ -269,11 +270,12 @@ EXPORT_SYMBOL_GPL(vhost_vq_work_queue);
  *
  * The worker's flush_mutex must be held.
  */
-static void __vhost_worker_flush(struct vhost_worker *worker)
+static void __vhost_worker_flush(struct vhost_worker *worker,
+				 bool ignore_attachment)
 {
 	struct vhost_flush_struct flush;
 
-	if (!worker->attachment_cnt || worker->killed)
+	if ((!ignore_attachment && !worker->attachment_cnt) || worker->killed)
 		return;
 
 	init_completion(&flush.wait_event);
@@ -292,7 +294,7 @@ static void __vhost_worker_flush(struct vhost_worker *worker)
 static void vhost_worker_flush(struct vhost_worker *worker)
 {
 	mutex_lock(&worker->mutex);
-	__vhost_worker_flush(worker);
+	__vhost_worker_flush(worker, false);
 	mutex_unlock(&worker->mutex);
 }
 
@@ -620,6 +622,36 @@ 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;
+
+	attach.owner = current;
+
+	vhost_work_init(&attach.work, vhost_attach_cgroups_work);
+	vhost_worker_queue(worker, &attach.work);
+
+	mutex_lock(&worker->mutex);
+	__vhost_worker_flush(worker, true);
+	mutex_unlock(&worker->mutex);
+
+	return attach.ret;
+}
+
 /* Caller should have device mutex */
 bool vhost_dev_has_owner(struct vhost_dev *dev)
 {
@@ -793,7 +825,7 @@ static void __vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 	/* Make sure new vq queue/flush/poll calls see the new worker */
 	synchronize_rcu();
 	/* Make sure whatever was queued gets run */
-	__vhost_worker_flush(old_worker);
+	__vhost_worker_flush(old_worker, false);
 	old_worker->attachment_cnt--;
 	mutex_unlock(&old_worker->mutex);
 }
@@ -852,7 +884,7 @@ static int vhost_free_worker(struct vhost_dev *dev,
 	 * to zero. Make sure flushes are flushed from the queue before
 	 * freeing.
 	 */
-	__vhost_worker_flush(worker);
+	__vhost_worker_flush(worker, false);
 	mutex_unlock(&worker->mutex);
 
 	vhost_worker_destroy(dev, worker);
-- 
2.45.0

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

[Jason Wang]

On Sun, Feb 23, 2025 at 11:41 PM Cindy Lu <lulu@redhat.com> wrote:
>
> 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().
>
> Reuse the function __vhost_worker_flush, but in this situation, the
> attachment_cnt is 0. Therefore, add a boolean to disable this check.
>

How about just tweaking its value to INT_MAX so we can avoid this new parameter?

Thanks

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

[Cindy Lu]

On Mon, Feb 24, 2025 at 9:40 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sun, Feb 23, 2025 at 11:41 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > 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().
> >
> > Reuse the function __vhost_worker_flush, but in this situation, the
> > attachment_cnt is 0. Therefore, add a boolean to disable this check.
> >
>
> How about just tweaking its value to INT_MAX so we can avoid this new parameter?
>
> Thanks
>
sure, will change this
thanks
cindy

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

[kernel test robot]

Hi Cindy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.14-rc3]
[also build test WARNING on linus/master next-20250224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cindy-Lu/vhost-Add-a-new-parameter-in-vhost_dev-to-allow-user-select-kthread/20250223-234405
base:   v6.14-rc3
patch link:    https://lore.kernel.org/r/20250223154042.556001-4-lulu%40redhat.com
patch subject: [PATCH v6 3/6] vhost: Add the cgroup related function
config: x86_64-buildonly-randconfig-002-20250224 (https://download.01.org/0day-ci/archive/20250225/202502251038.6UlCIMJy-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250225/202502251038.6UlCIMJy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502251038.6UlCIMJy-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/vhost/vhost.c:275: warning: Function parameter or struct member 'ignore_attachment' not described in '__vhost_worker_flush'


vim +275 drivers/vhost/vhost.c

0921dddcb589803 Mike Christie 2023-06-26  266  
228a27cf78afc63 Mike Christie 2023-06-26  267  /**
ba704ff4e142fd3 Mike Christie 2024-03-15  268   * __vhost_worker_flush - flush a worker
228a27cf78afc63 Mike Christie 2023-06-26  269   * @worker: worker to flush
228a27cf78afc63 Mike Christie 2023-06-26  270   *
ba704ff4e142fd3 Mike Christie 2024-03-15  271   * The worker's flush_mutex must be held.
228a27cf78afc63 Mike Christie 2023-06-26  272   */
84e88426e3bc18f Cindy Lu      2025-02-23  273  static void __vhost_worker_flush(struct vhost_worker *worker,
84e88426e3bc18f Cindy Lu      2025-02-23  274  				 bool ignore_attachment)
a6fc04739be7cd8 Mike Christie 2023-06-26 @275  {
228a27cf78afc63 Mike Christie 2023-06-26  276  	struct vhost_flush_struct flush;
228a27cf78afc63 Mike Christie 2023-06-26  277  
84e88426e3bc18f Cindy Lu      2025-02-23  278  	if ((!ignore_attachment && !worker->attachment_cnt) || worker->killed)
ba704ff4e142fd3 Mike Christie 2024-03-15  279  		return;
ba704ff4e142fd3 Mike Christie 2024-03-15  280  
228a27cf78afc63 Mike Christie 2023-06-26  281  	init_completion(&flush.wait_event);
228a27cf78afc63 Mike Christie 2023-06-26  282  	vhost_work_init(&flush.work, vhost_flush_work);
228a27cf78afc63 Mike Christie 2023-06-26  283  
228a27cf78afc63 Mike Christie 2023-06-26  284  	vhost_worker_queue(worker, &flush.work);
ba704ff4e142fd3 Mike Christie 2024-03-15  285  	/*
ba704ff4e142fd3 Mike Christie 2024-03-15  286  	 * Drop mutex in case our worker is killed and it needs to take the
ba704ff4e142fd3 Mike Christie 2024-03-15  287  	 * mutex to force cleanup.
ba704ff4e142fd3 Mike Christie 2024-03-15  288  	 */
ba704ff4e142fd3 Mike Christie 2024-03-15  289  	mutex_unlock(&worker->mutex);
228a27cf78afc63 Mike Christie 2023-06-26  290  	wait_for_completion(&flush.wait_event);
ba704ff4e142fd3 Mike Christie 2024-03-15  291  	mutex_lock(&worker->mutex);
ba704ff4e142fd3 Mike Christie 2024-03-15  292  }
ba704ff4e142fd3 Mike Christie 2024-03-15  293  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v6 4/6] vhost: introduce worker ops to support multiple thread models

[Cindy Lu]

This commit restores the previously removed functions kthread_wakeup and
kthread_stop, and introduces a new ops structure to handle worker wakeup,
stop, and creation. The function vhost_worker_create initializes these
ops pointers based on the inherit_owner value.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index adbb957c8b5f..d8c0ea118bb1 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);
 	}
 }
 
@@ -697,7 +697,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);
 }
 
@@ -720,42 +720,123 @@ 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 int vhost_task_worker_create_fn(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 (!vtsk)
+		return -ENOMEM;
+
+	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_stop_fn(worker);
+		return ret;
+	}
+	worker->id = id;
+	return 0;
+}
+
+static int kthread_worker_create_fn(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_stop_fn(worker);
+	return ret;
+}
+
+static const struct vhost_worker_ops vhost_task_ops = {
+	.create = vhost_task_worker_create_fn,
+	.stop = vhost_task_stop_fn,
+	.wakeup = vhost_task_wakeup_fn,
+};
+
+static const struct vhost_worker_ops kthread_ops = {
+	.create = kthread_worker_create_fn,
+	.stop = vhost_kthread_stop_fn,
+	.wakeup = vhost_kthread_wakeup_fn,
+};
+
 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 =
+		dev->inherit_owner ? &vhost_task_ops : &kthread_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 (!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);
+	/*
+	 * 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.
+	 */
 
-	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 c650c4506c70..029c203147be 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -26,7 +26,18 @@ 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 task_struct *kthread_task;
 	struct vhost_task	*vtsk;
 	struct vhost_dev	*dev;
 	/* Used to serialize device wide flushing with worker swapping. */
@@ -36,6 +47,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 v6 4/6] vhost: introduce worker ops to support multiple thread models

[Jason Wang]

On Sun, Feb 23, 2025 at 11:41 PM Cindy Lu <lulu@redhat.com> wrote:
>
> This commit restores the previously removed functions kthread_wakeup and
> kthread_stop, and introduces a new ops structure to handle worker wakeup,
> stop, and creation. The function vhost_worker_create initializes these
> ops pointers based on the inherit_owner value.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

Patch looks good but I have some some nits:

It might be better to have a separate patch to introduce the ops then
doing the kthread stuff on top.

> ---
>  drivers/vhost/vhost.c | 115 +++++++++++++++++++++++++++++++++++-------
>  drivers/vhost/vhost.h |  12 +++++
>  2 files changed, 110 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index adbb957c8b5f..d8c0ea118bb1 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);
>         }
>  }
>
> @@ -697,7 +697,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);
>  }
>
> @@ -720,42 +720,123 @@ 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 int vhost_task_worker_create_fn(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 (!vtsk)
> +               return -ENOMEM;
> +
> +       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_stop_fn(worker);
> +               return ret;
> +       }
> +       worker->id = id;
> +       return 0;
> +}
> +
> +static int kthread_worker_create_fn(struct vhost_worker *worker,

Let's have a consistent name, e.g vhost_kthread_worker_create.

> +                                   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_stop_fn(worker);
> +       return ret;
> +}
> +
> +static const struct vhost_worker_ops vhost_task_ops = {
> +       .create = vhost_task_worker_create_fn,

I think we can get rid of the fn suffix as "fn".

> +       .stop = vhost_task_stop_fn,
> +       .wakeup = vhost_task_wakeup_fn,
> +};
> +
> +static const struct vhost_worker_ops kthread_ops = {
> +       .create = kthread_worker_create_fn,
> +       .stop = vhost_kthread_stop_fn,
> +       .wakeup = vhost_kthread_wakeup_fn,
> +};
> +
>  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 =
> +               dev->inherit_owner ? &vhost_task_ops : &kthread_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 (!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);
> +       /*
> +        * 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.
> +        */

Is this better to move this to the definition of the inherit_owner?

>
> -       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 c650c4506c70..029c203147be 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -26,7 +26,18 @@ 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 task_struct *kthread_task;
>         struct vhost_task       *vtsk;
>         struct vhost_dev        *dev;
>         /* Used to serialize device wide flushing with worker swapping. */
> @@ -36,6 +47,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
>

Thanks

Re: [PATCH v6 4/6] vhost: introduce worker ops to support multiple thread models

[Cindy Lu]

On Mon, Feb 24, 2025 at 9:45 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sun, Feb 23, 2025 at 11:41 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > This commit restores the previously removed functions kthread_wakeup and
> > kthread_stop, and introduces a new ops structure to handle worker wakeup,
> > stop, and creation. The function vhost_worker_create initializes these
> > ops pointers based on the inherit_owner value.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
>
> Patch looks good but I have some some nits:
>
> It might be better to have a separate patch to introduce the ops then
> doing the kthread stuff on top.
>
sure, will do
thanks
cindy
> > ---
> >  drivers/vhost/vhost.c | 115 +++++++++++++++++++++++++++++++++++-------
> >  drivers/vhost/vhost.h |  12 +++++
> >  2 files changed, 110 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index adbb957c8b5f..d8c0ea118bb1 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);
> >         }
> >  }
> >
> > @@ -697,7 +697,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);
> >  }
> >
> > @@ -720,42 +720,123 @@ 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 int vhost_task_worker_create_fn(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 (!vtsk)
> > +               return -ENOMEM;
> > +
> > +       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_stop_fn(worker);
> > +               return ret;
> > +       }
> > +       worker->id = id;
> > +       return 0;
> > +}
> > +
> > +static int kthread_worker_create_fn(struct vhost_worker *worker,
>
> Let's have a consistent name, e.g vhost_kthread_worker_create.
>
> > +                                   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_stop_fn(worker);
> > +       return ret;
> > +}
> > +
> > +static const struct vhost_worker_ops vhost_task_ops = {
> > +       .create = vhost_task_worker_create_fn,
>
> I think we can get rid of the fn suffix as "fn".
>
sure, will do
thanks
Cindy
> > +       .stop = vhost_task_stop_fn,
> > +       .wakeup = vhost_task_wakeup_fn,
> > +};
> > +
> > +static const struct vhost_worker_ops kthread_ops = {
> > +       .create = kthread_worker_create_fn,
> > +       .stop = vhost_kthread_stop_fn,
> > +       .wakeup = vhost_kthread_wakeup_fn,
> > +};
> > +
> >  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 =
> > +               dev->inherit_owner ? &vhost_task_ops : &kthread_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 (!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);
> > +       /*
> > +        * 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.
> > +        */
>
> Is this better to move this to the definition of the inherit_owner?
>
sure will do
Thanks
cindy
> >
> > -       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 c650c4506c70..029c203147be 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -26,7 +26,18 @@ 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 task_struct *kthread_task;
> >         struct vhost_task       *vtsk;
> >         struct vhost_dev        *dev;
> >         /* Used to serialize device wide flushing with worker swapping. */
> > @@ -36,6 +47,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
> >
>
> Thanks
>

[PATCH v6 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      | 24 ++++++++++++++++++++++--
 include/uapi/linux/vhost.h | 18 ++++++++++++++++++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d8c0ea118bb1..45d8f5c5bca9 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1133,7 +1133,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.
@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,22 @@
  */
 #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
+ *
+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
+ *
+ * When inherit_owner is set to 1:
+ *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
+
 #endif
-- 
2.45.0

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

[Jason Wang]

On Sun, Feb 23, 2025 at 11:41 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      | 24 ++++++++++++++++++++++--
>  include/uapi/linux/vhost.h | 18 ++++++++++++++++++
>  2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index d8c0ea118bb1..45d8f5c5bca9 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1133,7 +1133,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
>         int i;
>
>         vhost_dev_cleanup(dev);
> -
> +       dev->inherit_owner = true;

Any reason this needs to be changed under reset_owner?

>         dev->umem = umem;
>         /* We don't need VQ locks below since vhost_dev_cleanup makes sure
>          * VQs aren't running.
> @@ -2278,15 +2278,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_FORK_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 */

Code explains itself, let's just drop this comment.

> +               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..8f558b433536 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,22 @@
>   */
>  #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
> + *
> + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> + *
> + * When inherit_owner is set to 1:
> + *   - The VHOST worker threads inherit its values/checks from
> + *     the thread that owns the VHOST device, The vhost threads will
> + *     be counted in the nproc rlimits.

Since this is uAPI, it's better to avoid mentioning too many
implementation details. So I would tweak this as.

"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:
> + *   - 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.

"Vhost will create tasks as kernel thread."

> + */
> +#define VHOST_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> +
>  #endif
> --
> 2.45.0
>

Thanks

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

[Michael S. Tsirkin]

On Sun, Feb 23, 2025 at 11:36:20PM +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>


Thanks Cindy,

can we add a KConfig knob to disable legacy app support?

It can be handy for security.

Pls make it a patch on top.

> ---
>  drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
>  include/uapi/linux/vhost.h | 18 ++++++++++++++++++
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index d8c0ea118bb1..45d8f5c5bca9 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1133,7 +1133,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.
> @@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,22 @@
>   */
>  #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
> + *
> + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> + *
> + * When inherit_owner is set to 1:
> + *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> +
>  #endif
> -- 
> 2.45.0

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

[Michael S. Tsirkin]

better subject:

vhost: uapi to control task mode (owner vs kthread)


On Sun, Feb 23, 2025 at 11:36:20PM +0800, Cindy Lu wrote:
> Add a new UAPI to enable setting the vhost device to task mode.

better:

Add a new UAPI to configure the vhost device to use the kthread mode


> The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
> to configure the mode

... to either owner or kthread.


> 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      | 24 ++++++++++++++++++++++--
>  include/uapi/linux/vhost.h | 18 ++++++++++++++++++
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index d8c0ea118bb1..45d8f5c5bca9 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1133,7 +1133,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.
> @@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,22 @@
>   */
>  #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
> + *
> + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> + *
> + * When inherit_owner is set to 1:
> + *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> +
>  #endif
> -- 
> 2.45.0

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

[Cindy Lu]

On Tue, Feb 25, 2025 at 5:46 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> better subject:
>
> vhost: uapi to control task mode (owner vs kthread)
>
>
> On Sun, Feb 23, 2025 at 11:36:20PM +0800, Cindy Lu wrote:
> > Add a new UAPI to enable setting the vhost device to task mode.
>
> better:
>
> Add a new UAPI to configure the vhost device to use the kthread mode
>
Thanks MST, will change this
>
> > The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
> > to configure the mode
>
> ... to either owner or kthread.
>
sure, will change this
thanks
cindy
>
> > 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      | 24 ++++++++++++++++++++++--
> >  include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> >  2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index d8c0ea118bb1..45d8f5c5bca9 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1133,7 +1133,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.
> > @@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -235,4 +235,22 @@
> >   */
> >  #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
> > + *
> > + * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> > + *
> > + * When inherit_owner is set to 1:
> > + *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> > +
> >  #endif
> > --
> > 2.45.0
>

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

[Stefano Garzarella]

On Sun, Feb 23, 2025 at 11:36:20PM +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>
>---
> drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
> include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index d8c0ea118bb1..45d8f5c5bca9 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -1133,7 +1133,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.
>@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
>--- a/include/uapi/linux/vhost.h
>+++ b/include/uapi/linux/vhost.h
>@@ -235,4 +235,22 @@
>  */
> #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
>+ *
>+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
>+ *
>+ * When inherit_owner is set to 1:
>+ *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)

I don't think we really care of the size of the parameter, so can we 
just use `bool` or `unsigned int` or `int` for this IOCTL?

As we did for other IOCTLs where we had to enable/disable something (e.g 
VHOST_VSOCK_SET_RUNNING, VHOST_VDPA_SET_VRING_ENABLE).

Thanks,
Stefano

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

[Cindy Lu]

On Tue, Feb 25, 2025 at 7:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Sun, Feb 23, 2025 at 11:36:20PM +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>
> >---
> > drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
> > include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> > 2 files changed, 40 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index d8c0ea118bb1..45d8f5c5bca9 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -1133,7 +1133,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.
> >@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> >--- a/include/uapi/linux/vhost.h
> >+++ b/include/uapi/linux/vhost.h
> >@@ -235,4 +235,22 @@
> >  */
> > #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
> >+ *
> >+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> >+ *
> >+ * When inherit_owner is set to 1:
> >+ *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
>
> I don't think we really care of the size of the parameter, so can we
> just use `bool` or `unsigned int` or `int` for this IOCTL?
>
> As we did for other IOCTLs where we had to enable/disable something (e.g
> VHOST_VSOCK_SET_RUNNING, VHOST_VDPA_SET_VRING_ENABLE).
>
hi Stefano
I initially used it as a boolean, but during the code review, the
maintainers considered it was unsuitable for the bool use as the
interface in ioctl (I think in version 3 ?). So I changed it to u8,
then will check if this is 1/0 in ioctl and the u8 should be
sufficient for us to use
Thanks
cindy
> Thanks,
> Stefano
>

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

[Stefano Garzarella]

Hi Cindy,

On Wed, 26 Feb 2025 at 07:14, Cindy Lu <lulu@redhat.com> wrote:
>
> On Tue, Feb 25, 2025 at 7:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Sun, Feb 23, 2025 at 11:36:20PM +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>
> > >---
> > > drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
> > > include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> > > 2 files changed, 40 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > >index d8c0ea118bb1..45d8f5c5bca9 100644
> > >--- a/drivers/vhost/vhost.c
> > >+++ b/drivers/vhost/vhost.c
> > >@@ -1133,7 +1133,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.
> > >@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> > >--- a/include/uapi/linux/vhost.h
> > >+++ b/include/uapi/linux/vhost.h
> > >@@ -235,4 +235,22 @@
> > >  */
> > > #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
> > >+ *
> > >+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> > >+ *
> > >+ * When inherit_owner is set to 1:
> > >+ *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> >
> > I don't think we really care of the size of the parameter, so can we
> > just use `bool` or `unsigned int` or `int` for this IOCTL?
> >
> > As we did for other IOCTLs where we had to enable/disable something (e.g
> > VHOST_VSOCK_SET_RUNNING, VHOST_VDPA_SET_VRING_ENABLE).
> >
> hi Stefano
> I initially used it as a boolean, but during the code review, the
> maintainers considered it was unsuitable for the bool use as the

I see, indeed I found only 1 case of bool:

include/uapi/misc/xilinx_sdfec.h:#define XSDFEC_SET_BYPASS
_IOW(XSDFEC_MAGIC, 9, bool)

> interface in ioctl (I think in version 3 ?). So I changed it to u8,
> then will check if this is 1/0 in ioctl and the u8 should be
> sufficient for us to use

Okay, if Michael and Jason are happy with it, it's fine.
It just seemed strange to me that for other IOCTLs we use int or
unsigned int when we need a boolean instead of a sized type.

Thanks for looking at it,
Stefano

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

[Michael S. Tsirkin]

On Wed, Feb 26, 2025 at 09:46:06AM +0100, Stefano Garzarella wrote:
> Hi Cindy,
> 
> On Wed, 26 Feb 2025 at 07:14, Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Tue, Feb 25, 2025 at 7:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >
> > > On Sun, Feb 23, 2025 at 11:36:20PM +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>
> > > >---
> > > > drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
> > > > include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> > > > 2 files changed, 40 insertions(+), 2 deletions(-)
> > > >
> > > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > >index d8c0ea118bb1..45d8f5c5bca9 100644
> > > >--- a/drivers/vhost/vhost.c
> > > >+++ b/drivers/vhost/vhost.c
> > > >@@ -1133,7 +1133,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.
> > > >@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> > > >--- a/include/uapi/linux/vhost.h
> > > >+++ b/include/uapi/linux/vhost.h
> > > >@@ -235,4 +235,22 @@
> > > >  */
> > > > #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
> > > >+ *
> > > >+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> > > >+ *
> > > >+ * When inherit_owner is set to 1:
> > > >+ *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> > >
> > > I don't think we really care of the size of the parameter, so can we
> > > just use `bool` or `unsigned int` or `int` for this IOCTL?
> > >
> > > As we did for other IOCTLs where we had to enable/disable something (e.g
> > > VHOST_VSOCK_SET_RUNNING, VHOST_VDPA_SET_VRING_ENABLE).
> > >
> > hi Stefano
> > I initially used it as a boolean, but during the code review, the
> > maintainers considered it was unsuitable for the bool use as the
> 
> I see, indeed I found only 1 case of bool:
> 
> include/uapi/misc/xilinx_sdfec.h:#define XSDFEC_SET_BYPASS
> _IOW(XSDFEC_MAGIC, 9, bool)
> 
> > interface in ioctl (I think in version 3 ?). So I changed it to u8,
> > then will check if this is 1/0 in ioctl and the u8 should be
> > sufficient for us to use
> 
> Okay, if Michael and Jason are happy with it, it's fine.
> It just seemed strange to me that for other IOCTLs we use int or
> unsigned int when we need a boolean instead of a sized type.

I only found VHOST_VSOCK_SET_RUNNING. which other ioctls?

> Thanks for looking at it,
> Stefano

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

[Stefano Garzarella]

On Wed, 26 Feb 2025 at 09:50, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 26, 2025 at 09:46:06AM +0100, Stefano Garzarella wrote:
> > Hi Cindy,
> >
> > On Wed, 26 Feb 2025 at 07:14, Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > On Tue, Feb 25, 2025 at 7:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > >
> > > > On Sun, Feb 23, 2025 at 11:36:20PM +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>
> > > > >---
> > > > > drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
> > > > > include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> > > > > 2 files changed, 40 insertions(+), 2 deletions(-)
> > > > >
> > > > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > >index d8c0ea118bb1..45d8f5c5bca9 100644
> > > > >--- a/drivers/vhost/vhost.c
> > > > >+++ b/drivers/vhost/vhost.c
> > > > >@@ -1133,7 +1133,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.
> > > > >@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> > > > >--- a/include/uapi/linux/vhost.h
> > > > >+++ b/include/uapi/linux/vhost.h
> > > > >@@ -235,4 +235,22 @@
> > > > >  */
> > > > > #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
> > > > >+ *
> > > > >+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> > > > >+ *
> > > > >+ * When inherit_owner is set to 1:
> > > > >+ *   - The VHOST worker threads inherit its 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> > > >
> > > > I don't think we really care of the size of the parameter, so can we
> > > > just use `bool` or `unsigned int` or `int` for this IOCTL?
> > > >
> > > > As we did for other IOCTLs where we had to enable/disable something (e.g
> > > > VHOST_VSOCK_SET_RUNNING, VHOST_VDPA_SET_VRING_ENABLE).
> > > >
> > > hi Stefano
> > > I initially used it as a boolean, but during the code review, the
> > > maintainers considered it was unsuitable for the bool use as the
> >
> > I see, indeed I found only 1 case of bool:
> >
> > include/uapi/misc/xilinx_sdfec.h:#define XSDFEC_SET_BYPASS
> > _IOW(XSDFEC_MAGIC, 9, bool)
> >
> > > interface in ioctl (I think in version 3 ?). So I changed it to u8,
> > > then will check if this is 1/0 in ioctl and the u8 should be
> > > sufficient for us to use
> >
> > Okay, if Michael and Jason are happy with it, it's fine.
> > It just seemed strange to me that for other IOCTLs we use int or
> > unsigned int when we need a boolean instead of a sized type.
>
> I only found VHOST_VSOCK_SET_RUNNING. which other ioctls?

VHOST_VDPA_SET_VRING_ENABLE use the `struct vhost_vring_state` where we 
use the `unsigned int num` field as boolean, but I see that this is a 
special case where we re-use the same struct for multiple ioctls.

Thanks,
Stefano

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

[Stefano Garzarella]

On Sun, Feb 23, 2025 at 11:36:20PM +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>
>---
> drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
> include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index d8c0ea118bb1..45d8f5c5bca9 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -1133,7 +1133,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.
>@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
>--- a/include/uapi/linux/vhost.h
>+++ b/include/uapi/linux/vhost.h
>@@ -235,4 +235,22 @@
>  */
> #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
>+ *
>+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
>+ *
>+ * When inherit_owner is set to 1:
>+ *   - The VHOST worker threads inherit its values/checks from
>+ *     the thread that owns the VHOST device, The vhost threads will
>+ *     be counted in the nproc rlimits.

Should we document that this (inherit_owner = 1) is the default, so if 
the user doesn't call VHOST_FORK_FROM_OWNER, this mode will be 
automatically selected?

Thanks,
Stefano

>+ *
>+ * 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
>+
> #endif
>-- 
>2.45.0
>

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

[Cindy Lu]

On Wed, Feb 26, 2025 at 5:05 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Sun, Feb 23, 2025 at 11:36:20PM +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>
> >---
> > drivers/vhost/vhost.c      | 24 ++++++++++++++++++++++--
> > include/uapi/linux/vhost.h | 18 ++++++++++++++++++
> > 2 files changed, 40 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index d8c0ea118bb1..45d8f5c5bca9 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -1133,7 +1133,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.
> >@@ -2278,15 +2278,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_FORK_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..8f558b433536 100644
> >--- a/include/uapi/linux/vhost.h
> >+++ b/include/uapi/linux/vhost.h
> >@@ -235,4 +235,22 @@
> >  */
> > #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
> >+ *
> >+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> >+ *
> >+ * When inherit_owner is set to 1:
> >+ *   - The VHOST worker threads inherit its values/checks from
> >+ *     the thread that owns the VHOST device, The vhost threads will
> >+ *     be counted in the nproc rlimits.
>
> Should we document that this (inherit_owner = 1) is the default, so if
> the user doesn't call VHOST_FORK_FROM_OWNER, this mode will be
> automatically selected?
>
> Thanks,
> Stefano
sure, I will add this
thanks
cindy
> >+ *
> >+ * 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_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> >+
> > #endif
> >--
> >2.45.0
> >
>

[PATCH v6 6/6] 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 45d8f5c5bca9..26da561c6685 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1017,6 +1017,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 v6 6/6] vhost: Add check for inherit_owner status

[Jason Wang]

On Sun, Feb 23, 2025 at 11:41 PM Cindy Lu <lulu@redhat.com> 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>
> ---

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

Thanks

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

[Lei Yang]

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

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


On Sun, Feb 23, 2025 at 11:41 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
>
> 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: Reintroduce vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: introduce worker ops to support multiple thread models
>   vhost: Add new UAPI to support change to task mode
>   vhost: Add check for inherit_owner status
>
>  drivers/vhost/vhost.c      | 227 +++++++++++++++++++++++++++++++++----
>  drivers/vhost/vhost.h      |  13 +++
>  include/uapi/linux/vhost.h |  18 +++
>  3 files changed, 234 insertions(+), 24 deletions(-)
>
> --
> 2.45.0
>
>

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

[Michael S. Tsirkin]

On Sun, Feb 23, 2025 at 11:36:15PM +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.  


This looks good to me.

Pls address Jason's comments and add a Kconfig knob, and I will apply/

> 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  
>  
> 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: Reintroduce vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: introduce worker ops to support multiple thread models
>   vhost: Add new UAPI to support change to task mode
>   vhost: Add check for inherit_owner status
> 
>  drivers/vhost/vhost.c      | 227 +++++++++++++++++++++++++++++++++----
>  drivers/vhost/vhost.h      |  13 +++
>  include/uapi/linux/vhost.h |  18 +++
>  3 files changed, 234 insertions(+), 24 deletions(-)
> 
> -- 
> 2.45.0

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

[Cindy Lu]

On Tue, Feb 25, 2025 at 5:43 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Feb 23, 2025 at 11:36:15PM +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.
>
>
> This looks good to me.
>
> Pls address Jason's comments and add a Kconfig knob, and I will apply/
>
Thank you MST, I will add this
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.
> >
> > 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
> >
> > 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: Reintroduce vhost_worker to support kthread
> >   vhost: Add the cgroup related function
> >   vhost: introduce worker ops to support multiple thread models
> >   vhost: Add new UAPI to support change to task mode
> >   vhost: Add check for inherit_owner status
> >
> >  drivers/vhost/vhost.c      | 227 +++++++++++++++++++++++++++++++++----
> >  drivers/vhost/vhost.h      |  13 +++
> >  include/uapi/linux/vhost.h |  18 +++
> >  3 files changed, 234 insertions(+), 24 deletions(-)
> >
> > --
> > 2.45.0
>