* Re: [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed [not found] <20230808051110.3492693-1-yuanyaogoog@chromium.org> @ 2023-08-08 5:43 ` Jason Wang 2023-08-08 5:59 ` Michael S. Tsirkin [not found] ` <CAOJyEHaXqmHStJnHrT0H4QsTJBxjBxVe+33EuWm9H3wApPKtxQ@mail.gmail.com> 0 siblings, 2 replies; 7+ messages in thread From: Jason Wang @ 2023-08-08 5:43 UTC (permalink / raw) To: Yuan Yao Cc: Tiwei Bie, Junichi Uekawa, Michael S. Tsirkin, LKML, virtualization, Takaya Saeki, Xuan Zhuo, David S. Miller On Tue, Aug 8, 2023 at 1:11 PM Yuan Yao <yuanyaogoog@chromium.org> wrote: > > In current packed virtqueue implementation, the avail_wrap_counter won't > flip, in the case when the driver supplies a descriptor chain with a > length equals to the queue size; total_sg == vq->packed.vring.num. > > Let’s assume the following situation: > vq->packed.vring.num=4 > vq->packed.next_avail_idx: 1 > vq->packed.avail_wrap_counter: 0 > > Then the driver adds a descriptor chain containing 4 descriptors. > > We expect the following result with avail_wrap_counter flipped: > vq->packed.next_avail_idx: 1 > vq->packed.avail_wrap_counter: 1 > > But, the current implementation gives the following result: > vq->packed.next_avail_idx: 1 > vq->packed.avail_wrap_counter: 0 > > To reproduce the bug, you can set a packed queue size as small as > possible, so that the driver is more likely to provide a descriptor > chain with a length equal to the packed queue size. For example, in > qemu run following commands: > sudo qemu-system-x86_64 \ > -enable-kvm \ > -nographic \ > -kernel "path/to/kernel_image" \ > -m 1G \ > -drive file="path/to/rootfs",if=none,id=disk \ > -device virtio-blk,drive=disk \ > -drive file="path/to/disk_image",if=none,id=rwdisk \ > -device virtio-blk,drive=rwdisk,packed=on,queue-size=4,\ > indirect_desc=off \ > -append "console=ttyS0 root=/dev/vda rw init=/bin/bash" > > Inside the VM, create a directory and mount the rwdisk device on it. The > rwdisk will hang and mount operation will not complete. > > This commit fixes the wrap counter error by flipping the > packed.avail_wrap_counter, when start of descriptor chain equals to the > end of descriptor chain (head == i). > > Fixes: 1ce9e6055fa0 ("virtio_ring: introduce packed ring support") > Signed-off-by: Yuan Yao <yuanyaogoog@chromium.org> > --- > > drivers/virtio/virtio_ring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index c5310eaf8b46..da1150d127c2 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -1461,7 +1461,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > } > } > > - if (i < head) > + if (i <= head) > vq->packed.avail_wrap_counter ^= 1; Would it be better to move the flipping to the place where we flip avail_used_flags? if ((unlikely(++i >= vq->packed.vring.num))) { i = 0; vq->packed.avail_used_flags ^= 1 << VRING_PACKED_DESC_F_AVAIL | 1 << VRING_PACKED_DESC_F_USED; } Thanks > > /* We're using some buffers from the free list. */ > -- > 2.41.0.640.ga95def55d0-goog > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed 2023-08-08 5:43 ` [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed Jason Wang @ 2023-08-08 5:59 ` Michael S. Tsirkin 2023-08-08 6:05 ` Jason Wang [not found] ` <CAOJyEHaXqmHStJnHrT0H4QsTJBxjBxVe+33EuWm9H3wApPKtxQ@mail.gmail.com> 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2023-08-08 5:59 UTC (permalink / raw) To: Jason Wang Cc: Tiwei Bie, Junichi Uekawa, LKML, virtualization, Yuan Yao, Takaya Saeki, Xuan Zhuo, David S. Miller On Tue, Aug 08, 2023 at 01:43:02PM +0800, Jason Wang wrote: > On Tue, Aug 8, 2023 at 1:11 PM Yuan Yao <yuanyaogoog@chromium.org> wrote: > > > > In current packed virtqueue implementation, the avail_wrap_counter won't > > flip, in the case when the driver supplies a descriptor chain with a > > length equals to the queue size; total_sg == vq->packed.vring.num. > > > > Let’s assume the following situation: > > vq->packed.vring.num=4 > > vq->packed.next_avail_idx: 1 > > vq->packed.avail_wrap_counter: 0 > > > > Then the driver adds a descriptor chain containing 4 descriptors. > > > > We expect the following result with avail_wrap_counter flipped: > > vq->packed.next_avail_idx: 1 > > vq->packed.avail_wrap_counter: 1 > > > > But, the current implementation gives the following result: > > vq->packed.next_avail_idx: 1 > > vq->packed.avail_wrap_counter: 0 > > > > To reproduce the bug, you can set a packed queue size as small as > > possible, so that the driver is more likely to provide a descriptor > > chain with a length equal to the packed queue size. For example, in > > qemu run following commands: > > sudo qemu-system-x86_64 \ > > -enable-kvm \ > > -nographic \ > > -kernel "path/to/kernel_image" \ > > -m 1G \ > > -drive file="path/to/rootfs",if=none,id=disk \ > > -device virtio-blk,drive=disk \ > > -drive file="path/to/disk_image",if=none,id=rwdisk \ > > -device virtio-blk,drive=rwdisk,packed=on,queue-size=4,\ > > indirect_desc=off \ > > -append "console=ttyS0 root=/dev/vda rw init=/bin/bash" > > > > Inside the VM, create a directory and mount the rwdisk device on it. The > > rwdisk will hang and mount operation will not complete. > > > > This commit fixes the wrap counter error by flipping the > > packed.avail_wrap_counter, when start of descriptor chain equals to the > > end of descriptor chain (head == i). > > > > Fixes: 1ce9e6055fa0 ("virtio_ring: introduce packed ring support") > > Signed-off-by: Yuan Yao <yuanyaogoog@chromium.org> > > --- > > > > drivers/virtio/virtio_ring.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index c5310eaf8b46..da1150d127c2 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -1461,7 +1461,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > } > > } > > > > - if (i < head) > > + if (i <= head) > > vq->packed.avail_wrap_counter ^= 1; > > Would it be better to move the flipping to the place where we flip > avail_used_flags? I think I prefer this patch for stable, refactoring can be done on top. > if ((unlikely(++i >= vq->packed.vring.num))) { > i = 0; > vq->packed.avail_used_flags ^= > 1 << VRING_PACKED_DESC_F_AVAIL | > 1 << VRING_PACKED_DESC_F_USED; > } > > Thanks > > > > > /* We're using some buffers from the free list. */ > > -- > > 2.41.0.640.ga95def55d0-goog > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed 2023-08-08 5:59 ` Michael S. Tsirkin @ 2023-08-08 6:05 ` Jason Wang 0 siblings, 0 replies; 7+ messages in thread From: Jason Wang @ 2023-08-08 6:05 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Tiwei Bie, Junichi Uekawa, LKML, virtualization, Yuan Yao, Takaya Saeki, Xuan Zhuo, David S. Miller On Tue, Aug 8, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Aug 08, 2023 at 01:43:02PM +0800, Jason Wang wrote: > > On Tue, Aug 8, 2023 at 1:11 PM Yuan Yao <yuanyaogoog@chromium.org> wrote: > > > > > > In current packed virtqueue implementation, the avail_wrap_counter won't > > > flip, in the case when the driver supplies a descriptor chain with a > > > length equals to the queue size; total_sg == vq->packed.vring.num. > > > > > > Let’s assume the following situation: > > > vq->packed.vring.num=4 > > > vq->packed.next_avail_idx: 1 > > > vq->packed.avail_wrap_counter: 0 > > > > > > Then the driver adds a descriptor chain containing 4 descriptors. > > > > > > We expect the following result with avail_wrap_counter flipped: > > > vq->packed.next_avail_idx: 1 > > > vq->packed.avail_wrap_counter: 1 > > > > > > But, the current implementation gives the following result: > > > vq->packed.next_avail_idx: 1 > > > vq->packed.avail_wrap_counter: 0 > > > > > > To reproduce the bug, you can set a packed queue size as small as > > > possible, so that the driver is more likely to provide a descriptor > > > chain with a length equal to the packed queue size. For example, in > > > qemu run following commands: > > > sudo qemu-system-x86_64 \ > > > -enable-kvm \ > > > -nographic \ > > > -kernel "path/to/kernel_image" \ > > > -m 1G \ > > > -drive file="path/to/rootfs",if=none,id=disk \ > > > -device virtio-blk,drive=disk \ > > > -drive file="path/to/disk_image",if=none,id=rwdisk \ > > > -device virtio-blk,drive=rwdisk,packed=on,queue-size=4,\ > > > indirect_desc=off \ > > > -append "console=ttyS0 root=/dev/vda rw init=/bin/bash" > > > > > > Inside the VM, create a directory and mount the rwdisk device on it. The > > > rwdisk will hang and mount operation will not complete. > > > > > > This commit fixes the wrap counter error by flipping the > > > packed.avail_wrap_counter, when start of descriptor chain equals to the > > > end of descriptor chain (head == i). > > > > > > Fixes: 1ce9e6055fa0 ("virtio_ring: introduce packed ring support") > > > Signed-off-by: Yuan Yao <yuanyaogoog@chromium.org> > > > --- > > > > > > drivers/virtio/virtio_ring.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index c5310eaf8b46..da1150d127c2 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -1461,7 +1461,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, > > > } > > > } > > > > > > - if (i < head) > > > + if (i <= head) > > > vq->packed.avail_wrap_counter ^= 1; > > > > Would it be better to move the flipping to the place where we flip > > avail_used_flags? > > I think I prefer this patch for stable, refactoring can > be done on top. Ok. So Acked-by: Jason Wang <jasowang@redhat.com> Thanks > > > if ((unlikely(++i >= vq->packed.vring.num))) { > > i = 0; > > vq->packed.avail_used_flags ^= > > 1 << VRING_PACKED_DESC_F_AVAIL | > > 1 << VRING_PACKED_DESC_F_USED; > > } > > > > Thanks > > > > > > > > /* We're using some buffers from the free list. */ > > > -- > > > 2.41.0.640.ga95def55d0-goog > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CAOJyEHaXqmHStJnHrT0H4QsTJBxjBxVe+33EuWm9H3wApPKtxQ@mail.gmail.com>]
* Re: [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed [not found] ` <CAOJyEHaXqmHStJnHrT0H4QsTJBxjBxVe+33EuWm9H3wApPKtxQ@mail.gmail.com> @ 2023-08-08 6:05 ` Jason Wang [not found] ` <CAOJyEHaR1Y3VsKNpLqxf-ewAEf8JJDChjmnFM_0mv=hOg+X-vA@mail.gmail.com> 0 siblings, 1 reply; 7+ messages in thread From: Jason Wang @ 2023-08-08 6:05 UTC (permalink / raw) To: Yuan Yao Cc: Tiwei Bie, Junichi Uekawa, Michael S. Tsirkin, LKML, virtualization, Takaya Saeki, Xuan Zhuo, David S. Miller On Tue, Aug 8, 2023 at 1:59 PM Yuan Yao <yuanyaogoog@chromium.org> wrote: > > Since there is a check for 0-length chain in code;BUG_ON(total_sg == 0), we won’t get a 0-length chain in practice. So, I think use (i <= head) makes the commit as small as possible. Ok, offered ack in other mail. Thanks > > > Best regards > > > > On Tue, Aug 8, 2023 at 2:43 PM Jason Wang <jasowang@redhat.com> wrote: >> >> On Tue, Aug 8, 2023 at 1:11 PM Yuan Yao <yuanyaogoog@chromium.org> wrote: >> > >> > In current packed virtqueue implementation, the avail_wrap_counter won't >> > flip, in the case when the driver supplies a descriptor chain with a >> > length equals to the queue size; total_sg == vq->packed.vring.num. >> > >> > Let’s assume the following situation: >> > vq->packed.vring.num=4 >> > vq->packed.next_avail_idx: 1 >> > vq->packed.avail_wrap_counter: 0 >> > >> > Then the driver adds a descriptor chain containing 4 descriptors. >> > >> > We expect the following result with avail_wrap_counter flipped: >> > vq->packed.next_avail_idx: 1 >> > vq->packed.avail_wrap_counter: 1 >> > >> > But, the current implementation gives the following result: >> > vq->packed.next_avail_idx: 1 >> > vq->packed.avail_wrap_counter: 0 >> > >> > To reproduce the bug, you can set a packed queue size as small as >> > possible, so that the driver is more likely to provide a descriptor >> > chain with a length equal to the packed queue size. For example, in >> > qemu run following commands: >> > sudo qemu-system-x86_64 \ >> > -enable-kvm \ >> > -nographic \ >> > -kernel "path/to/kernel_image" \ >> > -m 1G \ >> > -drive file="path/to/rootfs",if=none,id=disk \ >> > -device virtio-blk,drive=disk \ >> > -drive file="path/to/disk_image",if=none,id=rwdisk \ >> > -device virtio-blk,drive=rwdisk,packed=on,queue-size=4,\ >> > indirect_desc=off \ >> > -append "console=ttyS0 root=/dev/vda rw init=/bin/bash" >> > >> > Inside the VM, create a directory and mount the rwdisk device on it. The >> > rwdisk will hang and mount operation will not complete. >> > >> > This commit fixes the wrap counter error by flipping the >> > packed.avail_wrap_counter, when start of descriptor chain equals to the >> > end of descriptor chain (head == i). >> > >> > Fixes: 1ce9e6055fa0 ("virtio_ring: introduce packed ring support") >> > Signed-off-by: Yuan Yao <yuanyaogoog@chromium.org> >> > --- >> > >> > drivers/virtio/virtio_ring.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >> > index c5310eaf8b46..da1150d127c2 100644 >> > --- a/drivers/virtio/virtio_ring.c >> > +++ b/drivers/virtio/virtio_ring.c >> > @@ -1461,7 +1461,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, >> > } >> > } >> > >> > - if (i < head) >> > + if (i <= head) >> > vq->packed.avail_wrap_counter ^= 1; >> >> Would it be better to move the flipping to the place where we flip >> avail_used_flags? >> >> if ((unlikely(++i >= vq->packed.vring.num))) { >> i = 0; >> vq->packed.avail_used_flags ^= >> 1 << VRING_PACKED_DESC_F_AVAIL | >> 1 << VRING_PACKED_DESC_F_USED; >> } >> >> Thanks >> >> > >> > /* We're using some buffers from the free list. */ >> > -- >> > 2.41.0.640.ga95def55d0-goog >> > >> _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CAOJyEHaR1Y3VsKNpLqxf-ewAEf8JJDChjmnFM_0mv=hOg+X-vA@mail.gmail.com>]
* Re: [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed [not found] ` <CAOJyEHaR1Y3VsKNpLqxf-ewAEf8JJDChjmnFM_0mv=hOg+X-vA@mail.gmail.com> @ 2023-08-08 9:13 ` Michael S. Tsirkin [not found] ` <CAOJyEHYgvw7za0ksKNPu9TF1+8MwVFbctMbukgbAoQnf9da+hA@mail.gmail.com> 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2023-08-08 9:13 UTC (permalink / raw) To: Yuan Yao Cc: Tiwei Bie, Junichi Uekawa, LKML, virtualization, Takaya Saeki, Xuan Zhuo, David S. Miller On Tue, Aug 08, 2023 at 05:37:29PM +0900, Yuan Yao wrote: > Thank you for reviewing, sent a patch with your ack. Don't do this pls. And don't top post please. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CAOJyEHYgvw7za0ksKNPu9TF1+8MwVFbctMbukgbAoQnf9da+hA@mail.gmail.com>]
[parent not found: <CAOJyEHZs=59nZ=XTYu-mZWTz18OT7f6TknCxWksYeQZbPy2oUQ@mail.gmail.com>]
* Re: [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed [not found] ` <CAOJyEHZs=59nZ=XTYu-mZWTz18OT7f6TknCxWksYeQZbPy2oUQ@mail.gmail.com> @ 2023-08-28 4:43 ` Michael S. Tsirkin 0 siblings, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2023-08-28 4:43 UTC (permalink / raw) To: Yuan Yao Cc: Junichi Uekawa, LKML, virtualization, Takaya Saeki, Xuan Zhuo, David S. Miller Nope - it will be in the next linux release. On Mon, Aug 28, 2023 at 12:33:46PM +0900, Yuan Yao wrote: > I'm writing to confirm the process for the patch, since I'm not sure if this > thread is done for this patch. > Do I need any further steps to take to make this patch be launched? > > On Tue, Aug 8, 2023 at 7:00 PM Yuan Yao <yuanyaogoog@chromium.org> wrote: > > Sorry for the confusing mail, I didn't understand how it works. > Thanks > > On Tue, Aug 8, 2023 at 6:13 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Aug 08, 2023 at 05:37:29PM +0900, Yuan Yao wrote: > > Thank you for reviewing, sent a patch with your ack. > > Don't do this pls. And don't top post please. > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <20230808083257.3777012-1-yuanyaogoog@chromium.org>]
[parent not found: <CAOJyEHb_KjPawwH+U30iJCDjB-VqH_FR-4qAsUk9T6Sn8FZMBQ@mail.gmail.com>]
* Re: [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed [not found] ` <CAOJyEHb_KjPawwH+U30iJCDjB-VqH_FR-4qAsUk9T6Sn8FZMBQ@mail.gmail.com> @ 2023-08-10 19:19 ` Michael S. Tsirkin 0 siblings, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2023-08-10 19:19 UTC (permalink / raw) To: Yuan Yao Cc: Junichi Uekawa, Tiwei Bie, LKML, virtualization, Takaya Saeki, Xuan Zhuo, David S. Miller On Tue, Aug 08, 2023 at 07:05:38PM +0900, Yuan Yao wrote: > Sorry, but please ignore this thread. ok, dropped. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-08-28 4:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230808051110.3492693-1-yuanyaogoog@chromium.org>
2023-08-08 5:43 ` [PATCH] virtio_ring: fix avail_wrap_counter in virtqueue_add_packed Jason Wang
2023-08-08 5:59 ` Michael S. Tsirkin
2023-08-08 6:05 ` Jason Wang
[not found] ` <CAOJyEHaXqmHStJnHrT0H4QsTJBxjBxVe+33EuWm9H3wApPKtxQ@mail.gmail.com>
2023-08-08 6:05 ` Jason Wang
[not found] ` <CAOJyEHaR1Y3VsKNpLqxf-ewAEf8JJDChjmnFM_0mv=hOg+X-vA@mail.gmail.com>
2023-08-08 9:13 ` Michael S. Tsirkin
[not found] ` <CAOJyEHYgvw7za0ksKNPu9TF1+8MwVFbctMbukgbAoQnf9da+hA@mail.gmail.com>
[not found] ` <CAOJyEHZs=59nZ=XTYu-mZWTz18OT7f6TknCxWksYeQZbPy2oUQ@mail.gmail.com>
2023-08-28 4:43 ` Michael S. Tsirkin
[not found] <20230808083257.3777012-1-yuanyaogoog@chromium.org>
[not found] ` <CAOJyEHb_KjPawwH+U30iJCDjB-VqH_FR-4qAsUk9T6Sn8FZMBQ@mail.gmail.com>
2023-08-10 19:19 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).