On Tue, 15 Oct 2024 at 06:56, Mike Christie <michael.christie@xxxxxxxxxx> wrote:
>
> 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:
>
Thank you mike， I will try this and provide a new version
thanks
cindy
>
> 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);
> }
>
>
