[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions |
Date: |
Tue, 03 Jul 2012 14:24:49 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1 |
On 07/03/2012 07:52 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)
> +{
> + int i;
Type mismatch. Either 'i' needs to be int64_t, or you don't need
num_pages to be 64-bit. Although I think it will be unlikely to
encounter someone with a desire to use 2**32 pages of 4096 bytes each as
their cache, I can't rule it out, so I think 'i' needs to be bigger
rather than changing your API to be smaller.
> +
> + PageCache *cache = g_malloc(sizeof(PageCache));
Personally, I like the style g_malloc(sizeof(*cache)), as it is immune
to type changes on cache. If you later change the type of 'cache', your
style has to make two edits to this line, while my style only makes one
edit.
> +
> + if (num_pages <= 0) {
> + DPRINTF("invalid number pages\n");
s/number pages/number of pages/
> + cache->page_cache = g_malloc((cache->max_num_items) *
> + sizeof(CacheItem));
Here my style is even more useful. With your style, I have to search
elsewhere in the code to validate that cache->page_cache really does
have the type CacheItem; but my style means I can see the result at once
without having to care about typing:
cache->page_cache = g_malloc(cache->max_num_items *
sizeof(*cache->page_cache));
> +void cache_fini(PageCache *cache)
> +{
> + int i;
Type mismatch again; max_num_items can be larger than INT_MAX, so you
need a bigger type for 'i'.
> +
> + g_assert(cache);
> + g_assert(cache->page_cache);
> +
> + for (i = 0; i < cache->max_num_items; i++) {
> + g_free(cache->page_cache[i].it_data);
> + cache->page_cache[i].it_data = 0;
Wasted assignment, since you are just about to free cache->page_cache
anyways.
> +uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
> +{
> + return cache_get_by_addr(cache, addr)->it_data;
> +}
> +
> +void cache_insert(PageCache *cache, unsigned long addr, uint8_t *pdata)
Why are you using 'uint64_t addr' in some places, and 'unsigned long
addr' in others?
> + /* 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 */
s/ ,/,/ (no space before comma in English)
Shouldn't we instead prefer to keep the page with larger age, regardless
of whether we found it first or second? That is, a cache is more likely
to be useful on most-recently-used pages, rather than on which address
happened to map to the collision first.
--
Eric Blake address@hidden +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure, (continued)
- [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure, Orit Wasserman, 2012/07/03
- [Qemu-devel] [PATCH v14 06/13] Add save_block_hdr function, Orit Wasserman, 2012/07/03
- [Qemu-devel] [PATCH v14 01/13] Add MigrationParams structure, Orit Wasserman, 2012/07/03
- [Qemu-devel] [PATCH v14 08/13] Change ram_save_block to return -1 if there are no more changes, Orit Wasserman, 2012/07/03
- [Qemu-devel] [PATCH v14 04/13] Add cache handling functions, Orit Wasserman, 2012/07/03
- Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions,
Eric Blake <=
- [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions, Orit Wasserman, 2012/07/03
- Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions, Orit Wasserman, 2012/07/04
- Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions, Eric Blake, 2012/07/04
[Qemu-devel] [PATCH v14 05/13] Add uleb encoding/decoding functions, Orit Wasserman, 2012/07/03
[Qemu-devel] [PATCH v14 11/13] Add XBZRLE to ram_save_block and ram_save_live, Orit Wasserman, 2012/07/03
[Qemu-devel] [PATCH v14 02/13] Add migration capabilities, Orit Wasserman, 2012/07/03