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.
> +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 ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
