From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:32789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gwHiy-00057H-10 for qemu-devel@nongnu.org; Tue, 19 Feb 2019 21:35:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gwHiv-0002jo-Mb for qemu-devel@nongnu.org; Tue, 19 Feb 2019 21:35:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45928) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gwHiv-0002It-9b for qemu-devel@nongnu.org; Tue, 19 Feb 2019 21:35:01 -0500 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> <20190220015441.GB23868@wei-ubt> From: Jason Wang Message-ID: <9a9751aa-0cfc-151a-890c-ae70a9a37d64@redhat.com> Date: Wed, 20 Feb 2019 10:34:32 +0800 MIME-Version: 1.0 In-Reply-To: <20190220015441.GB23868@wei-ubt> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US 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: Wei Xu Cc: jfreiman@redhat.com, maxime.coquelin@redhat.com, qemu-devel@nongnu.org, tiwei.bie@intel.com, mst@redhat.com On 2019/2/20 =E4=B8=8A=E5=8D=889:54, Wei Xu wrote: > On Tue, Feb 19, 2019 at 09:09:33PM +0800, Jason Wang wrote: >> 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 up= dated >>>>> lastly since no 'avail_idx' like in packed ring to explicitly tell = the >>>>> driver side that all payload is ready after having done the chain, = so >>>>> 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 dev= ice. >>>> 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 userspac= e >>> 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 >> >> 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. The point is to hide layout specific things from device emulation. If it=20 helps for performance, it could be treated as a good byproduct. > > AFAICT, virtqueue_fill() is a generic API for all relevant userspace vi= rtio > devices that do not support batching , without touching virtqueue_fill(= ), > supporting batching changes the meaning of the parameter 'idx' which sh= ould > 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= vhost, > and introduce a new API(virtqueue_chain_fill()) functioning with a= n > additional parameter 'more' to the current virtqueue_fill() to ind= icate 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 = and clear > as wanted. It's as simple as accepting an array of elems in e.g=20 virtqueue_fill_batched()? > > Any better idea? > >> Your code assumes the device know the virtio layout specific assumptio= n >> 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 n= ew > feature somehow. It's ok to change the code as a result of introducing of generic helper=20 but it's bad to change to code for working around a bad API. Thanks > > Wei > =20 >> Thanks >> >> >>>> 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(NetClie= ntState *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(NetCli= entState *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(NetClie= ntState *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);