From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51782) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agSO8-00019I-2Y for qemu-devel@nongnu.org; Thu, 17 Mar 2016 03:30:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agSO4-0007aP-PJ for qemu-devel@nongnu.org; Thu, 17 Mar 2016 03:30:31 -0400 References: <1458130611-17304-1-git-send-email-thuth@redhat.com> <1458130611-17304-3-git-send-email-thuth@redhat.com> <20160317062307.GO9032@voom> From: Thomas Huth Message-ID: <56EA5D07.3000706@redhat.com> Date: Thu, 17 Mar 2016 08:30:15 +0100 MIME-Version: 1.0 In-Reply-To: <20160317062307.GO9032@voom> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="TqsneSGlN9N5nl0FG1TcoUuDpPcFuhO3L" Subject: Re: [Qemu-devel] [PATCH 2/3] hw/net/spapr_llan: Fix receive buffer handling for better performance List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: lvivier@redhat.com, Alexey Kardashevskiy , Jason Wang , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Anton Blanchard This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --TqsneSGlN9N5nl0FG1TcoUuDpPcFuhO3L Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 17.03.2016 07:23, David Gibson wrote: > On Wed, Mar 16, 2016 at 01:16:50PM +0100, Thomas Huth wrote: >> >> This patch introduces an alternate way of handling the receive >> buffers of the spapr-vlan device, resulting in much better >> receive performance for the guest. [...] >> Though it seems at a first glance like PAPR says that we should store >> the receive buffer descriptors in the page that is supplied during >> the H_REGISTER_LOGICAL_LAN call, chapter 16.4.1.2 in the LoPAPR spec >> declares that "the contents of these descriptors are architecturally >> opaque, none of these descriptors are manipulated by code above >> the architected interfaces". >=20 > Aaaahhh! I remember back when I first implemented this, that exposing > the pool of buffer descriptors via DMA seemed a silly and pointless > thing to do, but I did so because I thought that's what the spec said. >=20 > 16.4.1.2 seems to make it clearer that the page doesn't list actual Rx > buffers, but rather opaque handles on internal buffer pools. >=20 > I don't know if I just misread this back in 2011 (or whenever it was) > or if the PAPR wording at the time was less clear at the time. >=20 > I note that you don't actually put the buffer pool pointers into that > page in your patch below. I don't think that matters now, but I > wonder if we'd ever want to implement H_MIGRATE_DMA and if we'd need > it in that case. I also thought about putting the pointers to the pools into that page. But: If we put buffer pool pointers into that page, where should the buffer pools be located? Still in the memory of the hypervisor? Then this sounds like a very baaad design, the guest then could tinker with pointers to the host memory, causing very bad side effects or crashes. Or should the buffer pools be located in guest memory? That might be OK, but how do the pools get allocated in that case? So unless you've got a good idea here, I think it's better to keep the pointer list and the buffer pools both in host memory for now. [...] >> +/** >> + * Enqueuing receive buffer by adding it to one of our receive buffer= pools >> + */ >> +static target_long spapr_vlan_add_rxbuf_to_pool(VIOsPAPRVLANDevice *d= ev, >> + target_ulong buf) >> +{ >> + int size =3D VLAN_BD_LEN(buf); >> + int pool; >> + >> + pool =3D spapr_vlan_get_rx_pool_id(dev, size); >> + >> + /* No matching pool found? Try to create a new one */ >> + if (pool < 0) { >> + for (pool =3D RX_MAX_POOLS - 1; pool >=3D 0 ; pool--) { >=20 > I don't think this loop actually accomplishes anything. Either the > last slot is free, in which case you use it, then sort into place, or > it's not, in which case you've hit the maximum number of buffer pools. Oh, you're right. Well spotted! I'll rework my patch to do it without that loop. Thomas --TqsneSGlN9N5nl0FG1TcoUuDpPcFuhO3L Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJW6l0MAAoJEC7Z13T+cC21mXAP/iqfSvEZgURXueQyaof7NdFa 2Q1EDkoEJmaF7zGOCwpu1fGSQeUSLoXXm8KpK6aFZifCh6yVsBFDycj7NLfxcojN Bo20I0gL6WKbaGg1PNur4HIfS9dfdRtj1YpHA59p+HLs5q9Al4fvdp9SA6APFEDC qrRP7MDjh8YSOa3SrHy6JrnbHK8tnx8x1dH/oB7ge0bQl9XVbrmfR035+mo/+OhE FOo4GVhuXvqauqT+HGUr3PqgUvGAag3UjElq1H4hGNd5r/vSoZdCL7aPTV9J/WpP USCyFRdoKxDx6exC5/ussKybuZax3yy601Xcs1gA0MNZ/Wq76ZTFwz82RW0r6HvE K5OHXbYrlmMTuWJU/73MTeG7XXGtHb9J+MOS+trtb/KAXYqzZdi/KvDiGiMe1yAl eiQCtiLeKrX7qKhP+XSa0lpOAT7mjYWAk9WO49B9Eg86+SHRLJ1rSYznh5e5cnQK HZftlrak5cPdVELN3AJtnSgZAbWa/eZf9nQtVT5ielMBRSyZIhU1pG4eiXX23U0j SP8Xz3eUs/v+dchEfDzcBj1/22mA386JXg8IUMACp48DxDFIBDtCsbL4sDTiZqbw 7U4ZGboICq5fa/ZLkBEuNAia08YHmG8ejXHC3qE1zN9jfJzaBslnBfS36nhYhnjl BNpNhqovU8IlsziFqnHB =FRDw -----END PGP SIGNATURE----- --TqsneSGlN9N5nl0FG1TcoUuDpPcFuhO3L--