[PATCH v3 0/9] vhost: Add support of kthread API

[PATCH v3 0/9] vhost: Add support of kthread API

[Cindy Lu]

In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
The vhost now use vhost_task and workers working as a child of the owner thread,
which 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 patch, we introduce a module_param that allows users to select the
operating mode. Additionally, a new UAPI is implemented to enable
userspace applications to set their desired 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
 
Tested with QEMU.

Cindy Lu (9):
  vhost: Add a new parameter to allow user select kthread
  vhost: Add the vhost_worker to support kthread
  vhost: Add the cgroup related function
  vhost: Add kthread support in function vhost_worker_create
  vhost: Add kthread support in function vhost_worker_queue()
  vhost: Add kthread support in function vhost_worker_destroy()
  vhost: Add new UAPI to support change to task mode
  vhost_scsi: Add check for inherit_owner status
  vhost: Expose the modparam inherit_owner_default

 drivers/vhost/scsi.c       |   5 +
 drivers/vhost/vhost.c      | 194 ++++++++++++++++++++++++++++++++++---
 drivers/vhost/vhost.h      |   7 ++
 include/uapi/linux/vhost.h |   2 +
 4 files changed, 193 insertions(+), 15 deletions(-)

-- 
2.45.0

[PATCH v3 1/9] vhost: Add a new parameter 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. This will be exposed by module_param() later.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9ac25d08f473..eff6acbbb63b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -41,6 +41,7 @@ static int max_iotlb_entries = 2048;
 module_param(max_iotlb_entries, int, 0444);
 MODULE_PARM_DESC(max_iotlb_entries,
 	"Maximum number of iotlb entries. (default: 2048)");
+static bool inherit_owner_default = true;
 
 enum {
 	VHOST_MEMORY_F_LOG = 0x1,
@@ -552,6 +553,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 = inherit_owner_default;
 	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 v3 1/9] vhost: Add a new parameter to allow user select kthread

[Jason Wang]

On Tue, Nov 5, 2024 at 3:27 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. This will be exposed by module_param() later.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c | 2 ++
>  drivers/vhost/vhost.h | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 9ac25d08f473..eff6acbbb63b 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -41,6 +41,7 @@ static int max_iotlb_entries = 2048;
>  module_param(max_iotlb_entries, int, 0444);
>  MODULE_PARM_DESC(max_iotlb_entries,
>         "Maximum number of iotlb entries. (default: 2048)");
> +static bool inherit_owner_default = true;

I wonder how management can make a decision for this value.

Thanks

[PATCH v3 2/9] vhost: Add the vhost_worker to support kthread

[Cindy Lu]

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

The old function vhost_worker was 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 eff6acbbb63b..65fda810b96e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -389,6 +389,44 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	__vhost_vq_meta_reset(vq);
 }
 
+static int vhost_run_work_kthread_list(void *data)
+{
+	struct vhost_worker *worker = data;
+	struct vhost_work *work, *work_next;
+	struct vhost_dev *dev = worker->dev;
+	struct llist_node *node;
+
+	kthread_use_mm(dev->mm);
+
+	for (;;) {
+		/* mb paired w/ kthread_stop */
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		if (kthread_should_stop()) {
+			__set_current_state(TASK_RUNNING);
+			break;
+		}
+		node = llist_del_all(&worker->work_list);
+		if (!node)
+			schedule();
+
+		node = llist_reverse_order(node);
+		/* make sure flag is seen after deletion */
+		smp_wmb();
+		llist_for_each_entry_safe(work, work_next, node, node) {
+			clear_bit(VHOST_WORK_QUEUED, &work->flags);
+			__set_current_state(TASK_RUNNING);
+			kcov_remote_start_common(worker->kcov_handle);
+			work->fn(work);
+			kcov_remote_stop();
+			cond_resched();
+		}
+	}
+	kthread_unuse_mm(dev->mm);
+
+	return 0;
+}
+
 static bool vhost_run_work_list(void *data)
 {
 	struct vhost_worker *worker = data;
-- 
2.45.0

[PATCH v3 3/9] vhost: Add the cgroup related function

[Cindy Lu]

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

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 | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 65fda810b96e..e40cef3a1fa5 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>
@@ -621,6 +622,57 @@ long vhost_dev_check_owner(struct vhost_dev *dev)
 }
 EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
 
+struct vhost_attach_cgroups_struct {
+	struct vhost_work work;
+	struct task_struct *owner;
+	int ret;
+};
+
+static void vhost_attach_cgroups_work(struct vhost_work *work)
+{
+	struct vhost_attach_cgroups_struct *s;
+
+	s = container_of(work, struct vhost_attach_cgroups_struct, work);
+	s->ret = cgroup_attach_task_all(s->owner, current);
+}
+
+static int vhost_worker_cgroups_kthread(struct vhost_worker *worker)
+{
+	struct vhost_flush_struct flush;
+	struct vhost_attach_cgroups_struct attach;
+
+	attach.owner = current;
+
+	vhost_work_init(&attach.work, vhost_attach_cgroups_work);
+	vhost_worker_queue(worker, &attach.work);
+
+	init_completion(&flush.wait_event);
+	vhost_work_init(&flush.work, vhost_flush_work);
+	vhost_worker_queue(worker, &flush.work);
+	wait_for_completion(&flush.wait_event);
+
+	return attach.ret;
+}
+
+static int vhost_attach_cgroups(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	unsigned long i;
+	int ret;
+
+	/*
+	 * Free the default worker we created and cleanup workers userspace
+	 * created but couldn't clean up (it forgot or crashed).
+	 */
+
+	xa_for_each(&dev->worker_xa, i, worker) {
+		ret = vhost_worker_cgroups_kthread(worker);
+		if (ret)
+			return ret;
+	}
+	return ret;
+}
+
 /* Caller should have device mutex */
 bool vhost_dev_has_owner(struct vhost_dev *dev)
 {
-- 
2.45.0

Re: [PATCH v3 3/9] vhost: Add the cgroup related function

[Mike Christie]

On 11/5/24 1:25 AM, Cindy Lu wrote:
> +static int vhost_attach_cgroups(struct vhost_dev *dev)
> +{
> +	struct vhost_worker *worker;
> +	unsigned long i;
> +	int ret;
> +
> +	/*
> +	 * Free the default worker we created and cleanup workers userspace
> +	 * created but couldn't clean up (it forgot or crashed).
> +	 */

I think this comment got added here by accident.

> +
> +	xa_for_each(&dev->worker_xa, i, worker) {
> +		ret = vhost_worker_cgroups_kthread(worker);
> +		if (ret)
> +			return ret;
> +	}
> +	return ret;
> +}
> +
>  /* Caller should have device mutex */
>  bool vhost_dev_has_owner(struct vhost_dev *dev)
>  {

Re: [PATCH v3 3/9] vhost: Add the cgroup related function

[Cindy Lu]

On Mon, Nov 25, 2024 at 11:22 PM Mike Christie
<michael.christie@oracle.com> wrote:
>
> On 11/5/24 1:25 AM, Cindy Lu wrote:
> > +static int vhost_attach_cgroups(struct vhost_dev *dev)
> > +{
> > +     struct vhost_worker *worker;
> > +     unsigned long i;
> > +     int ret;
> > +
> > +     /*
> > +      * Free the default worker we created and cleanup workers userspace
> > +      * created but couldn't clean up (it forgot or crashed).
> > +      */
>
> I think this comment got added here by accident.
>
will remove this
Thanks
Cindy
> > +
> > +     xa_for_each(&dev->worker_xa, i, worker) {
> > +             ret = vhost_worker_cgroups_kthread(worker);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     return ret;
> > +}
> > +
> >  /* Caller should have device mutex */
> >  bool vhost_dev_has_owner(struct vhost_dev *dev)
> >  {
>

[PATCH v3 4/9] vhost: Add kthread support in function vhost_worker_create

[Cindy Lu]

Restored the previous functions kthread_wakeup and kthread_stop.
Also add a new structure, vhost_task_fn. The function vhost_worker_create
Will initializes this structure based on the value of inherit_owner.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e40cef3a1fa5..603b146fccc1 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -741,43 +741,92 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	xa_destroy(&dev->worker_xa);
 }
 
+static int vhost_task_wakeup_fn(void *vtsk)
+{
+	vhost_task_wake((struct vhost_task *)vtsk);
+	return 0;
+}
+static int vhost_kthread_wakeup_fn(void *p)
+{
+	return wake_up_process((struct task_struct *)p);
+}
+static int vhost_task_stop_fn(void *vtsk)
+{
+	vhost_task_stop((struct vhost_task *)vtsk);
+	return 0;
+}
+static int vhost_kthread_stop_fn(void *k)
+{
+	return kthread_stop((struct task_struct *)k);
+}
+
 static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
-	struct vhost_task *vtsk;
+	struct vhost_task *vtsk = NULL;
+	struct task_struct *task = NULL;
 	char name[TASK_COMM_LEN];
 	int ret;
 	u32 id;
 
+	/* Allocate resources for the worker */
 	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
 	if (!worker)
 		return NULL;
 
+	worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
+	if (!worker->fn) {
+		kfree(worker);
+		return NULL;
+	}
+
 	worker->dev = dev;
 	snprintf(name, sizeof(name), "vhost-%d", current->pid);
 
-	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
-				 worker, name);
-	if (!vtsk)
-		goto free_worker;
-
 	mutex_init(&worker->mutex);
 	init_llist_head(&worker->work_list);
 	worker->kcov_handle = kcov_common_handle();
-	worker->vtsk = vtsk;
 
-	vhost_task_start(vtsk);
+	if (dev->inherit_owner) {
+		/* Create and start a vhost task */
+		vtsk = vhost_task_create(vhost_run_work_list,
+					 vhost_worker_killed, worker, name);
+		if (!vtsk)
+			goto free_worker;
+
+		worker->vtsk = vtsk;
+		worker->fn->wakeup = vhost_task_wakeup_fn;
+		worker->fn->stop = vhost_task_stop_fn;
+
+		vhost_task_start(vtsk);
+	} else {
+		/* Create and start a kernel thread */
+		task = kthread_create(vhost_run_work_kthread_list, worker,
+				      "vhost-%d", current->pid);
+		if (IS_ERR(task)) {
+			ret = PTR_ERR(task);
+			goto free_worker;
+		}
+		worker->task = task;
+		worker->fn->wakeup = vhost_kthread_wakeup_fn;
+		worker->fn->stop = vhost_kthread_stop_fn;
+
+		wake_up_process(task);
+		/* Attach to the vhost cgroup */
+		ret = vhost_attach_cgroups(dev);
+		if (ret)
+			goto stop_worker;
+	}
 
 	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
 	if (ret < 0)
 		goto stop_worker;
 	worker->id = id;
-
 	return worker;
-
 stop_worker:
-	vhost_task_stop(vtsk);
+	worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);
 free_worker:
+	kfree(worker->fn);
 	kfree(worker);
 	return NULL;
 }
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index c650c4506c70..ebababa4e340 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -25,8 +25,13 @@ struct vhost_work {
 	vhost_work_fn_t		fn;
 	unsigned long		flags;
 };
+struct vhost_task_fn {
+	int (*wakeup)(void *task);
+	int (*stop)(void *task);
+};
 
 struct vhost_worker {
+	struct task_struct	*task;
 	struct vhost_task	*vtsk;
 	struct vhost_dev	*dev;
 	/* Used to serialize device wide flushing with worker swapping. */
@@ -36,6 +41,7 @@ struct vhost_worker {
 	u32			id;
 	int			attachment_cnt;
 	bool			killed;
+	struct vhost_task_fn *fn;
 };
 
 /* Poll a file (eventfd or socket) */
-- 
2.45.0

Re: [PATCH v3 4/9] vhost: Add kthread support in function vhost_worker_create

[Jason Wang]

On Tue, Nov 5, 2024 at 3:27 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Restored the previous functions kthread_wakeup and kthread_stop.
> Also add a new structure, vhost_task_fn. The function vhost_worker_create
> Will initializes this structure based on the value of inherit_owner.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c | 71 ++++++++++++++++++++++++++++++++++++-------
>  drivers/vhost/vhost.h |  6 ++++
>  2 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e40cef3a1fa5..603b146fccc1 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -741,43 +741,92 @@ static void vhost_workers_free(struct vhost_dev *dev)
>         xa_destroy(&dev->worker_xa);
>  }
>
> +static int vhost_task_wakeup_fn(void *vtsk)
> +{
> +       vhost_task_wake((struct vhost_task *)vtsk);
> +       return 0;
> +}

Let's have a newline between two functions.

> +static int vhost_kthread_wakeup_fn(void *p)
> +{
> +       return wake_up_process((struct task_struct *)p);
> +}
> +static int vhost_task_stop_fn(void *vtsk)
> +{
> +       vhost_task_stop((struct vhost_task *)vtsk);
> +       return 0;
> +}
> +static int vhost_kthread_stop_fn(void *k)
> +{
> +       return kthread_stop((struct task_struct *)k);
> +}
> +
>  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
>  {
>         struct vhost_worker *worker;
> -       struct vhost_task *vtsk;
> +       struct vhost_task *vtsk = NULL;
> +       struct task_struct *task = NULL;
>         char name[TASK_COMM_LEN];
>         int ret;
>         u32 id;
>
> +       /* Allocate resources for the worker */
>         worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
>         if (!worker)
>                 return NULL;
>
> +       worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
> +       if (!worker->fn) {
> +               kfree(worker);
> +               return NULL;
> +       }
> +
>         worker->dev = dev;
>         snprintf(name, sizeof(name), "vhost-%d", current->pid);
>
> -       vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> -                                worker, name);
> -       if (!vtsk)
> -               goto free_worker;
> -
>         mutex_init(&worker->mutex);
>         init_llist_head(&worker->work_list);
>         worker->kcov_handle = kcov_common_handle();
> -       worker->vtsk = vtsk;
>
> -       vhost_task_start(vtsk);
> +       if (dev->inherit_owner) {
> +               /* Create and start a vhost task */
> +               vtsk = vhost_task_create(vhost_run_work_list,
> +                                        vhost_worker_killed, worker, name);
> +               if (!vtsk)
> +                       goto free_worker;
> +
> +               worker->vtsk = vtsk;
> +               worker->fn->wakeup = vhost_task_wakeup_fn;
> +               worker->fn->stop = vhost_task_stop_fn;
> +
> +               vhost_task_start(vtsk);
> +       } else {
> +               /* Create and start a kernel thread */
> +               task = kthread_create(vhost_run_work_kthread_list, worker,
> +                                     "vhost-%d", current->pid);
> +               if (IS_ERR(task)) {
> +                       ret = PTR_ERR(task);
> +                       goto free_worker;
> +               }
> +               worker->task = task;
> +               worker->fn->wakeup = vhost_kthread_wakeup_fn;
> +               worker->fn->stop = vhost_kthread_stop_fn;
> +
> +               wake_up_process(task);
> +               /* Attach to the vhost cgroup */
> +               ret = vhost_attach_cgroups(dev);
> +               if (ret)
> +                       goto stop_worker;
> +       }
>
>         ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
>         if (ret < 0)
>                 goto stop_worker;
>         worker->id = id;
> -
>         return worker;
> -
>  stop_worker:
> -       vhost_task_stop(vtsk);
> +       worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);
>  free_worker:
> +       kfree(worker->fn);
>         kfree(worker);
>         return NULL;
>  }
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index c650c4506c70..ebababa4e340 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -25,8 +25,13 @@ struct vhost_work {
>         vhost_work_fn_t         fn;
>         unsigned long           flags;
>  };
> +struct vhost_task_fn {
> +       int (*wakeup)(void *task);

Let's have comments to explain the semantics of each operation.

> +       int (*stop)(void *task);
> +};

I think the goal is to reduce if/else, so while at this, let's
introduce more ops. For example the create_worker one?

>
>  struct vhost_worker {
> +       struct task_struct      *task;
>         struct vhost_task       *vtsk;
>         struct vhost_dev        *dev;
>         /* Used to serialize device wide flushing with worker swapping. */
> @@ -36,6 +41,7 @@ struct vhost_worker {
>         u32                     id;
>         int                     attachment_cnt;
>         bool                    killed;
> +       struct vhost_task_fn *fn;
>  };
>
>  /* Poll a file (eventfd or socket) */
> --
> 2.45.0
>

Thanks

Re: [PATCH v3 4/9] vhost: Add kthread support in function vhost_worker_create

[Cindy Lu]

On Tue, Nov 5, 2024 at 5:36 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Nov 5, 2024 at 3:27 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Restored the previous functions kthread_wakeup and kthread_stop.
> > Also add a new structure, vhost_task_fn. The function vhost_worker_create
> > Will initializes this structure based on the value of inherit_owner.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c | 71 ++++++++++++++++++++++++++++++++++++-------
> >  drivers/vhost/vhost.h |  6 ++++
> >  2 files changed, 66 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index e40cef3a1fa5..603b146fccc1 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -741,43 +741,92 @@ static void vhost_workers_free(struct vhost_dev *dev)
> >         xa_destroy(&dev->worker_xa);
> >  }
> >
> > +static int vhost_task_wakeup_fn(void *vtsk)
> > +{
> > +       vhost_task_wake((struct vhost_task *)vtsk);
> > +       return 0;
> > +}
>
> Let's have a newline between two functions.
>
will fix this
> > +static int vhost_kthread_wakeup_fn(void *p)
> > +{
> > +       return wake_up_process((struct task_struct *)p);
> > +}
> > +static int vhost_task_stop_fn(void *vtsk)
> > +{
> > +       vhost_task_stop((struct vhost_task *)vtsk);
> > +       return 0;
> > +}
> > +static int vhost_kthread_stop_fn(void *k)
> > +{
> > +       return kthread_stop((struct task_struct *)k);
> > +}
> > +
> >  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> >  {
> >         struct vhost_worker *worker;
> > -       struct vhost_task *vtsk;
> > +       struct vhost_task *vtsk = NULL;
> > +       struct task_struct *task = NULL;
> >         char name[TASK_COMM_LEN];
> >         int ret;
> >         u32 id;
> >
> > +       /* Allocate resources for the worker */
> >         worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> >         if (!worker)
> >                 return NULL;
> >
> > +       worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
> > +       if (!worker->fn) {
> > +               kfree(worker);
> > +               return NULL;
> > +       }
> > +
> >         worker->dev = dev;
> >         snprintf(name, sizeof(name), "vhost-%d", current->pid);
> >
> > -       vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> > -                                worker, name);
> > -       if (!vtsk)
> > -               goto free_worker;
> > -
> >         mutex_init(&worker->mutex);
> >         init_llist_head(&worker->work_list);
> >         worker->kcov_handle = kcov_common_handle();
> > -       worker->vtsk = vtsk;
> >
> > -       vhost_task_start(vtsk);
> > +       if (dev->inherit_owner) {
> > +               /* Create and start a vhost task */
> > +               vtsk = vhost_task_create(vhost_run_work_list,
> > +                                        vhost_worker_killed, worker, name);
> > +               if (!vtsk)
> > +                       goto free_worker;
> > +
> > +               worker->vtsk = vtsk;
> > +               worker->fn->wakeup = vhost_task_wakeup_fn;
> > +               worker->fn->stop = vhost_task_stop_fn;
> > +
> > +               vhost_task_start(vtsk);
> > +       } else {
> > +               /* Create and start a kernel thread */
> > +               task = kthread_create(vhost_run_work_kthread_list, worker,
> > +                                     "vhost-%d", current->pid);
> > +               if (IS_ERR(task)) {
> > +                       ret = PTR_ERR(task);
> > +                       goto free_worker;
> > +               }
> > +               worker->task = task;
> > +               worker->fn->wakeup = vhost_kthread_wakeup_fn;
> > +               worker->fn->stop = vhost_kthread_stop_fn;
> > +
> > +               wake_up_process(task);
> > +               /* Attach to the vhost cgroup */
> > +               ret = vhost_attach_cgroups(dev);
> > +               if (ret)
> > +                       goto stop_worker;
> > +       }
> >
> >         ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> >         if (ret < 0)
> >                 goto stop_worker;
> >         worker->id = id;
> > -
> >         return worker;
> > -
> >  stop_worker:
> > -       vhost_task_stop(vtsk);
> > +       worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);
> >  free_worker:
> > +       kfree(worker->fn);
> >         kfree(worker);
> >         return NULL;
> >  }
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index c650c4506c70..ebababa4e340 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -25,8 +25,13 @@ struct vhost_work {
> >         vhost_work_fn_t         fn;
> >         unsigned long           flags;
> >  };
> > +struct vhost_task_fn {
> > +       int (*wakeup)(void *task);
>
> Let's have comments to explain the semantics of each operation.
>
sure, will fix this
> > +       int (*stop)(void *task);
> > +};
>
> I think the goal is to reduce if/else, so while at this, let's
> introduce more ops. For example the create_worker one?
>
sure, will change this part
thanks
cindy
> >
> >  struct vhost_worker {
> > +       struct task_struct      *task;
> >         struct vhost_task       *vtsk;
> >         struct vhost_dev        *dev;
> >         /* Used to serialize device wide flushing with worker swapping. */
> > @@ -36,6 +41,7 @@ struct vhost_worker {
> >         u32                     id;
> >         int                     attachment_cnt;
> >         bool                    killed;
> > +       struct vhost_task_fn *fn;
> >  };
> >
> >  /* Poll a file (eventfd or socket) */
> > --
> > 2.45.0
> >
>
> Thanks
>

Re: [PATCH v3 4/9] vhost: Add kthread support in function vhost_worker_create

[michael.christie]

On 11/5/24 1:25 AM, Cindy Lu wrote:
>  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
>  {
>  	struct vhost_worker *worker;
> -	struct vhost_task *vtsk;
> +	struct vhost_task *vtsk = NULL;
> +	struct task_struct *task = NULL;
>  	char name[TASK_COMM_LEN];
>  	int ret;
>  	u32 id;
>  
> +	/* Allocate resources for the worker */
>  	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
>  	if (!worker)
>  		return NULL;
>  
> +	worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
> +	if (!worker->fn) {
> +		kfree(worker);
> +		return NULL;
> +	}

Why dynamically allocate this?

You could probably even just kill the vhost_task_fn struct since we just
have to the 2 callouts.

> +
>  	worker->dev = dev;
>  	snprintf(name, sizeof(name), "vhost-%d", current->pid);
>  
> -	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> -				 worker, name);
> -	if (!vtsk)
> -		goto free_worker;
> -
>  	mutex_init(&worker->mutex);
>  	init_llist_head(&worker->work_list);
>  	worker->kcov_handle = kcov_common_handle();
> -	worker->vtsk = vtsk;
>  
> -	vhost_task_start(vtsk);
> +	if (dev->inherit_owner) {
> +		/* Create and start a vhost task */

Maybe instead of this comment and the one below write something about
what inherit_owner means. We can see from the code we are creating a
vhost/kthread, but it's not really obvious why. Something like:

/*
 * 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.
 */



> +		vtsk = vhost_task_create(vhost_run_work_list,
> +					 vhost_worker_killed, worker, name);
> +		if (!vtsk)
> +			goto free_worker;
> +
> +		worker->vtsk = vtsk;
> +		worker->fn->wakeup = vhost_task_wakeup_fn;
> +		worker->fn->stop = vhost_task_stop_fn;
> +
> +		vhost_task_start(vtsk);
> +	} else {
> +		/* Create and start a kernel thread */
> +		task = kthread_create(vhost_run_work_kthread_list, worker,
> +				      "vhost-%d", current->pid);
> +		if (IS_ERR(task)) {
> +			ret = PTR_ERR(task);
> +			goto free_worker;
> +		}
> +		worker->task = task;
> +		worker->fn->wakeup = vhost_kthread_wakeup_fn;
> +		worker->fn->stop = vhost_kthread_stop_fn;
> +
> +		wake_up_process(task);
> +		/* Attach to the vhost cgroup */

You don't need this comment do you? The function name tells us the same
info.

> +		ret = vhost_attach_cgroups(dev);

I don't think this works. Patch 3/9 did:

+	xa_for_each(&dev->worker_xa, i, worker) {
+		ret = vhost_worker_cgroups_kthread(worker);

but we don't add the worker to the xa until below.

You also want to just call vhost_worker_cgroups_kthread above, because
you only want to add the one task and not loop over all of them.

I would then also maybe rename vhost_worker_cgroups_kthread to something
like vhost_attach_task_to_cgroups.



> +		if (ret)
> +			goto stop_worker;
> +	}
>  
>  	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
>  	if (ret < 0)
>  		goto stop_worker;
>  	worker->id = id;
> -
>  	return worker;
> -
>  stop_worker:
> -	vhost_task_stop(vtsk);
> +	worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);

I don't think you need to cast since the function takes a void pointer.
Same comment for the other patches like 6/9 where you are calling the
callout and casting.

Re: [PATCH v3 4/9] vhost: Add kthread support in function vhost_worker_create

[Cindy Lu]

On Wed, Nov 27, 2024 at 5:20 AM <michael.christie@oracle.com> wrote:
>
> On 11/5/24 1:25 AM, Cindy Lu wrote:
> >  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> >  {
> >       struct vhost_worker *worker;
> > -     struct vhost_task *vtsk;
> > +     struct vhost_task *vtsk = NULL;
> > +     struct task_struct *task = NULL;
> >       char name[TASK_COMM_LEN];
> >       int ret;
> >       u32 id;
> >
> > +     /* Allocate resources for the worker */
> >       worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> >       if (!worker)
> >               return NULL;
> >
> > +     worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
> > +     if (!worker->fn) {
> > +             kfree(worker);
> > +             return NULL;
> > +     }
>
> Why dynamically allocate this?
>
> You could probably even just kill the vhost_task_fn struct since we just
> have to the 2 callouts.

sure, will change this
>
>
> > +
> >       worker->dev = dev;
> >       snprintf(name, sizeof(name), "vhost-%d", current->pid);
> >
> > -     vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> > -                              worker, name);
> > -     if (!vtsk)
> > -             goto free_worker;
> > -
> >       mutex_init(&worker->mutex);
> >       init_llist_head(&worker->work_list);
> >       worker->kcov_handle = kcov_common_handle();
> > -     worker->vtsk = vtsk;
> >
> > -     vhost_task_start(vtsk);
> > +     if (dev->inherit_owner) {
> > +             /* Create and start a vhost task */
>
> Maybe instead of this comment and the one below write something about
> what inherit_owner means. We can see from the code we are creating a
> vhost/kthread, but it's not really obvious why. Something like:
>
> /*
>  * 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.
>  */
>
Thanks, Mike， I will change this

>
>
> > +             vtsk = vhost_task_create(vhost_run_work_list,
> > +                                      vhost_worker_killed, worker, name);
> > +             if (!vtsk)
> > +                     goto free_worker;
> > +
> > +             worker->vtsk = vtsk;
> > +             worker->fn->wakeup = vhost_task_wakeup_fn;
> > +             worker->fn->stop = vhost_task_stop_fn;
> > +
> > +             vhost_task_start(vtsk);
> > +     } else {
> > +             /* Create and start a kernel thread */
> > +             task = kthread_create(vhost_run_work_kthread_list, worker,
> > +                                   "vhost-%d", current->pid);
> > +             if (IS_ERR(task)) {
> > +                     ret = PTR_ERR(task);
> > +                     goto free_worker;
> > +             }
> > +             worker->task = task;
> > +             worker->fn->wakeup = vhost_kthread_wakeup_fn;
> > +             worker->fn->stop = vhost_kthread_stop_fn;
> > +
> > +             wake_up_process(task);
> > +             /* Attach to the vhost cgroup */
>
> You don't need this comment do you? The function name tells us the same
> info.
>
sure, Will remove  this
> > +             ret = vhost_attach_cgroups(dev);
>
> I don't think this works. Patch 3/9 did:
>
> +       xa_for_each(&dev->worker_xa, i, worker) {
> +               ret = vhost_worker_cgroups_kthread(worker);
>
> but we don't add the worker to the xa until below.
>
> You also want to just call vhost_worker_cgroups_kthread above, because
> you only want to add the one task and not loop over all of them.
>
> I would then also maybe rename vhost_worker_cgroups_kthread to something
> like vhost_attach_task_to_cgroups.
>
>
Will fix this. Thanks
>
> > +             if (ret)
> > +                     goto stop_worker;
> > +     }
> >
> >       ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> >       if (ret < 0)
> >               goto stop_worker;
> >       worker->id = id;
> > -
> >       return worker;
> > -
> >  stop_worker:
> > -     vhost_task_stop(vtsk);
> > +     worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);
>
> I don't think you need to cast since the function takes a void pointer.
> Same comment for the other patches like 6/9 where you are calling the
> callout and casting.
>
Sure, Thanks I will rewrite this part
Thanks
cindy

[PATCH v3 5/9] vhost: Add kthread support in function vhost_worker_queue()

[Cindy Lu]

The function vhost_worker_queue() uses vhost_task_fn and
selects the different mode based on the value of inherit_owner.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 603b146fccc1..8b7ddfb33c61 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -238,13 +238,18 @@ EXPORT_SYMBOL_GPL(vhost_poll_stop);
 static void vhost_worker_queue(struct vhost_worker *worker,
 			       struct vhost_work *work)
 {
+	if (!worker && !worker->fn)
+		return;
+
 	if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
 		/* We can only add the work to the list after we're
 		 * sure it was not in the list.
 		 * test_and_set_bit() implies a memory barrier.
 		 */
 		llist_add(&work->node, &worker->work_list);
-		vhost_task_wake(worker->vtsk);
+		worker->fn->wakeup(worker->dev->inherit_owner ?
+					   (void *)worker->vtsk :
+					   (void *)worker->task);
 	}
 }
 
-- 
2.45.0

Re: [PATCH v3 5/9] vhost: Add kthread support in function vhost_worker_queue()

[Jason Wang]

On Tue, Nov 5, 2024 at 3:27 PM Cindy Lu <lulu@redhat.com> wrote:
>
> The function vhost_worker_queue() uses vhost_task_fn and
> selects the different mode based on the value of inherit_owner.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 603b146fccc1..8b7ddfb33c61 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -238,13 +238,18 @@ EXPORT_SYMBOL_GPL(vhost_poll_stop);
>  static void vhost_worker_queue(struct vhost_worker *worker,
>                                struct vhost_work *work)
>  {
> +       if (!worker && !worker->fn)
> +               return;
> +
>         if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
>                 /* We can only add the work to the list after we're
>                  * sure it was not in the list.
>                  * test_and_set_bit() implies a memory barrier.
>                  */
>                 llist_add(&work->node, &worker->work_list);
> -               vhost_task_wake(worker->vtsk);
> +               worker->fn->wakeup(worker->dev->inherit_owner ?
> +                                          (void *)worker->vtsk :
> +                                          (void *)worker->task);

Logically, it looks better to introduce the ops before introducing
back the kthread behaviour?

Thanks

>         }
>  }
>
> --
> 2.45.0
>

Re: [PATCH v3 5/9] vhost: Add kthread support in function vhost_worker_queue()

[Dan Carpenter]

Hi Cindy,

kernel test robot noticed the following build warnings:

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-to-allow-user-select-kthread/20241105-153254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
patch link:    https://lore.kernel.org/r/20241105072642.898710-6-lulu%40redhat.com
patch subject: [PATCH v3 5/9] vhost: Add kthread support in function vhost_worker_queue()
config: x86_64-randconfig-161-20241106 (https://download.01.org/0day-ci/archive/20241107/202411071251.tQLG8K6C-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)

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>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202411071251.tQLG8K6C-lkp@intel.com/

New smatch warnings:
drivers/vhost/vhost.c:241 vhost_worker_queue() error: we previously assumed 'worker' could be null (see line 241)

Old smatch warnings:
drivers/vhost/vhost.c:311 vhost_dev_flush() warn: iterator 'i' not incremented
drivers/vhost/vhost.c:678 vhost_attach_cgroups() error: uninitialized symbol 'ret'.
drivers/vhost/vhost.c:673 vhost_attach_cgroups() warn: iterator 'i' not incremented

vim +/worker +241 drivers/vhost/vhost.c

228a27cf78afc63 Mike Christie      2023-06-26  238  static void vhost_worker_queue(struct vhost_worker *worker,
0921dddcb589803 Mike Christie      2023-06-26  239  			       struct vhost_work *work)
3a4d5c94e959359 Michael S. Tsirkin 2010-01-14  240  {
001268765c12bbf Cindy Lu           2024-11-05 @241  	if (!worker && !worker->fn)

|| was intended instead of &&.

001268765c12bbf Cindy Lu           2024-11-05  242  		return;
001268765c12bbf Cindy Lu           2024-11-05  243  
04b96e5528ca971 Jason Wang         2016-04-25  244  	if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
04b96e5528ca971 Jason Wang         2016-04-25  245  		/* We can only add the work to the list after we're
04b96e5528ca971 Jason Wang         2016-04-25  246  		 * sure it was not in the list.
635abf01918157e Peng Tao           2016-12-07  247  		 * test_and_set_bit() implies a memory barrier.
04b96e5528ca971 Jason Wang         2016-04-25  248  		 */
0921dddcb589803 Mike Christie      2023-06-26  249  		llist_add(&work->node, &worker->work_list);
001268765c12bbf Cindy Lu           2024-11-05  250  		worker->fn->wakeup(worker->dev->inherit_owner ?
001268765c12bbf Cindy Lu           2024-11-05  251  					   (void *)worker->vtsk :
001268765c12bbf Cindy Lu           2024-11-05  252  					   (void *)worker->task);
3a4d5c94e959359 Michael S. Tsirkin 2010-01-14  253  	}
ac9fde2474d04bd Qin Chuanyu        2013-06-07  254  }

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

Re: [PATCH v3 5/9] vhost: Add kthread support in function vhost_worker_queue()

[Dan Carpenter]

On Thu, Nov 07, 2024 at 01:38:56PM +0300, Dan Carpenter wrote:
> Hi Cindy,
> 
> kernel test robot noticed the following build warnings:
> 
> 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-to-allow-user-select-kthread/20241105-153254
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
> patch link:    https://lore.kernel.org/r/20241105072642.898710-6-lulu%40redhat.com
> patch subject: [PATCH v3 5/9] vhost: Add kthread support in function vhost_worker_queue()
> config: x86_64-randconfig-161-20241106 (https://download.01.org/0day-ci/archive/20241107/202411071251.tQLG8K6C-lkp@intel.com/config)
> compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
> 
> 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>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202411071251.tQLG8K6C-lkp@intel.com/
> 
> New smatch warnings:
> drivers/vhost/vhost.c:241 vhost_worker_queue() error: we previously assumed 'worker' could be null (see line 241)
> 

I only meant to send this warning.

> Old smatch warnings:
> drivers/vhost/vhost.c:311 vhost_dev_flush() warn: iterator 'i' not incremented
> drivers/vhost/vhost.c:678 vhost_attach_cgroups() error: uninitialized symbol 'ret'.
> drivers/vhost/vhost.c:673 vhost_attach_cgroups() warn: iterator 'i' not incremented
> 

Not these.  I need to fix these for systems without the cross function
DB...  #embarassing.

regards,
dan carpenter

[PATCH v3 6/9] vhost: Add kthread support in function vhost_worker_destroy()

[Cindy Lu]

The function vhost_worker_destroy() will use struct vhost_task_fn and
selects the different mode based on the value of inherit_owner.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 8b7ddfb33c61..c17dc01febcc 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -718,12 +718,14 @@ static void vhost_detach_mm(struct vhost_dev *dev)
 static void vhost_worker_destroy(struct vhost_dev *dev,
 				 struct vhost_worker *worker)
 {
-	if (!worker)
+	if (!worker && !worker->fn)
 		return;
 
 	WARN_ON(!llist_empty(&worker->work_list));
 	xa_erase(&dev->worker_xa, worker->id);
-	vhost_task_stop(worker->vtsk);
+	worker->fn->stop(dev->inherit_owner ? (void *)worker->vtsk :
+					      (void *)worker->task);
+	kfree(worker->fn);
 	kfree(worker);
 }
 
-- 
2.45.0

Re: [PATCH v3 6/9] vhost: Add kthread support in function vhost_worker_destroy()

[Dan Carpenter]

Hi Cindy,

kernel test robot noticed the following build warnings:

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-to-allow-user-select-kthread/20241105-153254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
patch link:    https://lore.kernel.org/r/20241105072642.898710-7-lulu%40redhat.com
patch subject: [PATCH v3 6/9] vhost: Add kthread support in function vhost_worker_destroy()
config: x86_64-randconfig-161-20241106 (https://download.01.org/0day-ci/archive/20241107/202411071945.ExiEHgoX-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)

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>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202411071945.ExiEHgoX-lkp@intel.com/

New smatch warnings:
drivers/vhost/vhost.c:721 vhost_worker_destroy() error: we previously assumed 'worker' could be null (see line 721)

Old smatch warnings:
drivers/vhost/vhost.c:241 vhost_worker_queue() error: we previously assumed 'worker' could be null (see line 241)

vim +/worker +721 drivers/vhost/vhost.c

1cdaafa1b8b4ef Mike Christie 2023-06-26  718  static void vhost_worker_destroy(struct vhost_dev *dev,
1cdaafa1b8b4ef Mike Christie 2023-06-26  719  				 struct vhost_worker *worker)
1a5f8090c6de99 Mike Christie 2023-03-10  720  {
e4dec2edddbd54 Cindy Lu      2024-11-05 @721  	if (!worker && !worker->fn)

Same thing.  && vs ||.

1a5f8090c6de99 Mike Christie 2023-03-10  722  		return;
1a5f8090c6de99 Mike Christie 2023-03-10  723  
1cdaafa1b8b4ef Mike Christie 2023-06-26  724  	WARN_ON(!llist_empty(&worker->work_list));
1cdaafa1b8b4ef Mike Christie 2023-06-26  725  	xa_erase(&dev->worker_xa, worker->id);
e4dec2edddbd54 Cindy Lu      2024-11-05  726  	worker->fn->stop(dev->inherit_owner ? (void *)worker->vtsk :
e4dec2edddbd54 Cindy Lu      2024-11-05  727  					      (void *)worker->task);
e4dec2edddbd54 Cindy Lu      2024-11-05  728  	kfree(worker->fn);
1cdaafa1b8b4ef Mike Christie 2023-06-26  729  	kfree(worker);
1cdaafa1b8b4ef Mike Christie 2023-06-26  730  }

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

[PATCH v3 7/9] 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      | 15 ++++++++++++++-
 include/uapi/linux/vhost.h |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c17dc01febcc..70c793b63905 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2274,8 +2274,9 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 {
 	struct eventfd_ctx *ctx;
 	u64 p;
-	long r;
+	long r = 0;
 	int i, fd;
+	bool inherit_owner;
 
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
@@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 		if (ctx)
 			eventfd_ctx_put(ctx);
 		break;
+	case VHOST_SET_INHERIT_FROM_OWNER:
+		/*inherit_owner can only be modified before owner is set*/
+		if (vhost_dev_has_owner(d))
+			break;
+
+		if (copy_from_user(&inherit_owner, argp,
+				   sizeof(inherit_owner))) {
+			r = -EFAULT;
+			break;
+		}
+		d->inherit_owner = inherit_owner;
+		break;
 	default:
 		r = -ENOIOCTLCMD;
 		break;
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index b95dd84eef2d..1e192038633d 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,6 @@
  */
 #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
 					      struct vhost_vring_state)
+
+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
 #endif
-- 
2.45.0

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Jason Wang]

On Tue, Nov 5, 2024 at 3:28 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      | 15 ++++++++++++++-
>  include/uapi/linux/vhost.h |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index c17dc01febcc..70c793b63905 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2274,8 +2274,9 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  {
>         struct eventfd_ctx *ctx;
>         u64 p;
> -       long r;
> +       long r = 0;
>         int i, fd;
> +       bool inherit_owner;
>
>         /* If you are not the owner, you can become one */
>         if (ioctl == VHOST_SET_OWNER) {
> @@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>                 if (ctx)
>                         eventfd_ctx_put(ctx);
>                 break;
> +       case VHOST_SET_INHERIT_FROM_OWNER:
> +               /*inherit_owner can only be modified before owner is set*/
> +               if (vhost_dev_has_owner(d))
> +                       break;
> +
> +               if (copy_from_user(&inherit_owner, argp,
> +                                  sizeof(inherit_owner))) {
> +                       r = -EFAULT;
> +                       break;
> +               }
> +               d->inherit_owner = inherit_owner;
> +               break;

Is there any case that we need to switch from owner back to kthread?
If not I would choose a more simplified API that is just
VHOST_INHERIT_OWNER.

>         default:
>                 r = -ENOIOCTLCMD;
>                 break;
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index b95dd84eef2d..1e192038633d 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,6 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE      _IOWR(VHOST_VIRTIO, 0x82,       \
>                                               struct vhost_vring_state)
> +
> +#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
>  #endif
> --
> 2.45.0

Thanks

>

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Mike Christie]

On 11/5/24 3:39 AM, Jason Wang wrote:
> On Tue, Nov 5, 2024 at 3:28 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      | 15 ++++++++++++++-
>>  include/uapi/linux/vhost.h |  2 ++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index c17dc01febcc..70c793b63905 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -2274,8 +2274,9 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>>  {
>>         struct eventfd_ctx *ctx;
>>         u64 p;
>> -       long r;
>> +       long r = 0;
>>         int i, fd;
>> +       bool inherit_owner;
>>
>>         /* If you are not the owner, you can become one */
>>         if (ioctl == VHOST_SET_OWNER) {
>> @@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>>                 if (ctx)
>>                         eventfd_ctx_put(ctx);
>>                 break;
>> +       case VHOST_SET_INHERIT_FROM_OWNER:
>> +               /*inherit_owner can only be modified before owner is set*/
>> +               if (vhost_dev_has_owner(d))
>> +                       break;
>> +
>> +               if (copy_from_user(&inherit_owner, argp,
>> +                                  sizeof(inherit_owner))) {
>> +                       r = -EFAULT;
>> +                       break;
>> +               }
>> +               d->inherit_owner = inherit_owner;
>> +               break;
> 
> Is there any case that we need to switch from owner back to kthread?
> If not I would choose a more simplified API that is just
> VHOST_INHERIT_OWNER.

I can't think of any need to be able to switch back and forth for
general use.

However for this patchset, I think in patch 9/9 we set the default as:

inherit_owner_default = true

so the default is to use vhost_tasks.

With that code, we would need VHOST_SET_INHERIT_FROM_OWNER so userspace
can set the kernel to use kthreads.

I'm not sure if in the past emails it was resolved what the default would
be.

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Stefano Garzarella]

On Tue, Nov 05, 2024 at 03:25:26PM +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      | 15 ++++++++++++++-
> include/uapi/linux/vhost.h |  2 ++
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index c17dc01febcc..70c793b63905 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -2274,8 +2274,9 @@ 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;

I don't know if something is missing in this patch, but I am confused:

`r` is set few lines below...

> 	int i, fd;
>+	bool inherit_owner;
>
> 	/* If you are not the owner, you can become one */
> 	if (ioctl == VHOST_SET_OWNER) {
...

	/* You must be the owner to do anything else */
	r = vhost_dev_check_owner(d);
	if (r)
		goto done;

So, why we are now initializing it to 0?

>@@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> 		if (ctx)
> 			eventfd_ctx_put(ctx);
> 		break;
>+	case VHOST_SET_INHERIT_FROM_OWNER:
>+		/*inherit_owner can only be modified before owner is set*/
>+		if (vhost_dev_has_owner(d))

And here, how this check can be false, if at the beginning of the
function we call vhost_dev_check_owner()?

Maybe your intention was to add this code before the
`vhost_dev_check_owner()` call, so this should explain why initialize
`r` to 0, but I'm not sure.

>+			break;

Should we return an error (e.g. -EPERM) in this case?

>+
>+		if (copy_from_user(&inherit_owner, argp,
>+				   sizeof(inherit_owner))) {
>+			r = -EFAULT;
>+			break;
>+		}
>+		d->inherit_owner = inherit_owner;
>+		break;
> 	default:
> 		r = -ENOIOCTLCMD;
> 		break;
>diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>index b95dd84eef2d..1e192038633d 100644
>--- a/include/uapi/linux/vhost.h
>+++ b/include/uapi/linux/vhost.h
>@@ -235,4 +235,6 @@
>  */
> #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
> 					      struct vhost_vring_state)
>+

Please add a documentation here, this is UAPI, so the user should
know what this ioctl does based on the parameter.

Thanks,
Stefano

>+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
> #endif
>-- 
>2.45.0
>

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Cindy Lu]

On Tue, Nov 5, 2024 at 6:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Nov 05, 2024 at 03:25:26PM +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      | 15 ++++++++++++++-
> > include/uapi/linux/vhost.h |  2 ++
> > 2 files changed, 16 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index c17dc01febcc..70c793b63905 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -2274,8 +2274,9 @@ 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;
>
> I don't know if something is missing in this patch, but I am confused:
>
> `r` is set few lines below...
>
> >       int i, fd;
> >+      bool inherit_owner;
> >
> >       /* If you are not the owner, you can become one */
> >       if (ioctl == VHOST_SET_OWNER) {
> ...
>
>         /* You must be the owner to do anything else */
>         r = vhost_dev_check_owner(d);
>         if (r)
>                 goto done;
>
> So, why we are now initializing it to 0?
>
r = 0 mean return successfully here.
Therefore, in the case VHOST_SET_INHERIT_FROM_OWNER function, I don't
need to set it again and can simply return.
....
    if (vhost_dev_has_owner(d))
       break;
.....
> >@@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >               if (ctx)
> >                       eventfd_ctx_put(ctx);
> >               break;
> >+      case VHOST_SET_INHERIT_FROM_OWNER:
> >+              /*inherit_owner can only be modified before owner is set*/
> >+              if (vhost_dev_has_owner(d))
>
> And here, how this check can be false, if at the beginning of the
> function we call vhost_dev_check_owner()?
>
> Maybe your intention was to add this code before the
> `vhost_dev_check_owner()` call, so this should explain why initialize
> `r` to 0, but I'm not sure.
>
Yes, in the function beginning, the code is
if (ioctl == VHOST_SET_OWNER) {
r = vhost_dev_set_owner(d);
goto done;
}
if the ioctl is not VHOST_SET_OWNER,  then the  code will not run the
function vhost_dev_set_owner.
This ioctl is used by userspace applications, so we cannot be certain
of the type and sequence of their calls; therefore, I added this
check.

> >+                      break;
>
> Should we return an error (e.g. -EPERM) in this case?
>
sure，will add this back
thanks
Cindy
> >+
> >+              if (copy_from_user(&inherit_owner, argp,
> >+                                 sizeof(inherit_owner))) {
> >+                      r = -EFAULT;
> >+                      break;
> >+              }
> >+              d->inherit_owner = inherit_owner;
> >+              break;
> >       default:
> >               r = -ENOIOCTLCMD;
> >               break;
> >diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> >index b95dd84eef2d..1e192038633d 100644
> >--- a/include/uapi/linux/vhost.h
> >+++ b/include/uapi/linux/vhost.h
> >@@ -235,4 +235,6 @@
> >  */
> > #define VHOST_VDPA_GET_VRING_SIZE     _IOWR(VHOST_VIRTIO, 0x82,       \
> >                                             struct vhost_vring_state)
> >+
>
> Please add a documentation here, this is UAPI, so the user should
> know what this ioctl does based on the parameter.
>
> Thanks,
> Stefano
>
> >+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
> > #endif
> >--
> >2.45.0
> >
>

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Stefano Garzarella]

On Thu, Nov 07, 2024 at 03:12:49PM +0800, Cindy Lu wrote:
>On Tue, Nov 5, 2024 at 6:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Tue, Nov 05, 2024 at 03:25:26PM +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      | 15 ++++++++++++++-
>> > include/uapi/linux/vhost.h |  2 ++
>> > 2 files changed, 16 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> >index c17dc01febcc..70c793b63905 100644
>> >--- a/drivers/vhost/vhost.c
>> >+++ b/drivers/vhost/vhost.c
>> >@@ -2274,8 +2274,9 @@ 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;
>>
>> I don't know if something is missing in this patch, but I am confused:
>>
>> `r` is set few lines below...
>>
>> >       int i, fd;
>> >+      bool inherit_owner;
>> >
>> >       /* If you are not the owner, you can become one */
>> >       if (ioctl == VHOST_SET_OWNER) {
>> ...
>>
>>         /* You must be the owner to do anything else */
>>         r = vhost_dev_check_owner(d);
>>         if (r)
>>                 goto done;
>>
>> So, why we are now initializing it to 0?
>>
>r = 0 mean return successfully here.
>Therefore, in the case VHOST_SET_INHERIT_FROM_OWNER function, I don't
>need to set it again and can simply return.
>....
>    if (vhost_dev_has_owner(d))
>       break;
>.....

Okay, but vhost_dev_check_owner() already set it to 0, so we can avoid 
that, no?

>> >@@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>> >               if (ctx)
>> >                       eventfd_ctx_put(ctx);
>> >               break;
>> >+      case VHOST_SET_INHERIT_FROM_OWNER:
>> >+              /*inherit_owner can only be modified before owner is set*/
>> >+              if (vhost_dev_has_owner(d))
>>
>> And here, how this check can be false, if at the beginning of the
>> function we call vhost_dev_check_owner()?
>>
>> Maybe your intention was to add this code before the
>> `vhost_dev_check_owner()` call, so this should explain why initialize
>> `r` to 0, but I'm not sure.
>>
>Yes, in the function beginning, the code is
>if (ioctl == VHOST_SET_OWNER) {
>r = vhost_dev_set_owner(d);
>goto done;
>}
>if the ioctl is not VHOST_SET_OWNER,  then the  code will not run the
>function vhost_dev_set_owner.

Sorry, I meant vhost_dev_check_owner(), not vhost_dev_set_owner().

I'll try to explain again.

After applying this series we have this code:

long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
{
	struct eventfd_ctx *ctx;
	u64 p;
	long r = 0;
	int i, fd;
	bool 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;
	}

	/* You must be the owner to do anything else */
	r = vhost_dev_check_owner(d);
	if (r)
		goto done;

	switch (ioctl) {
	...
     	case VHOST_SET_INHERIT_FROM_OWNER:
		/*inherit_owner can only be modified before owner is 
		 * set*/
		if (vhost_dev_has_owner(d))
			break;

IIUC this check is always true, so we always call `break` because at
the beginning of this function we call vhost_dev_check_owner() which
if `dev->mm != current->mm` (so it can't be null I guess) jumps directly
into `done`, returning an error.

So I still don't understand in which condition we can run the code after
this check.

Thanks,
Stefano

		if (copy_from_user(&inherit_owner, argp,
				   sizeof(inherit_owner))) {
			r = -EFAULT;
			break;
		}
		d->inherit_owner = inherit_owner;
		break;


>This ioctl is used by userspace applications, so we cannot be certain
>of the type and sequence of their calls; therefore, I added this
>check.
>
>> >+                      break;
>>
>> Should we return an error (e.g. -EPERM) in this case?
>>
>sure，will add this back
>thanks
>Cindy
>> >+
>> >+              if (copy_from_user(&inherit_owner, argp,
>> >+                                 sizeof(inherit_owner))) {
>> >+                      r = -EFAULT;
>> >+                      break;
>> >+              }
>> >+              d->inherit_owner = inherit_owner;
>> >+              break;
>> >       default:
>> >               r = -ENOIOCTLCMD;
>> >               break;
>> >diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>> >index b95dd84eef2d..1e192038633d 100644
>> >--- a/include/uapi/linux/vhost.h
>> >+++ b/include/uapi/linux/vhost.h
>> >@@ -235,4 +235,6 @@
>> >  */
>> > #define VHOST_VDPA_GET_VRING_SIZE     _IOWR(VHOST_VIRTIO, 0x82,       \
>> >                                             struct vhost_vring_state)
>> >+
>>
>> Please add a documentation here, this is UAPI, so the user should
>> know what this ioctl does based on the parameter.
>>
>> Thanks,
>> Stefano
>>
>> >+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
>> > #endif
>> >--
>> >2.45.0
>> >
>>
>

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Cindy Lu]

On Thu, Nov 7, 2024 at 6:03 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Nov 07, 2024 at 03:12:49PM +0800, Cindy Lu wrote:
> >On Tue, Nov 5, 2024 at 6:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Tue, Nov 05, 2024 at 03:25:26PM +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      | 15 ++++++++++++++-
> >> > include/uapi/linux/vhost.h |  2 ++
> >> > 2 files changed, 16 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >> >index c17dc01febcc..70c793b63905 100644
> >> >--- a/drivers/vhost/vhost.c
> >> >+++ b/drivers/vhost/vhost.c
> >> >@@ -2274,8 +2274,9 @@ 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;
> >>
> >> I don't know if something is missing in this patch, but I am confused:
> >>
> >> `r` is set few lines below...
> >>
> >> >       int i, fd;
> >> >+      bool inherit_owner;
> >> >
> >> >       /* If you are not the owner, you can become one */
> >> >       if (ioctl == VHOST_SET_OWNER) {
> >> ...
> >>
> >>         /* You must be the owner to do anything else */
> >>         r = vhost_dev_check_owner(d);
> >>         if (r)
> >>                 goto done;
> >>
> >> So, why we are now initializing it to 0?
> >>
> >r = 0 mean return successfully here.
> >Therefore, in the case VHOST_SET_INHERIT_FROM_OWNER function, I don't
> >need to set it again and can simply return.
> >....
> >    if (vhost_dev_has_owner(d))
> >       break;
> >.....
>
> Okay, but vhost_dev_check_owner() already set it to 0, so we can avoid
> that, no?
>
> >> >@@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >> >               if (ctx)
> >> >                       eventfd_ctx_put(ctx);
> >> >               break;
> >> >+      case VHOST_SET_INHERIT_FROM_OWNER:
> >> >+              /*inherit_owner can only be modified before owner is set*/
> >> >+              if (vhost_dev_has_owner(d))
> >>
> >> And here, how this check can be false, if at the beginning of the
> >> function we call vhost_dev_check_owner()?
> >>
> >> Maybe your intention was to add this code before the
> >> `vhost_dev_check_owner()` call, so this should explain why initialize
> >> `r` to 0, but I'm not sure.
> >>
> >Yes, in the function beginning, the code is
> >if (ioctl == VHOST_SET_OWNER) {
> >r = vhost_dev_set_owner(d);
> >goto done;
> >}
> >if the ioctl is not VHOST_SET_OWNER,  then the  code will not run the
> >function vhost_dev_set_owner.
>
> Sorry, I meant vhost_dev_check_owner(), not vhost_dev_set_owner().
>
> I'll try to explain again.
>
> After applying this series we have this code:
>
> long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> {
>         struct eventfd_ctx *ctx;
>         u64 p;
>         long r = 0;
>         int i, fd;
>         bool 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;
>         }
>
>         /* You must be the owner to do anything else */
>         r = vhost_dev_check_owner(d);
>         if (r)
>                 goto done;
>
>         switch (ioctl) {
>         ...
>         case VHOST_SET_INHERIT_FROM_OWNER:
>                 /*inherit_owner can only be modified before owner is
>                  * set*/
>                 if (vhost_dev_has_owner(d))
>                         break;
>
> IIUC this check is always true, so we always call `break` because at
> the beginning of this function we call vhost_dev_check_owner() which
> if `dev->mm != current->mm` (so it can't be null I guess) jumps directly
> into `done`, returning an error.
>
> So I still don't understand in which condition we can run the code after
> this check.
>
oh sorry I missed that check. I will move the new case back to the top
of function,
I didn't think it through before making this change; I just wanted to
clean up the code but forgot about the status.
Thanks
cindy
> Thanks,
> Stefano
>
>                 if (copy_from_user(&inherit_owner, argp,
>                                    sizeof(inherit_owner))) {
>                         r = -EFAULT;
>                         break;
>                 }
>                 d->inherit_owner = inherit_owner;
>                 break;
>
>
> >This ioctl is used by userspace applications, so we cannot be certain
> >of the type and sequence of their calls; therefore, I added this
> >check.
> >
> >> >+                      break;
> >>
> >> Should we return an error (e.g. -EPERM) in this case?
> >>
> >sure，will add this back
> >thanks
> >Cindy
> >> >+
> >> >+              if (copy_from_user(&inherit_owner, argp,
> >> >+                                 sizeof(inherit_owner))) {
> >> >+                      r = -EFAULT;
> >> >+                      break;
> >> >+              }
> >> >+              d->inherit_owner = inherit_owner;
> >> >+              break;
> >> >       default:
> >> >               r = -ENOIOCTLCMD;
> >> >               break;
> >> >diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> >> >index b95dd84eef2d..1e192038633d 100644
> >> >--- a/include/uapi/linux/vhost.h
> >> >+++ b/include/uapi/linux/vhost.h
> >> >@@ -235,4 +235,6 @@
> >> >  */
> >> > #define VHOST_VDPA_GET_VRING_SIZE     _IOWR(VHOST_VIRTIO, 0x82,       \
> >> >                                             struct vhost_vring_state)
> >> >+
> >>
> >> Please add a documentation here, this is UAPI, so the user should
> >> know what this ioctl does based on the parameter.
> >>
> >> Thanks,
> >> Stefano
> >>
> >> >+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
> >> > #endif
> >> >--
> >> >2.45.0
> >> >
> >>
> >
>

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Michael S. Tsirkin]

On Tue, Nov 05, 2024 at 03:25:26PM +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      | 15 ++++++++++++++-
>  include/uapi/linux/vhost.h |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index c17dc01febcc..70c793b63905 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2274,8 +2274,9 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  {
>  	struct eventfd_ctx *ctx;
>  	u64 p;
> -	long r;
> +	long r = 0;
>  	int i, fd;
> +	bool inherit_owner;
>  
>  	/* If you are not the owner, you can become one */
>  	if (ioctl == VHOST_SET_OWNER) {
> @@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  		if (ctx)
>  			eventfd_ctx_put(ctx);
>  		break;
> +	case VHOST_SET_INHERIT_FROM_OWNER:
> +		/*inherit_owner can only be modified before owner is set*/

bad coding style

> +		if (vhost_dev_has_owner(d))
> +			break;

is this silently failing? should return EBUSY or something like this.

> +
> +		if (copy_from_user(&inherit_owner, argp,
> +				   sizeof(inherit_owner))) {

not good, 


> +			r = -EFAULT;
> +			break;
> +		}
> +		d->inherit_owner = inherit_owner;




> +		break;
>  	default:
>  		r = -ENOIOCTLCMD;
>  		break;



This means any task can break out of jail
and steal root group system time by setting inherit_owner to 0
even if system is configured to inherit by default.

we need a modparam to block this.


> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index b95dd84eef2d..1e192038633d 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,6 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
>  					      struct vhost_vring_state)
> +
> +#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)

do not put bool in interfaces. something like u8 and validate it is 0 or
1.

>  #endif
> -- 
> 2.45.0

Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Michael S. Tsirkin]

On Tue, Nov 05, 2024 at 03:25:26PM +0800, Cindy Lu wrote:
> index b95dd84eef2d..1e192038633d 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,6 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
>  					      struct vhost_vring_state)
> +
> +#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)


set with no get is also not great.

>  #endif
> -- 
> 2.45.0

[PATCH v3 8/9] vhost_scsi: Add check for inherit_owner status

[Cindy Lu]

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

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

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 006ffacf1c56..05290298b5ab 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -2083,6 +2083,11 @@ vhost_scsi_ioctl(struct file *f,
 			return -EFAULT;
 		return vhost_scsi_set_features(vs, features);
 	case VHOST_NEW_WORKER:
+		/*vhost-scsi VHOST_NEW_WORKER requires inherit_owner to be true*/
+		if (vs->dev.inherit_owner != true)
+			return -EFAULT;
+
+		fallthrough;
 	case VHOST_FREE_WORKER:
 	case VHOST_ATTACH_VRING_WORKER:
 	case VHOST_GET_VRING_WORKER:
-- 
2.45.0

Re: [PATCH v3 8/9] vhost_scsi: Add check for inherit_owner status

[Mike Christie]

On 11/5/24 1:25 AM, Cindy Lu wrote:
> The vhost_scsi VHOST_NEW_WORKER requires the inherit_owner
> setting to be true. So we need to implement a check for this.
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/scsi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 006ffacf1c56..05290298b5ab 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -2083,6 +2083,11 @@ vhost_scsi_ioctl(struct file *f,
>  			return -EFAULT;
>  		return vhost_scsi_set_features(vs, features);
>  	case VHOST_NEW_WORKER:
> +		/*vhost-scsi VHOST_NEW_WORKER requires inherit_owner to be true*/

The above comment is not really needed since we know it's
required from the check being added.

If you want to add a comment, I think it should describe why we
require it as that's not obvious from just looking at the check.

So maybe a more useful comment would be:

/*
 * vhost_tasks will account for worker threads under the parent's
 * NPROC value but kthreads do not. To avoid userspace overflowing
 * the system with worker threads inherit_owner must be true.
 */

> +		if (vs->dev.inherit_owner != true)

This check and the comment would apply to all vhost drivers so it
should go in vhost_worker_ioctl's VHOST_NEW_WORKER handling.


> +			return -EFAULT;
> +
> +		fallthrough;
>  	case VHOST_FREE_WORKER:
>  	case VHOST_ATTACH_VRING_WORKER:
>  	case VHOST_GET_VRING_WORKER:

[PATCH v3 9/9] vhost: Expose the modparam inherit_owner_default

[Cindy Lu]

Expose the inherit_owner_default modparam by module_param().

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 70c793b63905..1a4ccf4f7316 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -43,6 +43,9 @@ module_param(max_iotlb_entries, int, 0444);
 MODULE_PARM_DESC(max_iotlb_entries,
 	"Maximum number of iotlb entries. (default: 2048)");
 static bool inherit_owner_default = true;
+module_param(inherit_owner_default, bool, 0444);
+MODULE_PARM_DESC(inherit_owner_default,
+		 "Set vhost_task mode as the default(default: Y)");
 
 enum {
 	VHOST_MEMORY_F_LOG = 0x1,
-- 
2.45.0

Re: [PATCH v3 0/9] vhost: Add support of kthread API

[Lei Yang]

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

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

On Tue, Nov 5, 2024 at 3:27 PM Cindy Lu <lulu@redhat.com> wrote:
>
> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> The vhost now use vhost_task and workers working as a child of the owner thread,
> which 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 patch, we introduce a module_param that allows users to select the
> operating mode. Additionally, a new UAPI is implemented to enable
> userspace applications to set their desired 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
>
> Tested with QEMU.
>
> Cindy Lu (9):
>   vhost: Add a new parameter to allow user select kthread
>   vhost: Add the vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: Add kthread support in function vhost_worker_create
>   vhost: Add kthread support in function vhost_worker_queue()
>   vhost: Add kthread support in function vhost_worker_destroy()
>   vhost: Add new UAPI to support change to task mode
>   vhost_scsi: Add check for inherit_owner status
>   vhost: Expose the modparam inherit_owner_default
>
>  drivers/vhost/scsi.c       |   5 +
>  drivers/vhost/vhost.c      | 194 ++++++++++++++++++++++++++++++++++---
>  drivers/vhost/vhost.h      |   7 ++
>  include/uapi/linux/vhost.h |   2 +
>  4 files changed, 193 insertions(+), 15 deletions(-)
>
> --
> 2.45.0
>
>