From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ahVZV-0000Sg-3k for qemu-devel@nongnu.org; Sun, 20 Mar 2016 01:06:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ahVZR-0000AN-Ta for qemu-devel@nongnu.org; Sun, 20 Mar 2016 01:06:37 -0400 Date: Sun, 20 Mar 2016 15:21:24 +1100 From: David Gibson Message-ID: <20160320042124.GZ9032@voom> References: <1458130611-17304-1-git-send-email-thuth@redhat.com> <1458130611-17304-3-git-send-email-thuth@redhat.com> <20160317062307.GO9032@voom> <56EA5D07.3000706@redhat.com> <56EACA1A.9090104@redhat.com> <20160317223331.GY9032@voom> <56EBB4C8.3030302@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="urKaCNvFwQ8jDQOg" Content-Disposition: inline In-Reply-To: <56EBB4C8.3030302@redhat.com> 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: Thomas Huth Cc: lvivier@redhat.com, Alexey Kardashevskiy , Jason Wang , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Anton Blanchard --urKaCNvFwQ8jDQOg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 18, 2016 at 08:56:56AM +0100, Thomas Huth wrote: > On 17.03.2016 23:33, David Gibson wrote: > > On Thu, Mar 17, 2016 at 04:15:38PM +0100, Thomas Huth wrote: > >> On 17.03.2016 08:30, Thomas Huth wrote: > >>> 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. > >> [...] > >>>>> +/** > >>>>> + * Enqueuing receive buffer by adding it to one of our receive buf= fer pools > >>>>> + */ > >>>>> +static target_long spapr_vlan_add_rxbuf_to_pool(VIOsPAPRVLANDevice= *dev, > >>>>> + 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--) { > >>>> > >>>> 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 pool= s. > >>> > >>> Oh, you're right. Well spotted! I'll rework my patch to do it without > >>> that loop. > >> > >> Wait, no, there was a case where this loop is actually really required: > >> > >> 1) All pools are in use and filled with at least one BD > >> 2) User in the guest suddenly decides to change the buffer size of > >> one of the pools in the /sys fs of the guest. > >> 3) Guest driver tries to add buffers with a new size that do not > >> match any size of one of the pools in the host > >> 4) After the pool on the host runs empty which contained the BDs with > >> the size that is not in use anymore, we should recycle that pool > >> for the buffers with the new size instead. Since that buffer pool > >> might not be at the end of the list, we've got to scan all buffers > >> here to make sure we find it. > >> > >> So I think the for-loop should stay as it is. > >=20 > > Ah, good point. I think I was assuming that the pools got sorted when > > one was emptied as well, but they're not and I suspect it's not a good > > idea to do so. > >=20 > > Hmm.. I wonder if there's a brief way of explaining the above to put > > in the comment. >=20 > Something like: >=20 > /* > * If the guest used all pools, but changed the size of one pool > * inbetween, we might need to recycle that pool here (if it has > * already been emptied). Thus we need to scan all buffer pools > * here, not only the last one (which has the highest probability > * of being empty) > */ >=20 > ? >=20 > Or is that too verbose already? Eh, it's written might as well throw it in. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --urKaCNvFwQ8jDQOg Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW7iVEAAoJEGw4ysog2bOSwNoQAKvmMfrS8UyM0rT4ZLRcXAT5 LuBBzIvT0xS2jWYJjVcVPqCIgBpfBS2PFXRrTMGxDUNjBUmVAMg81jeTm5Tfkmfk GC3+jWzGSoT0Ql+KUBI/epJIb4WHsz/f6VwrMTzXkYtvq0sUfFbV92NsxfSL20PF admTyPdfc9oc1sVkDw4ho4Kw42J/PgRh/1cRwsM5dFXVhCqBK2SSxcXhq+BfuzrG Q9hwh1VXO1pAJNtYRNxam1W7J8243zdHSr5lg3vPtOgTXwCtOA4GC0AY0usc/vAZ DlTQh0K6zQAUZ6g1La9OoUPb70qJEmSS8ofLsi4qZD2QVLxgXnuDx6caE9CIsJnx 2GslgNYcamw5VDRROI9McR3CSkLNNqXTGXYmemOQIBpVBpUwuXIMDFvYquUq/wBK C2keHgyXxwf5gfNROpNRId9JvLoBI5BpkdDNbSXgE0OKVYPEFAEgKh0oQLVxABdN M8zmt0X+QagVHNPbmEIHE85aISNSPAS7apZ8V7Pv+RkLp8u36wWV8+rWHHXKV7U2 T5qwlBn/Lf4qGgjj0Il1gF8pO09gY3f1iRqILrkQ6kb2w3QCu2jihS+lAsG+i/aS qGf5bQBW3DQrogjHoJJk8sq0E+2znj8YMJvLExfLe/xuH3tDQM0455tKO2YOXwr5 Sh4FRAalMyH23z+kkEKN =L7V6 -----END PGP SIGNATURE----- --urKaCNvFwQ8jDQOg--