On Thu, Aug 24, 2017 at 08:27:01PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <[email protected]>
>
> The choice of call to discard a block is getting more complicated
> for other cases. We use fallocate PUNCH_HOLE in any file cases;
> it works for both hugepage and for tmpfs.
> We use the DONTNEED for non-hugepage cases either where they're
> anonymous or where they're private.
>
> Care should be taken when trying other backing files.
>
> Signed-off-by: Dr. David Alan Gilbert <[email protected]>
> ---
> exec.c | 35 ++++++++++++++++++++++++-----------
> trace-events | 3 +++
> 2 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index d20c34ca83..67df2909ce 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3573,6 +3573,7 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t
> start, size_t length)
> }
>
> if ((start + length) <= rb->used_length) {
> + bool need_madvise, need_fallocate;
> uint8_t *host_endaddr = host_startaddr + length;
> if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
> error_report("ram_block_discard_range: Unaligned end address:
> %p",
> @@ -3582,23 +3583,35 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t
> start, size_t length)
>
> errno = ENOTSUP; /* If we are missing MADVISE etc */
>
> - if (rb->page_size == qemu_host_page_size) {
> -#if defined(CONFIG_MADVISE)
> - /* Note: We need the madvise MADV_DONTNEED behaviour of
> definitely
> - * freeing the page.
> - */
> - ret = madvise(host_startaddr, length, MADV_DONTNEED);
> -#endif
> - } else {
> - /* Huge page case - unfortunately it can't do DONTNEED, but
> - * it can do the equivalent by FALLOC_FL_PUNCH_HOLE in the
> - * huge page file.
> + /* The logic here is messy;
> + * madvise DONTNEED fails for hugepages
> + * fallocate works on hugepages and shmem
> + */
> + need_madvise = (rb->page_size == qemu_host_page_size);
> + need_fallocate = rb->fd != -1;
> + if (need_fallocate) {
> + /* For a file, this causes the area of the file to be zero'd
> + * if read, and for hugetlbfs also causes it to be unmapped
> + * so a userfault will trigger.
> */
> #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> ret = fallocate(rb->fd, FALLOC_FL_PUNCH_HOLE |
> FALLOC_FL_KEEP_SIZE,
> start, length);
> #endif
> }
> + /* i.e. need madvise but skip it if the fallocate failed */
> + if (need_madvise && (!need_fallocate || (ret == 0))) {
I'll slightly prefer:
trace_ram_block_discard_range();
if (need_fallocate) {
ret = fallocate();
if (ret) {
error_report();
goto err;
}
}
if (need_madvise) {
ret = madvise();
if (ret) {
error_report();
goto err;
}
}
But it is personal preference. For either way:
Reviewed-by: Peter Xu <[email protected]>
> + /* For normal RAM this causes it to be unmapped,
> + * for shared memory it causes the local mapping to disappear
> + * and to fall back on the file contents (which we just
> + * fallocate'd away).
> + */
> +#if defined(CONFIG_MADVISE)
> + ret = madvise(host_startaddr, length, MADV_DONTNEED);
> +#endif
> + }
> + trace_ram_block_discard_range(rb->idstr, host_startaddr,
> + need_madvise, need_fallocate, ret);
> if (ret) {
> ret = -errno;
> error_report("ram_block_discard_range: Failed to discard range "
> diff --git a/trace-events b/trace-events
> index 1f50f56d9d..213ee34f89 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -55,6 +55,9 @@ dma_complete(void *dbs, int ret, void *cb) "dbs=%p ret=%d
> cb=%p"
> dma_blk_cb(void *dbs, int ret) "dbs=%p ret=%d"
> dma_map_wait(void *dbs) "dbs=%p"
>
> +# exec.c
> +ram_block_discard_range(const char *rbname, void *hva, bool need_madvise,
> bool need_fallocate, int ret) "%s@%p: madvise: %d fallocate: %d ret: %d"
> +
> # memory.c
> memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t
> value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size
> %u"
> memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t
> value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size
> %u"
> --
> 2.13.5
>
--
Peter Xu
