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. 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). 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?
signature.asc
Description: PGP signature
