1. Patchew
  2. linux
  3. [PATCH v2 0/7] vhost: Add support of kthread API
  4. View patch

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

Cindy Lu posted 7 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task
Posted by Cindy Lu 1 month, 3 weeks ago 
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
Posted by Stefano Garzarella 1 month, 3 weeks ago 
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
Posted by Cindy Lu 1 month, 2 weeks ago 
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
Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace select vhost_task
Posted by Jason Wang 1 month, 2 weeks ago 
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
Posted by Stefano Garzarella 1 month, 2 weeks ago 
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
Posted by Jason Wang 1 month, 2 weeks ago 
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
Posted by Cindy Lu 1 month, 2 weeks ago 
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
Posted by kernel test robot 1 month, 3 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.