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.
Thanks,
Ladi
