All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v8 0/6] virtio-balloon: free page hint reporting support
@ 2018-06-08  8:10 ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

This is the deivce part implementation to add a new feature,
VIRTIO_BALLOON_F_FREE_PAGE_HINT to the virtio-balloon device. The device
receives the guest free page hints from the driver and clears the
corresponding bits in the dirty bitmap, so that those free pages are
not transferred by the migration thread to the destination.

- Test Environment
    Host: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz
    Guest: 8G RAM, 4 vCPU
    Migration setup: migrate_set_speed 100G, migrate_set_downtime 2 second

- Test Results
    - Idle Guest Live Migration Time (results are averaged over 10 runs):
        - Optimization v.s. Legacy = 271ms vs 1769ms --> ~86% reduction
    - Guest with Linux Compilation Workload (make bzImage -j4):
        - Live Migration Time (average)
          Optimization v.s. Legacy = 1265ms v.s. 2634ms --> ~51% reduction
        - Linux Compilation Time
          Optimization v.s. Legacy = 4min56s v.s. 5min3s
          --> no obvious difference

- Source Code
    - QEMU:  https://github.com/wei-w-wang/qemu-free-page-lm.git
    - Linux: https://github.com/wei-w-wang/linux-free-page-lm.git

ChangeLog:
v7->v8:
    1) migration:
        - qemu_guest_free_page_hint: 
            - check if this function is called during migration;
            - assert when the block of a given hint is not found;
        - add a ram save state notifier list;
        - move migrate_postcopy to an outside header for other
          subsystems (e.g. notifier callbacks) to use;
     2) virtio-balloon:
        - start/stop the optimization via a notifier, and reduce the
          related global balloon interfaces;
        - virtio_balloon_poll_free_page_hints: move the while loop into
          a function;
        - put page poison value into a separate vmsd subsection.
v6->v7:
      virtio-balloon/virtio_balloo_poll_free_page_hints:
          - add virtio_notify() at the end to notify the driver that
            the optimization is done, which indicates that the entries
            have all been put back to the vq and ready to detach them.
v5->v6:
      virtio-balloon: use iothread to get free page hint
v4->v5:
    1) migration:
        - bitmap_clear_dirty: update the dirty bitmap and dirty page
          count under the bitmap mutex as what other functions are doing;
        - qemu_guest_free_page_hint:
            - add comments for this function;
            - check the !block case;
            - check "offset > block->used_length" before proceed;
            - assign used_len inside the for{} body;
            - update the dirty bitmap and dirty page counter under the
              bitmap mutex;
        - ram_state_reset:
            - rs->free_page_support: && with use "migrate_postcopy"
              instead of migration_in_postcopy;
            - clear the ram_bulk_stage flag if free_page_support is true;
    2) balloon:
         - add the usage documentation of balloon_free_page_start and
           balloon_free_page_stop in code;
         - the optimization thread is named "balloon_fpo" to meet the
           requirement of "less than 14 characters";
         - virtio_balloon_poll_free_page_hints:
             - run on condition when runstate_is_running() is true;
             - add a qemu spin lock to synchronize accesses to the free
               page reporting related fields shared among the migration
               thread and the optimization thread;
          - virtio_balloon_free_page_start: just return if
            runstate_is_running is false;
          - virtio_balloon_free_page_stop: access to the free page
            reporting related fields under a qemu spin lock;
          - virtio_balloon_device_unrealize/reset: call
            virtio_balloon_free_page_stop is the free page hint feature is
            used;
          - virtio_balloon_set_status: call irtio_balloon_free_page_stop
            in case the guest is stopped by qmp when the optimization is
            running;
v3->v4:
    1) bitmap: add a new API to count 1s starting from an offset of a
       bitmap
    2) migration:
        - qemu_guest_free_page_hint: calculate
          ram_state->migration_dirty_pages by counting how many bits of
          free pages are truely cleared. If some of the bits were
          already 0, they shouldn't be deducted by
          ram_state->migration_dirty_pages. This wasn't needed for
          previous versions since we optimized bulk stage only,
          where all bits are guaranteed to be set. It's needed now
          because we extened the usage of this optimizaton to all stages
          except the last stop&copy stage. From 2nd stage onward, there
          are possibilities that some bits of free pages are already 0.
     3) virtio-balloon:
         - virtio_balloon_free_page_report_status: introduce a new status,
           FREE_PAGE_REPORT_S_EXIT. This status indicates that the
           optimization thread has exited. FREE_PAGE_REPORT_S_STOP means
           the reporting is stopped, but the optimization thread still needs
           to be joined by the migration thread.
v2->v3:
    1) virtio-balloon
        - virtio_balloon_free_page_start: poll the hints using a new
          thread;
        - use cmd id between [0x80000000, UINT_MAX];
        - virtio_balloon_poll_free_page_hints:
            - stop the optimization only when it has started;
            - don't skip free pages when !poison_val;
        - add poison_val to vmsd to migrate;
        - virtio_balloon_get_features: add the F_PAGE_POISON feature when
          host has F_FREE_PAGE_HINT;
        - remove the timer patch which is not needed now.
    2) migration
       - new api, qemu_guest_free_page_hint;
       - rs->free_page_support set only in the precopy case;
       - use the new balloon APIs.
v1->v2: 
    1) virtio-balloon
        - use subsections to save free_page_report_cmd_id;
        - poll the free page vq after sending a cmd id to the driver;
        - change the free page vq size to VIRTQUEUE_MAX_SIZE;
        - virtio_balloon_poll_free_page_hints: handle the corner case
          that the free page block reported from the driver may cross
          the RAMBlock boundary.
    2) migration/ram.c
        - use balloon_free_page_poll to start the optimization

Wei Wang (6):
  bitmap: bitmap_count_one_with_offset
  migration: use bitmap_mutex in migration_bitmap_clear_dirty
  migration: API to clear bits of guest free pages from the dirty bitmap
  migration/ram.c: add ram save state notifiers
  migration: move migrate_postcopy() to include/migration/misc.h
  virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

 hw/virtio/virtio-balloon.c                      | 260 ++++++++++++++++++++++++
 include/hw/virtio/virtio-balloon.h              |  28 ++-
 include/migration/misc.h                        |  55 +++++
 include/qemu/bitmap.h                           |  13 ++
 include/standard-headers/linux/virtio_balloon.h |   7 +
 migration/migration.c                           |   2 +-
 migration/migration.h                           |   3 +-
 migration/ram.c                                 | 115 +++++++----
 8 files changed, 438 insertions(+), 45 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [virtio-dev] [PATCH v8 0/6] virtio-balloon: free page hint reporting support
@ 2018-06-08  8:10 ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

This is the deivce part implementation to add a new feature,
VIRTIO_BALLOON_F_FREE_PAGE_HINT to the virtio-balloon device. The device
receives the guest free page hints from the driver and clears the
corresponding bits in the dirty bitmap, so that those free pages are
not transferred by the migration thread to the destination.

- Test Environment
    Host: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz
    Guest: 8G RAM, 4 vCPU
    Migration setup: migrate_set_speed 100G, migrate_set_downtime 2 second

- Test Results
    - Idle Guest Live Migration Time (results are averaged over 10 runs):
        - Optimization v.s. Legacy = 271ms vs 1769ms --> ~86% reduction
    - Guest with Linux Compilation Workload (make bzImage -j4):
        - Live Migration Time (average)
          Optimization v.s. Legacy = 1265ms v.s. 2634ms --> ~51% reduction
        - Linux Compilation Time
          Optimization v.s. Legacy = 4min56s v.s. 5min3s
          --> no obvious difference

- Source Code
    - QEMU:  https://github.com/wei-w-wang/qemu-free-page-lm.git
    - Linux: https://github.com/wei-w-wang/linux-free-page-lm.git

ChangeLog:
v7->v8:
    1) migration:
        - qemu_guest_free_page_hint: 
            - check if this function is called during migration;
            - assert when the block of a given hint is not found;
        - add a ram save state notifier list;
        - move migrate_postcopy to an outside header for other
          subsystems (e.g. notifier callbacks) to use;
     2) virtio-balloon:
        - start/stop the optimization via a notifier, and reduce the
          related global balloon interfaces;
        - virtio_balloon_poll_free_page_hints: move the while loop into
          a function;
        - put page poison value into a separate vmsd subsection.
v6->v7:
      virtio-balloon/virtio_balloo_poll_free_page_hints:
          - add virtio_notify() at the end to notify the driver that
            the optimization is done, which indicates that the entries
            have all been put back to the vq and ready to detach them.
v5->v6:
      virtio-balloon: use iothread to get free page hint
v4->v5:
    1) migration:
        - bitmap_clear_dirty: update the dirty bitmap and dirty page
          count under the bitmap mutex as what other functions are doing;
        - qemu_guest_free_page_hint:
            - add comments for this function;
            - check the !block case;
            - check "offset > block->used_length" before proceed;
            - assign used_len inside the for{} body;
            - update the dirty bitmap and dirty page counter under the
              bitmap mutex;
        - ram_state_reset:
            - rs->free_page_support: && with use "migrate_postcopy"
              instead of migration_in_postcopy;
            - clear the ram_bulk_stage flag if free_page_support is true;
    2) balloon:
         - add the usage documentation of balloon_free_page_start and
           balloon_free_page_stop in code;
         - the optimization thread is named "balloon_fpo" to meet the
           requirement of "less than 14 characters";
         - virtio_balloon_poll_free_page_hints:
             - run on condition when runstate_is_running() is true;
             - add a qemu spin lock to synchronize accesses to the free
               page reporting related fields shared among the migration
               thread and the optimization thread;
          - virtio_balloon_free_page_start: just return if
            runstate_is_running is false;
          - virtio_balloon_free_page_stop: access to the free page
            reporting related fields under a qemu spin lock;
          - virtio_balloon_device_unrealize/reset: call
            virtio_balloon_free_page_stop is the free page hint feature is
            used;
          - virtio_balloon_set_status: call irtio_balloon_free_page_stop
            in case the guest is stopped by qmp when the optimization is
            running;
v3->v4:
    1) bitmap: add a new API to count 1s starting from an offset of a
       bitmap
    2) migration:
        - qemu_guest_free_page_hint: calculate
          ram_state->migration_dirty_pages by counting how many bits of
          free pages are truely cleared. If some of the bits were
          already 0, they shouldn't be deducted by
          ram_state->migration_dirty_pages. This wasn't needed for
          previous versions since we optimized bulk stage only,
          where all bits are guaranteed to be set. It's needed now
          because we extened the usage of this optimizaton to all stages
          except the last stop&copy stage. From 2nd stage onward, there
          are possibilities that some bits of free pages are already 0.
     3) virtio-balloon:
         - virtio_balloon_free_page_report_status: introduce a new status,
           FREE_PAGE_REPORT_S_EXIT. This status indicates that the
           optimization thread has exited. FREE_PAGE_REPORT_S_STOP means
           the reporting is stopped, but the optimization thread still needs
           to be joined by the migration thread.
v2->v3:
    1) virtio-balloon
        - virtio_balloon_free_page_start: poll the hints using a new
          thread;
        - use cmd id between [0x80000000, UINT_MAX];
        - virtio_balloon_poll_free_page_hints:
            - stop the optimization only when it has started;
            - don't skip free pages when !poison_val;
        - add poison_val to vmsd to migrate;
        - virtio_balloon_get_features: add the F_PAGE_POISON feature when
          host has F_FREE_PAGE_HINT;
        - remove the timer patch which is not needed now.
    2) migration
       - new api, qemu_guest_free_page_hint;
       - rs->free_page_support set only in the precopy case;
       - use the new balloon APIs.
v1->v2: 
    1) virtio-balloon
        - use subsections to save free_page_report_cmd_id;
        - poll the free page vq after sending a cmd id to the driver;
        - change the free page vq size to VIRTQUEUE_MAX_SIZE;
        - virtio_balloon_poll_free_page_hints: handle the corner case
          that the free page block reported from the driver may cross
          the RAMBlock boundary.
    2) migration/ram.c
        - use balloon_free_page_poll to start the optimization

Wei Wang (6):
  bitmap: bitmap_count_one_with_offset
  migration: use bitmap_mutex in migration_bitmap_clear_dirty
  migration: API to clear bits of guest free pages from the dirty bitmap
  migration/ram.c: add ram save state notifiers
  migration: move migrate_postcopy() to include/migration/misc.h
  virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

 hw/virtio/virtio-balloon.c                      | 260 ++++++++++++++++++++++++
 include/hw/virtio/virtio-balloon.h              |  28 ++-
 include/migration/misc.h                        |  55 +++++
 include/qemu/bitmap.h                           |  13 ++
 include/standard-headers/linux/virtio_balloon.h |   7 +
 migration/migration.c                           |   2 +-
 migration/migration.h                           |   3 +-
 migration/ram.c                                 | 115 +++++++----
 8 files changed, 438 insertions(+), 45 deletions(-)

-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v8 1/6] bitmap: bitmap_count_one_with_offset
  2018-06-08  8:10 ` [virtio-dev] " Wei Wang
@ 2018-06-08  8:10   ` Wei Wang
  -1 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

Count the number of 1s in a bitmap starting from an offset.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/qemu/bitmap.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 509eedd..e3f31f1 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -228,6 +228,19 @@ static inline long bitmap_count_one(const unsigned long *bitmap, long nbits)
     }
 }
 
+static inline long bitmap_count_one_with_offset(const unsigned long *bitmap,
+                                                long offset, long nbits)
+{
+    long aligned_offset = QEMU_ALIGN_DOWN(offset, BITS_PER_LONG);
+    long redundant_bits = offset - aligned_offset;
+    long bits_to_count = nbits + redundant_bits;
+    const unsigned long *bitmap_start = bitmap +
+                                        aligned_offset / BITS_PER_LONG;
+
+    return bitmap_count_one(bitmap_start, bits_to_count) -
+           bitmap_count_one(bitmap_start, redundant_bits);
+}
+
 void bitmap_set(unsigned long *map, long i, long len);
 void bitmap_set_atomic(unsigned long *map, long i, long len);
 void bitmap_clear(unsigned long *map, long start, long nr);
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [virtio-dev] [PATCH v8 1/6] bitmap: bitmap_count_one_with_offset
@ 2018-06-08  8:10   ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

Count the number of 1s in a bitmap starting from an offset.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/qemu/bitmap.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 509eedd..e3f31f1 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -228,6 +228,19 @@ static inline long bitmap_count_one(const unsigned long *bitmap, long nbits)
     }
 }
 
+static inline long bitmap_count_one_with_offset(const unsigned long *bitmap,
+                                                long offset, long nbits)
+{
+    long aligned_offset = QEMU_ALIGN_DOWN(offset, BITS_PER_LONG);
+    long redundant_bits = offset - aligned_offset;
+    long bits_to_count = nbits + redundant_bits;
+    const unsigned long *bitmap_start = bitmap +
+                                        aligned_offset / BITS_PER_LONG;
+
+    return bitmap_count_one(bitmap_start, bits_to_count) -
+           bitmap_count_one(bitmap_start, redundant_bits);
+}
+
 void bitmap_set(unsigned long *map, long i, long len);
 void bitmap_set_atomic(unsigned long *map, long i, long len);
 void bitmap_clear(unsigned long *map, long start, long nr);
-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v8 2/6] migration: use bitmap_mutex in migration_bitmap_clear_dirty
  2018-06-08  8:10 ` [virtio-dev] " Wei Wang
@ 2018-06-08  8:10   ` Wei Wang
  -1 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

The bitmap mutex is used to synchronize threads to update the dirty
bitmap and the migration_dirty_pages counter. For example, the free
page optimization clears bits of free pages from the bitmap in an
iothread context. This patch makes migration_bitmap_clear_dirty update
the bitmap and counter under the mutex.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index c53e836..2eabbe9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1093,11 +1093,14 @@ static inline bool migration_bitmap_clear_dirty(RAMState *rs,
 {
     bool ret;
 
+    qemu_mutex_lock(&rs->bitmap_mutex);
     ret = test_and_clear_bit(page, rb->bmap);
 
     if (ret) {
         rs->migration_dirty_pages--;
     }
+    qemu_mutex_unlock(&rs->bitmap_mutex);
+
     return ret;
 }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [virtio-dev] [PATCH v8 2/6] migration: use bitmap_mutex in migration_bitmap_clear_dirty
@ 2018-06-08  8:10   ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

The bitmap mutex is used to synchronize threads to update the dirty
bitmap and the migration_dirty_pages counter. For example, the free
page optimization clears bits of free pages from the bitmap in an
iothread context. This patch makes migration_bitmap_clear_dirty update
the bitmap and counter under the mutex.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index c53e836..2eabbe9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1093,11 +1093,14 @@ static inline bool migration_bitmap_clear_dirty(RAMState *rs,
 {
     bool ret;
 
+    qemu_mutex_lock(&rs->bitmap_mutex);
     ret = test_and_clear_bit(page, rb->bmap);
 
     if (ret) {
         rs->migration_dirty_pages--;
     }
+    qemu_mutex_unlock(&rs->bitmap_mutex);
+
     return ret;
 }
 
-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v8 3/6] migration: API to clear bits of guest free pages from the dirty bitmap
  2018-06-08  8:10 ` [virtio-dev] " Wei Wang
@ 2018-06-08  8:10   ` Wei Wang
  -1 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

This patch adds an API to clear bits corresponding to guest free pages
from the dirty bitmap. Spilt the free page block if it crosses the QEMU
RAMBlock boundary.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  2 ++
 migration/migration.c    |  2 +-
 migration/migration.h    |  1 +
 migration/ram.c          | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 4ebf24c..113320e 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -14,11 +14,13 @@
 #ifndef MIGRATION_MISC_H
 #define MIGRATION_MISC_H
 
+#include "exec/cpu-common.h"
 #include "qemu/notify.h"
 
 /* migration/ram.c */
 
 void ram_mig_init(void);
+void qemu_guest_free_page_hint(void *addr, size_t len);
 
 /* migration/block.c */
 
diff --git a/migration/migration.c b/migration/migration.c
index 05aec2c..220ff48 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -647,7 +647,7 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
  * Return true if we're already in the middle of a migration
  * (i.e. any of the active or setup states)
  */
-static bool migration_is_setup_or_active(int state)
+bool migration_is_setup_or_active(int state)
 {
     switch (state) {
     case MIGRATION_STATUS_ACTIVE:
diff --git a/migration/migration.h b/migration/migration.h
index 8f0c821..5a74740 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -230,6 +230,7 @@ void migrate_fd_error(MigrationState *s, const Error *error);
 void migrate_fd_connect(MigrationState *s, Error *error_in);
 
 void migrate_init(MigrationState *s);
+bool migration_is_setup_or_active(int state);
 bool migration_is_blocked(Error **errp);
 /* True if outgoing migration has entered postcopy phase */
 bool migration_in_postcopy(void);
diff --git a/migration/ram.c b/migration/ram.c
index 2eabbe9..237f11e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2530,6 +2530,54 @@ static void ram_state_resume_prepare(RAMState *rs, QEMUFile *out)
 }
 
 /*
+ * This function clears bits of the free pages reported by the caller from the
+ * migration dirty bitmap. @addr is the host address corresponding to the
+ * start of the continuous guest free pages, and @len is the total bytes of
+ * those pages.
+ */
+void qemu_guest_free_page_hint(void *addr, size_t len)
+{
+    RAMBlock *block;
+    ram_addr_t offset;
+    size_t used_len, start, npages;
+    MigrationState *s = migrate_get_current();
+
+    /* This function is currently expected to be used during live migration */
+    if (!migration_is_setup_or_active(s->state)) {
+        return;
+    }
+
+    for (; len > 0; len -= used_len) {
+        block = qemu_ram_block_from_host(addr, false, &offset);
+        assert(block);
+
+        /*
+         * This handles the case that the RAMBlock is resized after the free
+         * page hint is reported.
+         */
+        if (unlikely(offset > block->used_length)) {
+            return;
+        }
+
+        if (len <= block->used_length - offset) {
+            used_len = len;
+        } else {
+            used_len = block->used_length - offset;
+            addr += used_len;
+        }
+
+        start = offset >> TARGET_PAGE_BITS;
+        npages = used_len >> TARGET_PAGE_BITS;
+
+        qemu_mutex_lock(&ram_state->bitmap_mutex);
+        ram_state->migration_dirty_pages -=
+                      bitmap_count_one_with_offset(block->bmap, start, npages);
+        bitmap_clear(block->bmap, start, npages);
+        qemu_mutex_unlock(&ram_state->bitmap_mutex);
+    }
+}
+
+/*
  * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
  * long-running RCU critical section.  When rcu-reclaims in the code
  * start to become numerous it will be necessary to reduce the
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [virtio-dev] [PATCH v8 3/6] migration: API to clear bits of guest free pages from the dirty bitmap
@ 2018-06-08  8:10   ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

This patch adds an API to clear bits corresponding to guest free pages
from the dirty bitmap. Spilt the free page block if it crosses the QEMU
RAMBlock boundary.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  2 ++
 migration/migration.c    |  2 +-
 migration/migration.h    |  1 +
 migration/ram.c          | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 4ebf24c..113320e 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -14,11 +14,13 @@
 #ifndef MIGRATION_MISC_H
 #define MIGRATION_MISC_H
 
+#include "exec/cpu-common.h"
 #include "qemu/notify.h"
 
 /* migration/ram.c */
 
 void ram_mig_init(void);
+void qemu_guest_free_page_hint(void *addr, size_t len);
 
 /* migration/block.c */
 
diff --git a/migration/migration.c b/migration/migration.c
index 05aec2c..220ff48 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -647,7 +647,7 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
  * Return true if we're already in the middle of a migration
  * (i.e. any of the active or setup states)
  */
-static bool migration_is_setup_or_active(int state)
+bool migration_is_setup_or_active(int state)
 {
     switch (state) {
     case MIGRATION_STATUS_ACTIVE:
diff --git a/migration/migration.h b/migration/migration.h
index 8f0c821..5a74740 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -230,6 +230,7 @@ void migrate_fd_error(MigrationState *s, const Error *error);
 void migrate_fd_connect(MigrationState *s, Error *error_in);
 
 void migrate_init(MigrationState *s);
+bool migration_is_setup_or_active(int state);
 bool migration_is_blocked(Error **errp);
 /* True if outgoing migration has entered postcopy phase */
 bool migration_in_postcopy(void);
diff --git a/migration/ram.c b/migration/ram.c
index 2eabbe9..237f11e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2530,6 +2530,54 @@ static void ram_state_resume_prepare(RAMState *rs, QEMUFile *out)
 }
 
 /*
+ * This function clears bits of the free pages reported by the caller from the
+ * migration dirty bitmap. @addr is the host address corresponding to the
+ * start of the continuous guest free pages, and @len is the total bytes of
+ * those pages.
+ */
+void qemu_guest_free_page_hint(void *addr, size_t len)
+{
+    RAMBlock *block;
+    ram_addr_t offset;
+    size_t used_len, start, npages;
+    MigrationState *s = migrate_get_current();
+
+    /* This function is currently expected to be used during live migration */
+    if (!migration_is_setup_or_active(s->state)) {
+        return;
+    }
+
+    for (; len > 0; len -= used_len) {
+        block = qemu_ram_block_from_host(addr, false, &offset);
+        assert(block);
+
+        /*
+         * This handles the case that the RAMBlock is resized after the free
+         * page hint is reported.
+         */
+        if (unlikely(offset > block->used_length)) {
+            return;
+        }
+
+        if (len <= block->used_length - offset) {
+            used_len = len;
+        } else {
+            used_len = block->used_length - offset;
+            addr += used_len;
+        }
+
+        start = offset >> TARGET_PAGE_BITS;
+        npages = used_len >> TARGET_PAGE_BITS;
+
+        qemu_mutex_lock(&ram_state->bitmap_mutex);
+        ram_state->migration_dirty_pages -=
+                      bitmap_count_one_with_offset(block->bmap, start, npages);
+        bitmap_clear(block->bmap, start, npages);
+        qemu_mutex_unlock(&ram_state->bitmap_mutex);
+    }
+}
+
+/*
  * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
  * long-running RCU critical section.  When rcu-reclaims in the code
  * start to become numerous it will be necessary to reduce the
-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
  2018-06-08  8:10 ` [virtio-dev] " Wei Wang
@ 2018-06-08  8:10   ` Wei Wang
  -1 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

This patch adds a ram save state notifier list, and expose RAMState for
the notifer callbacks to use.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
 migration/ram.c          | 64 +++++++++++++++++-------------------------------
 2 files changed, 75 insertions(+), 41 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 113320e..b970d7d 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -16,9 +16,61 @@
 
 #include "exec/cpu-common.h"
 #include "qemu/notify.h"
+#include "qemu/thread.h"
 
 /* migration/ram.c */
 
+typedef enum RamSaveState {
+    RAM_SAVE_ERR = 0,
+    RAM_SAVE_RESET = 1,
+    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
+    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
+    RAM_SAVE_MAX = 4,
+} RamSaveState;
+
+/* State of RAM for migration */
+typedef struct RAMState {
+    /* QEMUFile used for this migration */
+    QEMUFile *f;
+    /* Last block that we have visited searching for dirty pages */
+    RAMBlock *last_seen_block;
+    /* Last block from where we have sent data */
+    RAMBlock *last_sent_block;
+    /* Last dirty target page we have sent */
+    ram_addr_t last_page;
+    /* last ram version we have seen */
+    uint32_t last_version;
+    /* We are in the first round */
+    bool ram_bulk_stage;
+    /* How many times we have dirty too many pages */
+    int dirty_rate_high_cnt;
+    /* ram save states used for notifiers */
+    int ram_save_state;
+    /* these variables are used for bitmap sync */
+    /* last time we did a full bitmap_sync */
+    int64_t time_last_bitmap_sync;
+    /* bytes transferred at start_time */
+    uint64_t bytes_xfer_prev;
+    /* number of dirty pages since start_time */
+    uint64_t num_dirty_pages_period;
+    /* xbzrle misses since the beginning of the period */
+    uint64_t xbzrle_cache_miss_prev;
+    /* number of iterations at the beginning of period */
+    uint64_t iterations_prev;
+    /* Iterations since start */
+    uint64_t iterations;
+    /* number of dirty bits in the bitmap */
+    uint64_t migration_dirty_pages;
+    /* protects modification of the bitmap */
+    QemuMutex bitmap_mutex;
+    /* The RAMBlock used in the last src_page_requests */
+    RAMBlock *last_req_rb;
+    /* Queue of outstanding page requests from the destination */
+    QemuMutex src_page_req_mutex;
+    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
+} RAMState;
+
+void add_ram_save_state_change_notifier(Notifier *notify);
 void ram_mig_init(void);
 void qemu_guest_free_page_hint(void *addr, size_t len);
 
diff --git a/migration/ram.c b/migration/ram.c
index 237f11e..d45b5a4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -56,6 +56,9 @@
 #include "qemu/uuid.h"
 #include "savevm.h"
 
+static NotifierList ram_save_state_notifiers =
+    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
+
 /***********************************************************/
 /* ram save/restore */
 
@@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
     QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
 };
 
-/* State of RAM for migration */
-struct RAMState {
-    /* QEMUFile used for this migration */
-    QEMUFile *f;
-    /* Last block that we have visited searching for dirty pages */
-    RAMBlock *last_seen_block;
-    /* Last block from where we have sent data */
-    RAMBlock *last_sent_block;
-    /* Last dirty target page we have sent */
-    ram_addr_t last_page;
-    /* last ram version we have seen */
-    uint32_t last_version;
-    /* We are in the first round */
-    bool ram_bulk_stage;
-    /* How many times we have dirty too many pages */
-    int dirty_rate_high_cnt;
-    /* these variables are used for bitmap sync */
-    /* last time we did a full bitmap_sync */
-    int64_t time_last_bitmap_sync;
-    /* bytes transferred at start_time */
-    uint64_t bytes_xfer_prev;
-    /* number of dirty pages since start_time */
-    uint64_t num_dirty_pages_period;
-    /* xbzrle misses since the beginning of the period */
-    uint64_t xbzrle_cache_miss_prev;
-    /* number of iterations at the beginning of period */
-    uint64_t iterations_prev;
-    /* Iterations since start */
-    uint64_t iterations;
-    /* number of dirty bits in the bitmap */
-    uint64_t migration_dirty_pages;
-    /* protects modification of the bitmap */
-    QemuMutex bitmap_mutex;
-    /* The RAMBlock used in the last src_page_requests */
-    RAMBlock *last_req_rb;
-    /* Queue of outstanding page requests from the destination */
-    QemuMutex src_page_req_mutex;
-    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
-};
-typedef struct RAMState RAMState;
-
 static RAMState *ram_state;
 
+void add_ram_save_state_change_notifier(Notifier *notify)
+{
+    notifier_list_add(&ram_save_state_notifiers, notify);
+}
+
+static void notify_ram_save_state_change_notifier(void)
+{
+    notifier_list_notify(&ram_save_state_notifiers, ram_state);
+}
+
 uint64_t ram_bytes_remaining(void)
 {
     return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
@@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
     int64_t end_time;
     uint64_t bytes_xfer_now;
 
+    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;
+    notify_ram_save_state_change_notifier();
+
     ram_counters.dirty_sync_count++;
 
     if (!rs->time_last_bitmap_sync) {
@@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
     if (migrate_use_events()) {
         qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
     }
+
+    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
+    notify_ram_save_state_change_notifier();
 }
 
 /**
@@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
     rs->last_page = 0;
     rs->last_version = ram_list.version;
     rs->ram_bulk_stage = true;
+    rs->ram_save_state = RAM_SAVE_RESET;
+    notify_ram_save_state_change_notifier();
 }
 
 #define MAX_WAIT 50 /* ms, half buffered_file limit */
@@ -2709,6 +2689,8 @@ out:
 
     ret = qemu_file_get_error(f);
     if (ret < 0) {
+        rs->ram_save_state = RAM_SAVE_ERR;
+        notify_ram_save_state_change_notifier();
         return ret;
     }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [virtio-dev] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
@ 2018-06-08  8:10   ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

This patch adds a ram save state notifier list, and expose RAMState for
the notifer callbacks to use.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
 migration/ram.c          | 64 +++++++++++++++++-------------------------------
 2 files changed, 75 insertions(+), 41 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 113320e..b970d7d 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -16,9 +16,61 @@
 
 #include "exec/cpu-common.h"
 #include "qemu/notify.h"
+#include "qemu/thread.h"
 
 /* migration/ram.c */
 
+typedef enum RamSaveState {
+    RAM_SAVE_ERR = 0,
+    RAM_SAVE_RESET = 1,
+    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
+    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
+    RAM_SAVE_MAX = 4,
+} RamSaveState;
+
+/* State of RAM for migration */
+typedef struct RAMState {
+    /* QEMUFile used for this migration */
+    QEMUFile *f;
+    /* Last block that we have visited searching for dirty pages */
+    RAMBlock *last_seen_block;
+    /* Last block from where we have sent data */
+    RAMBlock *last_sent_block;
+    /* Last dirty target page we have sent */
+    ram_addr_t last_page;
+    /* last ram version we have seen */
+    uint32_t last_version;
+    /* We are in the first round */
+    bool ram_bulk_stage;
+    /* How many times we have dirty too many pages */
+    int dirty_rate_high_cnt;
+    /* ram save states used for notifiers */
+    int ram_save_state;
+    /* these variables are used for bitmap sync */
+    /* last time we did a full bitmap_sync */
+    int64_t time_last_bitmap_sync;
+    /* bytes transferred at start_time */
+    uint64_t bytes_xfer_prev;
+    /* number of dirty pages since start_time */
+    uint64_t num_dirty_pages_period;
+    /* xbzrle misses since the beginning of the period */
+    uint64_t xbzrle_cache_miss_prev;
+    /* number of iterations at the beginning of period */
+    uint64_t iterations_prev;
+    /* Iterations since start */
+    uint64_t iterations;
+    /* number of dirty bits in the bitmap */
+    uint64_t migration_dirty_pages;
+    /* protects modification of the bitmap */
+    QemuMutex bitmap_mutex;
+    /* The RAMBlock used in the last src_page_requests */
+    RAMBlock *last_req_rb;
+    /* Queue of outstanding page requests from the destination */
+    QemuMutex src_page_req_mutex;
+    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
+} RAMState;
+
+void add_ram_save_state_change_notifier(Notifier *notify);
 void ram_mig_init(void);
 void qemu_guest_free_page_hint(void *addr, size_t len);
 
diff --git a/migration/ram.c b/migration/ram.c
index 237f11e..d45b5a4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -56,6 +56,9 @@
 #include "qemu/uuid.h"
 #include "savevm.h"
 
+static NotifierList ram_save_state_notifiers =
+    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
+
 /***********************************************************/
 /* ram save/restore */
 
@@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
     QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
 };
 
-/* State of RAM for migration */
-struct RAMState {
-    /* QEMUFile used for this migration */
-    QEMUFile *f;
-    /* Last block that we have visited searching for dirty pages */
-    RAMBlock *last_seen_block;
-    /* Last block from where we have sent data */
-    RAMBlock *last_sent_block;
-    /* Last dirty target page we have sent */
-    ram_addr_t last_page;
-    /* last ram version we have seen */
-    uint32_t last_version;
-    /* We are in the first round */
-    bool ram_bulk_stage;
-    /* How many times we have dirty too many pages */
-    int dirty_rate_high_cnt;
-    /* these variables are used for bitmap sync */
-    /* last time we did a full bitmap_sync */
-    int64_t time_last_bitmap_sync;
-    /* bytes transferred at start_time */
-    uint64_t bytes_xfer_prev;
-    /* number of dirty pages since start_time */
-    uint64_t num_dirty_pages_period;
-    /* xbzrle misses since the beginning of the period */
-    uint64_t xbzrle_cache_miss_prev;
-    /* number of iterations at the beginning of period */
-    uint64_t iterations_prev;
-    /* Iterations since start */
-    uint64_t iterations;
-    /* number of dirty bits in the bitmap */
-    uint64_t migration_dirty_pages;
-    /* protects modification of the bitmap */
-    QemuMutex bitmap_mutex;
-    /* The RAMBlock used in the last src_page_requests */
-    RAMBlock *last_req_rb;
-    /* Queue of outstanding page requests from the destination */
-    QemuMutex src_page_req_mutex;
-    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
-};
-typedef struct RAMState RAMState;
-
 static RAMState *ram_state;
 
+void add_ram_save_state_change_notifier(Notifier *notify)
+{
+    notifier_list_add(&ram_save_state_notifiers, notify);
+}
+
+static void notify_ram_save_state_change_notifier(void)
+{
+    notifier_list_notify(&ram_save_state_notifiers, ram_state);
+}
+
 uint64_t ram_bytes_remaining(void)
 {
     return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
@@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
     int64_t end_time;
     uint64_t bytes_xfer_now;
 
+    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;
+    notify_ram_save_state_change_notifier();
+
     ram_counters.dirty_sync_count++;
 
     if (!rs->time_last_bitmap_sync) {
@@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
     if (migrate_use_events()) {
         qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
     }
+
+    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
+    notify_ram_save_state_change_notifier();
 }
 
 /**
@@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
     rs->last_page = 0;
     rs->last_version = ram_list.version;
     rs->ram_bulk_stage = true;
+    rs->ram_save_state = RAM_SAVE_RESET;
+    notify_ram_save_state_change_notifier();
 }
 
 #define MAX_WAIT 50 /* ms, half buffered_file limit */
@@ -2709,6 +2689,8 @@ out:
 
     ret = qemu_file_get_error(f);
     if (ret < 0) {
+        rs->ram_save_state = RAM_SAVE_ERR;
+        notify_ram_save_state_change_notifier();
         return ret;
     }
 
-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v8 5/6] migration: move migrate_postcopy() to include/migration/misc.h
  2018-06-08  8:10 ` [virtio-dev] " Wei Wang
@ 2018-06-08  8:10   ` Wei Wang
  -1 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

The ram save state notifier callback, for example the free page
optimization offerred by virtio-balloon, may need to check if
postcopy is in use, so move migrate_postcopy() to the outside header.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 1 +
 migration/migration.h    | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index b970d7d..911aaf3 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -109,6 +109,7 @@ bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
 void migration_global_dump(Monitor *mon);
+bool migrate_postcopy(void);
 
 /* migration/block-dirty-bitmap.c */
 void dirty_bitmap_mig_init(void);
diff --git a/migration/migration.h b/migration/migration.h
index 5a74740..fee5af8 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -236,8 +236,6 @@ bool migration_is_blocked(Error **errp);
 bool migration_in_postcopy(void);
 MigrationState *migrate_get_current(void);
 
-bool migrate_postcopy(void);
-
 bool migrate_release_ram(void);
 bool migrate_postcopy_ram(void);
 bool migrate_zero_blocks(void);
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [virtio-dev] [PATCH v8 5/6] migration: move migrate_postcopy() to include/migration/misc.h
@ 2018-06-08  8:10   ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

The ram save state notifier callback, for example the free page
optimization offerred by virtio-balloon, may need to check if
postcopy is in use, so move migrate_postcopy() to the outside header.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 1 +
 migration/migration.h    | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index b970d7d..911aaf3 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -109,6 +109,7 @@ bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
 void migration_global_dump(Monitor *mon);
+bool migrate_postcopy(void);
 
 /* migration/block-dirty-bitmap.c */
 void dirty_bitmap_mig_init(void);
diff --git a/migration/migration.h b/migration/migration.h
index 5a74740..fee5af8 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -236,8 +236,6 @@ bool migration_is_blocked(Error **errp);
 bool migration_in_postcopy(void);
 MigrationState *migrate_get_current(void);
 
-bool migrate_postcopy(void);
-
 bool migrate_release_ram(void);
 bool migrate_postcopy_ram(void);
 bool migrate_zero_blocks(void);
-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Qemu-devel] [PATCH v8 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
  2018-06-08  8:10 ` [virtio-dev] " Wei Wang
@ 2018-06-08  8:10   ` Wei Wang
  -1 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

The new feature enables the virtio-balloon device to receive hints of
guest free pages from the free page vq.

A notifier is registered to the migration ram save state notifier list.
The notifier calls free_page_start after the migration thread syncs the
dirty bitmap, so that the free page hinting optimization starts to clear
bits of free pages from the bitmap. It calls the free_page_stop
before the migration thread syncs the bitmap, which is the end of the
current round of ram save. The free_page_stop is also called to stop the
optimization in the case there is an error happened in the process of
ram save.

Note: balloon will report pages which were free at the time of this call.
As the reporting happens asynchronously, dirty bit logging must be
enabled before this free_page_start call is made. Guest reporting must be
disabled before the migration dirty bitmap is synchronized.

TODO:
- If free pages are poisoned by guest, the hints are dropped currently.
  We will support clearing bits of poisoned pages from the bitmap in the
  future.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 hw/virtio/virtio-balloon.c                      | 260 ++++++++++++++++++++++++
 include/hw/virtio/virtio-balloon.h              |  28 ++-
 include/standard-headers/linux/virtio_balloon.h |   7 +
 3 files changed, 294 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1f7a87f..a7bb971 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -28,6 +28,7 @@
 #include "qapi/visitor.h"
 #include "trace.h"
 #include "qemu/error-report.h"
+#include "migration/misc.h"
 
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
@@ -310,6 +311,166 @@ out:
     }
 }
 
+static void get_free_page_hints(VirtIOBalloon *dev)
+{
+    VirtQueueElement *elem;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtQueue *vq = dev->free_page_vq;
+    uint32_t id;
+    size_t size;
+
+    while (dev->block_iothread) {
+        qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
+    }
+
+    /*
+     * If the migration thread actively stops the reporting, exit
+     * immediately.
+     */
+    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
+        return;
+    }
+
+    elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+    if (!elem) {
+        return;
+    }
+
+    if (elem->out_num) {
+        size = iov_to_buf(elem->out_sg, elem->out_num, 0, &id, sizeof(id));
+        virtqueue_push(vq, elem, size);
+        g_free(elem);
+
+        virtio_tswap32s(vdev, &id);
+        if (unlikely(size != sizeof(id))) {
+            virtio_error(vdev, "received an incorrect cmd id");
+            return;
+        }
+        if (id == dev->free_page_report_cmd_id) {
+            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+        } else {
+            /*
+             * Stop the optimization only when it has started. This
+             * avoids a stale stop sign for the previous command.
+             */
+            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+                return;
+            }
+        }
+    }
+
+    if (elem->in_num) {
+        /* TODO: send the poison value to the destination */
+        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START &&
+            !dev->poison_val) {
+            qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
+                                      elem->in_sg[0].iov_len);
+        }
+        virtqueue_push(vq, elem, 0);
+        g_free(elem);
+    }
+}
+
+static void virtio_balloon_poll_free_page_hints(void *opaque)
+{
+    VirtIOBalloon *dev = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtQueue *vq = dev->free_page_vq;
+
+    while (dev->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+        qemu_mutex_lock(&dev->free_page_lock);
+        get_free_page_hints(dev);
+        qemu_mutex_unlock(&dev->free_page_lock);
+    }
+    virtio_notify(vdev, vq);
+}
+
+static bool virtio_balloon_free_page_support(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+}
+
+static bool virtio_balloon_page_poison_support(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+}
+
+static void virtio_balloon_free_page_start(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    /* For the stop and copy phase, we don't need to start the optimization */
+    if (!vdev->vm_running) {
+        return;
+    }
+
+    if (s->free_page_report_cmd_id == UINT_MAX) {
+        s->free_page_report_cmd_id =
+                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+    } else {
+        s->free_page_report_cmd_id++;
+    }
+
+    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
+    virtio_notify_config(vdev);
+    qemu_bh_schedule(s->free_page_bh);
+}
+
+static void virtio_balloon_free_page_stop(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+        /*
+         * The lock also guarantees us that the
+         * virtio_balloon_poll_free_page_hints exits after the
+         * free_page_report_status is set to S_STOP.
+         */
+        qemu_mutex_lock(&s->free_page_lock);
+        /*
+         * The guest hasn't done the reporting, so host sends a notification
+         * to the guest to actively stop the reporting.
+         */
+        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+        qemu_mutex_unlock(&s->free_page_lock);
+        virtio_notify_config(vdev);
+    }
+}
+
+static void virtio_balloon_free_page_report_notify(Notifier *notifier,
+                                                   void *data)
+{
+    VirtIOBalloon *dev = container_of(notifier, VirtIOBalloon,
+                                      free_page_report_notify);
+    RAMState *rs = data;
+
+    if (!virtio_balloon_free_page_support(dev) || migrate_postcopy()) {
+        return;
+    }
+
+    switch (rs->ram_save_state) {
+    case RAM_SAVE_RESET:
+        rs->ram_bulk_stage = false;
+        break;
+    case RAM_SAVE_ERR:
+    case RAM_SAVE_BEFORE_SYNC_BITMAP:
+        virtio_balloon_free_page_stop(dev);
+        break;
+    case RAM_SAVE_AFTER_SYNC_BITMAP:
+        virtio_balloon_free_page_start(dev);
+        break;
+    }
+}
+
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
@@ -317,6 +478,17 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
+    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
+        config.poison_val = cpu_to_le32(dev->poison_val);
+    }
+
+    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
+        config.free_page_report_cmd_id =
+                       cpu_to_le32(VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID);
+    } else {
+        config.free_page_report_cmd_id =
+                       cpu_to_le32(dev->free_page_report_cmd_id);
+    }
 
     trace_virtio_balloon_get_config(config.num_pages, config.actual);
     memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
@@ -370,6 +542,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
                         ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
                         &error_abort);
     }
+    dev->poison_val = le32_to_cpu(config.poison_val);
     trace_virtio_balloon_set_config(dev->actual, oldactual);
 }
 
@@ -379,6 +552,12 @@ static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f,
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
     f |= dev->host_features;
     virtio_add_feature(&f, VIRTIO_BALLOON_F_STATS_VQ);
+
+    if (virtio_has_feature(dev->host_features,
+                           VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+        virtio_add_feature(&f, VIRTIO_BALLOON_F_PAGE_POISON);
+    }
+
     return f;
 }
 
@@ -415,6 +594,28 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
     return 0;
 }
 
+static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
+    .name = "virtio-balloon-device/free-page-report",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = virtio_balloon_free_page_support,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_virtio_balloon_page_poison = {
+    .name = "virtio-balloon-device/page-poison",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = virtio_balloon_page_poison_support,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(poison_val, VirtIOBalloon),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio_balloon_device = {
     .name = "virtio-balloon-device",
     .version_id = 1,
@@ -425,6 +626,11 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
         VMSTATE_UINT32(actual, VirtIOBalloon),
         VMSTATE_END_OF_LIST()
     },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_virtio_balloon_free_page_report,
+        &vmstate_virtio_balloon_page_poison,
+        NULL
+    }
 };
 
 static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
@@ -449,6 +655,28 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
 
+    if (virtio_has_feature(s->host_features,
+                           VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+        s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE, NULL);
+        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+        s->free_page_report_cmd_id =
+                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+        s->free_page_report_notify.notify =
+                                       virtio_balloon_free_page_report_notify;
+        add_ram_save_state_change_notifier(&s->free_page_report_notify);
+        if (s->iothread) {
+            object_ref(OBJECT(s->iothread));
+            s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
+                                       virtio_balloon_poll_free_page_hints, s);
+            qemu_mutex_init(&s->free_page_lock);
+            qemu_cond_init(&s->free_page_cond);
+            s->block_iothread = false;
+        } else {
+            /* Simply disable this feature if the iothread wasn't created. */
+            s->host_features &= ~(1 << VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+            virtio_error(vdev, "iothread is missing");
+        }
+    }
     reset_stats(s);
 }
 
@@ -457,6 +685,10 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
 
+    if (virtio_balloon_free_page_support(s)) {
+        qemu_bh_delete(s->free_page_bh);
+        virtio_balloon_free_page_stop(s);
+    }
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
     virtio_cleanup(vdev);
@@ -466,6 +698,10 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
+    if (virtio_balloon_free_page_support(s)) {
+        virtio_balloon_free_page_stop(s);
+    }
+
     if (s->stats_vq_elem != NULL) {
         virtqueue_unpop(s->svq, s->stats_vq_elem, 0);
         g_free(s->stats_vq_elem);
@@ -483,6 +719,26 @@ static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
          * was stopped */
         virtio_balloon_receive_stats(vdev, s->svq);
     }
+
+    if (virtio_balloon_free_page_support(s)) {
+        /*
+         * The VM is woken up and the iothread was blocked, so signal it to
+         * continue.
+         */
+        if (vdev->vm_running && s->block_iothread) {
+            qemu_mutex_lock(&s->free_page_lock);
+            s->block_iothread = false;
+            qemu_cond_signal(&s->free_page_cond);
+            qemu_mutex_unlock(&s->free_page_lock);
+        }
+
+        /* The VM is stopped, block the iothread. */
+        if (!vdev->vm_running) {
+            qemu_mutex_lock(&s->free_page_lock);
+            s->block_iothread = true;
+            qemu_mutex_unlock(&s->free_page_lock);
+        }
+    }
 }
 
 static void virtio_balloon_instance_init(Object *obj)
@@ -511,6 +767,10 @@ static const VMStateDescription vmstate_virtio_balloon = {
 static Property virtio_balloon_properties[] = {
     DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,
                     VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
+    DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
+                    VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
+    DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
+                     IOThread *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index e0df352..e14e545 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -17,11 +17,14 @@
 
 #include "standard-headers/linux/virtio_balloon.h"
 #include "hw/virtio/virtio.h"
+#include "sysemu/iothread.h"
 
 #define TYPE_VIRTIO_BALLOON "virtio-balloon-device"
 #define VIRTIO_BALLOON(obj) \
         OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
 
+#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
+
 typedef struct virtio_balloon_stat VirtIOBalloonStat;
 
 typedef struct virtio_balloon_stat_modern {
@@ -30,15 +33,38 @@ typedef struct virtio_balloon_stat_modern {
        uint64_t val;
 } VirtIOBalloonStatModern;
 
+enum virtio_balloon_free_page_report_status {
+    FREE_PAGE_REPORT_S_STOP = 0,
+    FREE_PAGE_REPORT_S_REQUESTED = 1,
+    FREE_PAGE_REPORT_S_START = 2,
+};
+
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
-    VirtQueue *ivq, *dvq, *svq;
+    VirtQueue *ivq, *dvq, *svq, *free_page_vq;
+    uint32_t free_page_report_status;
     uint32_t num_pages;
     uint32_t actual;
+    uint32_t free_page_report_cmd_id;
+    uint32_t poison_val;
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
     QEMUTimer *stats_timer;
+    IOThread *iothread;
+    QEMUBH *free_page_bh;
+    /*
+     * Lock to synchronize threads to access the free page reporting related
+     * fields (e.g. free_page_report_status).
+     */
+    QemuMutex free_page_lock;
+    QemuCond  free_page_cond;
+    /*
+     * Set to block iothread to continue reading free page hints as the VM is
+     * stopped.
+     */
+    bool block_iothread;
+    Notifier free_page_report_notify;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;
diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index e446805..eb47e6c 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -34,15 +34,22 @@
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
 #define VIRTIO_BALLOON_F_STATS_VQ	1 /* Memory Stats virtqueue */
 #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM	2 /* Deflate balloon on OOM */
+#define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */
+#define VIRTIO_BALLOON_F_PAGE_POISON	4 /* Guest is using page poisoning */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
 
+#define VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID 0
 struct virtio_balloon_config {
 	/* Number of pages host wants Guest to give up. */
 	uint32_t num_pages;
 	/* Number of pages we've actually got in balloon. */
 	uint32_t actual;
+	/* Free page report command id, readonly by guest */
+	uint32_t free_page_report_cmd_id;
+	/* Stores PAGE_POISON if page poisoning is in use */
+	uint32_t poison_val;
 };
 
 #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [virtio-dev] [PATCH v8 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
@ 2018-06-08  8:10   ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-08  8:10 UTC (permalink / raw)
  To: qemu-devel, virtio-dev, mst, quintela, dgilbert
  Cc: peterx, pbonzini, wei.w.wang, liliang.opensource, yang.zhang.wz,
	quan.xu0, nilal, riel

The new feature enables the virtio-balloon device to receive hints of
guest free pages from the free page vq.

A notifier is registered to the migration ram save state notifier list.
The notifier calls free_page_start after the migration thread syncs the
dirty bitmap, so that the free page hinting optimization starts to clear
bits of free pages from the bitmap. It calls the free_page_stop
before the migration thread syncs the bitmap, which is the end of the
current round of ram save. The free_page_stop is also called to stop the
optimization in the case there is an error happened in the process of
ram save.

Note: balloon will report pages which were free at the time of this call.
As the reporting happens asynchronously, dirty bit logging must be
enabled before this free_page_start call is made. Guest reporting must be
disabled before the migration dirty bitmap is synchronized.

TODO:
- If free pages are poisoned by guest, the hints are dropped currently.
  We will support clearing bits of poisoned pages from the bitmap in the
  future.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 hw/virtio/virtio-balloon.c                      | 260 ++++++++++++++++++++++++
 include/hw/virtio/virtio-balloon.h              |  28 ++-
 include/standard-headers/linux/virtio_balloon.h |   7 +
 3 files changed, 294 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1f7a87f..a7bb971 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -28,6 +28,7 @@
 #include "qapi/visitor.h"
 #include "trace.h"
 #include "qemu/error-report.h"
+#include "migration/misc.h"
 
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
@@ -310,6 +311,166 @@ out:
     }
 }
 
+static void get_free_page_hints(VirtIOBalloon *dev)
+{
+    VirtQueueElement *elem;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtQueue *vq = dev->free_page_vq;
+    uint32_t id;
+    size_t size;
+
+    while (dev->block_iothread) {
+        qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
+    }
+
+    /*
+     * If the migration thread actively stops the reporting, exit
+     * immediately.
+     */
+    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
+        return;
+    }
+
+    elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+    if (!elem) {
+        return;
+    }
+
+    if (elem->out_num) {
+        size = iov_to_buf(elem->out_sg, elem->out_num, 0, &id, sizeof(id));
+        virtqueue_push(vq, elem, size);
+        g_free(elem);
+
+        virtio_tswap32s(vdev, &id);
+        if (unlikely(size != sizeof(id))) {
+            virtio_error(vdev, "received an incorrect cmd id");
+            return;
+        }
+        if (id == dev->free_page_report_cmd_id) {
+            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+        } else {
+            /*
+             * Stop the optimization only when it has started. This
+             * avoids a stale stop sign for the previous command.
+             */
+            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+                return;
+            }
+        }
+    }
+
+    if (elem->in_num) {
+        /* TODO: send the poison value to the destination */
+        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START &&
+            !dev->poison_val) {
+            qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
+                                      elem->in_sg[0].iov_len);
+        }
+        virtqueue_push(vq, elem, 0);
+        g_free(elem);
+    }
+}
+
+static void virtio_balloon_poll_free_page_hints(void *opaque)
+{
+    VirtIOBalloon *dev = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtQueue *vq = dev->free_page_vq;
+
+    while (dev->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+        qemu_mutex_lock(&dev->free_page_lock);
+        get_free_page_hints(dev);
+        qemu_mutex_unlock(&dev->free_page_lock);
+    }
+    virtio_notify(vdev, vq);
+}
+
+static bool virtio_balloon_free_page_support(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+}
+
+static bool virtio_balloon_page_poison_support(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+}
+
+static void virtio_balloon_free_page_start(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    /* For the stop and copy phase, we don't need to start the optimization */
+    if (!vdev->vm_running) {
+        return;
+    }
+
+    if (s->free_page_report_cmd_id == UINT_MAX) {
+        s->free_page_report_cmd_id =
+                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+    } else {
+        s->free_page_report_cmd_id++;
+    }
+
+    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
+    virtio_notify_config(vdev);
+    qemu_bh_schedule(s->free_page_bh);
+}
+
+static void virtio_balloon_free_page_stop(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+        /*
+         * The lock also guarantees us that the
+         * virtio_balloon_poll_free_page_hints exits after the
+         * free_page_report_status is set to S_STOP.
+         */
+        qemu_mutex_lock(&s->free_page_lock);
+        /*
+         * The guest hasn't done the reporting, so host sends a notification
+         * to the guest to actively stop the reporting.
+         */
+        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+        qemu_mutex_unlock(&s->free_page_lock);
+        virtio_notify_config(vdev);
+    }
+}
+
+static void virtio_balloon_free_page_report_notify(Notifier *notifier,
+                                                   void *data)
+{
+    VirtIOBalloon *dev = container_of(notifier, VirtIOBalloon,
+                                      free_page_report_notify);
+    RAMState *rs = data;
+
+    if (!virtio_balloon_free_page_support(dev) || migrate_postcopy()) {
+        return;
+    }
+
+    switch (rs->ram_save_state) {
+    case RAM_SAVE_RESET:
+        rs->ram_bulk_stage = false;
+        break;
+    case RAM_SAVE_ERR:
+    case RAM_SAVE_BEFORE_SYNC_BITMAP:
+        virtio_balloon_free_page_stop(dev);
+        break;
+    case RAM_SAVE_AFTER_SYNC_BITMAP:
+        virtio_balloon_free_page_start(dev);
+        break;
+    }
+}
+
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
@@ -317,6 +478,17 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
+    if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
+        config.poison_val = cpu_to_le32(dev->poison_val);
+    }
+
+    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
+        config.free_page_report_cmd_id =
+                       cpu_to_le32(VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID);
+    } else {
+        config.free_page_report_cmd_id =
+                       cpu_to_le32(dev->free_page_report_cmd_id);
+    }
 
     trace_virtio_balloon_get_config(config.num_pages, config.actual);
     memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
@@ -370,6 +542,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
                         ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
                         &error_abort);
     }
+    dev->poison_val = le32_to_cpu(config.poison_val);
     trace_virtio_balloon_set_config(dev->actual, oldactual);
 }
 
@@ -379,6 +552,12 @@ static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f,
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
     f |= dev->host_features;
     virtio_add_feature(&f, VIRTIO_BALLOON_F_STATS_VQ);
+
+    if (virtio_has_feature(dev->host_features,
+                           VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+        virtio_add_feature(&f, VIRTIO_BALLOON_F_PAGE_POISON);
+    }
+
     return f;
 }
 
@@ -415,6 +594,28 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
     return 0;
 }
 
+static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
+    .name = "virtio-balloon-device/free-page-report",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = virtio_balloon_free_page_support,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_virtio_balloon_page_poison = {
+    .name = "virtio-balloon-device/page-poison",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = virtio_balloon_page_poison_support,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(poison_val, VirtIOBalloon),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio_balloon_device = {
     .name = "virtio-balloon-device",
     .version_id = 1,
@@ -425,6 +626,11 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
         VMSTATE_UINT32(actual, VirtIOBalloon),
         VMSTATE_END_OF_LIST()
     },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_virtio_balloon_free_page_report,
+        &vmstate_virtio_balloon_page_poison,
+        NULL
+    }
 };
 
 static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
@@ -449,6 +655,28 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
 
+    if (virtio_has_feature(s->host_features,
+                           VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+        s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE, NULL);
+        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+        s->free_page_report_cmd_id =
+                           VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+        s->free_page_report_notify.notify =
+                                       virtio_balloon_free_page_report_notify;
+        add_ram_save_state_change_notifier(&s->free_page_report_notify);
+        if (s->iothread) {
+            object_ref(OBJECT(s->iothread));
+            s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
+                                       virtio_balloon_poll_free_page_hints, s);
+            qemu_mutex_init(&s->free_page_lock);
+            qemu_cond_init(&s->free_page_cond);
+            s->block_iothread = false;
+        } else {
+            /* Simply disable this feature if the iothread wasn't created. */
+            s->host_features &= ~(1 << VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+            virtio_error(vdev, "iothread is missing");
+        }
+    }
     reset_stats(s);
 }
 
@@ -457,6 +685,10 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
 
+    if (virtio_balloon_free_page_support(s)) {
+        qemu_bh_delete(s->free_page_bh);
+        virtio_balloon_free_page_stop(s);
+    }
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
     virtio_cleanup(vdev);
@@ -466,6 +698,10 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
+    if (virtio_balloon_free_page_support(s)) {
+        virtio_balloon_free_page_stop(s);
+    }
+
     if (s->stats_vq_elem != NULL) {
         virtqueue_unpop(s->svq, s->stats_vq_elem, 0);
         g_free(s->stats_vq_elem);
@@ -483,6 +719,26 @@ static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
          * was stopped */
         virtio_balloon_receive_stats(vdev, s->svq);
     }
+
+    if (virtio_balloon_free_page_support(s)) {
+        /*
+         * The VM is woken up and the iothread was blocked, so signal it to
+         * continue.
+         */
+        if (vdev->vm_running && s->block_iothread) {
+            qemu_mutex_lock(&s->free_page_lock);
+            s->block_iothread = false;
+            qemu_cond_signal(&s->free_page_cond);
+            qemu_mutex_unlock(&s->free_page_lock);
+        }
+
+        /* The VM is stopped, block the iothread. */
+        if (!vdev->vm_running) {
+            qemu_mutex_lock(&s->free_page_lock);
+            s->block_iothread = true;
+            qemu_mutex_unlock(&s->free_page_lock);
+        }
+    }
 }
 
 static void virtio_balloon_instance_init(Object *obj)
@@ -511,6 +767,10 @@ static const VMStateDescription vmstate_virtio_balloon = {
 static Property virtio_balloon_properties[] = {
     DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,
                     VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
+    DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
+                    VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
+    DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
+                     IOThread *),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index e0df352..e14e545 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -17,11 +17,14 @@
 
 #include "standard-headers/linux/virtio_balloon.h"
 #include "hw/virtio/virtio.h"
+#include "sysemu/iothread.h"
 
 #define TYPE_VIRTIO_BALLOON "virtio-balloon-device"
 #define VIRTIO_BALLOON(obj) \
         OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
 
+#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
+
 typedef struct virtio_balloon_stat VirtIOBalloonStat;
 
 typedef struct virtio_balloon_stat_modern {
@@ -30,15 +33,38 @@ typedef struct virtio_balloon_stat_modern {
        uint64_t val;
 } VirtIOBalloonStatModern;
 
+enum virtio_balloon_free_page_report_status {
+    FREE_PAGE_REPORT_S_STOP = 0,
+    FREE_PAGE_REPORT_S_REQUESTED = 1,
+    FREE_PAGE_REPORT_S_START = 2,
+};
+
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
-    VirtQueue *ivq, *dvq, *svq;
+    VirtQueue *ivq, *dvq, *svq, *free_page_vq;
+    uint32_t free_page_report_status;
     uint32_t num_pages;
     uint32_t actual;
+    uint32_t free_page_report_cmd_id;
+    uint32_t poison_val;
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
     QEMUTimer *stats_timer;
+    IOThread *iothread;
+    QEMUBH *free_page_bh;
+    /*
+     * Lock to synchronize threads to access the free page reporting related
+     * fields (e.g. free_page_report_status).
+     */
+    QemuMutex free_page_lock;
+    QemuCond  free_page_cond;
+    /*
+     * Set to block iothread to continue reading free page hints as the VM is
+     * stopped.
+     */
+    bool block_iothread;
+    Notifier free_page_report_notify;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;
diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index e446805..eb47e6c 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -34,15 +34,22 @@
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
 #define VIRTIO_BALLOON_F_STATS_VQ	1 /* Memory Stats virtqueue */
 #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM	2 /* Deflate balloon on OOM */
+#define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */
+#define VIRTIO_BALLOON_F_PAGE_POISON	4 /* Guest is using page poisoning */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
 
+#define VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID 0
 struct virtio_balloon_config {
 	/* Number of pages host wants Guest to give up. */
 	uint32_t num_pages;
 	/* Number of pages we've actually got in balloon. */
 	uint32_t actual;
+	/* Free page report command id, readonly by guest */
+	uint32_t free_page_report_cmd_id;
+	/* Stores PAGE_POISON if page poisoning is in use */
+	uint32_t poison_val;
 };
 
 #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
-- 
1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
  2018-06-08  8:10   ` [virtio-dev] " Wei Wang
  (?)
@ 2018-06-12  7:50   ` Peter Xu
  2018-06-12 11:12       ` [virtio-dev] " Wei Wang
  -1 siblings, 1 reply; 22+ messages in thread
From: Peter Xu @ 2018-06-12  7:50 UTC (permalink / raw)
  To: Wei Wang
  Cc: qemu-devel, virtio-dev, mst, quintela, dgilbert, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

On Fri, Jun 08, 2018 at 04:10:41PM +0800, Wei Wang wrote:
> This patch adds a ram save state notifier list, and expose RAMState for
> the notifer callbacks to use.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
>  migration/ram.c          | 64 +++++++++++++++++-------------------------------
>  2 files changed, 75 insertions(+), 41 deletions(-)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 113320e..b970d7d 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -16,9 +16,61 @@
>  
>  #include "exec/cpu-common.h"
>  #include "qemu/notify.h"
> +#include "qemu/thread.h"
>  
>  /* migration/ram.c */
>  
> +typedef enum RamSaveState {
> +    RAM_SAVE_ERR = 0,
> +    RAM_SAVE_RESET = 1,
> +    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
> +    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
> +    RAM_SAVE_MAX = 4,
> +} RamSaveState;
> +
> +/* State of RAM for migration */
> +typedef struct RAMState {
> +    /* QEMUFile used for this migration */
> +    QEMUFile *f;
> +    /* Last block that we have visited searching for dirty pages */
> +    RAMBlock *last_seen_block;
> +    /* Last block from where we have sent data */
> +    RAMBlock *last_sent_block;
> +    /* Last dirty target page we have sent */
> +    ram_addr_t last_page;
> +    /* last ram version we have seen */
> +    uint32_t last_version;
> +    /* We are in the first round */
> +    bool ram_bulk_stage;
> +    /* How many times we have dirty too many pages */
> +    int dirty_rate_high_cnt;
> +    /* ram save states used for notifiers */
> +    int ram_save_state;
> +    /* these variables are used for bitmap sync */
> +    /* last time we did a full bitmap_sync */
> +    int64_t time_last_bitmap_sync;
> +    /* bytes transferred at start_time */
> +    uint64_t bytes_xfer_prev;
> +    /* number of dirty pages since start_time */
> +    uint64_t num_dirty_pages_period;
> +    /* xbzrle misses since the beginning of the period */
> +    uint64_t xbzrle_cache_miss_prev;
> +    /* number of iterations at the beginning of period */
> +    uint64_t iterations_prev;
> +    /* Iterations since start */
> +    uint64_t iterations;
> +    /* number of dirty bits in the bitmap */
> +    uint64_t migration_dirty_pages;
> +    /* protects modification of the bitmap */
> +    QemuMutex bitmap_mutex;
> +    /* The RAMBlock used in the last src_page_requests */
> +    RAMBlock *last_req_rb;
> +    /* Queue of outstanding page requests from the destination */
> +    QemuMutex src_page_req_mutex;
> +    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
> +} RAMState;

Why move these chunk?  Can it be avoided?

> +
> +void add_ram_save_state_change_notifier(Notifier *notify);
>  void ram_mig_init(void);
>  void qemu_guest_free_page_hint(void *addr, size_t len);
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 237f11e..d45b5a4 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -56,6 +56,9 @@
>  #include "qemu/uuid.h"
>  #include "savevm.h"
>  
> +static NotifierList ram_save_state_notifiers =
> +    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
> +
>  /***********************************************************/
>  /* ram save/restore */
>  
> @@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
>      QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
>  };
>  
> -/* State of RAM for migration */
> -struct RAMState {
> -    /* QEMUFile used for this migration */
> -    QEMUFile *f;
> -    /* Last block that we have visited searching for dirty pages */
> -    RAMBlock *last_seen_block;
> -    /* Last block from where we have sent data */
> -    RAMBlock *last_sent_block;
> -    /* Last dirty target page we have sent */
> -    ram_addr_t last_page;
> -    /* last ram version we have seen */
> -    uint32_t last_version;
> -    /* We are in the first round */
> -    bool ram_bulk_stage;
> -    /* How many times we have dirty too many pages */
> -    int dirty_rate_high_cnt;
> -    /* these variables are used for bitmap sync */
> -    /* last time we did a full bitmap_sync */
> -    int64_t time_last_bitmap_sync;
> -    /* bytes transferred at start_time */
> -    uint64_t bytes_xfer_prev;
> -    /* number of dirty pages since start_time */
> -    uint64_t num_dirty_pages_period;
> -    /* xbzrle misses since the beginning of the period */
> -    uint64_t xbzrle_cache_miss_prev;
> -    /* number of iterations at the beginning of period */
> -    uint64_t iterations_prev;
> -    /* Iterations since start */
> -    uint64_t iterations;
> -    /* number of dirty bits in the bitmap */
> -    uint64_t migration_dirty_pages;
> -    /* protects modification of the bitmap */
> -    QemuMutex bitmap_mutex;
> -    /* The RAMBlock used in the last src_page_requests */
> -    RAMBlock *last_req_rb;
> -    /* Queue of outstanding page requests from the destination */
> -    QemuMutex src_page_req_mutex;
> -    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
> -};
> -typedef struct RAMState RAMState;
> -
>  static RAMState *ram_state;
>  
> +void add_ram_save_state_change_notifier(Notifier *notify)
> +{
> +    notifier_list_add(&ram_save_state_notifiers, notify);
> +}
> +
> +static void notify_ram_save_state_change_notifier(void)
> +{
> +    notifier_list_notify(&ram_save_state_notifiers, ram_state);
> +}
> +
>  uint64_t ram_bytes_remaining(void)
>  {
>      return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
> @@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
>      int64_t end_time;
>      uint64_t bytes_xfer_now;
>  
> +    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;

What's the point to keep this state after all?...

> +    notify_ram_save_state_change_notifier();
> +
>      ram_counters.dirty_sync_count++;
>  
>      if (!rs->time_last_bitmap_sync) {
> @@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
>      if (migrate_use_events()) {
>          qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
>      }
> +
> +    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
> +    notify_ram_save_state_change_notifier();
>  }
>  
>  /**
> @@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
>      rs->last_page = 0;
>      rs->last_version = ram_list.version;
>      rs->ram_bulk_stage = true;
> +    rs->ram_save_state = RAM_SAVE_RESET;

... and this state is much more meaningless afaict ...

> +    notify_ram_save_state_change_notifier();
>  }
>  
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> @@ -2709,6 +2689,8 @@ out:
>  
>      ret = qemu_file_get_error(f);
>      if (ret < 0) {
> +        rs->ram_save_state = RAM_SAVE_ERR;
> +        notify_ram_save_state_change_notifier();
>          return ret;
>      }

Also, I would prefer to add a general event framework for migration
rather than "ram save" specific.  Then we add SYNC_START/SYNC_END
events.  These notifiers aren't a lot, it'll be a bit awkward to
introduce the framework multiple times for similar purpuse (after we
have a common notifier we can merge the postcopy notifiers AFAIU).

Regards,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
  2018-06-12  7:50   ` [Qemu-devel] " Peter Xu
@ 2018-06-12 11:12       ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-12 11:12 UTC (permalink / raw)
  To: Peter Xu
  Cc: qemu-devel, virtio-dev, mst, quintela, dgilbert, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

On 06/12/2018 03:50 PM, Peter Xu wrote:
> On Fri, Jun 08, 2018 at 04:10:41PM +0800, Wei Wang wrote:
>> This patch adds a ram save state notifier list, and expose RAMState for
>> the notifer callbacks to use.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Peter Xu <peterx@redhat.com>
>> ---
>>   include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
>>   migration/ram.c          | 64 +++++++++++++++++-------------------------------
>>   2 files changed, 75 insertions(+), 41 deletions(-)
>>
>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>> index 113320e..b970d7d 100644
>> --- a/include/migration/misc.h
>> +++ b/include/migration/misc.h
>> @@ -16,9 +16,61 @@
>>   
>>   #include "exec/cpu-common.h"
>>   #include "qemu/notify.h"
>> +#include "qemu/thread.h"
>>   
>>   /* migration/ram.c */
>>   
>> +typedef enum RamSaveState {
>> +    RAM_SAVE_ERR = 0,
>> +    RAM_SAVE_RESET = 1,
>> +    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
>> +    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
>> +    RAM_SAVE_MAX = 4,
>> +} RamSaveState;
>> +
>> +/* State of RAM for migration */
>> +typedef struct RAMState {
>> +    /* QEMUFile used for this migration */
>> +    QEMUFile *f;
>> +    /* Last block that we have visited searching for dirty pages */
>> +    RAMBlock *last_seen_block;
>> +    /* Last block from where we have sent data */
>> +    RAMBlock *last_sent_block;
>> +    /* Last dirty target page we have sent */
>> +    ram_addr_t last_page;
>> +    /* last ram version we have seen */
>> +    uint32_t last_version;
>> +    /* We are in the first round */
>> +    bool ram_bulk_stage;
>> +    /* How many times we have dirty too many pages */
>> +    int dirty_rate_high_cnt;
>> +    /* ram save states used for notifiers */
>> +    int ram_save_state;
>> +    /* these variables are used for bitmap sync */
>> +    /* last time we did a full bitmap_sync */
>> +    int64_t time_last_bitmap_sync;
>> +    /* bytes transferred at start_time */
>> +    uint64_t bytes_xfer_prev;
>> +    /* number of dirty pages since start_time */
>> +    uint64_t num_dirty_pages_period;
>> +    /* xbzrle misses since the beginning of the period */
>> +    uint64_t xbzrle_cache_miss_prev;
>> +    /* number of iterations at the beginning of period */
>> +    uint64_t iterations_prev;
>> +    /* Iterations since start */
>> +    uint64_t iterations;
>> +    /* number of dirty bits in the bitmap */
>> +    uint64_t migration_dirty_pages;
>> +    /* protects modification of the bitmap */
>> +    QemuMutex bitmap_mutex;
>> +    /* The RAMBlock used in the last src_page_requests */
>> +    RAMBlock *last_req_rb;
>> +    /* Queue of outstanding page requests from the destination */
>> +    QemuMutex src_page_req_mutex;
>> +    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>> +} RAMState;
> Why move these chunk?  Can it be avoided?
>
>> +
>> +void add_ram_save_state_change_notifier(Notifier *notify);
>>   void ram_mig_init(void);
>>   void qemu_guest_free_page_hint(void *addr, size_t len);
>>   
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 237f11e..d45b5a4 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -56,6 +56,9 @@
>>   #include "qemu/uuid.h"
>>   #include "savevm.h"
>>   
>> +static NotifierList ram_save_state_notifiers =
>> +    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
>> +
>>   /***********************************************************/
>>   /* ram save/restore */
>>   
>> @@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
>>       QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
>>   };
>>   
>> -/* State of RAM for migration */
>> -struct RAMState {
>> -    /* QEMUFile used for this migration */
>> -    QEMUFile *f;
>> -    /* Last block that we have visited searching for dirty pages */
>> -    RAMBlock *last_seen_block;
>> -    /* Last block from where we have sent data */
>> -    RAMBlock *last_sent_block;
>> -    /* Last dirty target page we have sent */
>> -    ram_addr_t last_page;
>> -    /* last ram version we have seen */
>> -    uint32_t last_version;
>> -    /* We are in the first round */
>> -    bool ram_bulk_stage;
>> -    /* How many times we have dirty too many pages */
>> -    int dirty_rate_high_cnt;
>> -    /* these variables are used for bitmap sync */
>> -    /* last time we did a full bitmap_sync */
>> -    int64_t time_last_bitmap_sync;
>> -    /* bytes transferred at start_time */
>> -    uint64_t bytes_xfer_prev;
>> -    /* number of dirty pages since start_time */
>> -    uint64_t num_dirty_pages_period;
>> -    /* xbzrle misses since the beginning of the period */
>> -    uint64_t xbzrle_cache_miss_prev;
>> -    /* number of iterations at the beginning of period */
>> -    uint64_t iterations_prev;
>> -    /* Iterations since start */
>> -    uint64_t iterations;
>> -    /* number of dirty bits in the bitmap */
>> -    uint64_t migration_dirty_pages;
>> -    /* protects modification of the bitmap */
>> -    QemuMutex bitmap_mutex;
>> -    /* The RAMBlock used in the last src_page_requests */
>> -    RAMBlock *last_req_rb;
>> -    /* Queue of outstanding page requests from the destination */
>> -    QemuMutex src_page_req_mutex;
>> -    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>> -};
>> -typedef struct RAMState RAMState;
>> -
>>   static RAMState *ram_state;
>>   
>> +void add_ram_save_state_change_notifier(Notifier *notify)
>> +{
>> +    notifier_list_add(&ram_save_state_notifiers, notify);
>> +}
>> +
>> +static void notify_ram_save_state_change_notifier(void)
>> +{
>> +    notifier_list_notify(&ram_save_state_notifiers, ram_state);
>> +}
>> +
>>   uint64_t ram_bytes_remaining(void)
>>   {
>>       return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
>> @@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>       int64_t end_time;
>>       uint64_t bytes_xfer_now;
>>   
>> +    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;
> What's the point to keep this state after all?...
>
>> +    notify_ram_save_state_change_notifier();
>> +
>>       ram_counters.dirty_sync_count++;
>>   
>>       if (!rs->time_last_bitmap_sync) {
>> @@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>       if (migrate_use_events()) {
>>           qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
>>       }
>> +
>> +    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
>> +    notify_ram_save_state_change_notifier();
>>   }
>>   
>>   /**
>> @@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
>>       rs->last_page = 0;
>>       rs->last_version = ram_list.version;
>>       rs->ram_bulk_stage = true;
>> +    rs->ram_save_state = RAM_SAVE_RESET;
> ... and this state is much more meaningless afaict ...
>
>> +    notify_ram_save_state_change_notifier();
>>   }
>>   
>>   #define MAX_WAIT 50 /* ms, half buffered_file limit */
>> @@ -2709,6 +2689,8 @@ out:
>>   
>>       ret = qemu_file_get_error(f);
>>       if (ret < 0) {
>> +        rs->ram_save_state = RAM_SAVE_ERR;
>> +        notify_ram_save_state_change_notifier();
>>           return ret;
>>       }
> Also, I would prefer to add a general event framework for migration
> rather than "ram save" specific.  Then we add SYNC_START/SYNC_END
> events.  These notifiers aren't a lot, it'll be a bit awkward to
> introduce the framework multiple times for similar purpuse (after we
> have a common notifier we can merge the postcopy notifiers AFAIU).

Hi Peter,

Thanks for the review. We just got a little accident in the kernel part, 
which may cause some modifications to the QEMU side interfaces. So I 
will implement a new version and let you review in v9 patches.

Best,
Wei

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [virtio-dev] Re: [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
@ 2018-06-12 11:12       ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-12 11:12 UTC (permalink / raw)
  To: Peter Xu
  Cc: qemu-devel, virtio-dev, mst, quintela, dgilbert, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

On 06/12/2018 03:50 PM, Peter Xu wrote:
> On Fri, Jun 08, 2018 at 04:10:41PM +0800, Wei Wang wrote:
>> This patch adds a ram save state notifier list, and expose RAMState for
>> the notifer callbacks to use.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Peter Xu <peterx@redhat.com>
>> ---
>>   include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
>>   migration/ram.c          | 64 +++++++++++++++++-------------------------------
>>   2 files changed, 75 insertions(+), 41 deletions(-)
>>
>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>> index 113320e..b970d7d 100644
>> --- a/include/migration/misc.h
>> +++ b/include/migration/misc.h
>> @@ -16,9 +16,61 @@
>>   
>>   #include "exec/cpu-common.h"
>>   #include "qemu/notify.h"
>> +#include "qemu/thread.h"
>>   
>>   /* migration/ram.c */
>>   
>> +typedef enum RamSaveState {
>> +    RAM_SAVE_ERR = 0,
>> +    RAM_SAVE_RESET = 1,
>> +    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
>> +    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
>> +    RAM_SAVE_MAX = 4,
>> +} RamSaveState;
>> +
>> +/* State of RAM for migration */
>> +typedef struct RAMState {
>> +    /* QEMUFile used for this migration */
>> +    QEMUFile *f;
>> +    /* Last block that we have visited searching for dirty pages */
>> +    RAMBlock *last_seen_block;
>> +    /* Last block from where we have sent data */
>> +    RAMBlock *last_sent_block;
>> +    /* Last dirty target page we have sent */
>> +    ram_addr_t last_page;
>> +    /* last ram version we have seen */
>> +    uint32_t last_version;
>> +    /* We are in the first round */
>> +    bool ram_bulk_stage;
>> +    /* How many times we have dirty too many pages */
>> +    int dirty_rate_high_cnt;
>> +    /* ram save states used for notifiers */
>> +    int ram_save_state;
>> +    /* these variables are used for bitmap sync */
>> +    /* last time we did a full bitmap_sync */
>> +    int64_t time_last_bitmap_sync;
>> +    /* bytes transferred at start_time */
>> +    uint64_t bytes_xfer_prev;
>> +    /* number of dirty pages since start_time */
>> +    uint64_t num_dirty_pages_period;
>> +    /* xbzrle misses since the beginning of the period */
>> +    uint64_t xbzrle_cache_miss_prev;
>> +    /* number of iterations at the beginning of period */
>> +    uint64_t iterations_prev;
>> +    /* Iterations since start */
>> +    uint64_t iterations;
>> +    /* number of dirty bits in the bitmap */
>> +    uint64_t migration_dirty_pages;
>> +    /* protects modification of the bitmap */
>> +    QemuMutex bitmap_mutex;
>> +    /* The RAMBlock used in the last src_page_requests */
>> +    RAMBlock *last_req_rb;
>> +    /* Queue of outstanding page requests from the destination */
>> +    QemuMutex src_page_req_mutex;
>> +    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>> +} RAMState;
> Why move these chunk?  Can it be avoided?
>
>> +
>> +void add_ram_save_state_change_notifier(Notifier *notify);
>>   void ram_mig_init(void);
>>   void qemu_guest_free_page_hint(void *addr, size_t len);
>>   
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 237f11e..d45b5a4 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -56,6 +56,9 @@
>>   #include "qemu/uuid.h"
>>   #include "savevm.h"
>>   
>> +static NotifierList ram_save_state_notifiers =
>> +    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
>> +
>>   /***********************************************************/
>>   /* ram save/restore */
>>   
>> @@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
>>       QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
>>   };
>>   
>> -/* State of RAM for migration */
>> -struct RAMState {
>> -    /* QEMUFile used for this migration */
>> -    QEMUFile *f;
>> -    /* Last block that we have visited searching for dirty pages */
>> -    RAMBlock *last_seen_block;
>> -    /* Last block from where we have sent data */
>> -    RAMBlock *last_sent_block;
>> -    /* Last dirty target page we have sent */
>> -    ram_addr_t last_page;
>> -    /* last ram version we have seen */
>> -    uint32_t last_version;
>> -    /* We are in the first round */
>> -    bool ram_bulk_stage;
>> -    /* How many times we have dirty too many pages */
>> -    int dirty_rate_high_cnt;
>> -    /* these variables are used for bitmap sync */
>> -    /* last time we did a full bitmap_sync */
>> -    int64_t time_last_bitmap_sync;
>> -    /* bytes transferred at start_time */
>> -    uint64_t bytes_xfer_prev;
>> -    /* number of dirty pages since start_time */
>> -    uint64_t num_dirty_pages_period;
>> -    /* xbzrle misses since the beginning of the period */
>> -    uint64_t xbzrle_cache_miss_prev;
>> -    /* number of iterations at the beginning of period */
>> -    uint64_t iterations_prev;
>> -    /* Iterations since start */
>> -    uint64_t iterations;
>> -    /* number of dirty bits in the bitmap */
>> -    uint64_t migration_dirty_pages;
>> -    /* protects modification of the bitmap */
>> -    QemuMutex bitmap_mutex;
>> -    /* The RAMBlock used in the last src_page_requests */
>> -    RAMBlock *last_req_rb;
>> -    /* Queue of outstanding page requests from the destination */
>> -    QemuMutex src_page_req_mutex;
>> -    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>> -};
>> -typedef struct RAMState RAMState;
>> -
>>   static RAMState *ram_state;
>>   
>> +void add_ram_save_state_change_notifier(Notifier *notify)
>> +{
>> +    notifier_list_add(&ram_save_state_notifiers, notify);
>> +}
>> +
>> +static void notify_ram_save_state_change_notifier(void)
>> +{
>> +    notifier_list_notify(&ram_save_state_notifiers, ram_state);
>> +}
>> +
>>   uint64_t ram_bytes_remaining(void)
>>   {
>>       return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
>> @@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>       int64_t end_time;
>>       uint64_t bytes_xfer_now;
>>   
>> +    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;
> What's the point to keep this state after all?...
>
>> +    notify_ram_save_state_change_notifier();
>> +
>>       ram_counters.dirty_sync_count++;
>>   
>>       if (!rs->time_last_bitmap_sync) {
>> @@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>       if (migrate_use_events()) {
>>           qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
>>       }
>> +
>> +    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
>> +    notify_ram_save_state_change_notifier();
>>   }
>>   
>>   /**
>> @@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
>>       rs->last_page = 0;
>>       rs->last_version = ram_list.version;
>>       rs->ram_bulk_stage = true;
>> +    rs->ram_save_state = RAM_SAVE_RESET;
> ... and this state is much more meaningless afaict ...
>
>> +    notify_ram_save_state_change_notifier();
>>   }
>>   
>>   #define MAX_WAIT 50 /* ms, half buffered_file limit */
>> @@ -2709,6 +2689,8 @@ out:
>>   
>>       ret = qemu_file_get_error(f);
>>       if (ret < 0) {
>> +        rs->ram_save_state = RAM_SAVE_ERR;
>> +        notify_ram_save_state_change_notifier();
>>           return ret;
>>       }
> Also, I would prefer to add a general event framework for migration
> rather than "ram save" specific.  Then we add SYNC_START/SYNC_END
> events.  These notifiers aren't a lot, it'll be a bit awkward to
> introduce the framework multiple times for similar purpuse (after we
> have a common notifier we can merge the postcopy notifiers AFAIU).

Hi Peter,

Thanks for the review. We just got a little accident in the kernel part, 
which may cause some modifications to the QEMU side interfaces. So I 
will implement a new version and let you review in v9 patches.

Best,
Wei


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
  2018-06-12 11:12       ` [virtio-dev] " Wei Wang
  (?)
@ 2018-06-13  2:01       ` Peter Xu
  -1 siblings, 0 replies; 22+ messages in thread
From: Peter Xu @ 2018-06-13  2:01 UTC (permalink / raw)
  To: Wei Wang
  Cc: qemu-devel, virtio-dev, mst, quintela, dgilbert, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

On Tue, Jun 12, 2018 at 07:12:47PM +0800, Wei Wang wrote:

[...]

> Hi Peter,
> 
> Thanks for the review. We just got a little accident in the kernel part,
> which may cause some modifications to the QEMU side interfaces. So I will
> implement a new version and let you review in v9 patches.

No problem.  Yeah I actually noticed the accident.  Please feel free
to postpone the QEMU part until the kernel part is fully settled.

Regards,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
  2018-06-12 11:12       ` [virtio-dev] " Wei Wang
  (?)
  (?)
@ 2018-06-19 17:31       ` Dr. David Alan Gilbert
  2018-06-20 11:15           ` [virtio-dev] " Wei Wang
  -1 siblings, 1 reply; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2018-06-19 17:31 UTC (permalink / raw)
  To: Wei Wang
  Cc: Peter Xu, qemu-devel, virtio-dev, mst, quintela, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

* Wei Wang (wei.w.wang@intel.com) wrote:
> On 06/12/2018 03:50 PM, Peter Xu wrote:
> > On Fri, Jun 08, 2018 at 04:10:41PM +0800, Wei Wang wrote:
> > > This patch adds a ram save state notifier list, and expose RAMState for
> > > the notifer callbacks to use.
> > > 
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > CC: Juan Quintela <quintela@redhat.com>
> > > CC: Michael S. Tsirkin <mst@redhat.com>
> > > CC: Peter Xu <peterx@redhat.com>
> > > ---
> > >   include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
> > >   migration/ram.c          | 64 +++++++++++++++++-------------------------------
> > >   2 files changed, 75 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > > index 113320e..b970d7d 100644
> > > --- a/include/migration/misc.h
> > > +++ b/include/migration/misc.h
> > > @@ -16,9 +16,61 @@
> > >   #include "exec/cpu-common.h"
> > >   #include "qemu/notify.h"
> > > +#include "qemu/thread.h"
> > >   /* migration/ram.c */
> > > +typedef enum RamSaveState {
> > > +    RAM_SAVE_ERR = 0,
> > > +    RAM_SAVE_RESET = 1,
> > > +    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
> > > +    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
> > > +    RAM_SAVE_MAX = 4,
> > > +} RamSaveState;
> > > +
> > > +/* State of RAM for migration */
> > > +typedef struct RAMState {
> > > +    /* QEMUFile used for this migration */
> > > +    QEMUFile *f;
> > > +    /* Last block that we have visited searching for dirty pages */
> > > +    RAMBlock *last_seen_block;
> > > +    /* Last block from where we have sent data */
> > > +    RAMBlock *last_sent_block;
> > > +    /* Last dirty target page we have sent */
> > > +    ram_addr_t last_page;
> > > +    /* last ram version we have seen */
> > > +    uint32_t last_version;
> > > +    /* We are in the first round */
> > > +    bool ram_bulk_stage;
> > > +    /* How many times we have dirty too many pages */
> > > +    int dirty_rate_high_cnt;
> > > +    /* ram save states used for notifiers */
> > > +    int ram_save_state;
> > > +    /* these variables are used for bitmap sync */
> > > +    /* last time we did a full bitmap_sync */
> > > +    int64_t time_last_bitmap_sync;
> > > +    /* bytes transferred at start_time */
> > > +    uint64_t bytes_xfer_prev;
> > > +    /* number of dirty pages since start_time */
> > > +    uint64_t num_dirty_pages_period;
> > > +    /* xbzrle misses since the beginning of the period */
> > > +    uint64_t xbzrle_cache_miss_prev;
> > > +    /* number of iterations at the beginning of period */
> > > +    uint64_t iterations_prev;
> > > +    /* Iterations since start */
> > > +    uint64_t iterations;
> > > +    /* number of dirty bits in the bitmap */
> > > +    uint64_t migration_dirty_pages;
> > > +    /* protects modification of the bitmap */
> > > +    QemuMutex bitmap_mutex;
> > > +    /* The RAMBlock used in the last src_page_requests */
> > > +    RAMBlock *last_req_rb;
> > > +    /* Queue of outstanding page requests from the destination */
> > > +    QemuMutex src_page_req_mutex;
> > > +    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
> > > +} RAMState;
> > Why move these chunk?  Can it be avoided?
> > 
> > > +
> > > +void add_ram_save_state_change_notifier(Notifier *notify);
> > >   void ram_mig_init(void);
> > >   void qemu_guest_free_page_hint(void *addr, size_t len);
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 237f11e..d45b5a4 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -56,6 +56,9 @@
> > >   #include "qemu/uuid.h"
> > >   #include "savevm.h"
> > > +static NotifierList ram_save_state_notifiers =
> > > +    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
> > > +
> > >   /***********************************************************/
> > >   /* ram save/restore */
> > > @@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
> > >       QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
> > >   };
> > > -/* State of RAM for migration */
> > > -struct RAMState {
> > > -    /* QEMUFile used for this migration */
> > > -    QEMUFile *f;
> > > -    /* Last block that we have visited searching for dirty pages */
> > > -    RAMBlock *last_seen_block;
> > > -    /* Last block from where we have sent data */
> > > -    RAMBlock *last_sent_block;
> > > -    /* Last dirty target page we have sent */
> > > -    ram_addr_t last_page;
> > > -    /* last ram version we have seen */
> > > -    uint32_t last_version;
> > > -    /* We are in the first round */
> > > -    bool ram_bulk_stage;
> > > -    /* How many times we have dirty too many pages */
> > > -    int dirty_rate_high_cnt;
> > > -    /* these variables are used for bitmap sync */
> > > -    /* last time we did a full bitmap_sync */
> > > -    int64_t time_last_bitmap_sync;
> > > -    /* bytes transferred at start_time */
> > > -    uint64_t bytes_xfer_prev;
> > > -    /* number of dirty pages since start_time */
> > > -    uint64_t num_dirty_pages_period;
> > > -    /* xbzrle misses since the beginning of the period */
> > > -    uint64_t xbzrle_cache_miss_prev;
> > > -    /* number of iterations at the beginning of period */
> > > -    uint64_t iterations_prev;
> > > -    /* Iterations since start */
> > > -    uint64_t iterations;
> > > -    /* number of dirty bits in the bitmap */
> > > -    uint64_t migration_dirty_pages;
> > > -    /* protects modification of the bitmap */
> > > -    QemuMutex bitmap_mutex;
> > > -    /* The RAMBlock used in the last src_page_requests */
> > > -    RAMBlock *last_req_rb;
> > > -    /* Queue of outstanding page requests from the destination */
> > > -    QemuMutex src_page_req_mutex;
> > > -    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
> > > -};
> > > -typedef struct RAMState RAMState;
> > > -
> > >   static RAMState *ram_state;
> > > +void add_ram_save_state_change_notifier(Notifier *notify)
> > > +{
> > > +    notifier_list_add(&ram_save_state_notifiers, notify);
> > > +}
> > > +
> > > +static void notify_ram_save_state_change_notifier(void)
> > > +{
> > > +    notifier_list_notify(&ram_save_state_notifiers, ram_state);
> > > +}
> > > +
> > >   uint64_t ram_bytes_remaining(void)
> > >   {
> > >       return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
> > > @@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
> > >       int64_t end_time;
> > >       uint64_t bytes_xfer_now;
> > > +    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;
> > What's the point to keep this state after all?...
> > 
> > > +    notify_ram_save_state_change_notifier();
> > > +
> > >       ram_counters.dirty_sync_count++;
> > >       if (!rs->time_last_bitmap_sync) {
> > > @@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
> > >       if (migrate_use_events()) {
> > >           qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
> > >       }
> > > +
> > > +    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
> > > +    notify_ram_save_state_change_notifier();
> > >   }
> > >   /**
> > > @@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
> > >       rs->last_page = 0;
> > >       rs->last_version = ram_list.version;
> > >       rs->ram_bulk_stage = true;
> > > +    rs->ram_save_state = RAM_SAVE_RESET;
> > ... and this state is much more meaningless afaict ...
> > 
> > > +    notify_ram_save_state_change_notifier();
> > >   }
> > >   #define MAX_WAIT 50 /* ms, half buffered_file limit */
> > > @@ -2709,6 +2689,8 @@ out:
> > >       ret = qemu_file_get_error(f);
> > >       if (ret < 0) {
> > > +        rs->ram_save_state = RAM_SAVE_ERR;
> > > +        notify_ram_save_state_change_notifier();
> > >           return ret;
> > >       }
> > Also, I would prefer to add a general event framework for migration
> > rather than "ram save" specific.  Then we add SYNC_START/SYNC_END
> > events.  These notifiers aren't a lot, it'll be a bit awkward to
> > introduce the framework multiple times for similar purpuse (after we
> > have a common notifier we can merge the postcopy notifiers AFAIU).
> 
> Hi Peter,
> 
> Thanks for the review. We just got a little accident in the kernel part,
> which may cause some modifications to the QEMU side interfaces. So I will
> implement a new version and let you review in v9 patches.

Yes, but please do try and keep RAMState private to ram.c.

Dave

> Best,
> Wei
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
  2018-06-19 17:31       ` Dr. David Alan Gilbert
@ 2018-06-20 11:15           ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-20 11:15 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Peter Xu, qemu-devel, virtio-dev, mst, quintela, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

On 06/20/2018 01:31 AM, Dr. David Alan Gilbert wrote:
> * Wei Wang (wei.w.wang@intel.com) wrote:
>> On 06/12/2018 03:50 PM, Peter Xu wrote:
>>> On Fri, Jun 08, 2018 at 04:10:41PM +0800, Wei Wang wrote:
>>>> This patch adds a ram save state notifier list, and expose RAMState for
>>>> the notifer callbacks to use.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> CC: Juan Quintela <quintela@redhat.com>
>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>> CC: Peter Xu <peterx@redhat.com>
>>>> ---
>>>>    include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
>>>>    migration/ram.c          | 64 +++++++++++++++++-------------------------------
>>>>    2 files changed, 75 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>>> index 113320e..b970d7d 100644
>>>> --- a/include/migration/misc.h
>>>> +++ b/include/migration/misc.h
>>>> @@ -16,9 +16,61 @@
>>>>    #include "exec/cpu-common.h"
>>>>    #include "qemu/notify.h"
>>>> +#include "qemu/thread.h"
>>>>    /* migration/ram.c */
>>>> +typedef enum RamSaveState {
>>>> +    RAM_SAVE_ERR = 0,
>>>> +    RAM_SAVE_RESET = 1,
>>>> +    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
>>>> +    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
>>>> +    RAM_SAVE_MAX = 4,
>>>> +} RamSaveState;
>>>> +
>>>> +/* State of RAM for migration */
>>>> +typedef struct RAMState {
>>>> +    /* QEMUFile used for this migration */
>>>> +    QEMUFile *f;
>>>> +    /* Last block that we have visited searching for dirty pages */
>>>> +    RAMBlock *last_seen_block;
>>>> +    /* Last block from where we have sent data */
>>>> +    RAMBlock *last_sent_block;
>>>> +    /* Last dirty target page we have sent */
>>>> +    ram_addr_t last_page;
>>>> +    /* last ram version we have seen */
>>>> +    uint32_t last_version;
>>>> +    /* We are in the first round */
>>>> +    bool ram_bulk_stage;
>>>> +    /* How many times we have dirty too many pages */
>>>> +    int dirty_rate_high_cnt;
>>>> +    /* ram save states used for notifiers */
>>>> +    int ram_save_state;
>>>> +    /* these variables are used for bitmap sync */
>>>> +    /* last time we did a full bitmap_sync */
>>>> +    int64_t time_last_bitmap_sync;
>>>> +    /* bytes transferred at start_time */
>>>> +    uint64_t bytes_xfer_prev;
>>>> +    /* number of dirty pages since start_time */
>>>> +    uint64_t num_dirty_pages_period;
>>>> +    /* xbzrle misses since the beginning of the period */
>>>> +    uint64_t xbzrle_cache_miss_prev;
>>>> +    /* number of iterations at the beginning of period */
>>>> +    uint64_t iterations_prev;
>>>> +    /* Iterations since start */
>>>> +    uint64_t iterations;
>>>> +    /* number of dirty bits in the bitmap */
>>>> +    uint64_t migration_dirty_pages;
>>>> +    /* protects modification of the bitmap */
>>>> +    QemuMutex bitmap_mutex;
>>>> +    /* The RAMBlock used in the last src_page_requests */
>>>> +    RAMBlock *last_req_rb;
>>>> +    /* Queue of outstanding page requests from the destination */
>>>> +    QemuMutex src_page_req_mutex;
>>>> +    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>>>> +} RAMState;
>>> Why move these chunk?  Can it be avoided?
>>>
>>>> +
>>>> +void add_ram_save_state_change_notifier(Notifier *notify);
>>>>    void ram_mig_init(void);
>>>>    void qemu_guest_free_page_hint(void *addr, size_t len);
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index 237f11e..d45b5a4 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -56,6 +56,9 @@
>>>>    #include "qemu/uuid.h"
>>>>    #include "savevm.h"
>>>> +static NotifierList ram_save_state_notifiers =
>>>> +    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
>>>> +
>>>>    /***********************************************************/
>>>>    /* ram save/restore */
>>>> @@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
>>>>        QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
>>>>    };
>>>> -/* State of RAM for migration */
>>>> -struct RAMState {
>>>> -    /* QEMUFile used for this migration */
>>>> -    QEMUFile *f;
>>>> -    /* Last block that we have visited searching for dirty pages */
>>>> -    RAMBlock *last_seen_block;
>>>> -    /* Last block from where we have sent data */
>>>> -    RAMBlock *last_sent_block;
>>>> -    /* Last dirty target page we have sent */
>>>> -    ram_addr_t last_page;
>>>> -    /* last ram version we have seen */
>>>> -    uint32_t last_version;
>>>> -    /* We are in the first round */
>>>> -    bool ram_bulk_stage;
>>>> -    /* How many times we have dirty too many pages */
>>>> -    int dirty_rate_high_cnt;
>>>> -    /* these variables are used for bitmap sync */
>>>> -    /* last time we did a full bitmap_sync */
>>>> -    int64_t time_last_bitmap_sync;
>>>> -    /* bytes transferred at start_time */
>>>> -    uint64_t bytes_xfer_prev;
>>>> -    /* number of dirty pages since start_time */
>>>> -    uint64_t num_dirty_pages_period;
>>>> -    /* xbzrle misses since the beginning of the period */
>>>> -    uint64_t xbzrle_cache_miss_prev;
>>>> -    /* number of iterations at the beginning of period */
>>>> -    uint64_t iterations_prev;
>>>> -    /* Iterations since start */
>>>> -    uint64_t iterations;
>>>> -    /* number of dirty bits in the bitmap */
>>>> -    uint64_t migration_dirty_pages;
>>>> -    /* protects modification of the bitmap */
>>>> -    QemuMutex bitmap_mutex;
>>>> -    /* The RAMBlock used in the last src_page_requests */
>>>> -    RAMBlock *last_req_rb;
>>>> -    /* Queue of outstanding page requests from the destination */
>>>> -    QemuMutex src_page_req_mutex;
>>>> -    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>>>> -};
>>>> -typedef struct RAMState RAMState;
>>>> -
>>>>    static RAMState *ram_state;
>>>> +void add_ram_save_state_change_notifier(Notifier *notify)
>>>> +{
>>>> +    notifier_list_add(&ram_save_state_notifiers, notify);
>>>> +}
>>>> +
>>>> +static void notify_ram_save_state_change_notifier(void)
>>>> +{
>>>> +    notifier_list_notify(&ram_save_state_notifiers, ram_state);
>>>> +}
>>>> +
>>>>    uint64_t ram_bytes_remaining(void)
>>>>    {
>>>>        return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
>>>> @@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>>>        int64_t end_time;
>>>>        uint64_t bytes_xfer_now;
>>>> +    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;
>>> What's the point to keep this state after all?...
>>>
>>>> +    notify_ram_save_state_change_notifier();
>>>> +
>>>>        ram_counters.dirty_sync_count++;
>>>>        if (!rs->time_last_bitmap_sync) {
>>>> @@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>>>        if (migrate_use_events()) {
>>>>            qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
>>>>        }
>>>> +
>>>> +    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
>>>> +    notify_ram_save_state_change_notifier();
>>>>    }
>>>>    /**
>>>> @@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
>>>>        rs->last_page = 0;
>>>>        rs->last_version = ram_list.version;
>>>>        rs->ram_bulk_stage = true;
>>>> +    rs->ram_save_state = RAM_SAVE_RESET;
>>> ... and this state is much more meaningless afaict ...
>>>
>>>> +    notify_ram_save_state_change_notifier();
>>>>    }
>>>>    #define MAX_WAIT 50 /* ms, half buffered_file limit */
>>>> @@ -2709,6 +2689,8 @@ out:
>>>>        ret = qemu_file_get_error(f);
>>>>        if (ret < 0) {
>>>> +        rs->ram_save_state = RAM_SAVE_ERR;
>>>> +        notify_ram_save_state_change_notifier();
>>>>            return ret;
>>>>        }
>>> Also, I would prefer to add a general event framework for migration
>>> rather than "ram save" specific.  Then we add SYNC_START/SYNC_END
>>> events.  These notifiers aren't a lot, it'll be a bit awkward to
>>> introduce the framework multiple times for similar purpuse (after we
>>> have a common notifier we can merge the postcopy notifiers AFAIU).
>> Hi Peter,
>>
>> Thanks for the review. We just got a little accident in the kernel part,
>> which may cause some modifications to the QEMU side interfaces. So I will
>> implement a new version and let you review in v9 patches.
> Yes, but please do try and keep RAMState private to ram.c.
>

OK, will use functions to expose the related fields (e.g. ram_bulk_stage).

Best,
Wei

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [virtio-dev] Re: [PATCH v8 4/6] migration/ram.c: add ram save state notifiers
@ 2018-06-20 11:15           ` Wei Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Wei Wang @ 2018-06-20 11:15 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Peter Xu, qemu-devel, virtio-dev, mst, quintela, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

On 06/20/2018 01:31 AM, Dr. David Alan Gilbert wrote:
> * Wei Wang (wei.w.wang@intel.com) wrote:
>> On 06/12/2018 03:50 PM, Peter Xu wrote:
>>> On Fri, Jun 08, 2018 at 04:10:41PM +0800, Wei Wang wrote:
>>>> This patch adds a ram save state notifier list, and expose RAMState for
>>>> the notifer callbacks to use.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> CC: Juan Quintela <quintela@redhat.com>
>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>> CC: Peter Xu <peterx@redhat.com>
>>>> ---
>>>>    include/migration/misc.h | 52 +++++++++++++++++++++++++++++++++++++++
>>>>    migration/ram.c          | 64 +++++++++++++++++-------------------------------
>>>>    2 files changed, 75 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>>> index 113320e..b970d7d 100644
>>>> --- a/include/migration/misc.h
>>>> +++ b/include/migration/misc.h
>>>> @@ -16,9 +16,61 @@
>>>>    #include "exec/cpu-common.h"
>>>>    #include "qemu/notify.h"
>>>> +#include "qemu/thread.h"
>>>>    /* migration/ram.c */
>>>> +typedef enum RamSaveState {
>>>> +    RAM_SAVE_ERR = 0,
>>>> +    RAM_SAVE_RESET = 1,
>>>> +    RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
>>>> +    RAM_SAVE_AFTER_SYNC_BITMAP = 3,
>>>> +    RAM_SAVE_MAX = 4,
>>>> +} RamSaveState;
>>>> +
>>>> +/* State of RAM for migration */
>>>> +typedef struct RAMState {
>>>> +    /* QEMUFile used for this migration */
>>>> +    QEMUFile *f;
>>>> +    /* Last block that we have visited searching for dirty pages */
>>>> +    RAMBlock *last_seen_block;
>>>> +    /* Last block from where we have sent data */
>>>> +    RAMBlock *last_sent_block;
>>>> +    /* Last dirty target page we have sent */
>>>> +    ram_addr_t last_page;
>>>> +    /* last ram version we have seen */
>>>> +    uint32_t last_version;
>>>> +    /* We are in the first round */
>>>> +    bool ram_bulk_stage;
>>>> +    /* How many times we have dirty too many pages */
>>>> +    int dirty_rate_high_cnt;
>>>> +    /* ram save states used for notifiers */
>>>> +    int ram_save_state;
>>>> +    /* these variables are used for bitmap sync */
>>>> +    /* last time we did a full bitmap_sync */
>>>> +    int64_t time_last_bitmap_sync;
>>>> +    /* bytes transferred at start_time */
>>>> +    uint64_t bytes_xfer_prev;
>>>> +    /* number of dirty pages since start_time */
>>>> +    uint64_t num_dirty_pages_period;
>>>> +    /* xbzrle misses since the beginning of the period */
>>>> +    uint64_t xbzrle_cache_miss_prev;
>>>> +    /* number of iterations at the beginning of period */
>>>> +    uint64_t iterations_prev;
>>>> +    /* Iterations since start */
>>>> +    uint64_t iterations;
>>>> +    /* number of dirty bits in the bitmap */
>>>> +    uint64_t migration_dirty_pages;
>>>> +    /* protects modification of the bitmap */
>>>> +    QemuMutex bitmap_mutex;
>>>> +    /* The RAMBlock used in the last src_page_requests */
>>>> +    RAMBlock *last_req_rb;
>>>> +    /* Queue of outstanding page requests from the destination */
>>>> +    QemuMutex src_page_req_mutex;
>>>> +    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>>>> +} RAMState;
>>> Why move these chunk?  Can it be avoided?
>>>
>>>> +
>>>> +void add_ram_save_state_change_notifier(Notifier *notify);
>>>>    void ram_mig_init(void);
>>>>    void qemu_guest_free_page_hint(void *addr, size_t len);
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index 237f11e..d45b5a4 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -56,6 +56,9 @@
>>>>    #include "qemu/uuid.h"
>>>>    #include "savevm.h"
>>>> +static NotifierList ram_save_state_notifiers =
>>>> +    NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
>>>> +
>>>>    /***********************************************************/
>>>>    /* ram save/restore */
>>>> @@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
>>>>        QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
>>>>    };
>>>> -/* State of RAM for migration */
>>>> -struct RAMState {
>>>> -    /* QEMUFile used for this migration */
>>>> -    QEMUFile *f;
>>>> -    /* Last block that we have visited searching for dirty pages */
>>>> -    RAMBlock *last_seen_block;
>>>> -    /* Last block from where we have sent data */
>>>> -    RAMBlock *last_sent_block;
>>>> -    /* Last dirty target page we have sent */
>>>> -    ram_addr_t last_page;
>>>> -    /* last ram version we have seen */
>>>> -    uint32_t last_version;
>>>> -    /* We are in the first round */
>>>> -    bool ram_bulk_stage;
>>>> -    /* How many times we have dirty too many pages */
>>>> -    int dirty_rate_high_cnt;
>>>> -    /* these variables are used for bitmap sync */
>>>> -    /* last time we did a full bitmap_sync */
>>>> -    int64_t time_last_bitmap_sync;
>>>> -    /* bytes transferred at start_time */
>>>> -    uint64_t bytes_xfer_prev;
>>>> -    /* number of dirty pages since start_time */
>>>> -    uint64_t num_dirty_pages_period;
>>>> -    /* xbzrle misses since the beginning of the period */
>>>> -    uint64_t xbzrle_cache_miss_prev;
>>>> -    /* number of iterations at the beginning of period */
>>>> -    uint64_t iterations_prev;
>>>> -    /* Iterations since start */
>>>> -    uint64_t iterations;
>>>> -    /* number of dirty bits in the bitmap */
>>>> -    uint64_t migration_dirty_pages;
>>>> -    /* protects modification of the bitmap */
>>>> -    QemuMutex bitmap_mutex;
>>>> -    /* The RAMBlock used in the last src_page_requests */
>>>> -    RAMBlock *last_req_rb;
>>>> -    /* Queue of outstanding page requests from the destination */
>>>> -    QemuMutex src_page_req_mutex;
>>>> -    QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
>>>> -};
>>>> -typedef struct RAMState RAMState;
>>>> -
>>>>    static RAMState *ram_state;
>>>> +void add_ram_save_state_change_notifier(Notifier *notify)
>>>> +{
>>>> +    notifier_list_add(&ram_save_state_notifiers, notify);
>>>> +}
>>>> +
>>>> +static void notify_ram_save_state_change_notifier(void)
>>>> +{
>>>> +    notifier_list_notify(&ram_save_state_notifiers, ram_state);
>>>> +}
>>>> +
>>>>    uint64_t ram_bytes_remaining(void)
>>>>    {
>>>>        return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
>>>> @@ -1139,6 +1111,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>>>        int64_t end_time;
>>>>        uint64_t bytes_xfer_now;
>>>> +    rs->ram_save_state = RAM_SAVE_BEFORE_SYNC_BITMAP;
>>> What's the point to keep this state after all?...
>>>
>>>> +    notify_ram_save_state_change_notifier();
>>>> +
>>>>        ram_counters.dirty_sync_count++;
>>>>        if (!rs->time_last_bitmap_sync) {
>>>> @@ -1205,6 +1180,9 @@ static void migration_bitmap_sync(RAMState *rs)
>>>>        if (migrate_use_events()) {
>>>>            qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
>>>>        }
>>>> +
>>>> +    rs->ram_save_state = RAM_SAVE_AFTER_SYNC_BITMAP;
>>>> +    notify_ram_save_state_change_notifier();
>>>>    }
>>>>    /**
>>>> @@ -1961,6 +1939,8 @@ static void ram_state_reset(RAMState *rs)
>>>>        rs->last_page = 0;
>>>>        rs->last_version = ram_list.version;
>>>>        rs->ram_bulk_stage = true;
>>>> +    rs->ram_save_state = RAM_SAVE_RESET;
>>> ... and this state is much more meaningless afaict ...
>>>
>>>> +    notify_ram_save_state_change_notifier();
>>>>    }
>>>>    #define MAX_WAIT 50 /* ms, half buffered_file limit */
>>>> @@ -2709,6 +2689,8 @@ out:
>>>>        ret = qemu_file_get_error(f);
>>>>        if (ret < 0) {
>>>> +        rs->ram_save_state = RAM_SAVE_ERR;
>>>> +        notify_ram_save_state_change_notifier();
>>>>            return ret;
>>>>        }
>>> Also, I would prefer to add a general event framework for migration
>>> rather than "ram save" specific.  Then we add SYNC_START/SYNC_END
>>> events.  These notifiers aren't a lot, it'll be a bit awkward to
>>> introduce the framework multiple times for similar purpuse (after we
>>> have a common notifier we can merge the postcopy notifiers AFAIU).
>> Hi Peter,
>>
>> Thanks for the review. We just got a little accident in the kernel part,
>> which may cause some modifications to the QEMU side interfaces. So I will
>> implement a new version and let you review in v9 patches.
> Yes, but please do try and keep RAMState private to ram.c.
>

OK, will use functions to expose the related fields (e.g. ram_bulk_stage).

Best,
Wei



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Qemu-devel] [PATCH v8 5/6] migration: move migrate_postcopy() to include/migration/misc.h
  2018-06-08  8:10   ` [virtio-dev] " Wei Wang
  (?)
@ 2018-06-29 10:35   ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2018-06-29 10:35 UTC (permalink / raw)
  To: Wei Wang
  Cc: qemu-devel, virtio-dev, mst, quintela, peterx, pbonzini,
	liliang.opensource, yang.zhang.wz, quan.xu0, nilal, riel

* Wei Wang (wei.w.wang@intel.com) wrote:
> The ram save state notifier callback, for example the free page
> optimization offerred by virtio-balloon, may need to check if
> postcopy is in use, so move migrate_postcopy() to the outside header.

Reasonable,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h | 1 +
>  migration/migration.h    | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index b970d7d..911aaf3 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -109,6 +109,7 @@ bool migration_has_failed(MigrationState *);
>  /* ...and after the device transmission */
>  bool migration_in_postcopy_after_devices(MigrationState *);
>  void migration_global_dump(Monitor *mon);
> +bool migrate_postcopy(void);
>  
>  /* migration/block-dirty-bitmap.c */
>  void dirty_bitmap_mig_init(void);
> diff --git a/migration/migration.h b/migration/migration.h
> index 5a74740..fee5af8 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -236,8 +236,6 @@ bool migration_is_blocked(Error **errp);
>  bool migration_in_postcopy(void);
>  MigrationState *migrate_get_current(void);
>  
> -bool migrate_postcopy(void);
> -
>  bool migrate_release_ram(void);
>  bool migrate_postcopy_ram(void);
>  bool migrate_zero_blocks(void);
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2018-06-29 10:35 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-08  8:10 [Qemu-devel] [PATCH v8 0/6] virtio-balloon: free page hint reporting support Wei Wang
2018-06-08  8:10 ` [virtio-dev] " Wei Wang
2018-06-08  8:10 ` [Qemu-devel] [PATCH v8 1/6] bitmap: bitmap_count_one_with_offset Wei Wang
2018-06-08  8:10   ` [virtio-dev] " Wei Wang
2018-06-08  8:10 ` [Qemu-devel] [PATCH v8 2/6] migration: use bitmap_mutex in migration_bitmap_clear_dirty Wei Wang
2018-06-08  8:10   ` [virtio-dev] " Wei Wang
2018-06-08  8:10 ` [Qemu-devel] [PATCH v8 3/6] migration: API to clear bits of guest free pages from the dirty bitmap Wei Wang
2018-06-08  8:10   ` [virtio-dev] " Wei Wang
2018-06-08  8:10 ` [Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers Wei Wang
2018-06-08  8:10   ` [virtio-dev] " Wei Wang
2018-06-12  7:50   ` [Qemu-devel] " Peter Xu
2018-06-12 11:12     ` Wei Wang
2018-06-12 11:12       ` [virtio-dev] " Wei Wang
2018-06-13  2:01       ` [Qemu-devel] " Peter Xu
2018-06-19 17:31       ` Dr. David Alan Gilbert
2018-06-20 11:15         ` Wei Wang
2018-06-20 11:15           ` [virtio-dev] " Wei Wang
2018-06-08  8:10 ` [Qemu-devel] [PATCH v8 5/6] migration: move migrate_postcopy() to include/migration/misc.h Wei Wang
2018-06-08  8:10   ` [virtio-dev] " Wei Wang
2018-06-29 10:35   ` [Qemu-devel] " Dr. David Alan Gilbert
2018-06-08  8:10 ` [Qemu-devel] [PATCH v8 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT Wei Wang
2018-06-08  8:10   ` [virtio-dev] " Wei Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.