[04/15] Fold postcopy_ram_discard_range into ram_discard_range

Commit Message

Dr. David Alan Gilbert Jan. 6, 2017, 6:28 p.m. UTC

From: "Dr. David Alan Gilbert" <[e-mail]>

I'd created this weird pair of:
  ram_discard_range   that lived in migration/ram.c
     Which is built target-dependent so can access the insides of
     RAMBlock

  postcopy_ram_discard_range
     Which actually does the discard.

Flatten these down into ram_discard_range, this relies on the
CONFIG_MADVISE check to be sure we have the real DONTNEED madvise
rather than the posix variant that isn't guaranteed to discard pages.

Signed-off-by: Dr. David Alan Gilbert <[e-mail]>
---
 include/migration/postcopy-ram.h |  7 -------
 migration/postcopy-ram.c         | 30 +-----------------------------
 migration/ram.c                  | 12 +++++++++++-
 migration/trace-events           |  2 +-
 4 files changed, 13 insertions(+), 38 deletions(-)

Comments

Juan Quintela Jan. 25, 2017, 10:08 a.m. UTC | #1

"Dr. David Alan Gilbert (git)" <[e-mail]> wrote:
> From: "Dr. David Alan Gilbert" <[e-mail]>
>
> I'd created this weird pair of:
>   ram_discard_range   that lived in migration/ram.c
>      Which is built target-dependent so can access the insides of
>      RAMBlock
>
>   postcopy_ram_discard_range
>      Which actually does the discard.
>
> Flatten these down into ram_discard_range, this relies on the
> CONFIG_MADVISE check to be sure we have the real DONTNEED madvise
> rather than the posix variant that isn't guaranteed to discard pages.
>
> Signed-off-by: Dr. David Alan Gilbert <[e-mail]>

Reviewed-by: Juan Quintela <[e-mail]>

> @@ -1877,7 +1879,15 @@ int ram_discard_range(MigrationIncomingState *mis,
>                           host_endaddr);
>              goto err;
>          }
> -        ret = postcopy_ram_discard_range(mis, host_startaddr, length);
> +        errno = ENOTSUP;
> +#if defined(CONFIG_MADVISE)
> +        ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> +#endif

This really looks weird, we shouldn't arrive here if CONFIG_MADVISE is
not configured, no?

Later, Juan.

Dr. David Alan Gilbert Jan. 25, 2017, 4:43 p.m. UTC | #2

* Juan Quintela ([e-mail]) wrote:
> "Dr. David Alan Gilbert (git)" <[e-mail]> wrote:
> > From: "Dr. David Alan Gilbert" <[e-mail]>
> >
> > I'd created this weird pair of:
> >   ram_discard_range   that lived in migration/ram.c
> >      Which is built target-dependent so can access the insides of
> >      RAMBlock
> >
> >   postcopy_ram_discard_range
> >      Which actually does the discard.
> >
> > Flatten these down into ram_discard_range, this relies on the
> > CONFIG_MADVISE check to be sure we have the real DONTNEED madvise
> > rather than the posix variant that isn't guaranteed to discard pages.
> >
> > Signed-off-by: Dr. David Alan Gilbert <[e-mail]>
> 
> Reviewed-by: Juan Quintela <[e-mail]>
> 
> > @@ -1877,7 +1879,15 @@ int ram_discard_range(MigrationIncomingState *mis,
> >                           host_endaddr);
> >              goto err;
> >          }
> > -        ret = postcopy_ram_discard_range(mis, host_startaddr, length);
> > +        errno = ENOTSUP;
> > +#if defined(CONFIG_MADVISE)
> > +        ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > +#endif
> 
> This really looks weird, we shouldn't arrive here if CONFIG_MADVISE is
> not configured, no?

You'd hope so ...but.

ram.c is compiled for everyone, so it's not Linux specific for example.
The definitions of QEMU_MADV_DONTNEED and qemu_madvise is a bit odd;
if you're unlucky enough to be on a platform that is
 !CONFIG_MADVISE but is CONFIG_POSIX_MADVISE  then 
you end up using posix_madvise(POSIX_MADV_DONTNEED) - which is different
semantics than madvise MADV_DONTNEED - because the posix variant treats
it as advise for the kernel, where the madvise() case treats
it as an instruction to throw the page away (I'm not sure how
portable that semantic is).

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Patch

diff --git a/include/migration/postcopy-ram.h b/include/migration/postcopy-ram.h
index b6a7491f..43bbbca 100644
--- a/include/migration/postcopy-ram.h
+++ b/include/migration/postcopy-ram.h
@@ -35,13 +35,6 @@  int postcopy_ram_incoming_init(MigrationIncomingState *mis, size_t ram_pages);
 int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis);
 
 /*
- * Discard the contents of 'length' bytes from 'start'
- * We can assume that if we've been called postcopy_ram_hosttest returned true
- */
-int postcopy_ram_discard_range(MigrationIncomingState *mis, uint8_t *start,
-                               size_t length);
-
-/*
  * Userfault requires us to mark RAM as NOHUGEPAGE prior to discard
  * however leaving it until after precopy means that most of the precopy
  * data is still THPd
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index a40dddb..1e3d22f 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -200,27 +200,6 @@  out:
     return ret;
 }
 
-/**
- * postcopy_ram_discard_range: Discard a range of memory.
- * We can assume that if we've been called postcopy_ram_hosttest returned true.
- *
- * @mis: Current incoming migration state.
- * @start, @length: range of memory to discard.
- *
- * returns: 0 on success.
- */
-int postcopy_ram_discard_range(MigrationIncomingState *mis, uint8_t *start,
-                               size_t length)
-{
-    trace_postcopy_ram_discard_range(start, length);
-    if (madvise(start, length, MADV_DONTNEED)) {
-        error_report("%s MADV_DONTNEED: %s", __func__, strerror(errno));
-        return -1;
-    }
-
-    return 0;
-}
-
 /*
  * Setup an area of RAM so that it *can* be used for postcopy later; this
  * must be done right at the start prior to pre-copy.
@@ -239,7 +218,7 @@  static int init_range(const char *block_name, void *host_addr,
      * - we're going to get the copy from the source anyway.
      * (Precopy will just overwrite this data, so doesn't need the discard)
      */
-    if (postcopy_ram_discard_range(mis, host_addr, length)) {
+    if (ram_discard_range(mis, block_name, 0, length)) {
         return -1;
     }
 
@@ -658,13 +637,6 @@  int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
     return -1;
 }
 
-int postcopy_ram_discard_range(MigrationIncomingState *mis, uint8_t *start,
-                               size_t length)
-{
-    assert(0);
-    return -1;
-}
-
 int postcopy_ram_prepare_discard(MigrationIncomingState *mis)
 {
     assert(0);
diff --git a/migration/ram.c b/migration/ram.c
index c9d8290..fe32836 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1853,6 +1853,8 @@  int ram_discard_range(MigrationIncomingState *mis,
 {
     int ret = -1;
 
+    trace_ram_discard_range(block_name, start, length);
+
     rcu_read_lock();
     RAMBlock *rb = qemu_ram_block_by_name(block_name);
 
@@ -1877,7 +1879,15 @@  int ram_discard_range(MigrationIncomingState *mis,
                          host_endaddr);
             goto err;
         }
-        ret = postcopy_ram_discard_range(mis, host_startaddr, length);
+        errno = ENOTSUP;
+#if defined(CONFIG_MADVISE)
+        ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
+#endif
+        if (ret) {
+            error_report("ram_discard_range: Failed to discard  range "
+                         "%s:%" PRIx64 " +%zx (%d)",
+                         block_name, start, length, errno);
+        }
     } else {
         error_report("ram_discard_range: Overrun block '%s' (%" PRIu64
                      "/%zx/" RAM_ADDR_FMT")",
diff --git a/migration/trace-events b/migration/trace-events
index 94134f7..6dded4f 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -62,6 +62,7 @@  get_queued_page_not_dirty(const char *block_name, uint64_t tmp_offset, uint64_t
 migration_bitmap_sync_start(void) ""
 migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64
 migration_throttle(void) ""
+ram_discard_range(const char *rbname, uint64_t start, size_t len) "%s: start: %" PRIx64 " %zx"
 ram_load_postcopy_loop(uint64_t addr, int flags) "@%" PRIx64 " %x"
 ram_postcopy_send_discard_bitmap(void) ""
 ram_save_queue_pages(const char *rbname, size_t start, size_t len) "%s: start: %zx len: %zx"
@@ -171,7 +172,6 @@  rdma_start_outgoing_migration_after_rdma_source_init(void) ""
 # migration/postcopy-ram.c
 postcopy_discard_send_finish(const char *ramblock, int nwords, int ncmds) "%s mask words sent=%d in %d commands"
 postcopy_discard_send_range(const char *ramblock, unsigned long start, unsigned long length) "%s:%lx/%lx"
-postcopy_ram_discard_range(void *start, size_t length) "%p,+%zx"
 postcopy_cleanup_range(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=%zx length=%zx"
 postcopy_init_range(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=%zx length=%zx"
 postcopy_nhp_range(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=%zx length=%zx"