[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <olbm26f3ifb6ypfmfl22xszbpevqsxc3ogfzosrb44ujtzt6pw@uh7irbqfy5jr>
Date: Mon, 7 Oct 2024 15:31:44 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Cindy Lu <lulu@...hat.com>
Cc: jasowang@...hat.com, mst@...hat.com, michael.christie@...cle.com,
linux-kernel@...r.kernel.org, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v2 1/7] vhost: Add a new modparam to allow userspace
select vhost_task
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@...hat.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
Powered by blists - more mailing lists