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

[Stefan Hajnoczi]

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.

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