search for: vhost_dev_flush

...en when thread0 does exit_files() that will do the > >> fput that does vhost-$something's file_operations->release. > > > > So, at least in this simple case vhost_worker() can just exit after SIGKILL, > > and thread0 can flush the outstanding commands when it calls vhost_dev_flush() > > rather than wait for vhost_worker(). > > > > Right? > > With the current code, the answer is no. We would hang like I mentioned here: > > https://lore.kernel.org/lkml/ae250076-7d55-c407-1066-86b37014c69c at oracle.com/ If only I could fully understand this email...

...host-$something, then when thread0 does exit_files() that will do the >> fput that does vhost-$something's file_operations->release. > > So, at least in this simple case vhost_worker() can just exit after SIGKILL, > and thread0 can flush the outstanding commands when it calls vhost_dev_flush() > rather than wait for vhost_worker(). > > Right? With the current code, the answer is no. We would hang like I mentioned here: https://lore.kernel.org/lkml/ae250076-7d55-c407-1066-86b37014c69c at oracle.com/ We need to add code like I mentioned in that reply because we don't hav...

...ush_work); + + vhost_work_queue_on(worker, &flush.work); + wait_for_completion(&flush.wait_event); +} + void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) { vhost_work_queue_on(dev->worker, work); @@ -261,15 +275,7 @@ EXPORT_SYMBOL_GPL(vhost_vq_work_queue); void vhost_dev_flush(struct vhost_dev *dev) { - struct vhost_flush_struct flush; - - if (dev->worker) { - init_completion(&flush.wait_event); - vhost_work_init(&flush.work, vhost_flush_work); - - vhost_work_queue(dev, &flush.work); - wait_for_completion(&flush.wait_event); - } + vhost_work_flush...

...ore simple. I think RCU is just a little more complex/invasive because it will have the extra synchronize_rcu calls. diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index a92af08e7864..03fd47a22a73 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -235,7 +235,7 @@ void vhost_dev_flush(struct vhost_dev *dev) { struct vhost_flush_struct flush; - if (dev->worker) { + if (READ_ONCE(dev->worker.vtsk)) { init_completion(&flush.wait_event); vhost_work_init(&flush.work, vhost_flush_work); @@ -247,7 +247,9 @@ EXPORT_SYMBOL_GPL(vhost_dev_flush); void vhost_wo...

...+++++++---------------------------- drivers/vhost/vhost.h | 2 +- 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 074273020849..0ad9fea7c170 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -235,7 +235,7 @@ void vhost_dev_flush(struct vhost_dev *dev) { struct vhost_flush_struct flush; - if (dev->worker) { + if (dev->worker.vtsk) { init_completion(&flush.wait_event); vhost_work_init(&flush.work, vhost_flush_work); @@ -247,7 +247,7 @@ EXPORT_SYMBOL_GPL(vhost_dev_flush); void vhost_work_queue(st...

...+++++++---------------------------- drivers/vhost/vhost.h | 2 +- 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 074273020849..0ad9fea7c170 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -235,7 +235,7 @@ void vhost_dev_flush(struct vhost_dev *dev) { struct vhost_flush_struct flush; - if (dev->worker) { + if (dev->worker.vtsk) { init_completion(&flush.wait_event); vhost_work_init(&flush.work, vhost_flush_work); @@ -247,7 +247,7 @@ EXPORT_SYMBOL_GPL(vhost_dev_flush); void vhost_work_queue(st...

...case we should just need READ_ONCE/WRITE_ONCE if dev->worker is no longer a pointer. > > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >index a92af08e7864..03fd47a22a73 100644 >--- a/drivers/vhost/vhost.c >+++ b/drivers/vhost/vhost.c >@@ -235,7 +235,7 @@ void vhost_dev_flush(struct vhost_dev *dev) > { > struct vhost_flush_struct flush; > >- if (dev->worker) { >+ if (READ_ONCE(dev->worker.vtsk)) { > init_completion(&flush.wait_event); > vhost_work_init(&flush.work, vhost_flush_work); > >@@ -247,7 +247,9 @@ EXPORT_SYMBOL_GPL(...

The following patches were built over linux-next which contains various vhost patches in mst's tree and the vhost_task patchset in Christian Brauner's tree: git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git kernel.user_worker branch: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=kernel.user_worker The latter patchset handles the review comment

...se() on > /dev/vhost-$something, then when thread0 does exit_files() that will do the > fput that does vhost-$something's file_operations->release. So, at least in this simple case vhost_worker() can just exit after SIGKILL, and thread0 can flush the outstanding commands when it calls vhost_dev_flush() rather than wait for vhost_worker(). Right? not that I think this can help in the general case ... Oleg.

...do, SIGKILL/SIGSTOP/etc can come right before we call work->fn(). Is it "safe" to run this callback with signal_pending() or fatal_signal_pending() ? Finally. I never looked into drivers/vhost/ before so I don't understand this code at all, but let me ask anyway... Can we change vhost_dev_flush() to run the pending callbacks rather than wait for vhost_worker() ? I guess we can't, ->mm won't be correct, but can you confirm? Oleg.

...--- > drivers/vhost/vhost.h | 2 +- > 2 files changed, 18 insertions(+), 34 deletions(-) > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >index 074273020849..0ad9fea7c170 100644 >--- a/drivers/vhost/vhost.c >+++ b/drivers/vhost/vhost.c >@@ -235,7 +235,7 @@ void vhost_dev_flush(struct vhost_dev *dev) > { > struct vhost_flush_struct flush; > >- if (dev->worker) { >+ if (dev->worker.vtsk) { > init_completion(&flush.wait_event); > vhost_work_init(&flush.work, vhost_flush_work); > >@@ -247,7 +247,7 @@ EXPORT_SYMBOL_GPL(vhost_dev_f...

...ost_types.h | 15 +++ 4 files changed, 204 insertions(+), 14 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 1fa5e9a49092..e40699e83c6d 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -271,7 +271,11 @@ EXPORT_SYMBOL_GPL(vhost_vq_work_queue); void vhost_dev_flush(struct vhost_dev *dev) { - vhost_work_flush_on(dev->worker); + struct vhost_worker *worker; + unsigned long i; + + xa_for_each(&dev->worker_xa, i, worker) + vhost_work_flush_on(worker); } EXPORT_SYMBOL_GPL(vhost_dev_flush); @@ -489,7 +493,6 @@ void vhost_dev_init(struct vhost_dev *d...

...i_clear_endpoint > will have made sure the flush has completed when the clear function returns. > It does that with the device mutex so when we run __vhost_vq_attach_worker > It will only see a vq/worker with no flushes in progress. Ok. > > For the general case of can we be doing a vhost_dev_flush/vhost_work_flush_on > and __vhost_vq_attach_worker, then I thought we are ok as well because I > thought we have to currently have the device mutex when we flush so those can't > race with ioctl calls to vhost_vq_attach_worker since we hold the dev mutex > during that ioctls. I'...

On 6/5/23 10:10 AM, Oleg Nesterov wrote: > On 06/03, michael.christie at oracle.com wrote: >> >> On 6/2/23 11:15 PM, Eric W. Biederman wrote: >> The problem is that as part of the flush the drivers/vhost/scsi.c code >> will wait for outstanding commands, because we can't free the device and >> it's resources before the commands complete or we will hit the

On 05/22, Mike Christie wrote: > > On 5/22/23 7:30 AM, Oleg Nesterov wrote: > >> + /* > >> + * When we get a SIGKILL our release function will > >> + * be called. That will stop new IOs from being queued > >> + * and check for outstanding cmd responses. It will then > >> + * call vhost_task_stop to tell us to return and exit. >

...come right > before we call work->fn(). Is it "safe" to run this callback with > signal_pending() or fatal_signal_pending() ? > > > Finally. I never looked into drivers/vhost/ before so I don't understand > this code at all, but let me ask anyway... Can we change vhost_dev_flush() > to run the pending callbacks rather than wait for vhost_worker() ? > I guess we can't, ->mm won't be correct, but can you confirm? In a conversation long ago I remember hearing that vhost does not support file descriptor passing. Which means all of the file descriptors should...

...d the release function is called as part of freeing struct file. To avoid hangs in the coredump rendezvous and when killing threads in a multi-threaded exec. The coredump code and de_thread have been modified to ignore vhost threads. Remvoing the special case for exec appears to require teaching vhost_dev_flush how to directly complete transactions in case the vhost thread is no longer running. Removing the special case for coredump rendezvous requires either the above fix needed for exec or moving the coredump rendezvous into get_signal. Fixes: 6e890c5d5021 ("vhost: use vhost_tasks for worker thre...

Hi Mike, sorry, but somehow I can't understand this patch... I'll try to read it with a fresh head on Weekend, but for example, On 06/01, Mike Christie wrote: > > static int vhost_task_fn(void *data) > { > struct vhost_task *vtsk = data; > - int ret; > + bool dead = false; > + > + for (;;) { > + bool did_work; > + > + /* mb paired w/