* [PATCH v7 1/8] vhost: Add a new parameter in vhost_dev to allow user select kthread
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-02 14:32 ` [PATCH v7 2/8] vhost: Reintroduce vhost_worker to support kthread Cindy Lu
` (8 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
The vhost now uses vhost_task and workers as a child of the owner thread.
While this aligns with containerization principles,it confuses some legacy
userspace app, Therefore, we are reintroducing kthread API support.
Introduce a new parameter to enable users to choose between
kthread and task mode.
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/vhost.c | 1 +
drivers/vhost/vhost.h | 9 +++++++++
2 files changed, 10 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 63612faeab72..250dc43f1786 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -552,6 +552,7 @@ void vhost_dev_init(struct vhost_dev *dev,
dev->byte_weight = byte_weight;
dev->use_worker = use_worker;
dev->msg_handler = msg_handler;
+ dev->inherit_owner = true;
init_waitqueue_head(&dev->wait);
INIT_LIST_HEAD(&dev->read_list);
INIT_LIST_HEAD(&dev->pending_list);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..19bb94922a0e 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -176,6 +176,15 @@ struct vhost_dev {
int byte_weight;
struct xarray worker_xa;
bool use_worker;
+ /*
+ * If inherit_owner is true we use vhost_tasks to create
+ * the worker so all settings/limits like cgroups, NPROC,
+ * scheduler, etc are inherited from the owner. If false,
+ * we use kthreads and only attach to the same cgroups
+ * as the owner for compat with older kernels.
+ * here we use true as default value
+ */
+ bool inherit_owner;
int (*msg_handler)(struct vhost_dev *dev, u32 asid,
struct vhost_iotlb_msg *msg);
};
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v7 2/8] vhost: Reintroduce vhost_worker to support kthread
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
2025-03-02 14:32 ` [PATCH v7 1/8] vhost: Add a new parameter in vhost_dev to allow user select kthread Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-02 14:32 ` [PATCH v7 3/8] vhost: Add the cgroup related function Cindy Lu
` (7 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
Add the previously removed function vhost_worker() back
to support the kthread and rename it to vhost_run_work_kthread_list.
The old function vhost_worker was change to support task in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
change to xarray in
commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray")
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/vhost.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 250dc43f1786..9500e85b42ce 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -388,6 +388,44 @@ static void vhost_vq_reset(struct vhost_dev *dev,
__vhost_vq_meta_reset(vq);
}
+static int vhost_run_work_kthread_list(void *data)
+{
+ struct vhost_worker *worker = data;
+ struct vhost_work *work, *work_next;
+ struct vhost_dev *dev = worker->dev;
+ struct llist_node *node;
+
+ kthread_use_mm(dev->mm);
+
+ for (;;) {
+ /* mb paired w/ kthread_stop */
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ if (kthread_should_stop()) {
+ __set_current_state(TASK_RUNNING);
+ break;
+ }
+ node = llist_del_all(&worker->work_list);
+ if (!node)
+ schedule();
+
+ node = llist_reverse_order(node);
+ /* make sure flag is seen after deletion */
+ smp_wmb();
+ llist_for_each_entry_safe(work, work_next, node, node) {
+ clear_bit(VHOST_WORK_QUEUED, &work->flags);
+ __set_current_state(TASK_RUNNING);
+ kcov_remote_start_common(worker->kcov_handle);
+ work->fn(work);
+ kcov_remote_stop();
+ cond_resched();
+ }
+ }
+ kthread_unuse_mm(dev->mm);
+
+ return 0;
+}
+
static bool vhost_run_work_list(void *data)
{
struct vhost_worker *worker = data;
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v7 3/8] vhost: Add the cgroup related function
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
2025-03-02 14:32 ` [PATCH v7 1/8] vhost: Add a new parameter in vhost_dev to allow user select kthread Cindy Lu
2025-03-02 14:32 ` [PATCH v7 2/8] vhost: Reintroduce vhost_worker to support kthread Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-02 14:32 ` [PATCH v7 4/8] vhost: Introduce vhost_worker_ops in vhost_worker Cindy Lu
` (6 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
Add back the previously removed cgroup function to support the kthread
The biggest change for this part is in vhost_attach_cgroups() and
vhost_attach_task_to_cgroups().
The old function was remove in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/vhost.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9500e85b42ce..20571bd6f7bd 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -22,6 +22,7 @@
#include <linux/slab.h>
#include <linux/vmalloc.h>
#include <linux/kthread.h>
+#include <linux/cgroup.h>
#include <linux/module.h>
#include <linux/sort.h>
#include <linux/sched/mm.h>
@@ -620,6 +621,46 @@ long vhost_dev_check_owner(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
+struct vhost_attach_cgroups_struct {
+ struct vhost_work work;
+ struct task_struct *owner;
+ int ret;
+};
+
+static void vhost_attach_cgroups_work(struct vhost_work *work)
+{
+ struct vhost_attach_cgroups_struct *s;
+
+ s = container_of(work, struct vhost_attach_cgroups_struct, work);
+ s->ret = cgroup_attach_task_all(s->owner, current);
+}
+
+static int vhost_attach_task_to_cgroups(struct vhost_worker *worker)
+{
+ struct vhost_attach_cgroups_struct attach;
+ int saved_cnt;
+
+ attach.owner = current;
+
+ vhost_work_init(&attach.work, vhost_attach_cgroups_work);
+ vhost_worker_queue(worker, &attach.work);
+
+ mutex_lock(&worker->mutex);
+
+ /*
+ * Bypass attachment_cnt check in __vhost_worker_flush:
+ * Temporarily change it to INT_MAX to bypass the check
+ */
+ saved_cnt = worker->attachment_cnt;
+ worker->attachment_cnt = INT_MAX;
+ __vhost_worker_flush(worker);
+ worker->attachment_cnt = saved_cnt;
+
+ mutex_unlock(&worker->mutex);
+
+ return attach.ret;
+}
+
/* Caller should have device mutex */
bool vhost_dev_has_owner(struct vhost_dev *dev)
{
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v7 4/8] vhost: Introduce vhost_worker_ops in vhost_worker
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
` (2 preceding siblings ...)
2025-03-02 14:32 ` [PATCH v7 3/8] vhost: Add the cgroup related function Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-02 14:32 ` [PATCH v7 5/8] vhost: Reintroduce kthread mode support in vhost Cindy Lu
` (5 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
Abstract vhost worker operations (create/stop/wakeup) into an ops
structure to prepare for kthread mode support.
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/vhost.c | 63 ++++++++++++++++++++++++++++++-------------
drivers/vhost/vhost.h | 11 ++++++++
2 files changed, 56 insertions(+), 18 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 20571bd6f7bd..c162ad772f8f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -243,7 +243,7 @@ static void vhost_worker_queue(struct vhost_worker *worker,
* test_and_set_bit() implies a memory barrier.
*/
llist_add(&work->node, &worker->work_list);
- vhost_task_wake(worker->vtsk);
+ worker->ops->wakeup(worker);
}
}
@@ -706,7 +706,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
WARN_ON(!llist_empty(&worker->work_list));
xa_erase(&dev->worker_xa, worker->id);
- vhost_task_stop(worker->vtsk);
+ worker->ops->stop(worker);
kfree(worker);
}
@@ -729,42 +729,69 @@ static void vhost_workers_free(struct vhost_dev *dev)
xa_destroy(&dev->worker_xa);
}
+static void vhost_task_wakeup(struct vhost_worker *worker)
+{
+ return vhost_task_wake(worker->vtsk);
+}
+
+static void vhost_task_do_stop(struct vhost_worker *worker)
+{
+ return vhost_task_stop(worker->vtsk);
+}
+
+static int vhost_task_worker_create(struct vhost_worker *worker,
+ struct vhost_dev *dev, const char *name)
+{
+ struct vhost_task *vtsk;
+ u32 id;
+ int ret;
+
+ vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
+ worker, name);
+ if (IS_ERR(vtsk))
+ return PTR_ERR(vtsk);
+
+ worker->vtsk = vtsk;
+ vhost_task_start(vtsk);
+ ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+ if (ret < 0) {
+ vhost_task_do_stop(worker);
+ return ret;
+ }
+ worker->id = id;
+ return 0;
+}
+
+static const struct vhost_worker_ops vhost_task_ops = {
+ .create = vhost_task_worker_create,
+ .stop = vhost_task_do_stop,
+ .wakeup = vhost_task_wakeup,
+};
+
static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
{
struct vhost_worker *worker;
- struct vhost_task *vtsk;
char name[TASK_COMM_LEN];
int ret;
- u32 id;
+ const struct vhost_worker_ops *ops = &vhost_task_ops;
worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
if (!worker)
return NULL;
worker->dev = dev;
+ worker->ops = ops;
snprintf(name, sizeof(name), "vhost-%d", current->pid);
- vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
- worker, name);
- if (IS_ERR(vtsk))
- goto free_worker;
-
mutex_init(&worker->mutex);
init_llist_head(&worker->work_list);
worker->kcov_handle = kcov_common_handle();
- worker->vtsk = vtsk;
-
- vhost_task_start(vtsk);
-
- ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+ ret = ops->create(worker, dev, name);
if (ret < 0)
- goto stop_worker;
- worker->id = id;
+ goto free_worker;
return worker;
-stop_worker:
- vhost_task_stop(vtsk);
free_worker:
kfree(worker);
return NULL;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 19bb94922a0e..98895e299efa 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -26,6 +26,16 @@ struct vhost_work {
unsigned long flags;
};
+struct vhost_worker;
+struct vhost_dev;
+
+struct vhost_worker_ops {
+ int (*create)(struct vhost_worker *worker, struct vhost_dev *dev,
+ const char *name);
+ void (*stop)(struct vhost_worker *worker);
+ void (*wakeup)(struct vhost_worker *worker);
+};
+
struct vhost_worker {
struct vhost_task *vtsk;
struct vhost_dev *dev;
@@ -36,6 +46,7 @@ struct vhost_worker {
u32 id;
int attachment_cnt;
bool killed;
+ const struct vhost_worker_ops *ops;
};
/* Poll a file (eventfd or socket) */
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v7 5/8] vhost: Reintroduce kthread mode support in vhost
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
` (3 preceding siblings ...)
2025-03-02 14:32 ` [PATCH v7 4/8] vhost: Introduce vhost_worker_ops in vhost_worker Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-02 14:32 ` [PATCH v7 6/8] vhost: uapi to control task mode (owner vs kthread) Cindy Lu
` (4 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
This commit restores the previously removed functions kthread
wake/stop/create, and use ops structure vhost_worker_ops to
manage worker wakeup, stop and creation. The function
vhost_worker_create initializes these ops pointers based on
the value of inherit_owner
The old function was remove in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/vhost.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
drivers/vhost/vhost.h | 1 +
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c162ad772f8f..be97028a8baf 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -734,11 +734,21 @@ static void vhost_task_wakeup(struct vhost_worker *worker)
return vhost_task_wake(worker->vtsk);
}
+static void vhost_kthread_wakeup(struct vhost_worker *worker)
+{
+ wake_up_process(worker->kthread_task);
+}
+
static void vhost_task_do_stop(struct vhost_worker *worker)
{
return vhost_task_stop(worker->vtsk);
}
+static void vhost_kthread_do_stop(struct vhost_worker *worker)
+{
+ kthread_stop(worker->kthread_task);
+}
+
static int vhost_task_worker_create(struct vhost_worker *worker,
struct vhost_dev *dev, const char *name)
{
@@ -762,6 +772,41 @@ static int vhost_task_worker_create(struct vhost_worker *worker,
return 0;
}
+static int vhost_kthread_worker_create(struct vhost_worker *worker,
+ struct vhost_dev *dev, const char *name)
+{
+ struct task_struct *task;
+ u32 id;
+ int ret;
+
+ task = kthread_create(vhost_run_work_kthread_list, worker, "%s", name);
+ if (IS_ERR(task))
+ return PTR_ERR(task);
+
+ worker->kthread_task = task;
+ wake_up_process(task);
+ ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+ if (ret < 0)
+ goto stop_worker;
+
+ ret = vhost_attach_task_to_cgroups(worker);
+ if (ret)
+ goto stop_worker;
+
+ worker->id = id;
+ return 0;
+
+stop_worker:
+ vhost_kthread_do_stop(worker);
+ return ret;
+}
+
+static const struct vhost_worker_ops kthread_ops = {
+ .create = vhost_kthread_worker_create,
+ .stop = vhost_kthread_do_stop,
+ .wakeup = vhost_kthread_wakeup,
+};
+
static const struct vhost_worker_ops vhost_task_ops = {
.create = vhost_task_worker_create,
.stop = vhost_task_do_stop,
@@ -773,7 +818,8 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
struct vhost_worker *worker;
char name[TASK_COMM_LEN];
int ret;
- const struct vhost_worker_ops *ops = &vhost_task_ops;
+ const struct vhost_worker_ops *ops =
+ dev->inherit_owner ? &vhost_task_ops : &kthread_ops;
worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
if (!worker)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 98895e299efa..af4b2f7d3b91 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -37,6 +37,7 @@ struct vhost_worker_ops {
};
struct vhost_worker {
+ struct task_struct *kthread_task;
struct vhost_task *vtsk;
struct vhost_dev *dev;
/* Used to serialize device wide flushing with worker swapping. */
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v7 6/8] vhost: uapi to control task mode (owner vs kthread)
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
` (4 preceding siblings ...)
2025-03-02 14:32 ` [PATCH v7 5/8] vhost: Reintroduce kthread mode support in vhost Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-03 8:58 ` Stefano Garzarella
2025-03-02 14:32 ` [PATCH v7 7/8] vhost: Add check for inherit_owner status Cindy Lu
` (3 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
Add a new UAPI to configure the vhost device to use the kthread mode
The userspace application can use IOCTL VHOST_FORK_FROM_OWNER
to choose between owner and kthread mode if necessary
This setting must be applied before VHOST_SET_OWNER, as the worker
will be created in the VHOST_SET_OWNER function
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/vhost.c | 22 ++++++++++++++++++++--
include/uapi/linux/vhost.h | 15 +++++++++++++++
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index be97028a8baf..ff930c2e5b78 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1134,7 +1134,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
int i;
vhost_dev_cleanup(dev);
-
+ dev->inherit_owner = true;
dev->umem = umem;
/* We don't need VQ locks below since vhost_dev_cleanup makes sure
* VQs aren't running.
@@ -2287,7 +2287,25 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
r = vhost_dev_set_owner(d);
goto done;
}
-
+ if (ioctl == VHOST_FORK_FROM_OWNER) {
+ u8 inherit_owner;
+ /*inherit_owner can only be modified before owner is set*/
+ if (vhost_dev_has_owner(d)) {
+ r = -EBUSY;
+ goto done;
+ }
+ if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
+ r = -EFAULT;
+ goto done;
+ }
+ if (inherit_owner > 1) {
+ r = -EINVAL;
+ goto done;
+ }
+ d->inherit_owner = (bool)inherit_owner;
+ r = 0;
+ goto done;
+ }
/* You must be the owner to do anything else */
r = vhost_dev_check_owner(d);
if (r)
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index b95dd84eef2d..547b4fa4c3bd 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,19 @@
*/
#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(default value):
+ * - Vhost will create tasks similar to processes forked from the owner,
+ * inheriting all of the owner's attributes..
+ *
+ * When inherit_owner is set to 0:
+ * - Vhost will create tasks as kernel thread
+ */
+#define VHOST_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
+
#endif
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v7 6/8] vhost: uapi to control task mode (owner vs kthread)
2025-03-02 14:32 ` [PATCH v7 6/8] vhost: uapi to control task mode (owner vs kthread) Cindy Lu
@ 2025-03-03 8:58 ` Stefano Garzarella
2025-03-28 5:43 ` Cindy Lu
0 siblings, 1 reply; 22+ messages in thread
From: Stefano Garzarella @ 2025-03-03 8:58 UTC (permalink / raw)
To: Cindy Lu
Cc: jasowang, mst, michael.christie, linux-kernel, virtualization,
netdev
On Sun, Mar 02, 2025 at 10:32:08PM +0800, Cindy Lu wrote:
>Add a new UAPI to configure the vhost device to use the kthread mode
>The userspace application can use IOCTL VHOST_FORK_FROM_OWNER
>to choose between owner and kthread mode if necessary
>This setting must be applied before VHOST_SET_OWNER, as the worker
>will be created in the VHOST_SET_OWNER function
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 22 ++++++++++++++++++++--
> include/uapi/linux/vhost.h | 15 +++++++++++++++
> 2 files changed, 35 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index be97028a8baf..ff930c2e5b78 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -1134,7 +1134,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
> int i;
>
> vhost_dev_cleanup(dev);
>-
>+ dev->inherit_owner = true;
> dev->umem = umem;
> /* We don't need VQ locks below since vhost_dev_cleanup makes sure
> * VQs aren't running.
>@@ -2287,7 +2287,25 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> r = vhost_dev_set_owner(d);
> goto done;
> }
>-
>+ if (ioctl == VHOST_FORK_FROM_OWNER) {
>+ u8 inherit_owner;
>+ /*inherit_owner can only be modified before owner is set*/
>+ if (vhost_dev_has_owner(d)) {
>+ r = -EBUSY;
>+ goto done;
>+ }
>+ if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
>+ r = -EFAULT;
>+ goto done;
>+ }
>+ if (inherit_owner > 1) {
>+ r = -EINVAL;
>+ goto done;
>+ }
>+ d->inherit_owner = (bool)inherit_owner;
>+ r = 0;
>+ goto done;
>+ }
> /* You must be the owner to do anything else */
> r = vhost_dev_check_owner(d);
> if (r)
>diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>index b95dd84eef2d..547b4fa4c3bd 100644
>--- a/include/uapi/linux/vhost.h
>+++ b/include/uapi/linux/vhost.h
>@@ -235,4 +235,19 @@
> */
> #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
Should we mention that this IOCTL must be called before VHOST_SET_OWNER?
>+ *
>+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
>+ *
>+ * When inherit_owner is set to 1(default value):
>+ * - Vhost will create tasks similar to processes forked from the owner,
>+ * inheriting all of the owner's attributes..
^
nit: there 2 points here
>+ *
>+ * When inherit_owner is set to 0:
>+ * - Vhost will create tasks as kernel thread
>+ */
>+#define VHOST_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
>+
> #endif
>--
>2.45.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 6/8] vhost: uapi to control task mode (owner vs kthread)
2025-03-03 8:58 ` Stefano Garzarella
@ 2025-03-28 5:43 ` Cindy Lu
0 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-28 5:43 UTC (permalink / raw)
To: Stefano Garzarella
Cc: jasowang, mst, michael.christie, linux-kernel, virtualization,
netdev
On Mon, Mar 3, 2025 at 4:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Sun, Mar 02, 2025 at 10:32:08PM +0800, Cindy Lu wrote:
> >Add a new UAPI to configure the vhost device to use the kthread mode
> >The userspace application can use IOCTL VHOST_FORK_FROM_OWNER
> >to choose between owner and kthread mode if necessary
> >This setting must be applied before VHOST_SET_OWNER, as the worker
> >will be created in the VHOST_SET_OWNER function
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 22 ++++++++++++++++++++--
> > include/uapi/linux/vhost.h | 15 +++++++++++++++
> > 2 files changed, 35 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index be97028a8baf..ff930c2e5b78 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -1134,7 +1134,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
> > int i;
> >
> > vhost_dev_cleanup(dev);
> >-
> >+ dev->inherit_owner = true;
> > dev->umem = umem;
> > /* We don't need VQ locks below since vhost_dev_cleanup makes sure
> > * VQs aren't running.
> >@@ -2287,7 +2287,25 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > r = vhost_dev_set_owner(d);
> > goto done;
> > }
> >-
> >+ if (ioctl == VHOST_FORK_FROM_OWNER) {
> >+ u8 inherit_owner;
> >+ /*inherit_owner can only be modified before owner is set*/
> >+ if (vhost_dev_has_owner(d)) {
> >+ r = -EBUSY;
> >+ goto done;
> >+ }
> >+ if (copy_from_user(&inherit_owner, argp, sizeof(u8))) {
> >+ r = -EFAULT;
> >+ goto done;
> >+ }
> >+ if (inherit_owner > 1) {
> >+ r = -EINVAL;
> >+ goto done;
> >+ }
> >+ d->inherit_owner = (bool)inherit_owner;
> >+ r = 0;
> >+ goto done;
> >+ }
> > /* You must be the owner to do anything else */
> > r = vhost_dev_check_owner(d);
> > if (r)
> >diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> >index b95dd84eef2d..547b4fa4c3bd 100644
> >--- a/include/uapi/linux/vhost.h
> >+++ b/include/uapi/linux/vhost.h
> >@@ -235,4 +235,19 @@
> > */
> > #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
>
> Should we mention that this IOCTL must be called before VHOST_SET_OWNER?
>
> >+ *
> >+ * @param inherit_owner: An 8-bit value that determines the vhost thread mode
> >+ *
> >+ * When inherit_owner is set to 1(default value):
> >+ * - Vhost will create tasks similar to processes forked from the owner,
> >+ * inheriting all of the owner's attributes..
> ^
> nit: there 2 points here
>
Thanks Stefano, I will change this
Thanks
cindy
> >+ *
> >+ * When inherit_owner is set to 0:
> >+ * - Vhost will create tasks as kernel thread
> >+ */
> >+#define VHOST_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> >+
> > #endif
> >--
> >2.45.0
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 7/8] vhost: Add check for inherit_owner status
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
` (5 preceding siblings ...)
2025-03-02 14:32 ` [PATCH v7 6/8] vhost: uapi to control task mode (owner vs kthread) Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-02 14:32 ` [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER Cindy Lu
` (2 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
The VHOST_NEW_WORKER requires the inherit_owner
setting to be true. So we need to add a check for this.
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/vhost.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ff930c2e5b78..fb0c7fb43f78 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1018,6 +1018,13 @@ long vhost_worker_ioctl(struct vhost_dev *dev, unsigned int ioctl,
switch (ioctl) {
/* dev worker ioctls */
case VHOST_NEW_WORKER:
+ /*
+ * vhost_tasks will account for worker threads under the parent's
+ * NPROC value but kthreads do not. To avoid userspace overflowing
+ * the system with worker threads inherit_owner must be true.
+ */
+ if (!dev->inherit_owner)
+ return -EFAULT;
ret = vhost_new_worker(dev, &state);
if (!ret && copy_to_user(argp, &state, sizeof(state)))
ret = -EFAULT;
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
` (6 preceding siblings ...)
2025-03-02 14:32 ` [PATCH v7 7/8] vhost: Add check for inherit_owner status Cindy Lu
@ 2025-03-02 14:32 ` Cindy Lu
2025-03-03 5:52 ` Jason Wang
2025-03-03 4:25 ` [PATCH v7 0/8] vhost: Add support of kthread API Lei Yang
2025-03-21 19:36 ` Michael S. Tsirkin
9 siblings, 1 reply; 22+ messages in thread
From: Cindy Lu @ 2025-03-02 14:32 UTC (permalink / raw)
To: lulu, jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
is disabled, and any attempt to use it will result in failure.
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
drivers/vhost/Kconfig | 15 +++++++++++++++
drivers/vhost/vhost.c | 11 +++++++++++
2 files changed, 26 insertions(+)
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index b455d9ab6f3d..e5b9dcbf31b6 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
If unsure, say "N".
endif
+
+config VHOST_ENABLE_FORK_OWNER_IOCTL
+ bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
+ default n
+ help
+ This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
+ userspace applications to modify the thread mode for vhost devices.
+
+ By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
+ meaning the ioctl is disabled and any operation using this ioctl
+ will fail.
+ When the configuration is enabled (y), the ioctl becomes
+ available, allowing users to set the mode if needed.
+
+ If unsure, say "N".
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index fb0c7fb43f78..09e5e44dc516 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
r = vhost_dev_set_owner(d);
goto done;
}
+
+#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
if (ioctl == VHOST_FORK_FROM_OWNER) {
u8 inherit_owner;
/*inherit_owner can only be modified before owner is set*/
@@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
r = 0;
goto done;
}
+
+#else
+ if (ioctl == VHOST_FORK_FROM_OWNER) {
+ /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
+ r = -ENOTTY;
+ goto done;
+ }
+#endif
+
/* You must be the owner to do anything else */
r = vhost_dev_check_owner(d);
if (r)
--
2.45.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-02 14:32 ` [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER Cindy Lu
@ 2025-03-03 5:52 ` Jason Wang
2025-03-03 9:12 ` Stefano Garzarella
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Jason Wang @ 2025-03-03 5:52 UTC (permalink / raw)
To: Cindy Lu
Cc: mst, michael.christie, sgarzare, linux-kernel, virtualization,
netdev
On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> is disabled, and any attempt to use it will result in failure.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
> drivers/vhost/Kconfig | 15 +++++++++++++++
> drivers/vhost/vhost.c | 11 +++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index b455d9ab6f3d..e5b9dcbf31b6 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> If unsure, say "N".
>
> endif
> +
> +config VHOST_ENABLE_FORK_OWNER_IOCTL
> + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> + default n
> + help
> + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> + userspace applications to modify the thread mode for vhost devices.
> +
> + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> + meaning the ioctl is disabled and any operation using this ioctl
> + will fail.
> + When the configuration is enabled (y), the ioctl becomes
> + available, allowing users to set the mode if needed.
> +
> + If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fb0c7fb43f78..09e5e44dc516 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> r = vhost_dev_set_owner(d);
> goto done;
> }
> +
> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> if (ioctl == VHOST_FORK_FROM_OWNER) {
> u8 inherit_owner;
> /*inherit_owner can only be modified before owner is set*/
> @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> r = 0;
> goto done;
> }
> +
> +#else
> + if (ioctl == VHOST_FORK_FROM_OWNER) {
> + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
> + r = -ENOTTY;
> + goto done;
> + }
> +#endif
> +
> /* You must be the owner to do anything else */
> r = vhost_dev_check_owner(d);
> if (r)
> --
> 2.45.0
Do we need to change the default value of the inhert_owner? For example:
#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
inherit_owner = false;
#else
inherit_onwer = true;
#endif
?
Other patches look good to me.
Thanks
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-03 5:52 ` Jason Wang
@ 2025-03-03 9:12 ` Stefano Garzarella
2025-03-28 5:47 ` Cindy Lu
2025-03-03 17:33 ` Michael S. Tsirkin
2025-03-21 19:39 ` Michael S. Tsirkin
2 siblings, 1 reply; 22+ messages in thread
From: Stefano Garzarella @ 2025-03-03 9:12 UTC (permalink / raw)
To: Jason Wang
Cc: Cindy Lu, mst, michael.christie, linux-kernel, virtualization,
netdev
On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
>On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
>>
>> Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
>> to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
>> When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
>> is disabled, and any attempt to use it will result in failure.
>>
>> Signed-off-by: Cindy Lu <lulu@redhat.com>
>> ---
>> drivers/vhost/Kconfig | 15 +++++++++++++++
>> drivers/vhost/vhost.c | 11 +++++++++++
>> 2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
>> index b455d9ab6f3d..e5b9dcbf31b6 100644
>> --- a/drivers/vhost/Kconfig
>> +++ b/drivers/vhost/Kconfig
>> @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
>> If unsure, say "N".
>>
>> endif
>> +
>> +config VHOST_ENABLE_FORK_OWNER_IOCTL
>> + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
>> + default n
>> + help
>> + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
>> + userspace applications to modify the thread mode for vhost devices.
>> +
>> + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
>> + meaning the ioctl is disabled and any operation using this ioctl
>> + will fail.
>> + When the configuration is enabled (y), the ioctl becomes
>> + available, allowing users to set the mode if needed.
>> +
>> + If unsure, say "N".
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index fb0c7fb43f78..09e5e44dc516 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>> r = vhost_dev_set_owner(d);
>> goto done;
>> }
>> +
>> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
>> if (ioctl == VHOST_FORK_FROM_OWNER) {
>> u8 inherit_owner;
>> /*inherit_owner can only be modified before owner is set*/
>> @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>> r = 0;
>> goto done;
>> }
>> +
nit: this empyt line is not needed
>> +#else
>> + if (ioctl == VHOST_FORK_FROM_OWNER) {
>> + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
>> + r = -ENOTTY;
>> + goto done;
>> + }
>> +#endif
>> +
>> /* You must be the owner to do anything else */
>> r = vhost_dev_check_owner(d);
>> if (r)
>> --
>> 2.45.0
>
>Do we need to change the default value of the inhert_owner? For example:
>
>#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
>inherit_owner = false;
>#else
>inherit_onwer = true;
>#endif
>
>?
I'm not sure about this honestly, the user space has no way to figure
out the default value and still has to do the IOCTL.
So IMHO better to have a default value that is independent of the kernel
configuration and consistent with the current behavior.
Thanks,
Stefano
>
>Other patches look good to me.
>
>Thanks
>
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-03 9:12 ` Stefano Garzarella
@ 2025-03-28 5:47 ` Cindy Lu
0 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-28 5:47 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jason Wang, mst, michael.christie, linux-kernel, virtualization,
netdev
On Mon, Mar 3, 2025 at 5:12 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> >On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
> >>
> >> Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> >> to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> >> When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> >> is disabled, and any attempt to use it will result in failure.
> >>
> >> Signed-off-by: Cindy Lu <lulu@redhat.com>
> >> ---
> >> drivers/vhost/Kconfig | 15 +++++++++++++++
> >> drivers/vhost/vhost.c | 11 +++++++++++
> >> 2 files changed, 26 insertions(+)
> >>
> >> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> >> index b455d9ab6f3d..e5b9dcbf31b6 100644
> >> --- a/drivers/vhost/Kconfig
> >> +++ b/drivers/vhost/Kconfig
> >> @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> >> If unsure, say "N".
> >>
> >> endif
> >> +
> >> +config VHOST_ENABLE_FORK_OWNER_IOCTL
> >> + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> >> + default n
> >> + help
> >> + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> >> + userspace applications to modify the thread mode for vhost devices.
> >> +
> >> + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> >> + meaning the ioctl is disabled and any operation using this ioctl
> >> + will fail.
> >> + When the configuration is enabled (y), the ioctl becomes
> >> + available, allowing users to set the mode if needed.
> >> +
> >> + If unsure, say "N".
> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >> index fb0c7fb43f78..09e5e44dc516 100644
> >> --- a/drivers/vhost/vhost.c
> >> +++ b/drivers/vhost/vhost.c
> >> @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >> r = vhost_dev_set_owner(d);
> >> goto done;
> >> }
> >> +
> >> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> >> if (ioctl == VHOST_FORK_FROM_OWNER) {
> >> u8 inherit_owner;
> >> /*inherit_owner can only be modified before owner is set*/
> >> @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >> r = 0;
> >> goto done;
> >> }
> >> +
>
> nit: this empyt line is not needed
>
sure , will fix this
Thanks
Cindy
> >> +#else
> >> + if (ioctl == VHOST_FORK_FROM_OWNER) {
> >> + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
> >> + r = -ENOTTY;
> >> + goto done;
> >> + }
> >> +#endif
> >> +
> >> /* You must be the owner to do anything else */
> >> r = vhost_dev_check_owner(d);
> >> if (r)
> >> --
> >> 2.45.0
> >
> >Do we need to change the default value of the inhert_owner? For example:
> >
> >#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> >inherit_owner = false;
> >#else
> >inherit_onwer = true;
> >#endif
> >
> >?
>
> I'm not sure about this honestly, the user space has no way to figure
> out the default value and still has to do the IOCTL.
> So IMHO better to have a default value that is independent of the kernel
> configuration and consistent with the current behavior.
>
> Thanks,
> Stefano
>
> >
> >Other patches look good to me.
> >
> >Thanks
> >
> >>
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-03 5:52 ` Jason Wang
2025-03-03 9:12 ` Stefano Garzarella
@ 2025-03-03 17:33 ` Michael S. Tsirkin
2025-03-10 4:54 ` Jason Wang
2025-03-28 8:24 ` Cindy Lu
2025-03-21 19:39 ` Michael S. Tsirkin
2 siblings, 2 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2025-03-03 17:33 UTC (permalink / raw)
To: Jason Wang
Cc: Cindy Lu, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > is disabled, and any attempt to use it will result in failure.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> > drivers/vhost/Kconfig | 15 +++++++++++++++
> > drivers/vhost/vhost.c | 11 +++++++++++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > If unsure, say "N".
> >
> > endif
> > +
> > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > + default n
> > + help
> > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > + userspace applications to modify the thread mode for vhost devices.
> > +
> > + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > + meaning the ioctl is disabled and any operation using this ioctl
> > + will fail.
> > + When the configuration is enabled (y), the ioctl becomes
> > + available, allowing users to set the mode if needed.
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index fb0c7fb43f78..09e5e44dc516 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > r = vhost_dev_set_owner(d);
> > goto done;
> > }
> > +
> > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > u8 inherit_owner;
> > /*inherit_owner can only be modified before owner is set*/
> > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > r = 0;
> > goto done;
> > }
> > +
> > +#else
> > + if (ioctl == VHOST_FORK_FROM_OWNER) {
> > + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
> > + r = -ENOTTY;
> > + goto done;
> > + }
why do we need this? won't it fail as any other unsupported ioctl?
> > +#endif
> > +
> > /* You must be the owner to do anything else */
> > r = vhost_dev_check_owner(d);
> > if (r)
> > --
> > 2.45.0
>
> Do we need to change the default value of the inhert_owner? For example:
>
> #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> inherit_owner = false;
> #else
> inherit_onwer = true;
> #endif
>
> ?
I feel it is best to keep the default consistent.
All the kconfig should do, is block the ioctl.
> Other patches look good to me.
>
> Thanks
>
> >
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-03 17:33 ` Michael S. Tsirkin
@ 2025-03-10 4:54 ` Jason Wang
2025-03-20 9:38 ` Cindy Lu
2025-03-28 8:24 ` Cindy Lu
1 sibling, 1 reply; 22+ messages in thread
From: Jason Wang @ 2025-03-10 4:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cindy Lu, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
On Tue, Mar 4, 2025 at 1:33 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> > On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > > is disabled, and any attempt to use it will result in failure.
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > > drivers/vhost/Kconfig | 15 +++++++++++++++
> > > drivers/vhost/vhost.c | 11 +++++++++++
> > > 2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > > --- a/drivers/vhost/Kconfig
> > > +++ b/drivers/vhost/Kconfig
> > > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > > If unsure, say "N".
> > >
> > > endif
> > > +
> > > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > > + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > > + default n
> > > + help
> > > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > > + userspace applications to modify the thread mode for vhost devices.
> > > +
> > > + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > > + meaning the ioctl is disabled and any operation using this ioctl
> > > + will fail.
> > > + When the configuration is enabled (y), the ioctl becomes
> > > + available, allowing users to set the mode if needed.
> > > +
> > > + If unsure, say "N".
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index fb0c7fb43f78..09e5e44dc516 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > > r = vhost_dev_set_owner(d);
> > > goto done;
> > > }
> > > +
> > > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > u8 inherit_owner;
> > > /*inherit_owner can only be modified before owner is set*/
> > > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > > r = 0;
> > > goto done;
> > > }
> > > +
> > > +#else
> > > + if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
> > > + r = -ENOTTY;
> > > + goto done;
> > > + }
>
> why do we need this? won't it fail as any other unsupported ioctl?
>
> > > +#endif
> > > +
> > > /* You must be the owner to do anything else */
> > > r = vhost_dev_check_owner(d);
> > > if (r)
> > > --
> > > 2.45.0
> >
> > Do we need to change the default value of the inhert_owner? For example:
> >
> > #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > inherit_owner = false;
> > #else
> > inherit_onwer = true;
> > #endif
> >
> > ?
>
> I feel it is best to keep the default consistent.
Just want to make sure we are on the same page.
For "default", did you mean inherit_owner = false which is consistent
with behaviour without the vhost task?
Or inherit_onwer = true, then the new ioctl to make it false is
useless. And if legacy applications want kthread behaviour it needs to
be patched which seems self-contradictory.
> All the kconfig should do, is block the ioctl.
>
Thanks
>
> > Other patches look good to me.
> >
> > Thanks
> >
> > >
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-10 4:54 ` Jason Wang
@ 2025-03-20 9:38 ` Cindy Lu
0 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-20 9:38 UTC (permalink / raw)
To: Jason Wang, Michael Tsirkin
Cc: michael.christie, sgarzare, linux-kernel, virtualization, netdev
Ping :)
On Mon, Mar 10, 2025 at 12:54 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Mar 4, 2025 at 1:33 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> > > On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
> > > >
> > > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > > > is disabled, and any attempt to use it will result in failure.
> > > >
> > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > ---
> > > > drivers/vhost/Kconfig | 15 +++++++++++++++
> > > > drivers/vhost/vhost.c | 11 +++++++++++
> > > > 2 files changed, 26 insertions(+)
> > > >
> > > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > > > --- a/drivers/vhost/Kconfig
> > > > +++ b/drivers/vhost/Kconfig
> > > > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > > > If unsure, say "N".
> > > >
> > > > endif
> > > > +
> > > > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > > > + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > > > + default n
> > > > + help
> > > > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > > > + userspace applications to modify the thread mode for vhost devices.
> > > > +
> > > > + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > > > + meaning the ioctl is disabled and any operation using this ioctl
> > > > + will fail.
> > > > + When the configuration is enabled (y), the ioctl becomes
> > > > + available, allowing users to set the mode if needed.
> > > > +
> > > > + If unsure, say "N".
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index fb0c7fb43f78..09e5e44dc516 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > > > r = vhost_dev_set_owner(d);
> > > > goto done;
> > > > }
> > > > +
> > > > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > > u8 inherit_owner;
> > > > /*inherit_owner can only be modified before owner is set*/
> > > > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > > > r = 0;
> > > > goto done;
> > > > }
> > > > +
> > > > +#else
> > > > + if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > > + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
> > > > + r = -ENOTTY;
> > > > + goto done;
> > > > + }
> >
> > why do we need this? won't it fail as any other unsupported ioctl?
> >
> > > > +#endif
> > > > +
> > > > /* You must be the owner to do anything else */
> > > > r = vhost_dev_check_owner(d);
> > > > if (r)
> > > > --
> > > > 2.45.0
> > >
> > > Do we need to change the default value of the inhert_owner? For example:
> > >
> > > #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > inherit_owner = false;
> > > #else
> > > inherit_onwer = true;
> > > #endif
> > >
> > > ?
> >
> > I feel it is best to keep the default consistent.
>
> Just want to make sure we are on the same page.
>
> For "default", did you mean inherit_owner = false which is consistent
> with behaviour without the vhost task?
>
> Or inherit_onwer = true, then the new ioctl to make it false is
> useless. And if legacy applications want kthread behaviour it needs to
> be patched which seems self-contradictory.
>
> > All the kconfig should do, is block the ioctl.
> >
>
> Thanks
>
> >
> > > Other patches look good to me.
> > >
> > > Thanks
> > >
> > > >
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-03 17:33 ` Michael S. Tsirkin
2025-03-10 4:54 ` Jason Wang
@ 2025-03-28 8:24 ` Cindy Lu
1 sibling, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-28 8:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
On Tue, Mar 4, 2025 at 1:33 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> > On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > > is disabled, and any attempt to use it will result in failure.
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > > drivers/vhost/Kconfig | 15 +++++++++++++++
> > > drivers/vhost/vhost.c | 11 +++++++++++
> > > 2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > > --- a/drivers/vhost/Kconfig
> > > +++ b/drivers/vhost/Kconfig
> > > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > > If unsure, say "N".
> > >
> > > endif
> > > +
> > > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > > + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > > + default n
> > > + help
> > > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > > + userspace applications to modify the thread mode for vhost devices.
> > > +
> > > + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > > + meaning the ioctl is disabled and any operation using this ioctl
> > > + will fail.
> > > + When the configuration is enabled (y), the ioctl becomes
> > > + available, allowing users to set the mode if needed.
> > > +
> > > + If unsure, say "N".
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index fb0c7fb43f78..09e5e44dc516 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > > r = vhost_dev_set_owner(d);
> > > goto done;
> > > }
> > > +
> > > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > u8 inherit_owner;
> > > /*inherit_owner can only be modified before owner is set*/
> > > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > > r = 0;
> > > goto done;
> > > }
> > > +
> > > +#else
> > > + if (ioctl == VHOST_FORK_FROM_OWNER) {
> > > + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
> > > + r = -ENOTTY;
> > > + goto done;
> > > + }
>
> why do we need this? won't it fail as any other unsupported ioctl?
>
Sure, will remove this
thanks
cindy
> > > +#endif
> > > +
> > > /* You must be the owner to do anything else */
> > > r = vhost_dev_check_owner(d);
> > > if (r)
> > > --
> > > 2.45.0
> >
> > Do we need to change the default value of the inhert_owner? For example:
> >
> > #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > inherit_owner = false;
> > #else
> > inherit_onwer = true;
> > #endif
> >
> > ?
>
> I feel it is best to keep the default consistent.
> All the kconfig should do, is block the ioctl.
>
>
> > Other patches look good to me.
> >
> > Thanks
> >
> > >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
2025-03-03 5:52 ` Jason Wang
2025-03-03 9:12 ` Stefano Garzarella
2025-03-03 17:33 ` Michael S. Tsirkin
@ 2025-03-21 19:39 ` Michael S. Tsirkin
2 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2025-03-21 19:39 UTC (permalink / raw)
To: Jason Wang
Cc: Cindy Lu, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
On Mon, Mar 03, 2025 at 01:52:06PM +0800, Jason Wang wrote:
> On Sun, Mar 2, 2025 at 10:34 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Introduce a new config knob `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL`,
> > to control the availability of the `VHOST_FORK_FROM_OWNER` ioctl.
> > When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is set to n, the ioctl
> > is disabled, and any attempt to use it will result in failure.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> > drivers/vhost/Kconfig | 15 +++++++++++++++
> > drivers/vhost/vhost.c | 11 +++++++++++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index b455d9ab6f3d..e5b9dcbf31b6 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -95,3 +95,18 @@ config VHOST_CROSS_ENDIAN_LEGACY
> > If unsure, say "N".
> >
> > endif
> > +
> > +config VHOST_ENABLE_FORK_OWNER_IOCTL
> > + bool "Enable IOCTL VHOST_FORK_FROM_OWNER"
> > + default n
> > + help
> > + This option enables the IOCTL VHOST_FORK_FROM_OWNER, which allows
> > + userspace applications to modify the thread mode for vhost devices.
> > +
> > + By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL` is set to `n`,
> > + meaning the ioctl is disabled and any operation using this ioctl
> > + will fail.
> > + When the configuration is enabled (y), the ioctl becomes
> > + available, allowing users to set the mode if needed.
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index fb0c7fb43f78..09e5e44dc516 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2294,6 +2294,8 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > r = vhost_dev_set_owner(d);
> > goto done;
> > }
> > +
> > +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> > if (ioctl == VHOST_FORK_FROM_OWNER) {
> > u8 inherit_owner;
> > /*inherit_owner can only be modified before owner is set*/
> > @@ -2313,6 +2315,15 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > r = 0;
> > goto done;
> > }
> > +
> > +#else
> > + if (ioctl == VHOST_FORK_FROM_OWNER) {
> > + /* When CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL is 'n', return error */
> > + r = -ENOTTY;
> > + goto done;
> > + }
> > +#endif
> > +
> > /* You must be the owner to do anything else */
> > r = vhost_dev_check_owner(d);
> > if (r)
> > --
> > 2.45.0
>
> Do we need to change the default value of the inhert_owner? For example:
>
> #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_IOCTL
> inherit_owner = false;
> #else
> inherit_onwer = true;
> #endif
>
> ?
>
> Other patches look good to me.
>
> Thanks
I think I agree with Cindy. kconfig just tells us whether the
ioctl is allowed. should not affect the default, if we want
a kconfig for default, it should be separate, but I doubt it.
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 0/8] vhost: Add support of kthread API
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
` (7 preceding siblings ...)
2025-03-02 14:32 ` [PATCH v7 8/8] vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER Cindy Lu
@ 2025-03-03 4:25 ` Lei Yang
2025-03-21 19:36 ` Michael S. Tsirkin
9 siblings, 0 replies; 22+ messages in thread
From: Lei Yang @ 2025-03-03 4:25 UTC (permalink / raw)
To: Cindy Lu
Cc: jasowang, mst, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
QE tested this series of patches with virtio-net regression tests,
everything works fine.
Tested-by: Lei Yang <leiyang@redhat.com>
On Sun, Mar 2, 2025 at 10:33 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 s.
>
> Changelog v7:
> 1. add a KConfig knob to disable legacy app support
> 2. Split the changes into two patches to separately introduce the ops and add kthread support.
> 3. Utilized INX_MAX to avoid modifications in __vhost_worker_flush
> 4. Rebased on the latest kernel
> 5. Address other comments
>
> Tested with QEMU with kthread mode/task mode/kthread+task mode
>
> Cindy Lu (8):
> vhost: Add a new parameter in vhost_dev to allow user select kthread
> vhost: Reintroduce vhost_worker to support kthread
> vhost: Add the cgroup related function
> vhost: Introduce vhost_worker_ops in vhost_worker
> vhost: Reintroduce kthread mode support in vhost
> vhost: uapi to control task mode (owner vs kthread)
> vhost: Add check for inherit_owner status
> vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
>
> drivers/vhost/Kconfig | 15 +++
> drivers/vhost/vhost.c | 227 +++++++++++++++++++++++++++++++++----
> drivers/vhost/vhost.h | 21 ++++
> include/uapi/linux/vhost.h | 15 +++
> 4 files changed, 259 insertions(+), 19 deletions(-)
>
> --
> 2.45.0
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 0/8] vhost: Add support of kthread API
2025-03-02 14:32 [PATCH v7 0/8] vhost: Add support of kthread API Cindy Lu
` (8 preceding siblings ...)
2025-03-03 4:25 ` [PATCH v7 0/8] vhost: Add support of kthread API Lei Yang
@ 2025-03-21 19:36 ` Michael S. Tsirkin
2025-03-28 5:43 ` Cindy Lu
9 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2025-03-21 19:36 UTC (permalink / raw)
To: Cindy Lu
Cc: jasowang, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
On Sun, Mar 02, 2025 at 10:32:02PM +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.
>
> In this series, a new UAPI is implemented to allow
> userspace applications to configure their thread mode.
This seems to be on top of an old tree.
Can you rebase pls?
> 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 s.
>
> Changelog v7:
> 1. add a KConfig knob to disable legacy app support
> 2. Split the changes into two patches to separately introduce the ops and add kthread support.
> 3. Utilized INX_MAX to avoid modifications in __vhost_worker_flush
> 4. Rebased on the latest kernel
> 5. Address other comments
>
> Tested with QEMU with kthread mode/task mode/kthread+task mode
>
> Cindy Lu (8):
> vhost: Add a new parameter in vhost_dev to allow user select kthread
> vhost: Reintroduce vhost_worker to support kthread
> vhost: Add the cgroup related function
> vhost: Introduce vhost_worker_ops in vhost_worker
> vhost: Reintroduce kthread mode support in vhost
> vhost: uapi to control task mode (owner vs kthread)
> vhost: Add check for inherit_owner status
> vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
>
> drivers/vhost/Kconfig | 15 +++
> drivers/vhost/vhost.c | 227 +++++++++++++++++++++++++++++++++----
> drivers/vhost/vhost.h | 21 ++++
> include/uapi/linux/vhost.h | 15 +++
> 4 files changed, 259 insertions(+), 19 deletions(-)
>
> --
> 2.45.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 0/8] vhost: Add support of kthread API
2025-03-21 19:36 ` Michael S. Tsirkin
@ 2025-03-28 5:43 ` Cindy Lu
0 siblings, 0 replies; 22+ messages in thread
From: Cindy Lu @ 2025-03-28 5:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, michael.christie, sgarzare, linux-kernel,
virtualization, netdev
On Sat, Mar 22, 2025 at 3:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Mar 02, 2025 at 10:32:02PM +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.
> >
> > In this series, a new UAPI is implemented to allow
> > userspace applications to configure their thread mode.
>
> This seems to be on top of an old tree.
> Can you rebase pls?
>
sure, I will rebase this
Thanks
Cindy
> > 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 s.
> >
> > Changelog v7:
> > 1. add a KConfig knob to disable legacy app support
> > 2. Split the changes into two patches to separately introduce the ops and add kthread support.
> > 3. Utilized INX_MAX to avoid modifications in __vhost_worker_flush
> > 4. Rebased on the latest kernel
> > 5. Address other comments
> >
> > Tested with QEMU with kthread mode/task mode/kthread+task mode
> >
> > Cindy Lu (8):
> > vhost: Add a new parameter in vhost_dev to allow user select kthread
> > vhost: Reintroduce vhost_worker to support kthread
> > vhost: Add the cgroup related function
> > vhost: Introduce vhost_worker_ops in vhost_worker
> > vhost: Reintroduce kthread mode support in vhost
> > vhost: uapi to control task mode (owner vs kthread)
> > vhost: Add check for inherit_owner status
> > vhost: Add a KConfig knob to enable IOCTL VHOST_FORK_FROM_OWNER
> >
> > drivers/vhost/Kconfig | 15 +++
> > drivers/vhost/vhost.c | 227 +++++++++++++++++++++++++++++++++----
> > drivers/vhost/vhost.h | 21 ++++
> > include/uapi/linux/vhost.h | 15 +++
> > 4 files changed, 259 insertions(+), 19 deletions(-)
> >
> > --
> > 2.45.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread