Re: [Qemu-devel] [PATCH 04/11] Add cache handling functions

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3131 bytes --]

On 07/25/2012 08:50 AM, Orit Wasserman wrote:
> Add LRU page cache mechanism.
> The page are accessed by their address.
> 
> Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard <petters@cs.umu.se>
> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>

> +
> +PageCache *cache_init(int64_t num_pages, unsigned int page_size)
> +{
> +    int64_t i;
> +
> +    PageCache *cache = g_malloc(sizeof(*cache));
> +
> +    if (num_pages <= 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 = (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 = 0; i < cache->max_num_items; i++) {
> +        old_it = &cache->page_cache[i];
> +        if (old_it->it_addr != -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 = cache_get_by_addr(new_cache, old_it->it_addr);
> +            if (new_it->it_data) {
> +                /* keep the oldest page */

...also wrong, you are keeping the MRU page, not the oldest page...

> +                if (new_it->it_age >= 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))
>  
> +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?

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]