From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55040) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agpHV-0001oJ-Eo for qemu-devel@nongnu.org; Fri, 18 Mar 2016 03:57:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agpHQ-00083W-DQ for qemu-devel@nongnu.org; Fri, 18 Mar 2016 03:57:13 -0400 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> From: Thomas Huth Message-ID: <56EBB4C8.3030302@redhat.com> Date: Fri, 18 Mar 2016 08:56:56 +0100 MIME-Version: 1.0 In-Reply-To: <20160317223331.GY9032@voom> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="lkMmuG68TXKS7W5SQagscx2I6f2dwtVno" 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) --lkMmuG68TXKS7W5SQagscx2I6f2dwtVno Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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, o= r >>>> 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. Something like: /* * 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) */ ? Or is that too verbose already? Thomas --lkMmuG68TXKS7W5SQagscx2I6f2dwtVno 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) iQIcBAEBAgAGBQJW67TNAAoJEC7Z13T+cC21wcwP/ipbcNmiaH0BCPxmjV7BcWcC kYc8DzuOEpX5bR98bMZtzvNPU9vOL3CDrGb4KzVUELvaGJZGO4WAKTYehy9+OC8Y KtGV42maF2TrVd58XtNBGerGs7cXf4MAV7139M0F18+ipTDlA6xmcCM9iLh/rCb8 Sh8pqsQqFTnQ8+hFPsQl1bGntfnSw0K83Ri7XLhVfRvuWxWtf5luNf9dMBw12MfS jk4syU+z/hpiVqDAU1A0R8bZLqr7/l2VvAoFL6HMd6eo+7j5Vjn3CDzt8AL0tTqy C6XYNkVm+HOfbtwsojfi+Zx4jvYtraJUBrPSa3CCUpOpq5TSwRmGNsYkaPx/qS67 izeFREbNswR/gWAXtetLPwCm6Pgv8z7zbN6CH+BIjN9baccRgwCKaQ+E/YM2bTkw 0+o9WJddc9aN9e2a+mCJ6I16X+10ApOMriFSqW1zIERm2CxJqVragT4IwXWEde6a nL4EIAi+DTkUFFi7RMK09EoxVu54LfG/gZwEwU+C+U1+/HTGqOsxqwOiSt43iw9z n/XuqAC6ugZVbw6JXxwPlMY7xzZDlGQvk3TH0UuPjWriSKlay3Ei4Yx+FZksAmuH IhBTJN5QL7PV9je2UzK94IEk9HLepF7bkMWh7bJNiAySK7GXZAQWk7FW43cQ8jav zb7rgwPyjiaih7WIi5Xl =1gPd -----END PGP SIGNATURE----- --lkMmuG68TXKS7W5SQagscx2I6f2dwtVno--