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 <benoit.hud...@sap.com> >> Signed-off-by: Petter Svard <pett...@cs.umu.se> >> Signed-off-by: Aidan Shribman <aidan.shrib...@sap.com> >> Signed-off-by: Orit Wasserman <owass...@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. > 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