| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <timclk34js4wmtlsnvpjl3fuq3p2e5stdqxr47mky42hctgsl2@r3hhc3lx6wig>
Date: Tue, 10 Dec 2024 18:53:42 +0100
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,
netdev@...r.kernel.org
Subject: Re: [PATCH v4 4/8] vhost: Add kthread support in function
vhost_worker_create
On Wed, Dec 11, 2024 at 12:41:43AM +0800, Cindy Lu wrote:
>Restored the previous functions kthread_wakeup and kthread_stop.
nit: "Add back the previously removed"
>Also add 2 new function pointer.
"Also add 2 new function pointers to wakeup and stop the workers."
> The function vhost_worker_create
>Will initializes this pointer based on the value of inherit_owner.
nit: s/Will/will
>
>Signed-off-by: Cindy Lu <lulu@...hat.com>
>---
> drivers/vhost/vhost.c | 84 +++++++++++++++++++++++++++++++++++--------
> drivers/vhost/vhost.h | 3 ++
> 2 files changed, 73 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 812dfd218bc2..0175bbf4d8b3 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -721,14 +721,38 @@ static void vhost_workers_free(struct vhost_dev *dev)
> xa_destroy(&dev->worker_xa);
> }
>
>+static int vhost_task_wakeup_fn(struct vhost_worker *worker)
>+{
>+ vhost_task_wake(worker->vtsk);
>+ return 0;
>+}
>+
>+static int vhost_kthread_wakeup_fn(struct vhost_worker *worker)
>+{
>+ return wake_up_process(worker->kthread_task);
>+}
>+
>+static int vhost_task_stop_fn(struct vhost_worker *worker)
>+{
>+ vhost_task_stop(worker->vtsk);
>+ return 0;
>+}
>+
>+static int vhost_kthread_stop_fn(struct vhost_worker *worker)
>+{
>+ return kthread_stop(worker->kthread_task);
>+}
>+
> static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
> {
> struct vhost_worker *worker;
>- struct vhost_task *vtsk;
>+ struct vhost_task *vtsk = NULL;
>+ struct task_struct *task = NULL;
> char name[TASK_COMM_LEN];
> int ret;
> u32 id;
>
>+ /* Allocate resources for the worker */
> worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
> if (!worker)
> return NULL;
>@@ -736,27 +760,59 @@ static struct vhost_worker
>*vhost_worker_create(struct vhost_dev *dev)
> worker->dev = dev;
> snprintf(name, sizeof(name), "vhost-%d", current->pid);
>
>- vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
>- worker, name);
>- if (!vtsk)
>- goto free_worker;
>-
> mutex_init(&worker->mutex);
> init_llist_head(&worker->work_list);
> worker->kcov_handle = kcov_common_handle();
>- worker->vtsk = vtsk;
>
>- vhost_task_start(vtsk);
>+ if (dev->inherit_owner) {
>+ /*
>+ * If inherit_owner is true we use vhost_tasks to create
>+ * the worker so all settings/limits like cgroups, NPROC,
>+ * scheduler, etc are inherited from the owner. If false,
>+ * we use kthreads and only attach to the same cgroups
>+ * as the owner for compat with older kernels.
>+ */
>+ vtsk = vhost_task_create(vhost_run_work_list,
>+ vhost_worker_killed, worker, name);
>+ if (!vtsk)
>+ goto free_worker;
>+
>+ worker->vtsk = vtsk;
>+ worker->task_wakeup = vhost_task_wakeup_fn;
>+ worker->task_stop = vhost_task_stop_fn;
>+
>+ vhost_task_start(vtsk);
>+ ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
>+ GFP_KERNEL);
>+ if (ret < 0)
>+ goto stop_worker;
>+ } else {
>+ /* Create and start a kernel thread */
I would move here the comment included in the previous branch:
"If false we use kthreads and only attach to... "
Or move the entire comment block before the if.
>+ task = kthread_create(vhost_run_work_kthread_list, worker,
>+ "vhost-%d", current->pid);
>+ if (IS_ERR(task)) {
>+ ret = PTR_ERR(task);
>+ goto free_worker;
>+ }
>+ worker->kthread_task = task;
>+ worker->task_wakeup = vhost_kthread_wakeup_fn;
>+ worker->task_stop = vhost_kthread_stop_fn;
>
>- ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
>- if (ret < 0)
>- goto stop_worker;
>- worker->id = id;
>+ wake_up_process(task);
>+ ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b,
>+ GFP_KERNEL);
>+ if (ret < 0)
>+ goto stop_worker;
>
>- return worker;
>+ ret = vhost_attach_task_to_cgroups(worker);
>+ if (ret)
>+ goto stop_worker;
>+ }
>
>+ worker->id = id;
>+ return worker;
> stop_worker:
>- vhost_task_stop(vtsk);
>+ worker->task_stop(worker);
> free_worker:
> kfree(worker);
> return NULL;
>diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>index c650c4506c70..a7dc6e168753 100644
>--- a/drivers/vhost/vhost.h
>+++ b/drivers/vhost/vhost.h
>@@ -27,6 +27,7 @@ struct vhost_work {
> };
>
> struct vhost_worker {
>+ struct task_struct *kthread_task;
> struct vhost_task *vtsk;
> struct vhost_dev *dev;
> /* Used to serialize device wide flushing with worker swapping. */
>@@ -36,6 +37,8 @@ struct vhost_worker {
> u32 id;
> int attachment_cnt;
> bool killed;
>+ int (*task_wakeup)(struct vhost_worker *worker);
>+ int (*task_stop)(struct vhost_worker *worker);
We never read the return values of these functions, so either mark them
both void or check their return value.
What about renaming in worker_wakeup and worker_stop?
Or even better since they are part of vhost_worker, just wakeup and
stop.
Thanks,
Stefano
> };
>
> /* Poll a file (eventfd or socket) */
>--
>2.45.0
>
Powered by blists - more mailing lists