On Thu, Sep 08, 2016 at 09:30:13AM +0200, Ladi Prosek wrote: > On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <rka...@virtuozzo.com> wrote: > > On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote: > >> From: Stefan Hajnoczi <stefa...@redhat.com> > >> > >> virtqueue_discard() requires a VirtQueueElement but virtio-balloon does > >> not migrate its in-use element. Introduce a new function that is > >> similar to virtqueue_discard() but doesn't require a VirtQueueElement. > >> > >> This will allow virtio-balloon to access element again after migration > >> with the usual proviso that the guest may have modified the vring since > >> last time. > >> > >> Cc: Michael S. Tsirkin <m...@redhat.com> > >> Cc: Roman Kagan <rka...@virtuozzo.com> > >> Cc: Stefan Hajnoczi <stefa...@redhat.com> > >> Signed-off-by: Ladi Prosek <lpro...@redhat.com> > >> --- > >> hw/virtio/virtio.c | 22 ++++++++++++++++++++++ > >> include/hw/virtio/virtio.h | 1 + > >> 2 files changed, 23 insertions(+) > > > > Reviewed-by: Roman Kagan <rka...@virtuozzo.com> > > > > One question though: > > > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >> index 74c085c..3de6029 100644 > >> --- a/hw/virtio/virtio.c > >> +++ b/hw/virtio/virtio.c > >> @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const > >> VirtQueueElement *elem, > >> virtqueue_unmap_sg(vq, elem, len); > >> } > >> > >> +/* virtqueue_rewind: > >> + * @vq: The #VirtQueue > >> + * @num: Number of elements to push back > >> + * > >> + * Pretend that elements weren't popped from the virtqueue. The next > >> + * virtqueue_pop() will refetch the oldest element. > >> + * > >> + * Use virtqueue_discard() instead if you have a VirtQueueElement. > >> + * > >> + * Returns: true on success, false if @num is greater than the number of > >> in use > >> + * elements. > >> + */ > >> +bool virtqueue_rewind(VirtQueue *vq, unsigned int num) > >> +{ > >> + if (num > vq->inuse) { > >> + return false; > >> + } > >> + vq->last_avail_idx -= num; > >> + vq->inuse -= num; > >> + return true; > >> +} > >> + > > > > Presumably you envision rewinding by something other than ->inuse. Do > > you have in mind a usecase for that, or is it just a matter of API > > symmetry or whatnot? > > It may not always be correct to rewind by ->inuse. The elements that > are in use do not necessarily have to be at the end of the available > ring. Example: > > elem1 = virtqueue_pop(); > elem2 = virtqueue_pop(); > elem3 = virtqueue_pop(); > > virtqueue_push(elem2); > > Now inuse == 2 but rewinding by 2 would be incorrect because elem2 has > already been pushed to the used ring. > > So it is a dangerous API, which I believe is why Stefan added the num > parameter even though it is currently not needed.
The function could be hard-coded to rewind just 1 element. I wanted to make it easy for a device to rewind the last n elements, but this functionality isn't used. Stefan
signature.asc
Description: PGP signature