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

[Orit Wasserman]

On 07/27/2012 12:51 AM, Eric Blake wrote:
> 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 <address@hidden>
>> Signed-off-by: Petter Svard <address@hidden>
>> Signed-off-by: Aidan Shribman <address@hidden>
>> Signed-off-by: Orit Wasserman <address@hidden>
> 
>> +
>> +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.
> 
good catch I will move the check up.
>> +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...
I will switch it to  uint64_t.
> 
>> +
>> +    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.
> 
same here
>> +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'...
> 
I will fix it
>> +            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...
here too
> 
>> +                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?
> 
good idea.

Thanks,
Orit