In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
vhost removed the support for the kthread API. However, there are
still situations where there is a request to use kthread.
In this PATCH, the support of kthread is added back. Additionally,
a module_param is added to enforce which mode we are using, and
a new UAPI is introduced to allow the userspace app to set the
mode they want to use.

Tested the user application with QEMU.

Cindy Lu (7):
  vhost: Add a new module_param for enable kthread
  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      | 246 +++++++++++++++++++++++++++++++++++--
 drivers/vhost/vhost.h      |   1 +
 include/uapi/linux/vhost.h |   2 +
 3 files changed, 240 insertions(+), 9 deletions(-)

-- 
2.45.0

On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> vhost removed the support for the kthread API. However, there are
> still situations where there is a request to use kthread.
> In this PATCH, the support of kthread is added back. Additionally,
> a module_param is added to enforce which mode we are using, and
> a new UAPI is introduced to allow the userspace app to set the
> mode they want to use.
> 
> Tested the user application with QEMU.

Cindy, the APIs do not make sense, security does not make sense,
and you are not describing the issue and the fix.


The name should reflect what this does from userspace POV, not from
kernel API POV.  kthread and task mode is not something that any users
have any business even to consider.


To help you out, some ideas:

I think the issue is something like "vhost is now 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."

Now, what can be done? Given we already released a secure kernel,
I am not inclined to revert it back to be insecure by default.
But I'm fine with a modparam to allow userspace to get insecure
behaviour.


Now, a modparam is annoying in that it affects all userspace,
and people might be running a mix of old and new userspace.
So if we do that, we also want a flag that will get
safe behaviour even if unsafe one is allowed.


Is this clear enough, or do I need to elaborate more?

Thanks!

> Cindy Lu (7):
>   vhost: Add a new module_param for enable kthread
>   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      | 246 +++++++++++++++++++++++++++++++++++--
>  drivers/vhost/vhost.h      |   1 +
>  include/uapi/linux/vhost.h |   2 +
>  3 files changed, 240 insertions(+), 9 deletions(-)
> 
> -- 
> 2.45.0

On 9/10/24 2:41 AM, Michael S. Tsirkin wrote:
> On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
>> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
>> vhost removed the support for the kthread API. However, there are
>> still situations where there is a request to use kthread.
>> In this PATCH, the support of kthread is added back. Additionally,
>> a module_param is added to enforce which mode we are using, and
>> a new UAPI is introduced to allow the userspace app to set the
>> mode they want to use.
>>
>> Tested the user application with QEMU.
> 
> Cindy, the APIs do not make sense, security does not make sense,
> and you are not describing the issue and the fix.
> 
> 
> The name should reflect what this does from userspace POV, not from
> kernel API POV.  kthread and task mode is not something that any users
> have any business even to consider.
> 
> 
> To help you out, some ideas:
> 
> I think the issue is something like "vhost is now 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."
> 
> Now, what can be done? Given we already released a secure kernel,
> I am not inclined to revert it back to be insecure by default.
> But I'm fine with a modparam to allow userspace to get insecure
> behaviour.
> 
> 
> Now, a modparam is annoying in that it affects all userspace,
> and people might be running a mix of old and new userspace.
> So if we do that, we also want a flag that will get
> safe behaviour even if unsafe one is allowed.
> 
> 
> Is this clear enough, or do I need to elaborate more?
> 
Thanks for working on this Cindy. I've been trying to implement
this in a way where we don't have to duplicate a lot of code but
have been hitting various issues. For example, I've been trying
to modify the vhost_task code so it can emulate the kthread
behavior we had before.

If people are ok with something similar as in this patchset where
we have both vhost_tasks and kthreads, then I can send something.

On Wed, Sep 11, 2024 at 11:20:30AM -0500, Mike Christie wrote:
> If people are ok with something similar as in this patchset where
> we have both vhost_tasks and kthreads, then I can send something.


It would be better, as you say, to modify the vhost_task code so it can
emulate the kthread behavior. However, given that apparently some users
are unhappy, what you describe would be a good stopgap solution
until we have that.

The main difficulty seems to be to describe what the
behaviour is, exactly, so userspace can make informed
decisions on what to use.

-- 
MST

On Thu, Sep 12, 2024 at 3:02 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Sep 11, 2024 at 11:20:30AM -0500, Mike Christie wrote:
> > If people are ok with something similar as in this patchset where
> > we have both vhost_tasks and kthreads, then I can send something.
>
>
> It would be better, as you say, to modify the vhost_task code so it can
> emulate the kthread behavior. However, given that apparently some users
> are unhappy, what you describe would be a good stopgap solution
> until we have that.
>
> The main difficulty seems to be to describe what the
> behaviour is, exactly, so userspace can make informed
> decisions on what to use.

Exactly.

Thanks

>
> --
> MST
>

On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > vhost removed the support for the kthread API. However, there are
> > still situations where there is a request to use kthread.
> > In this PATCH, the support of kthread is added back. Additionally,
> > a module_param is added to enforce which mode we are using, and
> > a new UAPI is introduced to allow the userspace app to set the
> > mode they want to use.
> >
> > Tested the user application with QEMU.
>
> Cindy, the APIs do not make sense, security does not make sense,
> and you are not describing the issue and the fix.
>
>
> The name should reflect what this does from userspace POV, not from
> kernel API POV.  kthread and task mode is not something that any users
> have any business even to consider.
>
>
> To help you out, some ideas:
>
> I think the issue is something like "vhost is now 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."
>
> Now, what can be done? Given we already released a secure kernel,
> I am not inclined to revert it back to be insecure by default.

It depends on how we define "secure". There's plenty of users of
kthread and if I was not wrong, mike may still need to fix some bugs.


> But I'm fine with a modparam to allow userspace to get insecure
> behaviour.
>
>
> Now, a modparam is annoying in that it affects all userspace,
> and people might be running a mix of old and new userspace.
> So if we do that, we also want a flag that will get
> safe behaviour even if unsafe one is allowed.

I am not sure this can help. My understanding is that the flag is
sufficient. Otherwise the layered product needs to know if there's old
user space or new which seems to be a challenge.

Thanks

>
>
> Is this clear enough, or do I need to elaborate more?
>
> Thanks!
>
> > Cindy Lu (7):
> >   vhost: Add a new module_param for enable kthread
> >   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      | 246 +++++++++++++++++++++++++++++++++++--
> >  drivers/vhost/vhost.h      |   1 +
> >  include/uapi/linux/vhost.h |   2 +
> >  3 files changed, 240 insertions(+), 9 deletions(-)
> >
> > --
> > 2.45.0
>

On Tue, Sep 10, 2024 at 04:37:52PM +0800, Jason Wang wrote:
> On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > > vhost removed the support for the kthread API. However, there are
> > > still situations where there is a request to use kthread.
> > > In this PATCH, the support of kthread is added back. Additionally,
> > > a module_param is added to enforce which mode we are using, and
> > > a new UAPI is introduced to allow the userspace app to set the
> > > mode they want to use.
> > >
> > > Tested the user application with QEMU.
> >
> > Cindy, the APIs do not make sense, security does not make sense,
> > and you are not describing the issue and the fix.
> >
> >
> > The name should reflect what this does from userspace POV, not from
> > kernel API POV.  kthread and task mode is not something that any users
> > have any business even to consider.
> >
> >
> > To help you out, some ideas:
> >
> > I think the issue is something like "vhost is now 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."
> >
> > Now, what can be done? Given we already released a secure kernel,
> > I am not inclined to revert it back to be insecure by default.
> 
> It depends on how we define "secure". There's plenty of users of
> kthread and if I was not wrong, mike may still need to fix some bugs.
> 

which bugs?

> > But I'm fine with a modparam to allow userspace to get insecure
> > behaviour.
> >
> >
> > Now, a modparam is annoying in that it affects all userspace,
> > and people might be running a mix of old and new userspace.
> > So if we do that, we also want a flag that will get
> > safe behaviour even if unsafe one is allowed.
> 
> I am not sure this can help. My understanding is that the flag is
> sufficient. Otherwise the layered product needs to know if there's old
> user space or new which seems to be a challenge.
> 
> Thanks

this will be up to userspace to resolve.


> >
> >
> > Is this clear enough, or do I need to elaborate more?
> >
> > Thanks!
> >
> > > Cindy Lu (7):
> > >   vhost: Add a new module_param for enable kthread
> > >   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      | 246 +++++++++++++++++++++++++++++++++++--
> > >  drivers/vhost/vhost.h      |   1 +
> > >  include/uapi/linux/vhost.h |   2 +
> > >  3 files changed, 240 insertions(+), 9 deletions(-)
> > >
> > > --
> > > 2.45.0
> >

On Tue, Sep 10, 2024 at 4:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Sep 10, 2024 at 04:37:52PM +0800, Jason Wang wrote:
> > On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > > > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > > > vhost removed the support for the kthread API. However, there are
> > > > still situations where there is a request to use kthread.
> > > > In this PATCH, the support of kthread is added back. Additionally,
> > > > a module_param is added to enforce which mode we are using, and
> > > > a new UAPI is introduced to allow the userspace app to set the
> > > > mode they want to use.
> > > >
> > > > Tested the user application with QEMU.
> > >
> > > Cindy, the APIs do not make sense, security does not make sense,
> > > and you are not describing the issue and the fix.
> > >
> > >
> > > The name should reflect what this does from userspace POV, not from
> > > kernel API POV.  kthread and task mode is not something that any users
> > > have any business even to consider.
> > >
> > >
> > > To help you out, some ideas:
> > >
> > > I think the issue is something like "vhost is now 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."
> > >
> > > Now, what can be done? Given we already released a secure kernel,
> > > I am not inclined to revert it back to be insecure by default.
> >
> > It depends on how we define "secure". There's plenty of users of
> > kthread and if I was not wrong, mike may still need to fix some bugs.
> >
>
> which bugs?

https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/

>
> > > But I'm fine with a modparam to allow userspace to get insecure
> > > behaviour.
> > >
> > >
> > > Now, a modparam is annoying in that it affects all userspace,
> > > and people might be running a mix of old and new userspace.
> > > So if we do that, we also want a flag that will get
> > > safe behaviour even if unsafe one is allowed.
> >
> > I am not sure this can help. My understanding is that the flag is
> > sufficient. Otherwise the layered product needs to know if there's old
> > user space or new which seems to be a challenge.
> >
> > Thanks
>
> this will be up to userspace to resolve.

I actually mean the module parameter. It would be very hard for the
layered product to decide the value for that.

Thanks

>
>
> > >
> > >
> > > Is this clear enough, or do I need to elaborate more?
> > >
> > > Thanks!
> > >
> > > > Cindy Lu (7):
> > > >   vhost: Add a new module_param for enable kthread
> > > >   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      | 246 +++++++++++++++++++++++++++++++++++--
> > > >  drivers/vhost/vhost.h      |   1 +
> > > >  include/uapi/linux/vhost.h |   2 +
> > > >  3 files changed, 240 insertions(+), 9 deletions(-)
> > > >
> > > > --
> > > > 2.45.0
> > >
>

On 9/10/24 10:45 PM, Jason Wang wrote:
>>> It depends on how we define "secure". There's plenty of users of
>>> kthread and if I was not wrong, mike may still need to fix some bugs.
>>>
>>
>> which bugs?
> 
> https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/

The SIGKILL issue in that specific email is fixed. The patches are
merged upstream. I don't know of any other bugs like that (crashes,
hangs, etc). There is the one we can't reproduce so are not sure if
it's the vhost changes.

There is the change in behavior type of bug we discussed in that
thread:

https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/#m026f7dd96e064e3a604155e45e7ae9d5c949ae23

On Wed, Sep 11, 2024 at 11:45:33AM +0800, Jason Wang wrote:
> On Tue, Sep 10, 2024 at 4:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 04:37:52PM +0800, Jason Wang wrote:
> > > On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > > > > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > > > > vhost removed the support for the kthread API. However, there are
> > > > > still situations where there is a request to use kthread.
> > > > > In this PATCH, the support of kthread is added back. Additionally,
> > > > > a module_param is added to enforce which mode we are using, and
> > > > > a new UAPI is introduced to allow the userspace app to set the
> > > > > mode they want to use.
> > > > >
> > > > > Tested the user application with QEMU.
> > > >
> > > > Cindy, the APIs do not make sense, security does not make sense,
> > > > and you are not describing the issue and the fix.
> > > >
> > > >
> > > > The name should reflect what this does from userspace POV, not from
> > > > kernel API POV.  kthread and task mode is not something that any users
> > > > have any business even to consider.
> > > >
> > > >
> > > > To help you out, some ideas:
> > > >
> > > > I think the issue is something like "vhost is now 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."
> > > >
> > > > Now, what can be done? Given we already released a secure kernel,
> > > > I am not inclined to revert it back to be insecure by default.
> > >
> > > It depends on how we define "secure". There's plenty of users of
> > > kthread and if I was not wrong, mike may still need to fix some bugs.
> > >
> >
> > which bugs?
> 
> https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/

Isn't this exactly what Cindy is trying to address?
You made it sound like there is something else.

> >
> > > > But I'm fine with a modparam to allow userspace to get insecure
> > > > behaviour.
> > > >
> > > >
> > > > Now, a modparam is annoying in that it affects all userspace,
> > > > and people might be running a mix of old and new userspace.
> > > > So if we do that, we also want a flag that will get
> > > > safe behaviour even if unsafe one is allowed.
> > >
> > > I am not sure this can help. My understanding is that the flag is
> > > sufficient. Otherwise the layered product needs to know if there's old
> > > user space or new which seems to be a challenge.
> > >
> > > Thanks
> >
> > this will be up to userspace to resolve.
> 
> I actually mean the module parameter. It would be very hard for the
> layered product to decide the value for that.
> 
> Thanks

We can make it writeable, if that helps.


> >
> >
> > > >
> > > >
> > > > Is this clear enough, or do I need to elaborate more?
> > > >
> > > > Thanks!
> > > >
> > > > > Cindy Lu (7):
> > > > >   vhost: Add a new module_param for enable kthread
> > > > >   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      | 246 +++++++++++++++++++++++++++++++++++--
> > > > >  drivers/vhost/vhost.h      |   1 +
> > > > >  include/uapi/linux/vhost.h |   2 +
> > > > >  3 files changed, 240 insertions(+), 9 deletions(-)
> > > > >
> > > > > --
> > > > > 2.45.0
> > > >
> >