From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNTfY-0000Dh-98 for qemu-devel@nongnu.org; Tue, 11 Mar 2014 16:53:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNTfT-0005ee-UW for qemu-devel@nongnu.org; Tue, 11 Mar 2014 16:53:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47328) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNTfT-0005eN-N6 for qemu-devel@nongnu.org; Tue, 11 Mar 2014 16:52:55 -0400 Message-ID: <531F77A2.80502@redhat.com> Date: Tue, 11 Mar 2014 14:52:50 -0600 From: Eric Blake MIME-Version: 1.0 References: <1394542415-5152-1-git-send-email-arei.gonglei@huawei.com> <1394542415-5152-4-git-send-email-arei.gonglei@huawei.com> In-Reply-To: <1394542415-5152-4-git-send-email-arei.gonglei@huawei.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="pONCrHekdx9UjjjtswvBteqEv0cH3jLMs" Subject: Re: [Qemu-devel] [PATCH 03/10] XBZRLE: optimize XBZRLE to decrease the cache missing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: arei.gonglei@huawei.com, qemu-devel@nongnu.org Cc: ChenLiang , owasserm@redhat.com, pbonzini@redhat.com, weidong.huang@huawei.com, quintela@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --pONCrHekdx9UjjjtswvBteqEv0cH3jLMs Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/11/2014 06:53 AM, arei.gonglei@huawei.com wrote: > From: ChenLiang >=20 > Avoid the hot pages being replaced by others to remarkable decrease cac= he s/the hot/hot/ s/remarkable/remarkably/ > missing. The counter of updating dirty bitmap is used to indicate the c= ached s/missing/misses/ > page age. Please give more rationale - when claiming that something gave a speed improvement, it helps to back up that claim with the sample program you ran in the guest along with before and after numbers to back up your point. A "remarkable decrease" is subjective, but pre- and post-patch numbers is objective. This may mean rebasing your series to re-order patches, and FIRST export the counters that you then use to justify this patch (the current order of applying the optimization, and only THEN exposing the numbers through QMP, makes it harder to prove that this patch helped). >=20 > Signed-off-by: ChenLiang > Signed-off-by: Gonglei > --- > arch_init.c | 8 +++++--- > include/migration/page_cache.h | 10 +++++++--- > page_cache.c | 23 +++++++++++++++++++---- > 3 files changed, 31 insertions(+), 10 deletions(-) >=20 > acct_info.xbzrle_cache_miss++; > if (!last_stage) { > - if (cache_insert(XBZRLE.cache, current_addr, *current_data= ) =3D=3D -1) { > + if (cache_insert(XBZRLE.cache, current_addr, *current_data= , > + get_bitmap_sync_cnt()) =3D=3D -1) { Indentation - I'd write: if (cache_insert(XBZRLE.cache, current_addr, *current_data, get_bitmap_sync_cnt()) =3D=3D -1) { This patch needs to be rebased once you fix 2/10 to just directly use the variable instead of wrapping it inside pointless static inline accessor functions. > @@ -161,6 +171,11 @@ int cache_insert(PageCache *cache, uint64_t addr, = const uint8_t *pdata) > /* actual update of entry */ > it =3D cache_get_by_addr(cache, addr); > =20 > + if ((it->it_data !=3D NULL) && (it->it_age + > + CACHED_PAGE_LIFETIME > current_age)) { Unnecessary (), and odd choice of line break location coupled with poor indentation. It fits fine on one 80-column line (hmm, my email may wrap because I write email in fewer columns than 80): if (it->it_data && it->it_age + CACHED_PAGE_LIFETIME > current_age) {= and if you MUST break lines, breaking on the lower-priority operators makes it easier to read the grouping: if (it->it_data && it->it_age + CACHED_PAGE_LIFETIME > current_age) { --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --pONCrHekdx9UjjjtswvBteqEv0cH3jLMs Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTH3eiAAoJEKeha0olJ0Nq4hQH/jFGkDrS0GicykpsZVg+OJ7m hypXFkdMiVPT1TlcvsRh4woCzD7X9bdcMyiLBZGsR+0abLl2sqGynaBVF9MU07+X txwGNnohmrJihZ3FE82PS/B4WIyGy51E/iZG5fXxg1xw4pEIz4VUYeSXexnky1rl qrddqtMh2ZNwY0inxNlAuTe8bWLeH7NY393KNGshd2RrNihnsWtlQiZisXc5SGRg NcswtVnmbGZZIVIXFNh44yMRx5B6D+680qiy2c1cIe7tezDNPiF2eO6NyZcYVe8i bw7oxqMxmkeTa6BJfgsA+Lj/VU/+U89VDE7MKVV3w6xijtiT8olcmm7SxcPpBL8= =kNLc -----END PGP SIGNATURE----- --pONCrHekdx9UjjjtswvBteqEv0cH3jLMs--