From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] virtio_mem: break device on remove
Date: Mon, 17 Jan 2022 02:55:42 -0500 [thread overview]
Message-ID: <20220117025341-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <1f703ebf-0f78-e530-0fe1-163613397cad@redhat.com>
On Mon, Jan 17, 2022 at 02:40:11PM +0800, Jason Wang wrote:
>
> 在 2022/1/15 上午5:43, Michael S. Tsirkin 写道:
> > A common pattern for device reset is currently:
> > vdev->config->reset(vdev);
> > .. cleanup ..
> >
> > reset prevents new interrupts from arriving and waits for interrupt
> > handlers to finish.
> >
> > However if - as is common - the handler queues a work request which is
> > flushed during the cleanup stage, we have code adding buffers / trying
> > to get buffers while device is reset. Not good.
> >
> > This was reproduced by running
> > modprobe virtio_console
> > modprobe -r virtio_console
> > in a loop, and this reasoning seems to apply to virtio mem though
> > I could not reproduce it there.
> >
> > Fix this up by calling virtio_break_device + flush before reset.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/virtio/virtio_mem.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> > index 38becd8d578c..33b8a118a3ae 100644
> > --- a/drivers/virtio/virtio_mem.c
> > +++ b/drivers/virtio/virtio_mem.c
> > @@ -2888,6 +2888,8 @@ static void virtio_mem_remove(struct virtio_device *vdev)
> > virtio_mem_deinit_hotplug(vm);
> > /* reset the device and cleanup the queues */
> > + virtio_break_device(vdev);
> > + flush_work(&vm->wq);
>
>
> We set vm->removing to true and call cancel_work_sync() in
> virtio_mem_deinit_hotplug(). Isn't is sufficient?
>
> Thanks
Hmm I think you are right. David, I will drop this for now.
Up to you to consider whether some central capability will be
helpful as a replacement for the virtio-mem specific "removing" flag.
>
> > virtio_reset_device(vdev);
> > vdev->config->del_vqs(vdev);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: linux-kernel@vger.kernel.org,
David Hildenbrand <david@redhat.com>,
virtualization@lists.linux-foundation.org
Subject: Re: [PATCH] virtio_mem: break device on remove
Date: Mon, 17 Jan 2022 02:55:42 -0500 [thread overview]
Message-ID: <20220117025341-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <1f703ebf-0f78-e530-0fe1-163613397cad@redhat.com>
On Mon, Jan 17, 2022 at 02:40:11PM +0800, Jason Wang wrote:
>
> 在 2022/1/15 上午5:43, Michael S. Tsirkin 写道:
> > A common pattern for device reset is currently:
> > vdev->config->reset(vdev);
> > .. cleanup ..
> >
> > reset prevents new interrupts from arriving and waits for interrupt
> > handlers to finish.
> >
> > However if - as is common - the handler queues a work request which is
> > flushed during the cleanup stage, we have code adding buffers / trying
> > to get buffers while device is reset. Not good.
> >
> > This was reproduced by running
> > modprobe virtio_console
> > modprobe -r virtio_console
> > in a loop, and this reasoning seems to apply to virtio mem though
> > I could not reproduce it there.
> >
> > Fix this up by calling virtio_break_device + flush before reset.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/virtio/virtio_mem.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> > index 38becd8d578c..33b8a118a3ae 100644
> > --- a/drivers/virtio/virtio_mem.c
> > +++ b/drivers/virtio/virtio_mem.c
> > @@ -2888,6 +2888,8 @@ static void virtio_mem_remove(struct virtio_device *vdev)
> > virtio_mem_deinit_hotplug(vm);
> > /* reset the device and cleanup the queues */
> > + virtio_break_device(vdev);
> > + flush_work(&vm->wq);
>
>
> We set vm->removing to true and call cancel_work_sync() in
> virtio_mem_deinit_hotplug(). Isn't is sufficient?
>
> Thanks
Hmm I think you are right. David, I will drop this for now.
Up to you to consider whether some central capability will be
helpful as a replacement for the virtio-mem specific "removing" flag.
>
> > virtio_reset_device(vdev);
> > vdev->config->del_vqs(vdev);
next prev parent reply other threads:[~2022-01-17 7:55 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-14 21:43 [PATCH] virtio_mem: break device on remove Michael S. Tsirkin
2022-01-14 21:43 ` Michael S. Tsirkin
2022-01-17 6:40 ` Jason Wang
2022-01-17 6:40 ` Jason Wang
2022-01-17 7:55 ` Michael S. Tsirkin [this message]
2022-01-17 7:55 ` Michael S. Tsirkin
2022-01-17 8:31 ` David Hildenbrand
2022-01-17 8:31 ` David Hildenbrand
2022-01-17 8:40 ` Michael S. Tsirkin
2022-01-17 8:40 ` Michael S. Tsirkin
2022-01-17 10:25 ` David Hildenbrand
2022-01-17 10:25 ` David Hildenbrand
2022-01-17 21:44 ` Michael S. Tsirkin
2022-01-17 21:44 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220117025341-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=jasowang@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.