[PATCH v2 0/7] vhost: Add support of kthread API

[PATCH v2 0/7] vhost: Add support of kthread API

[Cindy Lu]

In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
vhost is now a child of the owner thread. but some old userspace is confused, In these patch we will let userspace to select the mode 

In this PATCH, the support of kthread is added back.
a module_param is added to select which mode we are using, and
a new UAPI is introduced to allow the userspace app to set which
mode they want to use.

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.
 
Tested with QEMU.

Cindy Lu (7):
  vhost: Add a new modparam to allow userspace select vhost_task
  vhost: Add kthread support in function vhost_worker_queue()
  vhost: Add kthread support in function vhost_workers_free()
  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 new UAPI to support change to task mode

 drivers/vhost/vhost.c      | 249 +++++++++++++++++++++++++++++++++++--
 drivers/vhost/vhost.h      |   1 +
 include/uapi/linux/vhost.h |   2 +
 3 files changed, 243 insertions(+), 9 deletions(-)

-- 
2.45.0

[PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[Cindy Lu]

The vhost is now using vhost_task and working as a child of the owner thread.
While this makes sense from containerization POV, some old userspace is
confused, as previously vhost not and so was allowed to steal cpu resources
from outside the container. So we add the kthread API support back

Add a new module parameter to allow userspace to select behaviour
between using kthread and task

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9ac25d08f473..a4a0bc34f59b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -41,6 +41,10 @@ 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)");
+bool enforce_inherit_owner = true;
+module_param(enforce_inherit_owner, bool, 0444);
+MODULE_PARM_DESC(enforce_inherit_owner,
+		 "enforce vhost use vhost_task(default: Y)");
 
 enum {
 	VHOST_MEMORY_F_LOG = 0x1,
-- 
2.45.0

Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[kernel test robot]

Hi Cindy,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Cindy-Lu/vhost-Add-a-new-modparam-to-allow-userspace-select-vhost_task/20241004-100307
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
patch link:    https://lore.kernel.org/r/20241004015937.2286459-2-lulu%40redhat.com
patch subject: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task
config: x86_64-randconfig-122-20241005 (https://download.01.org/0day-ci/archive/20241005/202410052351.1dCg9uLx-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241005/202410052351.1dCg9uLx-lkp@intel.com/reproduce)

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

sparse warnings: (new ones prefixed by >>)
>> drivers/vhost/vhost.c:44:6: sparse: sparse: symbol 'enforce_inherit_owner' was not declared. Should it be static?
   drivers/vhost/vhost.c:1899:54: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:1900:54: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:1901:55: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:2156:46: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:2236:48: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c: note: in included file (through include/linux/wait.h, include/linux/eventfd.h):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +/enforce_inherit_owner +44 drivers/vhost/vhost.c

    35	
    36	static ushort max_mem_regions = 64;
    37	module_param(max_mem_regions, ushort, 0444);
    38	MODULE_PARM_DESC(max_mem_regions,
    39		"Maximum number of memory regions in memory map. (default: 64)");
    40	static int max_iotlb_entries = 2048;
    41	module_param(max_iotlb_entries, int, 0444);
    42	MODULE_PARM_DESC(max_iotlb_entries,
    43		"Maximum number of iotlb entries. (default: 2048)");
  > 44	bool enforce_inherit_owner = true;
    45	module_param(enforce_inherit_owner, bool, 0444);
    46	MODULE_PARM_DESC(enforce_inherit_owner,
    47			 "enforce vhost use vhost_task(default: Y)");
    48	

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

Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[Stefano Garzarella]

On Fri, Oct 04, 2024 at 09:58:15AM GMT, Cindy Lu wrote:
>The vhost is now using vhost_task and working as a child of the owner thread.
>While this makes sense from containerization POV, some old userspace is
>confused, as previously vhost not 

not what?

> and so was allowed to steal cpu resources
>from outside the container. So we add the kthread API support back

Sorry, but it's not clear the reason.

I understand that we want to provide a way to bring back the previous 
behavior when we had kthreads, but why do we want that?
Do you have examples where the new mechanism is causing problems?

>
>Add a new module parameter to allow userspace to select behaviour
>between using kthread and task
>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 9ac25d08f473..a4a0bc34f59b 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -41,6 +41,10 @@ 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)");
>+bool enforce_inherit_owner = true;
        ^
This should be static:

$ make -j6 O=build C=2 drivers/vhost/
...
   CHECK   ../drivers/vhost/vhost.c
../drivers/vhost/vhost.c:45:6: warning: symbol 'enforce_inherit_owner' 
was not declared. Should it be static?

>+module_param(enforce_inherit_owner, bool, 0444);
>+MODULE_PARM_DESC(enforce_inherit_owner,
>+		 "enforce vhost use vhost_task(default: Y)");

I would follow the style of the other 2 parameters:
                  "Enforce vhost use vhost_task. (default: Y)"

With a view to simplifying bisection, we added this parameter in this 
patch, but it does nothing, so IMHO we should only add it at the end of 
the series when we have all the code ready. Maybe you can just add 
`enforce_inherit_owner` here or in the first patch where you need it, 
but I'd expose it with module_param() only when we have all the pieces 
in place.

About the param name, I'm not sure "enforce" is the right word, since 
IIUC the user can still change it using the ioctl. It would seem that 
set `enforce_inherit_owner` to true, it is always forced, but instead 
ioctl allows you to change it, right?

Is it more of a default behavior?
Something like `inherit_owner_default` ?

Thanks,
Stefano

Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[Jason Wang]

On Mon, Oct 7, 2024 at 9:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Oct 04, 2024 at 09:58:15AM GMT, Cindy Lu wrote:
> >The vhost is now using vhost_task and working as a child of the owner thread.
> >While this makes sense from containerization POV, some old userspace is
> >confused, as previously vhost not
>
> not what?
>
> > and so was allowed to steal cpu resources
> >from outside the container. So we add the kthread API support back
>
> Sorry, but it's not clear the reason.
>
> I understand that we want to provide a way to bring back the previous
> behavior when we had kthreads, but why do we want that?
> Do you have examples where the new mechanism is causing problems?
>

The main difference is whose (kthreadd or the owner) attributes
(namespace, affinities) would vhost thread inherit.

The owner's attributes tend to be different from kthreadd, so
management might be surprised for example, vhost might be created in
different namespaces or having different scheduling affinities.

Thanks

Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[Stefano Garzarella]

On Wed, Oct 09, 2024 at 03:28:19PM GMT, Jason Wang wrote:
>On Mon, Oct 7, 2024 at 9:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Fri, Oct 04, 2024 at 09:58:15AM GMT, Cindy Lu wrote:
>> >The vhost is now using vhost_task and working as a child of the owner thread.
>> >While this makes sense from containerization POV, some old userspace is
>> >confused, as previously vhost not
>>
>> not what?
>>
>> > and so was allowed to steal cpu resources
>> >from outside the container. So we add the kthread API support back
>>
>> Sorry, but it's not clear the reason.
>>
>> I understand that we want to provide a way to bring back the previous
>> behavior when we had kthreads, but why do we want that?
>> Do you have examples where the new mechanism is causing problems?
>>
>
>The main difference is whose (kthreadd or the owner) attributes
>(namespace, affinities) would vhost thread inherit.
>
>The owner's attributes tend to be different from kthreadd, so
>management might be surprised for example, vhost might be created in
>different namespaces or having different scheduling affinities.

Okay, so this requires some API to allow the managment to understand how 
the device vhost will be created.

But why do we need to restore the old behavior with a kthread where the 
resource accounting is completely disconnected from userspace?
For the old managments that don't expect this?

BTW I would suggest adding all this information in this commit, in the 
parameter/IOCTL documentation, and in the cover letter because IMHO it 
is very important.

Thanks,
Stefano

Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[Jason Wang]

On Wed, Oct 9, 2024 at 4:10 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Oct 09, 2024 at 03:28:19PM GMT, Jason Wang wrote:
> >On Mon, Oct 7, 2024 at 9:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Fri, Oct 04, 2024 at 09:58:15AM GMT, Cindy Lu wrote:
> >> >The vhost is now using vhost_task and working as a child of the owner thread.
> >> >While this makes sense from containerization POV, some old userspace is
> >> >confused, as previously vhost not
> >>
> >> not what?
> >>
> >> > and so was allowed to steal cpu resources
> >> >from outside the container. So we add the kthread API support back
> >>
> >> Sorry, but it's not clear the reason.
> >>
> >> I understand that we want to provide a way to bring back the previous
> >> behavior when we had kthreads, but why do we want that?
> >> Do you have examples where the new mechanism is causing problems?
> >>
> >
> >The main difference is whose (kthreadd or the owner) attributes
> >(namespace, affinities) would vhost thread inherit.
> >
> >The owner's attributes tend to be different from kthreadd, so
> >management might be surprised for example, vhost might be created in
> >different namespaces or having different scheduling affinities.
>
> Okay, so this requires some API to allow the managment to understand how
> the device vhost will be created.
>
> But why do we need to restore the old behavior with a kthread where the
> resource accounting is completely disconnected from userspace?
> For the old managments that don't expect this?

Yes, as such change can be easily noticed by the user space that
breaks existing management layers or tools.

>
> BTW I would suggest adding all this information in this commit, in the
> parameter/IOCTL documentation, and in the cover letter because IMHO it
> is very important.

I've asked this in the past. Cindy, please make sure such information
were included in the next version in the cover letter.

Thanks

>
> Thanks,
> Stefano
>

Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[Cindy Lu]

On Wed, 9 Oct 2024 at 16:20, Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 9, 2024 at 4:10 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Wed, Oct 09, 2024 at 03:28:19PM GMT, Jason Wang wrote:
> > >On Mon, Oct 7, 2024 at 9:31 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >>
> > >> On Fri, Oct 04, 2024 at 09:58:15AM GMT, Cindy Lu wrote:
> > >> >The vhost is now using vhost_task and working as a child of the owner thread.
> > >> >While this makes sense from containerization POV, some old userspace is
> > >> >confused, as previously vhost not
> > >>
> > >> not what?
> > >>
> > >> > and so was allowed to steal cpu resources
> > >> >from outside the container. So we add the kthread API support back
> > >>
> > >> Sorry, but it's not clear the reason.
> > >>
> > >> I understand that we want to provide a way to bring back the previous
> > >> behavior when we had kthreads, but why do we want that?
> > >> Do you have examples where the new mechanism is causing problems?
> > >>
> > >
> > >The main difference is whose (kthreadd or the owner) attributes
> > >(namespace, affinities) would vhost thread inherit.
> > >
> > >The owner's attributes tend to be different from kthreadd, so
> > >management might be surprised for example, vhost might be created in
> > >different namespaces or having different scheduling affinities.
> >
> > Okay, so this requires some API to allow the managment to understand how
> > the device vhost will be created.
> >
> > But why do we need to restore the old behavior with a kthread where the
> > resource accounting is completely disconnected from userspace?
> > For the old managments that don't expect this?
>
> Yes, as such change can be easily noticed by the user space that
> breaks existing management layers or tools.
>
> >
> > BTW I would suggest adding all this information in this commit, in the
> > parameter/IOCTL documentation, and in the cover letter because IMHO it
> > is very important.
>
> I've asked this in the past. Cindy, please make sure such information
> were included in the next version in the cover letter.
>
> Thanks
>
Thanks Jason, I will add this
Thanks
cindy
> >
> > Thanks,
> > Stefano
> >
>

Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task

[Cindy Lu]

On Mon, 7 Oct 2024 at 21:31, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Oct 04, 2024 at 09:58:15AM GMT, Cindy Lu wrote:
> >The vhost is now using vhost_task and working as a child of the owner thread.
> >While this makes sense from containerization POV, some old userspace is
> >confused, as previously vhost not
>
> not what?
>
> > and so was allowed to steal cpu resources
> >from outside the container. So we add the kthread API support back
>
> Sorry, but it's not clear the reason.
>
> I understand that we want to provide a way to bring back the previous
> behavior when we had kthreads, but why do we want that?
> Do you have examples where the new mechanism is causing problems?
>
> >
> >Add a new module parameter to allow userspace to select behaviour
> >between using kthread and task
> >
> >Signed-off-by: Cindy Lu <lulu@redhat.com>
> >---
> > drivers/vhost/vhost.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index 9ac25d08f473..a4a0bc34f59b 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -41,6 +41,10 @@ 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)");
> >+bool enforce_inherit_owner = true;
>         ^
> This should be static:
>
> $ make -j6 O=build C=2 drivers/vhost/
> ...
>    CHECK   ../drivers/vhost/vhost.c
> ../drivers/vhost/vhost.c:45:6: warning: symbol 'enforce_inherit_owner'
> was not declared. Should it be static?
>
> >+module_param(enforce_inherit_owner, bool, 0444);
> >+MODULE_PARM_DESC(enforce_inherit_owner,
> >+               "enforce vhost use vhost_task(default: Y)");
>
> I would follow the style of the other 2 parameters:
>                   "Enforce vhost use vhost_task. (default: Y)"
>
> With a view to simplifying bisection, we added this parameter in this
> patch, but it does nothing, so IMHO we should only add it at the end of
> the series when we have all the code ready. Maybe you can just add
> `enforce_inherit_owner` here or in the first patch where you need it,
> but I'd expose it with module_param() only when we have all the pieces
> in place.
>
> About the param name, I'm not sure "enforce" is the right word, since
> IIUC the user can still change it using the ioctl. It would seem that
> set `enforce_inherit_owner` to true, it is always forced, but instead
> ioctl allows you to change it, right?
>
> Is it more of a default behavior?
> Something like `inherit_owner_default` ?
>
> Thanks,
> Stefano
>
thanks, Stefano, I will change this
thanks
cindy

[PATCH v2 2/7] vhost: Add kthread support in function vhost_worker_queue()

[Cindy Lu]

Added back the previously removed function vhost_worker_queue() and
renamed it to vhost_worker_queue_khtread(). The new vhost_worker_queue()
will select the different mode based on the value of the parameter.

The old function vhost_work_queue()  was change to support task in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
changed in
commit f9010dbdce91 ("fork, vhost: Use CLONE_THREAD to fix freezer/ps regression")
and also was change the name of function to vhost_worker_queue() in
commit 0921dddcb589 ("vhost: take worker or vq instead of dev for queueing")

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index a4a0bc34f59b..ad359d4b725f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -237,8 +237,8 @@ void vhost_poll_stop(struct vhost_poll *poll)
 }
 EXPORT_SYMBOL_GPL(vhost_poll_stop);
 
-static void vhost_worker_queue(struct vhost_worker *worker,
-			       struct vhost_work *work)
+static void vhost_worker_queue_task(struct vhost_worker *worker,
+				    struct vhost_work *work)
 {
 	if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
 		/* We can only add the work to the list after we're
@@ -250,6 +250,32 @@ static void vhost_worker_queue(struct vhost_worker *worker,
 	}
 }
 
+static void vhost_work_queue_kthread(struct vhost_worker *worker,
+				     struct vhost_work *work)
+{
+	if (!worker)
+		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);
+
+		wake_up_process(worker->task);
+	}
+}
+
+static void vhost_worker_queue(struct vhost_worker *worker,
+			       struct vhost_work *work)
+{
+	if (enforce_inherit_owner) {
+		vhost_worker_queue_task(worker, work);
+	} else {
+		vhost_work_queue_kthread(worker, work);
+	}
+}
 bool vhost_vq_work_queue(struct vhost_virtqueue *vq, struct vhost_work *work)
 {
 	struct vhost_worker *worker;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..c7f126fd09e8 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -27,6 +27,7 @@ struct vhost_work {
 };
 
 struct vhost_worker {
+	struct task_struct	*task;
 	struct vhost_task	*vtsk;
 	struct vhost_dev	*dev;
 	/* Used to serialize device wide flushing with worker swapping. */
-- 
2.45.0

[PATCH v2 3/7] vhost: Add kthread support in function vhost_workers_free()

[Cindy Lu]

Added back the previously removed function vhost_workers_free() and
renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
will select the different mode based on the value of the parameter.

The old function vhost_workers_free was change to support task in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
also changed in
commit a284f09effea ("vhost: Fix crash during early vhost_transport_send_pkt calls")
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 | 52 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ad359d4b725f..fed6671c1ffb 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -648,8 +648,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
 	dev->mm = NULL;
 }
 
-static void vhost_worker_destroy(struct vhost_dev *dev,
-				 struct vhost_worker *worker)
+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
+					 struct vhost_worker *worker)
+{
+	if (!worker)
+		return;
+
+	WARN_ON(!llist_empty(&worker->work_list));
+
+	xa_erase(&dev->worker_xa, worker->id);
+	kthread_stop(worker->task);
+	kfree(worker);
+}
+
+static void vhost_worker_destroy_task(struct vhost_dev *dev,
+				      struct vhost_worker *worker)
 {
 	if (!worker)
 		return;
@@ -660,7 +673,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
 	kfree(worker);
 }
 
-static void vhost_workers_free(struct vhost_dev *dev)
+static void vhost_workers_free_task(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
 	unsigned long i;
@@ -675,10 +688,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	 * created but couldn't clean up (it forgot or crashed).
 	 */
 	xa_for_each(&dev->worker_xa, i, worker)
-		vhost_worker_destroy(dev, worker);
+		vhost_worker_destroy_task(dev, worker);
 	xa_destroy(&dev->worker_xa);
 }
 
+static void vhost_workers_free_kthread(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	unsigned long i;
+
+	if (!dev->use_worker)
+		return;
+
+	for (i = 0; i < dev->nvqs; i++)
+		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
+	/*
+	 * 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)
+		vhost_worker_destroy_kthread(dev, worker);
+	xa_destroy(&dev->worker_xa);
+}
+
+static void vhost_workers_free(struct vhost_dev *dev)
+{
+	if (enforce_inherit_owner)
+		vhost_workers_free_task(dev);
+	else
+		vhost_workers_free_kthread(dev);
+}
 static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
@@ -846,7 +885,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
 	__vhost_worker_flush(worker);
 	mutex_unlock(&worker->mutex);
 
-	vhost_worker_destroy(dev, worker);
+	if (enforce_inherit_owner)
+		vhost_worker_destroy_task(dev, worker);
+	else
+		vhost_worker_destroy_kthread(dev, worker);
 	return 0;
 }
 
-- 
2.45.0

Re: [PATCH v2 3/7] vhost: Add kthread support in function vhost_workers_free()

[Stefano Garzarella]

On Fri, Oct 04, 2024 at 09:58:17AM GMT, Cindy Lu wrote:
>Added back the previously removed function vhost_workers_free() and
>renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
>will select the different mode based on the value of the parameter.
>
>The old function vhost_workers_free was change to support task in
>commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
>also changed in
>commit a284f09effea ("vhost: Fix crash during early vhost_transport_send_pkt calls")
>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 | 52 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 47 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index ad359d4b725f..fed6671c1ffb 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -648,8 +648,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
> 	dev->mm = NULL;
> }
>
>-static void vhost_worker_destroy(struct vhost_dev *dev,
>-				 struct vhost_worker *worker)
>+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
>+					 struct vhost_worker *worker)
>+{
>+	if (!worker)
>+		return;
>+
>+	WARN_ON(!llist_empty(&worker->work_list));
>+
>+	xa_erase(&dev->worker_xa, worker->id);
>+	kthread_stop(worker->task);
>+	kfree(worker);
>+}
>+
>+static void vhost_worker_destroy_task(struct vhost_dev *dev,
>+				      struct vhost_worker *worker)
> {
> 	if (!worker)
> 		return;
>@@ -660,7 +673,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
> 	kfree(worker);
> }
>
>-static void vhost_workers_free(struct vhost_dev *dev)
>+static void vhost_workers_free_task(struct vhost_dev *dev)
> {
> 	struct vhost_worker *worker;
> 	unsigned long i;
>@@ -675,10 +688,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
> 	 * created but couldn't clean up (it forgot or crashed).
> 	 */
> 	xa_for_each(&dev->worker_xa, i, worker)
>-		vhost_worker_destroy(dev, worker);
>+		vhost_worker_destroy_task(dev, worker);
> 	xa_destroy(&dev->worker_xa);
> }
>
>+static void vhost_workers_free_kthread(struct vhost_dev *dev)
>+{
>+	struct vhost_worker *worker;
>+	unsigned long i;
>+
>+	if (!dev->use_worker)
>+		return;
>+
>+	for (i = 0; i < dev->nvqs; i++)
>+		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>+	/*
>+	 * 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)
>+		vhost_worker_destroy_kthread(dev, worker);
>+	xa_destroy(&dev->worker_xa);
>+}
>+
>+static void vhost_workers_free(struct vhost_dev *dev)
>+{
>+	if (enforce_inherit_owner)
>+		vhost_workers_free_task(dev);
>+	else
>+		vhost_workers_free_kthread(dev);
>+}

nit: new line here

> static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> {
> 	struct vhost_worker *worker;
>@@ -846,7 +885,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
> 	__vhost_worker_flush(worker);
> 	mutex_unlock(&worker->mutex);
>
>-	vhost_worker_destroy(dev, worker);
>+	if (enforce_inherit_owner)
>+		vhost_worker_destroy_task(dev, worker);
>+	else
>+		vhost_worker_destroy_kthread(dev, worker);
> 	return 0;
> }
>
>-- 
>2.45.0
>
>

Re: [PATCH v2 3/7] vhost: Add kthread support in function vhost_workers_free()

[Cindy Lu]

On Mon, 7 Oct 2024 at 21:33, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Oct 04, 2024 at 09:58:17AM GMT, Cindy Lu wrote:
> >Added back the previously removed function vhost_workers_free() and
> >renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
> >will select the different mode based on the value of the parameter.
> >
> >The old function vhost_workers_free was change to support task in
> >commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> >also changed in
> >commit a284f09effea ("vhost: Fix crash during early vhost_transport_send_pkt calls")
> >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 | 52 ++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 47 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >index ad359d4b725f..fed6671c1ffb 100644
> >--- a/drivers/vhost/vhost.c
> >+++ b/drivers/vhost/vhost.c
> >@@ -648,8 +648,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
> >       dev->mm = NULL;
> > }
> >
> >-static void vhost_worker_destroy(struct vhost_dev *dev,
> >-                               struct vhost_worker *worker)
> >+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
> >+                                       struct vhost_worker *worker)
> >+{
> >+      if (!worker)
> >+              return;
> >+
> >+      WARN_ON(!llist_empty(&worker->work_list));
> >+
> >+      xa_erase(&dev->worker_xa, worker->id);
> >+      kthread_stop(worker->task);
> >+      kfree(worker);
> >+}
> >+
> >+static void vhost_worker_destroy_task(struct vhost_dev *dev,
> >+                                    struct vhost_worker *worker)
> > {
> >       if (!worker)
> >               return;
> >@@ -660,7 +673,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
> >       kfree(worker);
> > }
> >
> >-static void vhost_workers_free(struct vhost_dev *dev)
> >+static void vhost_workers_free_task(struct vhost_dev *dev)
> > {
> >       struct vhost_worker *worker;
> >       unsigned long i;
> >@@ -675,10 +688,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
> >        * created but couldn't clean up (it forgot or crashed).
> >        */
> >       xa_for_each(&dev->worker_xa, i, worker)
> >-              vhost_worker_destroy(dev, worker);
> >+              vhost_worker_destroy_task(dev, worker);
> >       xa_destroy(&dev->worker_xa);
> > }
> >
> >+static void vhost_workers_free_kthread(struct vhost_dev *dev)
> >+{
> >+      struct vhost_worker *worker;
> >+      unsigned long i;
> >+
> >+      if (!dev->use_worker)
> >+              return;
> >+
> >+      for (i = 0; i < dev->nvqs; i++)
> >+              rcu_assign_pointer(dev->vqs[i]->worker, NULL);
> >+      /*
> >+       * 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)
> >+              vhost_worker_destroy_kthread(dev, worker);
> >+      xa_destroy(&dev->worker_xa);
> >+}
> >+
> >+static void vhost_workers_free(struct vhost_dev *dev)
> >+{
> >+      if (enforce_inherit_owner)
> >+              vhost_workers_free_task(dev);
> >+      else
> >+              vhost_workers_free_kthread(dev);
> >+}
>
> nit: new line here
>
sure, will fix this
> > static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> > {
> >       struct vhost_worker *worker;
> >@@ -846,7 +885,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
> >       __vhost_worker_flush(worker);
> >       mutex_unlock(&worker->mutex);
> >
> >-      vhost_worker_destroy(dev, worker);
> >+      if (enforce_inherit_owner)
> >+              vhost_worker_destroy_task(dev, worker);
> >+      else
> >+              vhost_worker_destroy_kthread(dev, worker);
> >       return 0;
> > }
> >
> >--
> >2.45.0
> >
> >
>

Re: [PATCH v2 3/7] vhost: Add kthread support in function vhost_workers_free()

[Mike Christie]

On 10/3/24 8:58 PM, Cindy Lu wrote:
> +static void vhost_workers_free(struct vhost_dev *dev)
> +{
> +	if (enforce_inherit_owner)
> +		vhost_workers_free_task(dev);
> +	else
> +		vhost_workers_free_kthread(dev);
> +}

With patch 7, userspace could change enforce_inherit_owner after
we created thread and we would call the wrong function above.

Re: [PATCH v2 3/7] vhost: Add kthread support in function vhost_workers_free()

[Cindy Lu]

On Tue, 15 Oct 2024 at 05:06, Mike Christie <michael.christie@oracle.com> wrote:
>
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > +static void vhost_workers_free(struct vhost_dev *dev)
> > +{
> > +     if (enforce_inherit_owner)
> > +             vhost_workers_free_task(dev);
> > +     else
> > +             vhost_workers_free_kthread(dev);
> > +}
>
> With patch 7, userspace could change enforce_inherit_owner after
> we created thread and we would call the wrong function above.
>
enforce_inherit_owner will only change before the owner was set.
the process is like set enforce_inherit_owner---->set owner->
thread/task creating
in in patch 7's code I have add the check for vhost's owner, if the
owner was set, the ioctl
to set enforce_inherit_owner will fail
Thanks
Cindy

Re: [PATCH v2 3/7] vhost: Add kthread support in function vhost_workers_free()

[Stefano Garzarella]

On Tue, Oct 15, 2024 at 02:05:47PM +0800, Cindy Lu wrote:
>On Tue, 15 Oct 2024 at 05:06, Mike Christie <michael.christie@oracle.com> wrote:
>>
>> On 10/3/24 8:58 PM, Cindy Lu wrote:
>> > +static void vhost_workers_free(struct vhost_dev *dev)
>> > +{
>> > +     if (enforce_inherit_owner)
>> > +             vhost_workers_free_task(dev);
>> > +     else
>> > +             vhost_workers_free_kthread(dev);
>> > +}
>>
>> With patch 7, userspace could change enforce_inherit_owner after
>> we created thread and we would call the wrong function above.
>>
>enforce_inherit_owner will only change before the owner was set.

As I pointed out in patch 7, enforce_inherit_owner seems to be shared 
among all vhost devices, so what happens if for example a user sets it 
to /dev/vhost-net, while /dev/vhost-vsock is already initialized and 
therefore already has an owner?

Thanks,
Stefano

>the process is like set enforce_inherit_owner---->set owner->
>thread/task creating
>in in patch 7's code I have add the check for vhost's owner, if the
>owner was set, the ioctl
>to set enforce_inherit_owner will fail
>Thanks
>Cindy
>
>

Re: [PATCH v2 3/7] vhost: Add kthread support in function vhost_workers_free()

[Cindy Lu]

On Tue, 15 Oct 2024 at 14:52, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Oct 15, 2024 at 02:05:47PM +0800, Cindy Lu wrote:
> >On Tue, 15 Oct 2024 at 05:06, Mike Christie <michael.christie@oracle.com> wrote:
> >>
> >> On 10/3/24 8:58 PM, Cindy Lu wrote:
> >> > +static void vhost_workers_free(struct vhost_dev *dev)
> >> > +{
> >> > +     if (enforce_inherit_owner)
> >> > +             vhost_workers_free_task(dev);
> >> > +     else
> >> > +             vhost_workers_free_kthread(dev);
> >> > +}
> >>
> >> With patch 7, userspace could change enforce_inherit_owner after
> >> we created thread and we would call the wrong function above.
> >>
> >enforce_inherit_owner will only change before the owner was set.
>
> As I pointed out in patch 7, enforce_inherit_owner seems to be shared
> among all vhost devices, so what happens if for example a user sets it
> to /dev/vhost-net, while /dev/vhost-vsock is already initialized and
> therefore already has an owner?
>
> Thanks,
> Stefano
>
You are correct, I will think about this and provide a new version
Thanks
cindy

> >the process is like set enforce_inherit_owner---->set owner->
> >thread/task creating
> >in in patch 7's code I have add the check for vhost's owner, if the
> >owner was set, the ioctl
> >to set enforce_inherit_owner will fail
> >Thanks
> >Cindy
> >
> >
>

[PATCH v2 4/7] vhost: Add the vhost_worker to support kthread

[Cindy Lu]

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

Re: [PATCH v2 4/7] vhost: Add the vhost_worker to support kthread

[Mike Christie]

On 10/3/24 8:58 PM, Cindy Lu wrote:
> Add back the previously removed vhost_worker function to support the kthread
> and rename it 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 fed6671c1ffb..349499139f4f 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -418,6 +418,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;
> +}
I think there is a lot of unneeded code duplication where in the functions
you are adding there's only 1-3 lines different. To fix this I think we
could:


1. Go really invasive and modify copy_process and its helpers so they take
a task_struct instead of using "current". We can then just pass in "current"
or kthreadd. We can then use most of the existing vhost code. We just
need a per vhost_worker check/field for the mm and cgroup use like:

vhost_task_fn():
{
...
	/* The mm would be passed in during creation for the kthread case */
	if (vtsk->mm)
		kthread_use_mm(vtsk->mm);

Or

2. Go hacky and in the vhost code, when we get a VHOST_SET_OWNER call create
a tmp kthread. The tmp kthread would then call the existing vhost_worker_create
function. The resulting vhost_task would inherit the kthreadd settings like we
want. We then just need a per vhost_worker check/field for the mm and cgroup use
like above.

Or

3. There doesn't seem to be a lot of differences in the functions you are
adding. In the function above the only differences are the mm calls and kthread
should stop. In the destroy functions it's kthread_stop. In the queue function its
wake_up_process. In create its kthread_create, stop and the cgroup functions.

I think we could add just some callouts on the vhost_task or vhost_worker for stop,
wakeup and use mm. For create we would do something like

vhost_worker_create()
....
	worker = kzalloc();

	if (inherit from caller) {
		worker->stop = vhost_task_stop;
		worker->wakeup = vhost_task_wakeup;

		worker->vtsk = vhost_task_create();
	} else {
		worker->stop = vhost_kthread_stop;
		worker->wakeup = vhost_kthread_wakeup;

		worker->tsk = kthread->create();
		vhost_kthread_setup_cgroups();
	}
...
}

vhost_worker_destroy()
{
        if (!worker)
                return;

        WARN_ON(!llist_empty(&worker->work_list));
        xa_erase(&dev->worker_xa, worker->id);
        worker->stop(worker);
        kfree(worker);
}

Re: [PATCH v2 4/7] vhost: Add the vhost_worker to support kthread

[Cindy Lu]

On Tue, 15 Oct 2024 at 06:56, Mike Christie <michael.christie@oracle.com> wrote:
>
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > Add back the previously removed vhost_worker function to support the kthread
> > and rename it 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 fed6671c1ffb..349499139f4f 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -418,6 +418,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;
> > +}
> I think there is a lot of unneeded code duplication where in the functions
> you are adding there's only 1-3 lines different. To fix this I think we
> could:
>
Thank you mike， I will try this and provide a new version
thanks
cindy
>
> 1. Go really invasive and modify copy_process and its helpers so they take
> a task_struct instead of using "current". We can then just pass in "current"
> or kthreadd. We can then use most of the existing vhost code. We just
> need a per vhost_worker check/field for the mm and cgroup use like:
>
> vhost_task_fn():
> {
> ...
>         /* The mm would be passed in during creation for the kthread case */
>         if (vtsk->mm)
>                 kthread_use_mm(vtsk->mm);
>
> Or
>
> 2. Go hacky and in the vhost code, when we get a VHOST_SET_OWNER call create
> a tmp kthread. The tmp kthread would then call the existing vhost_worker_create
> function. The resulting vhost_task would inherit the kthreadd settings like we
> want. We then just need a per vhost_worker check/field for the mm and cgroup use
> like above.
>
> Or
>
> 3. There doesn't seem to be a lot of differences in the functions you are
> adding. In the function above the only differences are the mm calls and kthread
> should stop. In the destroy functions it's kthread_stop. In the queue function its
> wake_up_process. In create its kthread_create, stop and the cgroup functions.
>
> I think we could add just some callouts on the vhost_task or vhost_worker for stop,
> wakeup and use mm. For create we would do something like
>
> vhost_worker_create()
> ....
>         worker = kzalloc();
>
>         if (inherit from caller) {
>                 worker->stop = vhost_task_stop;
>                 worker->wakeup = vhost_task_wakeup;
>
>                 worker->vtsk = vhost_task_create();
>         } else {
>                 worker->stop = vhost_kthread_stop;
>                 worker->wakeup = vhost_kthread_wakeup;
>
>                 worker->tsk = kthread->create();
>                 vhost_kthread_setup_cgroups();
>         }
> ...
> }
>
> vhost_worker_destroy()
> {
>         if (!worker)
>                 return;
>
>         WARN_ON(!llist_empty(&worker->work_list));
>         xa_erase(&dev->worker_xa, worker->id);
>         worker->stop(worker);
>         kfree(worker);
> }
>
>

[PATCH v2 5/7] 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 349499139f4f..eb30da658bfe 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>
@@ -649,6 +650,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

[PATCH v2 6/7] vhost: Add kthread support in function vhost_worker_create

[Cindy Lu]

Split the function vhost_worker_create to support both task and kthread

Added back the previous old function vhost_worker_create and rename it to
vhost_worker_create_khtread to support the khtread.

The new vhost_worker_create will be selected which to use based on the
value of the parameter.

the old function vhost_worker_create was change to support task in
commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
also changed in
commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray")
commit c011bb669ddc ("vhost: dynamically allocate vhost_worker")

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index eb30da658bfe..08c9e77916ca 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -808,7 +808,8 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	else
 		vhost_workers_free_kthread(dev);
 }
-static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
+
+static struct vhost_worker *vhost_worker_create_task(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
 	struct vhost_task *vtsk;
@@ -849,6 +850,50 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 	return NULL;
 }
 
+static struct vhost_worker *vhost_worker_create_kthread(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	struct task_struct *task;
+	int ret;
+	u32 id;
+
+	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
+	if (!worker)
+		return NULL;
+
+	worker->dev = dev;
+	worker->kcov_handle = kcov_common_handle();
+
+	mutex_init(&worker->mutex);
+	init_llist_head(&worker->work_list);
+
+	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;
+	wake_up_process(task); /* avoid contributing to loadavg */
+	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+	if (ret < 0)
+		goto stop_worker;
+	worker->id = id;
+
+	ret = vhost_attach_cgroups(dev);
+	if (ret)
+		goto stop_worker;
+
+	return worker;
+
+stop_worker:
+	kthread_stop(worker->task);
+free_worker:
+	kfree(worker);
+	return NULL;
+}
+
 /* Caller must have device mutex */
 static void __vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 				     struct vhost_worker *worker)
@@ -937,6 +982,14 @@ static int vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 	return 0;
 }
 
+static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
+{
+	if (enforce_inherit_owner)
+		return vhost_worker_create_task(dev);
+	else
+		return vhost_worker_create_kthread(dev);
+}
+
 /* Caller must have device mutex */
 static int vhost_new_worker(struct vhost_dev *dev,
 			    struct vhost_worker_state *info)
-- 
2.45.0

Re: [PATCH v2 6/7] vhost: Add kthread support in function vhost_worker_create

[Mike Christie]

On 10/3/24 8:58 PM, Cindy Lu wrote:
> +static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> +{
> +	if (enforce_inherit_owner)
> +		return vhost_worker_create_task(dev);
> +	else
> +		return vhost_worker_create_kthread(dev);
> +}

The reason we are in this whole mess is because for vhost-scsi
we wanted to create multiple worker threads as the single thread
became a bottleneck. We couldn't use kthreads because they were
not accounted for under thread that created it and we could
exceed the NPROC_LIMIT.

So with the above code, we could hit that problem again if userspace
did the VHOST_NEW_WORKER ioctl and enforce_inherit_owner=false.

You would want a check for enforce_inherit_owner in vhost_worker_ioctl.

Re: [PATCH v2 6/7] vhost: Add kthread support in function vhost_worker_create

[Cindy Lu]

On Tue, 15 Oct 2024 at 05:02, Mike Christie <michael.christie@oracle.com> wrote:
>
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > +static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> > +{
> > +     if (enforce_inherit_owner)
> > +             return vhost_worker_create_task(dev);
> > +     else
> > +             return vhost_worker_create_kthread(dev);
> > +}
>
> The reason we are in this whole mess is because for vhost-scsi
> we wanted to create multiple worker threads as the single thread
> became a bottleneck. We couldn't use kthreads because they were
> not accounted for under thread that created it and we could
> exceed the NPROC_LIMIT.
>
> So with the above code, we could hit that problem again if userspace
> did the VHOST_NEW_WORKER ioctl and enforce_inherit_owner=false.
>
> You would want a check for enforce_inherit_owner in vhost_worker_ioctl.
>
sure, will add this check
Thanks
cindy

[PATCH v2 7/7] vhost: Add new UAPI to support change to task mode

[Cindy Lu]

Add a new UAPI to support setting the vhost device to
use task mode. The user space application needs to use
VHOST_SET_INHERIT_FROM_OWNER to set the mode.
This setting must be set before VHOST_SET_OWNER is set.

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

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 08c9e77916ca..0e5c81026acd 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2341,8 +2341,24 @@ 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 (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
+		/* Is there an owner already? */
+		if (vhost_dev_has_owner(d)) {
+			r = -EBUSY;
+			goto done;
+		}
+		if (copy_from_user(&inherit_owner, argp,
+				   sizeof(inherit_owner))) {
+			r = -EFAULT;
+			goto done;
+		}
+		enforce_inherit_owner = inherit_owner;
+		goto done;
+	}
 
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
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 v2 7/7] vhost: Add new UAPI to support change to task mode

[Stefano Garzarella]

On Fri, Oct 04, 2024 at 09:58:21AM GMT, Cindy Lu wrote:
>Add a new UAPI to support setting the vhost device to
>use task mode. The user space application needs to use
>VHOST_SET_INHERIT_FROM_OWNER to set the mode.
>This setting must be set before VHOST_SET_OWNER is set.

Why?

>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> include/uapi/linux/vhost.h |  2 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 08c9e77916ca..0e5c81026acd 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -2341,8 +2341,24 @@ 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;
              ^
              This can be moved ...
>+
>+	if (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {

                 ^
                 ... here.

>+		/* Is there an owner already? */

This comment is superfluous, I would instead explain why there has to be 
an owner, what problem would we have if not?

>+		if (vhost_dev_has_owner(d)) {
>+			r = -EBUSY;
>+			goto done;
>+		}
>+		if (copy_from_user(&inherit_owner, argp,
>+				   sizeof(inherit_owner))) {
>+			r = -EFAULT;
>+			goto done;
>+		}
>+		enforce_inherit_owner = inherit_owner;

mmmm, I'm confused.

IIUC with this change, an user, for example, can call 
VHOST_SET_INHERIT_FROM_OWNER on /dev/vhost-vsock and change the 
behaviour of /dev/vhost-net.

Is that really what we want?

Maybe it's better if the module parameter controls the default of any 
device, but the ioctl changes the behavior only for that device.

>+		goto done;
>+	}
>
> 	/* If you are not the owner, you can become one */
> 	if (ioctl == VHOST_SET_OWNER) {
>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 documentation here.

>+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
                                        ^
                                        Please, add a tab like we did
                                        for all previous definitions.

Thanks,
Stefano

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

[Mike Christie]

On 10/3/24 8:58 PM, Cindy Lu wrote:
> Add a new UAPI to support setting the vhost device to
> use task mode. The user space application needs to use
> VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> This setting must be set before VHOST_SET_OWNER is set.
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
>  include/uapi/linux/vhost.h |  2 ++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 08c9e77916ca..0e5c81026acd 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2341,8 +2341,24 @@ 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 (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {

Maybe instead of a modparam and this ioctl we just want a new ioctl:

/*
 * This will setup the owner based on the calling thread instead of
 * using kthread.
 */
#define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)

It would initially be used by vhost-scsi when worker_per_virtqueue=true
since that is a new use case and there will be no regressions.

For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
tool XYZ can use the new ioctl when they are ready.

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

[Cindy Lu]

On Tue, 15 Oct 2024 at 04:56, Mike Christie <michael.christie@oracle.com> wrote:
>
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > Add a new UAPI to support setting the vhost device to
> > use task mode. The user space application needs to use
> > VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> > This setting must be set before VHOST_SET_OWNER is set.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> >  include/uapi/linux/vhost.h |  2 ++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 08c9e77916ca..0e5c81026acd 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2341,8 +2341,24 @@ 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 (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
>
> Maybe instead of a modparam and this ioctl we just want a new ioctl:
>
> /*
>  * This will setup the owner based on the calling thread instead of
>  * using kthread.
>  */
> #define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)
>
> It would initially be used by vhost-scsi when worker_per_virtqueue=true
> since that is a new use case and there will be no regressions.
>
> For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
> tool XYZ can use the new ioctl when they are ready.
>
If I understand correctly, this means the default vhost function is
using kthread?
Thanks
Cindy

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

[Michael S. Tsirkin]

On Mon, Oct 14, 2024 at 03:56:33PM -0500, Mike Christie wrote:
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > Add a new UAPI to support setting the vhost device to
> > use task mode. The user space application needs to use
> > VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> > This setting must be set before VHOST_SET_OWNER is set.
> > 
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> >  include/uapi/linux/vhost.h |  2 ++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 08c9e77916ca..0e5c81026acd 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2341,8 +2341,24 @@ 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 (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
> 
> Maybe instead of a modparam and this ioctl we just want a new ioctl:
> 
> /*
>  * This will setup the owner based on the calling thread instead of
>  * using kthread.
>  */
> #define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)

I feel this is not good because it is insecure -
process should not normally have a say in whether
namespaces work correctly.
So we want the system admin to be able to block the
old mode.

> It would initially be used by vhost-scsi when worker_per_virtqueue=true
> since that is a new use case and there will be no regressions.
> 
> For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
> tool XYZ can use the new ioctl when they are ready.

I do not like it that we switched default now we apparently will be
switching it back. Will break some userspace whatever we do.

-- 
MST

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

[Jason Wang]

On Tue, Oct 15, 2024 at 6:19 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 14, 2024 at 03:56:33PM -0500, Mike Christie wrote:
> > On 10/3/24 8:58 PM, Cindy Lu wrote:
> > > Add a new UAPI to support setting the vhost device to
> > > use task mode. The user space application needs to use
> > > VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> > > This setting must be set before VHOST_SET_OWNER is set.
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> > >  include/uapi/linux/vhost.h |  2 ++
> > >  2 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 08c9e77916ca..0e5c81026acd 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -2341,8 +2341,24 @@ 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 (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
> >
> > Maybe instead of a modparam and this ioctl we just want a new ioctl:
> >
> > /*
> >  * This will setup the owner based on the calling thread instead of
> >  * using kthread.
> >  */
> > #define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)
>
> I feel this is not good because it is insecure -
> process should not normally have a say in whether
> namespaces work correctly.

Note there's still a lot of kthread users, so the "problem" is not
specific to vhost.

> So we want the system admin to be able to block the
> old mode.

Then we will break the userspace silently which seems not good.

>
> > It would initially be used by vhost-scsi when worker_per_virtqueue=true
> > since that is a new use case and there will be no regressions.
> >
> > For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
> > tool XYZ can use the new ioctl when they are ready.
>
> I do not like it that we switched default now we apparently will be
> switching it back. Will break some userspace whatever we do.
>
> --
> MST
>

Thanks