Re: [PATCH v2 4/7] vhost: Add the vhost_worker to support kthread

[Mike Christie <michael.christie@xxxxxxxxxx>]

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 <lulu@xxxxxxxxxx>
> ---
>  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);
}