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