| Message ID | 20240506150428.1203387-3-jonah.palmer@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio,vhost: Add VIRTIO_F_IN_ORDER support | expand |
On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote: > > Add VIRTIO_F_IN_ORDER feature support in virtqueue_split_pop and > virtqueue_packed_pop. > > VirtQueueElements popped from the available/descritpor ring are added to > the VirtQueue's used_elems array in-order and in the same fashion as > they would be added the used and descriptor rings, respectively. > > This will allow us to keep track of the current order, what elements > have been written, as well as an element's essential data after being > processed. > > Tested-by: Lei Yang <leiyang@redhat.com> > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> > --- > hw/virtio/virtio.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 893a072c9d..e6eb1bb453 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu > > static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) > { > - unsigned int i, head, max; > + unsigned int i, j, head, max; > VRingMemoryRegionCaches *caches; > MemoryRegionCache indirect_desc_cache; > MemoryRegionCache *desc_cache; > @@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) > goto done; > } > > + j = vq->last_avail_idx; > + > if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) { > goto done; > } > @@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) > elem->in_sg[i] = iov[out_num + i]; > } > > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) { > + vq->used_elems[j].index = elem->index; > + vq->used_elems[j].len = elem->len; > + vq->used_elems[j].ndescs = elem->ndescs; > + } > + > vq->inuse++; > > trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num); > @@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz) > > elem->index = id; > elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries; > + > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) { > + vq->used_elems[vq->last_avail_idx].index = elem->index; > + vq->used_elems[vq->last_avail_idx].len = elem->len; > + vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs; > + } > + I suggest using a consistent style between packed and split: Either always use vq->last_avail_idx or j. If you use j, please rename to something more related to the usage, as j is usually for iterations. In my opinion I think vq->last_avail_idx is better. > vq->last_avail_idx += elem->ndescs; > vq->inuse += elem->ndescs; > > -- > 2.39.3 >
On 5/9/24 9:13 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 in virtqueue_split_pop and >> virtqueue_packed_pop. >> >> VirtQueueElements popped from the available/descritpor ring are added to >> the VirtQueue's used_elems array in-order and in the same fashion as >> they would be added the used and descriptor rings, respectively. >> >> This will allow us to keep track of the current order, what elements >> have been written, as well as an element's essential data after being >> processed. >> >> Tested-by: Lei Yang <leiyang@redhat.com> >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> >> --- >> hw/virtio/virtio.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 893a072c9d..e6eb1bb453 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu >> >> static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) >> { >> - unsigned int i, head, max; >> + unsigned int i, j, head, max; >> VRingMemoryRegionCaches *caches; >> MemoryRegionCache indirect_desc_cache; >> MemoryRegionCache *desc_cache; >> @@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) >> goto done; >> } >> >> + j = vq->last_avail_idx; >> + >> if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) { >> goto done; >> } >> @@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) >> elem->in_sg[i] = iov[out_num + i]; >> } >> >> + if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) { >> + vq->used_elems[j].index = elem->index; >> + vq->used_elems[j].len = elem->len; >> + vq->used_elems[j].ndescs = elem->ndescs; >> + } >> + >> vq->inuse++; >> >> trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num); >> @@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz) >> >> elem->index = id; >> elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries; >> + >> + if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) { >> + vq->used_elems[vq->last_avail_idx].index = elem->index; >> + vq->used_elems[vq->last_avail_idx].len = elem->len; >> + vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs; >> + } >> + > > I suggest using a consistent style between packed and split: Either > always use vq->last_avail_idx or j. If you use j, please rename to > something more related to the usage, as j is usually for iterations. > > In my opinion I think vq->last_avail_idx is better. > > Totally agree. The reason I used a separate variable in virtqueue_split_pop was to capture the value of vq->last_avail_idx before it got incremented in the next line. Not sure if it actually matters whether or not I use the value of last_avail_idx before or after it's incremented. I don't think it does but, in any case, I opted to use the value before it was incremented so as to be consistent with virtqueue_packed_pop, where last_avail_idx is used before it's incremented. I'll change j to something more meaningful though. Maybe 'init_last_avail_idx'? Hmm... will need to think on it. >> vq->last_avail_idx += elem->ndescs; >> vq->inuse += elem->ndescs; >> >> -- >> 2.39.3 >> >
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 893a072c9d..e6eb1bb453 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) { - unsigned int i, head, max; + unsigned int i, j, head, max; VRingMemoryRegionCaches *caches; MemoryRegionCache indirect_desc_cache; MemoryRegionCache *desc_cache; @@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) goto done; } + j = vq->last_avail_idx; + if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) { goto done; } @@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz) elem->in_sg[i] = iov[out_num + i]; } + if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) { + vq->used_elems[j].index = elem->index; + vq->used_elems[j].len = elem->len; + vq->used_elems[j].ndescs = elem->ndescs; + } + vq->inuse++; trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num); @@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz) elem->index = id; elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries; + + if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) { + vq->used_elems[vq->last_avail_idx].index = elem->index; + vq->used_elems[vq->last_avail_idx].len = elem->len; + vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs; + } + vq->last_avail_idx += elem->ndescs; vq->inuse += elem->ndescs;