On 10/3/24 8:58 PM, Cindy Lu wrote:
> Add back the previously removed vhost_worker function to support the kthread
> and rename it vhost_run_work_kthread_list.
>
> The old function vhost_worker was change to support task in
> commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads")
> change to xarray in
> commit 1cdaafa1b8b4 ("vhost: replace single worker pointer with xarray")
>
> Signed-off-by: Cindy Lu <l...@redhat.com>
> ---
> drivers/vhost/vhost.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fed6671c1ffb..349499139f4f 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -418,6 +418,44 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> __vhost_vq_meta_reset(vq);
> }
>
> +static int vhost_run_work_kthread_list(void *data)
> +{
> + struct vhost_worker *worker = data;
> + struct vhost_work *work, *work_next;
> + struct vhost_dev *dev = worker->dev;
> + struct llist_node *node;
> +
> + kthread_use_mm(dev->mm);
> +
> + for (;;) {
> + /* mb paired w/ kthread_stop */
> + set_current_state(TASK_INTERRUPTIBLE);
> +
> + if (kthread_should_stop()) {
> + __set_current_state(TASK_RUNNING);
> + break;
> + }
> + node = llist_del_all(&worker->work_list);
> + if (!node)
> + schedule();
> +
> + node = llist_reverse_order(node);
> + /* make sure flag is seen after deletion */
> + smp_wmb();
> + llist_for_each_entry_safe(work, work_next, node, node) {
> + clear_bit(VHOST_WORK_QUEUED, &work->flags);
> + __set_current_state(TASK_RUNNING);
> + kcov_remote_start_common(worker->kcov_handle);
> + work->fn(work);
> + kcov_remote_stop();
> + cond_resched();
> + }
> + }
> + kthread_unuse_mm(dev->mm);
> +
> + return 0;
> +}
I think there is a lot of unneeded code duplication where in the functions
you are adding there's only 1-3 lines different. To fix this I think we
could:
1. Go really invasive and modify copy_process and its helpers so they take
a task_struct instead of using "current". We can then just pass in "current"
or kthreadd. We can then use most of the existing vhost code. We just
need a per vhost_worker check/field for the mm and cgroup use like:
vhost_task_fn():
{
...
/* The mm would be passed in during creation for the kthread case */
if (vtsk->mm)
kthread_use_mm(vtsk->mm);
Or
2. Go hacky and in the vhost code, when we get a VHOST_SET_OWNER call create
a tmp kthread. The tmp kthread would then call the existing vhost_worker_create
function. The resulting vhost_task would inherit the kthreadd settings like we
want. We then just need a per vhost_worker check/field for the mm and cgroup use
like above.
Or
3. There doesn't seem to be a lot of differences in the functions you are
adding. In the function above the only differences are the mm calls and kthread
should stop. In the destroy functions it's kthread_stop. In the queue function
its
wake_up_process. In create its kthread_create, stop and the cgroup functions.
I think we could add just some callouts on the vhost_task or vhost_worker for
stop,
wakeup and use mm. For create we would do something like
vhost_worker_create()
....
worker = kzalloc();
if (inherit from caller) {
worker->stop = vhost_task_stop;
worker->wakeup = vhost_task_wakeup;
worker->vtsk = vhost_task_create();
} else {
worker->stop = vhost_kthread_stop;
worker->wakeup = vhost_kthread_wakeup;
worker->tsk = kthread->create();
vhost_kthread_setup_cgroups();
}
...
}
vhost_worker_destroy()
{
if (!worker)
return;
WARN_ON(!llist_empty(&worker->work_list));
xa_erase(&dev->worker_xa, worker->id);
worker->stop(worker);
kfree(worker);
}
