[Qemu-devel] [PATCH v3 0/2] Remove old MigrationParams

[Qemu-devel] [PATCH v3 0/2] Remove old MigrationParams

[Juan Quintela]

Hi

changes from v2
- Patch 1 included in pull request
- disable block_shared when we disable block_enable
- enable block_enabled when we enable block_shared

Please, review.

[v2]

- make migrate_block_set_* take a boolean
- disable block migration in colo to maintain semantics.

[v1]
Upon a time there were MigrationParms (only used for block migration)
and then MigrationParams used for everything else.  This series:

- create migration capabilities for block parameters
- make the migrate command line parameters to use capabilities
- remove MigrationParams completely


Juan Quintela (2):
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 include/migration/migration.h | 11 ++---------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/block.c             | 17 ++---------------
 migration/colo.c              |  7 ++++---
 migration/migration.c         | 22 ++++++++++++++--------
 migration/savevm.c            | 18 +++---------------
 8 files changed, 26 insertions(+), 54 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/2] migration: Remove use of old MigrationParams

[Juan Quintela]

We have change in the previous patch to use migration capabilities for
it.  Notice that we continue using the old command line flags from
migrate command from the time being.  Remove the set_params method as
now it is empty.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

--
- Maintain shared/enabled dependency (Xu suggestien)
---
 include/migration/migration.h |  3 +--
 migration/block.c             | 17 ++---------------
 migration/colo.c              |  5 +++--
 migration/migration.c         | 14 +++++++++++---
 migration/savevm.c            |  2 --
 5 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 258e4ff..d228f29 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -43,8 +43,7 @@
 extern int only_migratable;
 
 struct MigrationParams {
-    bool blk;
-    bool shared;
+    bool unused; /* C doesn't allow empty structs */
 };
 
 /* Messages sent on the return path from destination to source */
diff --git a/migration/block.c b/migration/block.c
index 060087f..fcfa823 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
 } BlkMigBlock;
 
 typedef struct BlkMigState {
-    /* Written during setup phase.  Can be read without a lock.  */
-    int blk_enable;
-    int shared_base;
     QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
     int64_t total_sector_sum;
     bool zero_blocks;
@@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
         bmds->bulk_completed = 0;
         bmds->total_sectors = sectors;
         bmds->completed_sectors = 0;
-        bmds->shared_base = block_mig_state.shared_base;
+        bmds->shared_base = migrate_use_block_shared();
 
         assert(i < num_bs);
         bmds_bs[i].bmds = bmds;
@@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static void block_set_params(const MigrationParams *params, void *opaque)
-{
-    block_mig_state.blk_enable = params->blk;
-    block_mig_state.shared_base = params->shared;
-
-    /* shared base means that blk_enable = 1 */
-    block_mig_state.blk_enable |= params->shared;
-}
-
 static bool block_is_active(void *opaque)
 {
-    return block_mig_state.blk_enable == 1;
+    return migrate_use_block_enabled();
 }
 
 static SaveVMHandlers savevm_block_handlers = {
-    .set_params = block_set_params,
     .save_live_setup = block_save_setup,
     .save_live_iterate = block_save_iterate,
     .save_live_complete_precopy = block_save_complete,
diff --git a/migration/colo.c b/migration/colo.c
index 963c802..e772384 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -14,6 +14,7 @@
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "migration/colo.h"
+#include "migration/block.h"
 #include "io/channel-buffer.h"
 #include "trace.h"
 #include "qemu/error-report.h"
@@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     /* Disable block migration */
-    s->params.blk = 0;
-    s->params.shared = 0;
+    migrate_set_block_enabled(s, false);
+    migrate_set_block_shared(s, false);
     qemu_savevm_state_header(fb);
     qemu_savevm_state_begin(fb, &s->params);
     qemu_mutex_lock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 78102eb..0c32609 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -788,6 +788,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
         s->enabled_capabilities[cap->value->capability] = cap->value->state;
     }
 
+    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+    }
+
     if (migrate_postcopy_ram()) {
         if (migrate_use_compression()) {
             /* The decompression threads asynchronously write into RAM
@@ -1214,11 +1218,17 @@ bool migration_is_blocked(Error **errp)
 void migrate_set_block_shared(MigrationState *s, bool value)
 {
     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
+    if (value) {
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+    }
 }
 
 void migrate_set_block_enabled(MigrationState *s, bool value)
 {
     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = value;
+    if (!value) {
+        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = false;
+    }
 }
 
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
@@ -1230,9 +1240,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationParams params;
     const char *p;
 
-    params.blk = has_blk && blk;
-    params.shared = has_inc && inc;
-
     if (migration_is_setup_or_active(s->state) ||
         s->state == MIGRATION_STATUS_CANCELLING ||
         s->state == MIGRATION_STATUS_COLO) {
@@ -1255,6 +1262,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     }
 
     if (has_inc && inc) {
+        migrate_set_block_enabled(s, true);
         migrate_set_block_shared(s, true);
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index b2fd06f..8aaefe9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
-        .blk = 0,
-        .shared = 0
     };
     MigrationState *ms = migrate_init(&params);
     MigrationStatus status;
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/2] migration: Remove use of old MigrationParams

[Peter Xu]

On Fri, May 12, 2017 at 05:54:48PM +0200, Juan Quintela wrote:
> We have change in the previous patch to use migration capabilities for
> it.  Notice that we continue using the old command line flags from
> migrate command from the time being.  Remove the set_params method as
> now it is empty.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> 
> --
> - Maintain shared/enabled dependency (Xu suggestien)
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c             | 17 ++---------------
>  migration/colo.c              |  5 +++--
>  migration/migration.c         | 14 +++++++++++---
>  migration/savevm.c            |  2 --
>  5 files changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 258e4ff..d228f29 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -43,8 +43,7 @@
>  extern int only_migratable;
>  
>  struct MigrationParams {
> -    bool blk;
> -    bool shared;
> +    bool unused; /* C doesn't allow empty structs */
>  };
>  
>  /* Messages sent on the return path from destination to source */
> diff --git a/migration/block.c b/migration/block.c
> index 060087f..fcfa823 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -94,9 +94,6 @@ typedef struct BlkMigBlock {
>  } BlkMigBlock;
>  
>  typedef struct BlkMigState {
> -    /* Written during setup phase.  Can be read without a lock.  */
> -    int blk_enable;
> -    int shared_base;
>      QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
>      int64_t total_sector_sum;
>      bool zero_blocks;
> @@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f)
>          bmds->bulk_completed = 0;
>          bmds->total_sectors = sectors;
>          bmds->completed_sectors = 0;
> -        bmds->shared_base = block_mig_state.shared_base;
> +        bmds->shared_base = migrate_use_block_shared();
>  
>          assert(i < num_bs);
>          bmds_bs[i].bmds = bmds;
> @@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> -static void block_set_params(const MigrationParams *params, void *opaque)
> -{
> -    block_mig_state.blk_enable = params->blk;
> -    block_mig_state.shared_base = params->shared;
> -
> -    /* shared base means that blk_enable = 1 */
> -    block_mig_state.blk_enable |= params->shared;
> -}
> -
>  static bool block_is_active(void *opaque)
>  {
> -    return block_mig_state.blk_enable == 1;
> +    return migrate_use_block_enabled();
>  }
>  
>  static SaveVMHandlers savevm_block_handlers = {
> -    .set_params = block_set_params,
>      .save_live_setup = block_save_setup,
>      .save_live_iterate = block_save_iterate,
>      .save_live_complete_precopy = block_save_complete,
> diff --git a/migration/colo.c b/migration/colo.c
> index 963c802..e772384 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -14,6 +14,7 @@
>  #include "qemu/timer.h"
>  #include "sysemu/sysemu.h"
>  #include "migration/colo.h"
> +#include "migration/block.h"
>  #include "io/channel-buffer.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
> @@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      }
>  
>      /* Disable block migration */
> -    s->params.blk = 0;
> -    s->params.shared = 0;
> +    migrate_set_block_enabled(s, false);
> +    migrate_set_block_shared(s, false);
>      qemu_savevm_state_header(fb);
>      qemu_savevm_state_begin(fb, &s->params);
>      qemu_mutex_lock_iothread();
> diff --git a/migration/migration.c b/migration/migration.c
> index 78102eb..0c32609 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -788,6 +788,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>          s->enabled_capabilities[cap->value->capability] = cap->value->state;
>      }
>  
> +    if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) {
> +        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
> +    }
> +
>      if (migrate_postcopy_ram()) {
>          if (migrate_use_compression()) {
>              /* The decompression threads asynchronously write into RAM
> @@ -1214,11 +1218,17 @@ bool migration_is_blocked(Error **errp)
>  void migrate_set_block_shared(MigrationState *s, bool value)
>  {
>      s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
> +    if (value) {
> +        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
> +    }
>  }
>  
>  void migrate_set_block_enabled(MigrationState *s, bool value)
>  {
>      s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = value;
> +    if (!value) {
> +        s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = false;
> +    }
>  }
>  
>  void qmp_migrate(const char *uri, bool has_blk, bool blk,
> @@ -1230,9 +1240,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationParams params;
>      const char *p;
>  
> -    params.blk = has_blk && blk;
> -    params.shared = has_inc && inc;
> -
>      if (migration_is_setup_or_active(s->state) ||
>          s->state == MIGRATION_STATUS_CANCELLING ||
>          s->state == MIGRATION_STATUS_COLO) {
> @@ -1255,6 +1262,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      }
>  
>      if (has_inc && inc) {
> +        migrate_set_block_enabled(s, true);

nit: We don't need this line now?

Either with/without above line:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

>          migrate_set_block_shared(s, true);
>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b2fd06f..8aaefe9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  {
>      int ret;
>      MigrationParams params = {
> -        .blk = 0,
> -        .shared = 0
>      };
>      MigrationState *ms = migrate_init(&params);
>      MigrationStatus status;
> -- 
> 2.9.3
> 

-- 
Peter Xu

[Qemu-devel] [PATCH 2/2] migration: Remove old MigrationParams

[Juan Quintela]

Not used anymore after moving block migration to use capabilities.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 include/migration/migration.h | 10 ++--------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/colo.c              |  2 +-
 migration/migration.c         |  8 +++-----
 migration/savevm.c            | 16 +++-------------
 7 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index d228f29..1e23536 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -42,10 +42,6 @@
 /* for vl.c */
 extern int only_migratable;
 
-struct MigrationParams {
-    bool unused; /* C doesn't allow empty structs */
-};
-
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
     MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
@@ -135,12 +131,10 @@ struct MigrationState
     QEMUBH *cleanup_bh;
     QEMUFile *to_dst_file;
 
-    /* New style params from 'migrate-set-parameters' */
+    /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
 
     int state;
-    /* Old style params from 'migrate' command */
-    MigrationParams params;
 
     /* State related to return path */
     struct {
@@ -230,7 +224,7 @@ void migrate_fd_connect(MigrationState *s);
 
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-MigrationState *migrate_init(const MigrationParams *params);
+MigrationState *migrate_init(void);
 bool migration_is_blocked(Error **errp);
 bool migration_in_setup(MigrationState *);
 bool migration_is_idle(void);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index f4bf3f1..81af5fb 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -37,7 +37,6 @@ typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 
 typedef struct SaveVMHandlers {
     /* This runs inside the iothread lock.  */
-    void (*set_params)(const MigrationParams *params, void * opaque);
     SaveStateHandler *save_state;
 
     void (*cleanup)(void *opaque);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index f08d327..a388243 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -49,7 +49,6 @@ typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionCache MemoryRegionCache;
 typedef struct MemoryRegionSection MemoryRegionSection;
 typedef struct MigrationIncomingState MigrationIncomingState;
-typedef struct MigrationParams MigrationParams;
 typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 058d5eb..3340202 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -102,8 +102,7 @@ enum qemu_vm_cmd {
 #define MAX_VM_CMD_PACKAGED_SIZE (1ul << 24)
 
 bool qemu_savevm_state_blocked(Error **errp);
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params);
+void qemu_savevm_state_begin(QEMUFile *f);
 void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
diff --git a/migration/colo.c b/migration/colo.c
index e772384..430bae5 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -349,7 +349,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     migrate_set_block_enabled(s, false);
     migrate_set_block_shared(s, false);
     qemu_savevm_state_header(fb);
-    qemu_savevm_state_begin(fb, &s->params);
+    qemu_savevm_state_begin(fb);
     qemu_mutex_lock_iothread();
     qemu_savevm_state_complete_precopy(fb, false);
     qemu_mutex_unlock_iothread();
diff --git a/migration/migration.c b/migration/migration.c
index 0c32609..a6a9922 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1103,7 +1103,7 @@ bool migration_is_idle(void)
     return false;
 }
 
-MigrationState *migrate_init(const MigrationParams *params)
+MigrationState *migrate_init(void)
 {
     MigrationState *s = migrate_get_current();
 
@@ -1117,7 +1117,6 @@ MigrationState *migrate_init(const MigrationParams *params)
     s->cleanup_bh = 0;
     s->to_dst_file = NULL;
     s->state = MIGRATION_STATUS_NONE;
-    s->params = *params;
     s->rp_state.from_dst_file = NULL;
     s->rp_state.error = false;
     s->mbps = 0.0;
@@ -1237,7 +1236,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    MigrationParams params;
     const char *p;
 
     if (migration_is_setup_or_active(s->state) ||
@@ -1255,7 +1253,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
-    s = migrate_init(&params);
+    s = migrate_init();
 
     if (has_blk && blk) {
         migrate_set_block_enabled(s, true);
@@ -1988,7 +1986,7 @@ static void *migration_thread(void *opaque)
         qemu_savevm_send_postcopy_advise(s->to_dst_file);
     }
 
-    qemu_savevm_state_begin(s->to_dst_file, &s->params);
+    qemu_savevm_state_begin(s->to_dst_file);
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
     migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
diff --git a/migration/savevm.c b/migration/savevm.c
index 8aaefe9..b24946f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -966,21 +966,13 @@ void qemu_savevm_state_header(QEMUFile *f)
 
 }
 
-void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params)
+void qemu_savevm_state_begin(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret;
 
     trace_savevm_state_begin();
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!se->ops || !se->ops->set_params) {
-            continue;
-        }
-        se->ops->set_params(params, se->opaque);
-    }
-
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->save_live_setup) {
             continue;
         }
@@ -1232,9 +1224,7 @@ void qemu_savevm_state_cleanup(void)
 static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
-    MigrationParams params = {
-    };
-    MigrationState *ms = migrate_init(&params);
+    MigrationState *ms = migrate_init();
     MigrationStatus status;
     ms->to_dst_file = f;
 
@@ -1245,7 +1235,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     qemu_mutex_unlock_iothread();
     qemu_savevm_state_header(f);
-    qemu_savevm_state_begin(f, &params);
+    qemu_savevm_state_begin(f);
     qemu_mutex_lock_iothread();
 
     while (qemu_file_get_error(f) == 0) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/2] migration: Remove old MigrationParams

[Peter Xu]

On Fri, May 12, 2017 at 05:54:49PM +0200, Juan Quintela wrote:
> Not used anymore after moving block migration to use capabilities.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Peter Xu <peterx@redhat.com>