Re: [Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind()

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3036 bytes --]

On Thu, Sep 08, 2016 at 09:30:13AM +0200, Ladi Prosek wrote:
> On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> > On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote:
> >> From: Stefan Hajnoczi <stefanha@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 <mst@redhat.com>
> >> Cc: Roman Kagan <rkagan@virtuozzo.com>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >> ---
> >>  hw/virtio/virtio.c         | 22 ++++++++++++++++++++++
> >>  include/hw/virtio/virtio.h |  1 +
> >>  2 files changed, 23 insertions(+)
> >
> > Reviewed-by: Roman Kagan <rkagan@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]