On 11/5/24 1:25 AM, Cindy Lu wrote:
> 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;
>
> + worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
> + if (!worker->fn) {
> + kfree(worker);
> + return NULL;
> + }
Why dynamically allocate this?
You could probably even just kill the vhost_task_fn struct since we just
have to the 2 callouts.
> +
> 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) {
> + /* Create and start a vhost task */
Maybe instead of this comment and the one below write something about
what inherit_owner means. We can see from the code we are creating a
vhost/kthread, but it's not really obvious why. Something like:
/*
* 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->fn->wakeup = vhost_task_wakeup_fn;
> + worker->fn->stop = vhost_task_stop_fn;
> +
> + vhost_task_start(vtsk);
> + } else {
> + /* Create and start a kernel thread */
> + 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->task = task;
> + worker->fn->wakeup = vhost_kthread_wakeup_fn;
> + worker->fn->stop = vhost_kthread_stop_fn;
> +
> + wake_up_process(task);
> + /* Attach to the vhost cgroup */
You don't need this comment do you? The function name tells us the same
info.
> + ret = vhost_attach_cgroups(dev);
I don't think this works. Patch 3/9 did:
+ xa_for_each(&dev->worker_xa, i, worker) {
+ ret = vhost_worker_cgroups_kthread(worker);
but we don't add the worker to the xa until below.
You also want to just call vhost_worker_cgroups_kthread above, because
you only want to add the one task and not loop over all of them.
I would then also maybe rename vhost_worker_cgroups_kthread to something
like vhost_attach_task_to_cgroups.
> + if (ret)
> + goto stop_worker;
> + }
>
> ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
> if (ret < 0)
> goto stop_worker;
> worker->id = id;
> -
> return worker;
> -
> stop_worker:
> - vhost_task_stop(vtsk);
> + worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);
I don't think you need to cast since the function takes a void pointer.
Same comment for the other patches like 6/9 where you are calling the
callout and casting.
