From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:50538) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gwH9A-00069Q-Uk for qemu-devel@nongnu.org; Tue, 19 Feb 2019 20:58:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gwH93-00022V-Qs for qemu-devel@nongnu.org; Tue, 19 Feb 2019 20:58:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48338) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gwH90-0001vD-8D for qemu-devel@nongnu.org; Tue, 19 Feb 2019 20:57:55 -0500 Date: Wed, 20 Feb 2019 09:54:41 +0800 From: Wei Xu Message-ID: <20190220015441.GB23868@wei-ubt> References: <1550118402-4057-1-git-send-email-wexu@redhat.com> <1550118402-4057-10-git-send-email-wexu@redhat.com> <1c16f234-af83-23c8-2194-94be63574299@redhat.com> <20190219105155.GD15343@wei-ubt> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 09/11] virtio-net: update the head descriptor in a chain lastly List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: jfreiman@redhat.com, maxime.coquelin@redhat.com, qemu-devel@nongnu.org, tiwei.bie@intel.com, mst@redhat.com On Tue, Feb 19, 2019 at 09:09:33PM +0800, Jason Wang wrote: >=20 > On 2019/2/19 =E4=B8=8B=E5=8D=886:51, Wei Xu wrote: > >On Tue, Feb 19, 2019 at 03:23:01PM +0800, Jason Wang wrote: > >>On 2019/2/14 =E4=B8=8B=E5=8D=8812:26, wexu@redhat.com wrote: > >>>From: Wei Xu > >>> > >>>This is a helper for packed ring. > >>> > >>>To support packed ring, the head descriptor in a chain should be upd= ated > >>>lastly since no 'avail_idx' like in packed ring to explicitly tell t= he > >>>driver side that all payload is ready after having done the chain, s= o > >>>the head is always visible immediately. > >>> > >>>This patch fills the header after done all the other ones. > >>> > >>>Signed-off-by: Wei Xu > >> > >>It's really odd to workaround API issue in the implementation of devi= ce. > >>Please introduce batched used updating helpers instead. > >Can you elaborate a bit more? I don't get it as well. > > > >The exact batch as vhost-net or dpdk pmd is not supported by userspace > >backend. The change here is to keep the header descriptor updated at > >last in case of a chaining descriptors and the helper might not help > >too much. > > > >Wei >=20 >=20 > Of course we can add batching support why not? It is always good to improve performance with anything, while probably this could be done in another separate batch, also we need to bear in mind that usually qemu userspace backend is not the first option for performance oriented user. AFAICT, virtqueue_fill() is a generic API for all relevant userspace virt= io devices that do not support batching , without touching virtqueue_fill(), supporting batching changes the meaning of the parameter 'idx' which shou= ld=20 be kept overall. To fix it, I got two proposals so far: 1). batching support(two APIs needed to keep compatibility) 2). save a head elem for a vq instead of caching an array of elems like v= host, and introduce a new API(virtqueue_chain_fill()) functioning with an additional parameter 'more' to the current virtqueue_fill() to indica= te if there are more descriptor(s) coming in a chain. Either way it changes the API somehow and it does not seem to be clean an= d clear as wanted. Any better idea? >=20 > Your code assumes the device know the virtio layout specific assumption > whih breaks the layer. Device should not care about the actual layout. > Good point, but anyway, change to virtio-net receiving code path is unavoidable to support split and packed rings, and batching is like a new feature somehow. Wei =20 > Thanks >=20 >=20 > >>Thanks > >> > >> > >>>--- > >>> hw/net/virtio-net.c | 11 ++++++++++- > >>> 1 file changed, 10 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>index 3f319ef..330abea 100644 > >>>--- a/hw/net/virtio-net.c > >>>+++ b/hw/net/virtio-net.c > >>>@@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClien= tState *nc, const uint8_t *buf, > >>> struct virtio_net_hdr_mrg_rxbuf mhdr; > >>> unsigned mhdr_cnt =3D 0; > >>> size_t offset, i, guest_offset; > >>>+ VirtQueueElement head; > >>>+ int head_len =3D 0; > >>> if (!virtio_net_can_receive(nc)) { > >>> return -1; > >>>@@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClie= ntState *nc, const uint8_t *buf, > >>> } > >>> /* signal other side */ > >>>- virtqueue_fill(q->rx_vq, elem, total, i++); > >>>+ if (i =3D=3D 0) { > >>>+ head_len =3D total; > >>>+ head =3D *elem; > >>>+ } else { > >>>+ virtqueue_fill(q->rx_vq, elem, len, i); > >>>+ } > >>>+ i++; > >>> g_free(elem); > >>> } > >>>@@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClien= tState *nc, const uint8_t *buf, > >>> &mhdr.num_buffers, sizeof mhdr.num_buffers); > >>> } > >>>+ virtqueue_fill(q->rx_vq, &head, head_len, 0); > >>> virtqueue_flush(q->rx_vq, i); > >>> virtio_notify(vdev, q->rx_vq); >=20