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

[Kevin Wolf]

Am 02.06.2015 um 12:50 hat Alberto Garcia geschrieben:
> On Tue 02 Jun 2015 12:05:59 PM CEST, Kevin Wolf <address@hidden> wrote:
> 
> >> +#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>
> 
> Ok, I can use #if defined(CONFIG_MADVISE) || defined(CONFIG_POSIX_MADVISE)
> as it's done in util/osdep.c for the same header.
> 
> >> +#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.
> 
> The getpagesize() manual page actually recommends using sysconf() for
> portability reasons. But other than that I don't have a problem with
> getpagesize() if it's the preferred choice in QEMU.

There is a Windows implementation for getpagesize() in qemu, so I guess
it's actually more portable in our context.

Now, as you'll #ifdef this code out for Windows, that probably doesn't
matter, so it's just about consistency.

> >> +    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?
> 
> The tables should have the right alignment and their size should be a
> multiple of the page size. The latter condition is not necessarily true
> (a cluster can be smaller than one page) so I'd keep that code in any
> case.

We could adjust the size to MAX(size, pagesize), but perhaps that wastes
a bit too much memory indeed.

Kevin