Re: [PATCH v4 4/8] vhost: Add kthread support in function vhost_worker_create
From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2024-12-10 17:53:50
Also in:
lkml, virtualization
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
quoted hunk ↗ jump to hunk
Signed-off-by: Cindy Lu <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ 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