[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty() |
Date: |
Tue, 2 Jun 2015 12:56:10 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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