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@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;
+}
+
static bool vhost_run_work_list(void *data)
{
struct vhost_worker *worker = data;
--
2.45.0On 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@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);
}
On Tue, 15 Oct 2024 at 06:56, Mike Christie <michael.christie@oracle.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@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:
>
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);
> }
>
>
© 2016 - 2024 Red Hat, Inc.