Message ID | 20200804145317.51633-1-alexandru.elisei@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2,kvmtool] virtio: Fix ordering of virtio_queue__should_signal() | expand |
Hello, > I *think* this also fixes the VM hang reported in [2], where several > processes in the guest were stuck in uninterruptible sleep. I am not > familiar with the block layer, but my theory is that the threads were stuck > in wait_for_completion_io(), from blk_execute_rq() executing a flush > request. Milan has agreed to give this patch a spin [3], but that might > take a while because the bug is not easily reproducible. I believe the > patch can be merged on its own. > > [1] http://diy.inria.fr/www/index.html?record=aarch64&cat=aarch64-v04&litmus=SB%2Bdmb.sys&cfg=new-web > [2] https://www.spinics.net/lists/kvm/msg204543.html > [3] https://www.spinics.net/lists/kvm/msg222201.html > Unfortunately it didn't help. I can see the problem again now. Thanks. Best regards.
Hi Milan, On 8/5/20 8:53 AM, Milan Kocian wrote: > Hello, > >> I *think* this also fixes the VM hang reported in [2], where several >> processes in the guest were stuck in uninterruptible sleep. I am not >> familiar with the block layer, but my theory is that the threads were stuck >> in wait_for_completion_io(), from blk_execute_rq() executing a flush >> request. Milan has agreed to give this patch a spin [3], but that might >> take a while because the bug is not easily reproducible. I believe the >> patch can be merged on its own. >> >> [1] http://diy.inria.fr/www/index.html?record=aarch64&cat=aarch64-v04&litmus=SB%2Bdmb.sys&cfg=new-web >> [2] https://www.spinics.net/lists/kvm/msg204543.html >> [3] https://www.spinics.net/lists/kvm/msg222201.html >> > Unfortunately it didn't help. I can see the problem again now. Thank you for giving this a go. Unfortunately this means there's another bug lurking in kvmtool. I'll put this on my todo list and I'll try to figure something out when I have some spare time. Thanks, Alex
On Tue, 4 Aug 2020 15:53:17 +0100, Alexandru Elisei wrote: > The guest programs used_event in the avail ring to let the host know when > it wants a notification from the device. The host notifies the guest when > the used ring index passes used_event. It is possible for the guest to > submit a buffer, and then go into uninterruptible sleep waiting for this > notification. > > The virtio-blk guest driver, in the notification callback virtblk_done(), > increments the last known used ring index, then sets used_event to this > value, which means it will get a notification after the next buffer is > consumed by the host. virtblk_done() exits after the value of the used > ring idx has been propagated from the host thread. > > [...] Applied to kvmtool (master), thanks! [1/1] virtio: Fix ordering of virtio_queue__should_signal() https://git.kernel.org/will/kvmtool/c/d7d79bd51412 Cheers,
diff --git a/virtio/core.c b/virtio/core.c index f5b3c07fc100..90a661d12e14 100644 --- a/virtio/core.c +++ b/virtio/core.c @@ -33,13 +33,6 @@ void virt_queue__used_idx_advance(struct virt_queue *queue, u16 jump) wmb(); idx += jump; queue->vring.used->idx = virtio_host_to_guest_u16(queue, idx); - - /* - * Use wmb to assure used idx has been increased before we signal the guest. - * Without a wmb here the guest may ignore the queue since it won't see - * an updated idx. - */ - wmb(); } struct vring_used_elem * @@ -194,6 +187,14 @@ bool virtio_queue__should_signal(struct virt_queue *vq) { u16 old_idx, new_idx, event_idx; + /* + * Use mb to assure used idx has been increased before we signal the + * guest, and we don't read a stale value for used_event. Without a mb + * here we might not send a notification that we need to send, or the + * guest may ignore the queue since it won't see an updated idx. + */ + mb(); + if (!vq->use_event_idx) { /* * When VIRTIO_RING_F_EVENT_IDX isn't negotiated, interrupt the