From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52398) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bopKV-0000e5-K7 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 06:09:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bopKR-0002Ni-BL for qemu-devel@nongnu.org; Tue, 27 Sep 2016 06:09:38 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35443) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bopKR-0002NK-5G for qemu-devel@nongnu.org; Tue, 27 Sep 2016 06:09:35 -0400 Received: by mail-wm0-f66.google.com with SMTP id b4so347400wmb.2 for ; Tue, 27 Sep 2016 03:09:35 -0700 (PDT) Date: Tue, 27 Sep 2016 11:08:32 +0100 From: Stefan Hajnoczi Message-ID: <20160927100832.GC563@stefanha-x1.localdomain> References: <1474291685-24226-1-git-send-email-stefanha@redhat.com> <1474291685-24226-2-git-send-email-stefanha@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="KDt/GgjP6HVcx58l" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1/3] virtio: add virtio_detach_element() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ladi Prosek Cc: Stefan Hajnoczi , qemu-devel , "Michael S. Tsirkin" --KDt/GgjP6HVcx58l Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 27, 2016 at 09:32:40AM +0200, Ladi Prosek wrote: > On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi wr= ote: > > 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); >=20 > 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? --KDt/GgjP6HVcx58l Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJX6kUgAAoJEJykq7OBq3PIwOIH/264u0lNWGsaZS7nc+UxUC/8 GU6XlshGWrzR07Bp6sGE1HLaJ/BUWWcshH/6FPXj9JUvzGTFAhQBh6ptocbNYKgv ipWAkusXAGv8xwFkprrzGLEphhwcrKy+Osx8NBFnWOYKByVnyhj4EFUAypHGyw1n 7pkrYv2izJJ7hGGN/NnJogntQYnBf3yPSglZ8ZB5XATbtxpykv/LXElkPWxvaR7e YQ/V0N3L2fz7nfVSPIMsW4/rMm4ruAjHDQ/edkrpqgCwszB85C2GFQ/bIvlp2o1X EED/rJlxMNmO08LInpApoSuPCi+vT4LMzUcGVxEvZ+IudD/OQ64wDJvA9tyQO5E= =9YqG -----END PGP SIGNATURE----- --KDt/GgjP6HVcx58l--