Re: [Qemu-devel] [PATCH 1/3] virtio: add virtio_detach_element()

[Ladi Prosek]

On Tue, Sep 27, 2016 at 12:08 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Tue, Sep 27, 2016 at 09:32:40AM +0200, Ladi Prosek wrote:
>> On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi <address@hidden> wrote:
>> > During device res> > +/* virtqueue_discard:
>> > + * @vq: The #VirtQueue
>> > + * @elem: The #VirtQueueElement
>> > + * @len: number of bytes written
>> > + *
>> > + * Pretend the most recent element wasn't popped from the virtqueue.  The 
>> > next
>> > + * call to virtqueue_pop() will refetch the element.
>> > + */
>> >  void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>> >                         unsigned int len)
>> >  {
>> >      vq->last_avail_idx--;
>> > -    vq->inuse--;
>> > -    virtqueue_unmap_sg(vq, elem, len);
>> > +    virtqueue_detach_element(vq, elem, len);
>>
>> Random comment, not directly related to this change. Would it be worth
>> adding an assert to this function that elem->index and
>> vq->last_avail_idx match? In other words, enforce the "most recent"
>> qualifier mentioned in the comment. As more virtqueue_* functions are
>> added and the complexity goes up, it is easy to get confused. Also, I
>> think that naming this function virtqueue_unpop instead of
>> virtqueue_discard would help.
>
> elem->index is a descriptor ring index.  vq->last_avail_idx is an index
> into the available ring.  They are different but your suggestion makes
> sense in general.

Oh, right, I didn't mean they would be identical but something like this:

  g_assert(elem->index == virtqueue_get_head(vq, vq->last_avail_idx));

> We shouldn't read from vring memory again for an assertion so
> deferencing the available ring isn't possible (because we cannot rely on
> vring memory contents after processing the request).

Not sure I follow, shouldn't available ring memory at that index still
be the same? Basically I'd like to assert that the next virtqueue_pop
would return the same element.

> One way to
> implement the assertion is to put VirtQueueElements on a linked list
> (vq->inuse_elems) but that probably needs live migration support.
>
> I agree that renaming to unpop makes the semantics clearer.
>
> Would you like to submit a patch for either or both?

Yes, I'll do both.