From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55963) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuVyj-0002cE-EY for qemu-devel@nongnu.org; Thu, 26 Jul 2012 17:52:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuVyg-0005aW-0i for qemu-devel@nongnu.org; Thu, 26 Jul 2012 17:52:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27499) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuVyf-0005Uo-OE for qemu-devel@nongnu.org; Thu, 26 Jul 2012 17:52:13 -0400 Message-ID: <5011BBE8.7010901@redhat.com> Date: Thu, 26 Jul 2012 15:51:36 -0600 From: Eric Blake MIME-Version: 1.0 References: <1343227834-5400-1-git-send-email-owasserm@redhat.com> <1343227834-5400-6-git-send-email-owasserm@redhat.com> In-Reply-To: <1343227834-5400-6-git-send-email-owasserm@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig45CFD9940FEB74F529A25568" Subject: Re: [Qemu-devel] [PATCH 04/11] 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, lcapitulino@redhat.com, chegu_vinod@hp.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig45CFD9940FEB74F529A25568 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/25/2012 08:50 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) > +{ > + int64_t i; > + > + PageCache *cache =3D g_malloc(sizeof(*cache)); > + > + if (num_pages <=3D 0) { > + DPRINTF("invalid number of pages\n"); > + return NULL; Unless memory returned by g_malloc() is automatically garbage collected, then this is a memory leak. > +static unsigned long cache_get_cache_pos(const PageCache *cache, > + uint64_t address) > +{ > + unsigned long pos; On a 32-bit platform, this could be 32 bits... > + > + g_assert(cache->max_num_items); > + pos =3D (address / cache->page_size) & (cache->max_num_items - 1);= while cache->max_num_items is int64_t and could thus overflow. Then again, a 32-bit platform can't access more than 4G memory, so I think this limitation is theoretical; still, I can't help but wonder if you should be consistently using size_t instead of a mix of 'unsigned int', 'int32_t', and 'unsigned long' in referring to sizing within your cache table. > +int64_t cache_resize(PageCache *cache, int64_t new_num_pages) > +{ > + > + /* 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= */ No space before ',' in English sentences. The comment about 'keep the first value' is wrong, you are keeping the 'MRU page'... > + new_it =3D cache_get_by_addr(new_cache, old_it->it_addr); > + if (new_it->it_data) { > + /* keep the oldest page */ =2E..also wrong, you are keeping the MRU page, not the oldest page... > + if (new_it->it_age >=3D old_it->it_age) { > + g_free(old_it->it_data); since a larger it_age implies more recently used. > +++ b/qemu-common.h > @@ -1,3 +1,4 @@ > + > /* Common header file that is included by all of qemu. */ > #ifndef QEMU_COMMON_H > #define QEMU_COMMON_H > @@ -411,6 +412,18 @@ static inline uint64_t muldiv64(uint64_t a, uint32= _t b, uint32_t c) > /* Round number up to multiple */ > #define QEMU_ALIGN_UP(n, m) QEMU_ALIGN_DOWN((n) + (m) - 1, (m)) > =20 > +static inline bool is_power_of_2(int64_t value) > +{ > + if (!value) { > + return 0; > + } > + > + return !(value & (value - 1)); Technically undefined by C99 if value is INT64_MIN, since 'value - 1' then overflows. Do you want this function to take uint64_t instead, to guarantee defined results even for 0x8000000000000000? --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig45CFD9940FEB74F529A25568 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/ iQEcBAEBCAAGBQJQEbvpAAoJEKeha0olJ0NqyMQIAIgrA1XHVRND0b1PZwXi3XJV xjtkVZI9ZxVVxj2XNh5aCSyVNujG5zcUJ2Eit/VOG64tPN1TNesXqoAodyJSBJMQ w/3CK9uXWlzxYAWbTb28lpVaj1eQe2fA4OZgvBOULFm5tHoJna/P1E1QCKxRdf/T 8zqZ65Yc1cAcdA9WD4HQLN8Z2pbB+aY5Fr0wurSArNQBCnBBKs9b3HYjYY3CTfCl avddEQYR65jn3p8rBFmEcZ/AaJ/EjJ1nclyhKP9/xRdYxzRKPWBM0TMwhULjxV8q Z06IWPQbt3Su9MxS8F2fvuoudhOIynpSKFAZqAGnvldrmQ/HhIVaLkRpWKoRE7U= =3Cxm -----END PGP SIGNATURE----- --------------enig45CFD9940FEB74F529A25568--