linux-kernel - Re: [PATCH v2 4/7] vhost: Add the vhost

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACLfguX-pHYpwQZA3QjT+P5Ev-7bPFede=7sBZJzZkE-QsPKxg@mail.gmail.com>
Date: Tue, 15 Oct 2024 17:03:47 +0800
From: Cindy Lu <lulu@...hat.com>
To: Mike Christie <michael.christie@...cle.com>
Cc: jasowang@...hat.com, mst@...hat.com, linux-kernel@...r.kernel.org, 
	virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v2 4/7] vhost: Add the vhost_worker to support kthread

On Tue, 15 Oct 2024 at 06:56, Mike Christie <michael.christie@...cle.com> 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@...hat.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:
>
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);
> }
>
>