Re: [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty()

[Kevin Wolf <kwolf@redhat.com>]

Am 29.05.2015 um 11:24 hat Alberto Garcia geschrieben:
> After having emptied the cache, the data in the cache tables is no
> longer useful, so we can tell the kernel that we are done with it. In
> Linux this frees the resources associated with it.
> 
> The effect of this can be seen in the HMP commit operation: it moves
> data from the top to the base image (and fills both caches), then it
> empties the top image. At this point the data in that cache is no
> longer needed so it's just wasting memory.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cache.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index ed92a09..ed14a92 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -22,8 +22,10 @@
>   * THE SOFTWARE.
>   */
>  
> +#include <sys/mman.h>
>  #include "block/block_int.h"
>  #include "qemu-common.h"
> +#include "qemu/osdep.h"
>  #include "qcow2.h"
>  #include "trace.h"

This breaks the mingw build:

/mnt/qemu/block/qcow2-cache.c:25:22: fatal error: sys/mman.h: No such file or directory
 #include <sys/mman.h>

> @@ -60,6 +62,22 @@ static inline int qcow2_cache_get_table_idx(BlockDriverState *bs,
>      return idx;
>  }
>  
> +static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
> +                                      int i, int num_tables)
> +{
> +#if QEMU_MADV_DONTNEED != QEMU_MADV_INVALID
> +    BDRVQcowState *s = bs->opaque;
> +    void *t = qcow2_cache_get_table_addr(bs, c, i);
> +    long align = sysconf(_SC_PAGESIZE);

It seems that getpagesize() is usually used in qemu.

> +    size_t mem_size = (size_t) s->cluster_size * num_tables;
> +    size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
> +    size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);

Instead of all the aligning here, shouldn't we just make sure that the
tables are created with the right alignment?

> +    if (length > 0) {
> +        qemu_madvise((uint8_t *) t + offset, length, QEMU_MADV_DONTNEED);
> +    }
> +#endif
> +}

Kevin