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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >---
> > 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