[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind()
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind() |
Date: |
Mon, 12 Sep 2016 10:20:26 +0100 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Thu, Sep 08, 2016 at 09:30:13AM +0200, Ladi Prosek wrote:
> On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <address@hidden> wrote:
> > On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote:
> >> From: Stefan Hajnoczi <address@hidden>
> >>
> >> 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 <address@hidden>
> >> Cc: Roman Kagan <address@hidden>
> >> Cc: Stefan Hajnoczi <address@hidden>
> >> Signed-off-by: Ladi Prosek <address@hidden>
> >> ---
> >> hw/virtio/virtio.c | 22 ++++++++++++++++++++++
> >> include/hw/virtio/virtio.h | 1 +
> >> 2 files changed, 23 insertions(+)
> >
> > Reviewed-by: Roman Kagan <address@hidden>
> >
> > 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