From: Jonah Palmer <jonah.palmer@oracle.com>
To: Eugenio Perez Martin <eperezma@redhat.com>
Cc: qemu-devel@nongnu.org, mst@redhat.com, raphael@enfabrica.net,
kwolf@redhat.com, hreitz@redhat.com, jasowang@redhat.com,
pbonzini@redhat.com, fam@euphon.net, stefanha@redhat.com,
qemu-block@nongnu.org, schalla@marvell.com, leiyang@redhat.com,
virtio-fs@lists.linux.dev, si-wei.liu@oracle.com,
boris.ostrovsky@oracle.com
Subject: Re: [PATCH 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support
Date: Fri, 10 May 2024 09:07:31 -0400 [thread overview]
Message-ID: <db452030-2ff3-42e6-94b7-175169123ac4@oracle.com> (raw)
In-Reply-To: <CAJaqyWd8ALnpr5BVOFpWo1ndUu9Y3XHO0UKtHFzEOFW3FsmWpA@mail.gmail.com>
On 5/10/24 3:48 AM, Eugenio Perez Martin wrote:
> On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> Add VIRTIO_F_IN_ORDER feature support for virtqueue_flush operations.
>>
>> The goal of the virtqueue_flush operation when the VIRTIO_F_IN_ORDER
>> feature has been negotiated is to write elements to the used/descriptor
>> ring in-order and then update used_idx.
>>
>> The function iterates through the VirtQueueElement used_elems array
>> in-order starting at vq->used_idx. If the element is valid (filled), the
>> element is written to the used/descriptor ring. This process continues
>> until we find an invalid (not filled) element.
>>
>> If any elements were written, the used_idx is updated.
>>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>> hw/virtio/virtio.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 064046b5e2..0efed2c88e 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1006,6 +1006,77 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>> }
>> }
>>
>> +static void virtqueue_ordered_flush(VirtQueue *vq)
>> +{
>> + unsigned int i = vq->used_idx;
>> + unsigned int ndescs = 0;
>> + uint16_t old = vq->used_idx;
>> + bool packed;
>> + VRingUsedElem uelem;
>> +
>> + packed = virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED);
>> +
>> + if (packed) {
>> + if (unlikely(!vq->vring.desc)) {
>> + return;
>> + }
>> + } else if (unlikely(!vq->vring.used)) {
>> + return;
>> + }
>> +
>> + /* First expected in-order element isn't ready, nothing to do */
>> + if (!vq->used_elems[i].filled) {
>> + return;
>> + }
>> +
>> + /* Write first expected in-order element to used ring (split VQs) */
>> + if (!packed) {
>> + uelem.id = vq->used_elems[i].index;
>> + uelem.len = vq->used_elems[i].len;
>> + vring_used_write(vq, &uelem, i);
>> + }
>> +
>> + ndescs += vq->used_elems[i].ndescs;
>> + i += ndescs;
>> + if (i >= vq->vring.num) {
>> + i -= vq->vring.num;
>> + }
>> +
>> + /* Search for more filled elements in-order */
>> + while (vq->used_elems[i].filled) {
>> + if (packed) {
>> + virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false);
>> + } else {
>> + uelem.id = vq->used_elems[i].index;
>> + uelem.len = vq->used_elems[i].len;
>> + vring_used_write(vq, &uelem, i);
>> + }
>> +
>> + vq->used_elems[i].filled = false;
>> + ndescs += vq->used_elems[i].ndescs;
>> + i += ndescs;
>> + if (i >= vq->vring.num) {
>> + i -= vq->vring.num;
>> + }
>> + }
>> +
>
> I may be missing something, but you have split out the first case as a
> special one, totally out of the while loop. Can't it be contained in
> the loop checking !(packed && i == vq->used_idx)? That would avoid
> code duplication.
>
> A comment can be added in the line of "first entry of packed is
> written the last so the guest does not see invalid descriptors".
>
Yea this was intentional for the reason you've given above. It was
either the solution above or, as you suggest, handling this in the while
loop:
if (!vq->used_elems[i].filled) {
return;
}
while (vq->used_elems[i].filled) {
if (packed && i != vq->used_idx) {
virtqueue_packed_fill_desc(...);
} else {
...
}
...
}
I did consider this option at the time of writing this patch but I
must've overcomplicated it in my head somehow and thought the current
solution was the simpler one. However, after looking it over again, your
suggestion is indeed the cleaner one.
Will adjust this in v2! Thanks for your time reviewing these!
>> + if (packed) {
>> + virtqueue_packed_fill_desc(vq, &vq->used_elems[vq->used_idx], 0, true);
>> + vq->used_idx += ndescs;
>> + if (vq->used_idx >= vq->vring.num) {
>> + vq->used_idx -= vq->vring.num;
>> + vq->used_wrap_counter ^= 1;
>> + vq->signalled_used_valid = false;
>> + }
>> + } else {
>> + vring_used_idx_set(vq, i);
>> + if (unlikely((int16_t)(i - vq->signalled_used) < (uint16_t)(i - old))) {
>> + vq->signalled_used_valid = false;
>> + }
>> + }
>> + vq->inuse -= ndescs;
>> +}
>> +
>> void virtqueue_flush(VirtQueue *vq, unsigned int count)
>> {
>> if (virtio_device_disabled(vq->vdev)) {
>> @@ -1013,7 +1084,9 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>> return;
>> }
>>
>> - if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>> + if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
>> + virtqueue_ordered_flush(vq);
>> + } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>> virtqueue_packed_flush(vq, count);
>> } else {
>> virtqueue_split_flush(vq, count);
>> --
>> 2.39.3
>>
>
next prev parent reply other threads:[~2024-05-10 13:08 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 15:04 [PATCH 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-06 15:04 ` [PATCH 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
2024-05-09 12:32 ` Eugenio Perez Martin
2024-05-10 10:26 ` Jonah Palmer
2024-05-06 15:04 ` [PATCH 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-09 13:13 ` Eugenio Perez Martin
2024-05-10 10:52 ` Jonah Palmer
2024-05-06 15:04 ` [PATCH 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
2024-05-09 14:08 ` Eugenio Perez Martin
2024-05-10 12:01 ` Jonah Palmer
2024-05-06 15:04 ` [PATCH 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
2024-05-10 7:48 ` Eugenio Perez Martin
2024-05-10 13:07 ` Jonah Palmer [this message]
2024-05-06 15:04 ` [PATCH 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits Jonah Palmer via
2024-05-06 15:04 ` [PATCH 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
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=db452030-2ff3-42e6-94b7-175169123ac4@oracle.com \
--to=jonah.palmer@oracle.com \
--cc=boris.ostrovsky@oracle.com \
--cc=eperezma@redhat.com \
--cc=fam@euphon.net \
--cc=hreitz@redhat.com \
--cc=jasowang@redhat.com \
--cc=kwolf@redhat.com \
--cc=leiyang@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=raphael@enfabrica.net \
--cc=schalla@marvell.com \
--cc=si-wei.liu@oracle.com \
--cc=stefanha@redhat.com \
--cc=virtio-fs@lists.linux.dev \
/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 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 read-only IMAP folder(s) and NNTP newsgroup(s).