Jason Wang
2021-Dec-08 04:07 UTC
[PATCH 6/7] vhost-scsi: drop flush after vhost_dev_cleanup
On Tue, Dec 7, 2021 at 10:45 AM Mike Christie <michael.christie at oracle.com> wrote:> > The flush after vhost_dev_cleanup is not needed because: > > 1. It doesn't do anything. vhost_dev_cleanup will stop the worker thread > so the flush call will just return since the worker has not device. > > 2. It's not needed for the re-queue case. vhost_scsi_evt_handle_kick grabs > the mutex and if the backend is NULL will return without queueing a work. > vhost_scsi_clear_endpoint will set the backend to NULL under the vq->mutex > then drops the mutex and does a flush. So we know when > vhost_scsi_clear_endpoint has dropped the mutex after clearing the backend > no evt related work will be able to requeue. The flush would then make sure > any queued evts are run and return.What happens if a kick after vhost_scsi_clear_endpoint() but before vhost_dev_stop()? Is this safe or the kthread_stop() in vhost_dev_cleanup() makes us safe? Thanks> > Signed-off-by: Mike Christie <michael.christie at oracle.com> > --- > drivers/vhost/scsi.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c > index 532e204f2b1b..94535c813ef7 100644 > --- a/drivers/vhost/scsi.c > +++ b/drivers/vhost/scsi.c > @@ -1827,8 +1827,6 @@ static int vhost_scsi_release(struct inode *inode, struct file *f) > vhost_scsi_clear_endpoint(vs, &t); > vhost_dev_stop(&vs->dev); > vhost_dev_cleanup(&vs->dev); > - /* Jobs can re-queue themselves in evt kick handler. Do extra flush. */ > - vhost_scsi_flush(vs); > kfree(vs->dev.vqs); > kvfree(vs); > return 0; > -- > 2.25.1 >
Mike Christie
2021-Dec-08 16:45 UTC
[PATCH 6/7] vhost-scsi: drop flush after vhost_dev_cleanup
On 12/7/21 10:07 PM, Jason Wang wrote:> On Tue, Dec 7, 2021 at 10:45 AM Mike Christie > <michael.christie at oracle.com> wrote: >> >> The flush after vhost_dev_cleanup is not needed because: >> >> 1. It doesn't do anything. vhost_dev_cleanup will stop the worker thread >> so the flush call will just return since the worker has not device. >> >> 2. It's not needed for the re-queue case. vhost_scsi_evt_handle_kick grabs >> the mutex and if the backend is NULL will return without queueing a work. >> vhost_scsi_clear_endpoint will set the backend to NULL under the vq->mutex >> then drops the mutex and does a flush. So we know when >> vhost_scsi_clear_endpoint has dropped the mutex after clearing the backend >> no evt related work will be able to requeue. The flush would then make sure >> any queued evts are run and return. > > What happens if a kick after vhost_scsi_clear_endpoint() but before > vhost_dev_stop()?vhost_dev_stop also does a flush, so: 1. The kick handler would see the backend is null and return immediately. 2. The flush in vhost_dev_stop would wait for those kicks in #1 to complete.> > Is this safe or the kthread_stop() in vhost_dev_cleanup() makes us safe? > > Thanks > >> >> Signed-off-by: Mike Christie <michael.christie at oracle.com> >> --- >> drivers/vhost/scsi.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c >> index 532e204f2b1b..94535c813ef7 100644 >> --- a/drivers/vhost/scsi.c >> +++ b/drivers/vhost/scsi.c >> @@ -1827,8 +1827,6 @@ static int vhost_scsi_release(struct inode *inode, struct file *f) >> vhost_scsi_clear_endpoint(vs, &t); >> vhost_dev_stop(&vs->dev); >> vhost_dev_cleanup(&vs->dev); >> - /* Jobs can re-queue themselves in evt kick handler. Do extra flush. */ >> - vhost_scsi_flush(vs); >> kfree(vs->dev.vqs); >> kvfree(vs); >> return 0; >> -- >> 2.25.1 >> >