From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmAX8-0005NW-D6 for qemu-devel@nongnu.org; Tue, 03 Jul 2012 17:21:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SmAX5-00079y-SH for qemu-devel@nongnu.org; Tue, 03 Jul 2012 17:21:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11698) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmAX5-00079W-Kz for qemu-devel@nongnu.org; Tue, 03 Jul 2012 17:21:15 -0400 Message-ID: <4FF35511.9010500@redhat.com> Date: Tue, 03 Jul 2012 14:24:49 -0600 From: Eric Blake MIME-Version: 1.0 References: <1341323574-23206-1-git-send-email-owasserm@redhat.com> <1341323574-23206-5-git-send-email-owasserm@redhat.com> In-Reply-To: <1341323574-23206-5-git-send-email-owasserm@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig533D4F67291FDBF6576ECA0A" Subject: Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Orit Wasserman Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, quintela@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, blauwirbel@gmail.com, Petter Svard , Benoit Hudzia , avi@redhat.com, Aidan Shribman , pbonzini@redhat.com, chegu_vinod@hp.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig533D4F67291FDBF6576ECA0A Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/03/2012 07:52 AM, Orit Wasserman wrote: > Add LRU page cache mechanism. > The page are accessed by their address. >=20 > Signed-off-by: Benoit Hudzia > Signed-off-by: Petter Svard > Signed-off-by: Aidan Shribman > Signed-off-by: Orit Wasserman > +PageCache *cache_init(int64_t num_pages, unsigned int page_size) > +{ > + int i; Type mismatch. Either 'i' needs to be int64_t, or you don't need num_pages to be 64-bit. Although I think it will be unlikely to encounter someone with a desire to use 2**32 pages of 4096 bytes each as their cache, I can't rule it out, so I think 'i' needs to be bigger rather than changing your API to be smaller. > + > + PageCache *cache =3D g_malloc(sizeof(PageCache)); Personally, I like the style g_malloc(sizeof(*cache)), as it is immune to type changes on cache. If you later change the type of 'cache', your style has to make two edits to this line, while my style only makes one edit. > + > + if (num_pages <=3D 0) { > + DPRINTF("invalid number pages\n"); s/number pages/number of pages/ > + cache->page_cache =3D g_malloc((cache->max_num_items) * > + sizeof(CacheItem)); Here my style is even more useful. With your style, I have to search elsewhere in the code to validate that cache->page_cache really does have the type CacheItem; but my style means I can see the result at once without having to care about typing: cache->page_cache =3D g_malloc(cache->max_num_items * sizeof(*cache->page_cache)); > +void cache_fini(PageCache *cache) > +{ > + int i; Type mismatch again; max_num_items can be larger than INT_MAX, so you need a bigger type for 'i'. > + > + g_assert(cache); > + g_assert(cache->page_cache); > + > + for (i =3D 0; i < cache->max_num_items; i++) { > + g_free(cache->page_cache[i].it_data); > + cache->page_cache[i].it_data =3D 0; Wasted assignment, since you are just about to free cache->page_cache anyways. > +uint8_t *get_cached_data(const PageCache *cache, uint64_t addr) > +{ > + return cache_get_by_addr(cache, addr)->it_data; > +} > + > +void cache_insert(PageCache *cache, unsigned long addr, uint8_t *pdata= ) Why are you using 'uint64_t addr' in some places, and 'unsigned long addr' in others? > + /* move all data from old cache */ > + for (i =3D 0; i < cache->max_num_items; i++) { > + old_it =3D &cache->page_cache[i]; > + if (old_it->it_addr !=3D -1) { > + /* check for collision , if there is, keep the first value= */ s/ ,/,/ (no space before comma in English) Shouldn't we instead prefer to keep the page with larger age, regardless of whether we found it first or second? That is, a cache is more likely to be useful on most-recently-used pages, rather than on which address happened to map to the collision first. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig533D4F67291FDBF6576ECA0A 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.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBCAAGBQJP81USAAoJEKeha0olJ0Nq0VMH/RZCChwF+KlPJ319uZFyIZNL s5sf5s1Zb+aSRWN6tagKjrko44BNI6Py1Wt5R2i/w/kxbElf5EoW7wIKbs5es8UH C16nT1h9sQeBmlpEKVHAQRyAGrSZ2L4Hb7rlXR1EnO8bkOkHY0+S0tQxpmVX3AHS l6/4DRbjZ576uctphEz4uJk/DNp8KpNbUkzfWtWruwbi9GKa8wZSsBh05IbUvPlJ pr2F5zaCOXhvYWHqzWAam8zeZLLqf+sfRoxAMAglWV5jnYZ8vlTD3s5mfNVlc4jn 2G/dKdbJDnOCahl7Pt2WIAcm3Lhfw8pxHAItZ9mUZNuC/t9DWj8iVNFIFBPo2r8= =juX6 -----END PGP SIGNATURE----- --------------enig533D4F67291FDBF6576ECA0A--