On Tue, Sep 27, 2016 at 12:08 PM, Stefan Hajnoczi <stefa...@gmail.com> 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 <stefa...@redhat.com> 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.
