[Qemu-devel] [PATCH 0/3] Remove old MigrationParams

[Qemu-devel] [PATCH 0/3] Remove old MigrationParams

[Juan Quintela]

Hi

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

Please, review.


Juan Quintela (3):
  migration: Create block capabilities for shared and enable
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 include/migration/migration.h | 14 +++++-------
 include/migration/vmstate.h   |  1 -
 include/qemu/typedefs.h       |  1 -
 include/sysemu/sysemu.h       |  3 +--
 migration/block.c             | 17 ++------------
 migration/colo.c              |  5 +----
 migration/migration.c         | 52 ++++++++++++++++++++++++++++++++++++-------
 migration/savevm.c            | 18 +++------------
 qapi-schema.json              |  7 +++++-
 9 files changed, 62 insertions(+), 56 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Those two capabilities were added through the command line.  Notice that
we just created them.  This is just the boilerplate.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/migration/migration.h |  3 +++
 migration/migration.c         | 36 ++++++++++++++++++++++++++++++++++++
 qapi-schema.json              |  7 ++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index dfeca38..618ab0e 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -307,6 +307,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block_enabled(void);
+bool migrate_use_block_shared(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index 5447cab..775b24c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1195,6 +1195,16 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
+static void migrate_set_block_shared(MigrationState *s)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = true;
+}
+
+static void migrate_set_block_enabled(MigrationState *s)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true;
+}
+
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
@@ -1224,6 +1234,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
     s = migrate_init(&params);
 
+    if (has_blk && blk) {
+        migrate_set_block_enabled(s);
+    }
+
+    if (has_inc && inc) {
+        migrate_set_block_shared(s);
+    }
+
     if (strstart(uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
@@ -1419,6 +1437,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block_enabled(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED];
+}
+
+bool migrate_use_block_shared(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 01b087f..e963bb3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,16 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block-enabled: enable block migration (Since 2.10)
+#
+# @block-shared: enable block shared migration (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block-enabled', 'block-shared' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.9.3

[Qemu-devel] [PATCH 2/3] 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>
---
 include/migration/migration.h |  3 +--
 migration/block.c             | 17 ++---------------
 migration/colo.c              |  3 ---
 migration/migration.c         |  8 +++++---
 migration/savevm.c            |  2 --
 5 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 618ab0e..2917baa 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -42,8 +42,7 @@
 extern int only_migratable;
 
 struct MigrationParams {
-    bool blk;
-    bool shared;
+    bool unused; /* C don'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 c19eb3f..5c6c2f0 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
         goto out;
     }
 
-    /* Disable block migration */
-    s->params.blk = 0;
-    s->params.shared = 0;
     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 775b24c..9b96f1a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -787,6 +787,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,9 +1218,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) {
@@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     }
 
     if (has_inc && inc) {
+        migrate_set_block_enabled(s);
         migrate_set_block_shared(s);
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 0c01988..102b11d 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 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) 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>
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c             | 17 ++---------------
>  migration/colo.c              |  3 ---
>  migration/migration.c         |  8 +++++---
>  migration/savevm.c            |  2 --
>  5 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index c19eb3f..5c6c2f0 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>          goto out;
>      }
>  
> -    /* Disable block migration */
> -    s->params.blk = 0;
> -    s->params.shared = 0;

Hmm you don't seem to have replaced this with anything.
I think that's a behavioural change; the trick COLO did (I'm not sure if this
is still the way it works) is that they initiate the first migration
with block migration enabled so that the two hosts (with non-shared storage)
get sync'd storage, and then at the completion of that first migration
they then switch into the checkpointing mode where they're only
doing updates - that's why it gets switched off at this point
prior to the 1st checkpoint.

Dave

>      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 775b24c..9b96f1a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -787,6 +787,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,9 +1218,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) {
> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      }
>  
>      if (has_inc && inc) {
> +        migrate_set_block_enabled(s);
>          migrate_set_block_shared(s);
>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0c01988..102b11d 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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) 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>
>> ---
>>  include/migration/migration.h |  3 +--
>>  migration/block.c             | 17 ++---------------
>>  migration/colo.c              |  3 ---
>>  migration/migration.c         |  8 +++++---
>>  migration/savevm.c            |  2 --
>>  5 files changed, 8 insertions(+), 25 deletions(-)
>> 
>> diff --git a/migration/colo.c b/migration/colo.c
>> index c19eb3f..5c6c2f0 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>>          goto out;
>>      }
>>  
>> -    /* Disable block migration */
>> -    s->params.blk = 0;
>> -    s->params.shared = 0;
>
> Hmm you don't seem to have replaced this with anything.
> I think that's a behavioural change; the trick COLO did (I'm not sure if this
> is still the way it works) is that they initiate the first migration
> with block migration enabled so that the two hosts (with non-shared storage)
> get sync'd storage, and then at the completion of that first migration
> they then switch into the checkpointing mode where they're only
> doing updates - that's why it gets switched off at this point
> prior to the 1st checkpoint.


Weird, really.

I did't catch that.

Will investigate.

Thanks.

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

[Hailiang Zhang]

Hi,

On 2017/5/4 16:51, Juan Quintela wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> * Juan Quintela (quintela@redhat.com) 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>
>>> ---
>>>   include/migration/migration.h |  3 +--
>>>   migration/block.c             | 17 ++---------------
>>>   migration/colo.c              |  3 ---
>>>   migration/migration.c         |  8 +++++---
>>>   migration/savevm.c            |  2 --
>>>   5 files changed, 8 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/migration/colo.c b/migration/colo.c
>>> index c19eb3f..5c6c2f0 100644
>>> --- a/migration/colo.c
>>> +++ b/migration/colo.c
>>> @@ -332,9 +332,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>>>           goto out;
>>>       }
>>>   
>>> -    /* Disable block migration */
>>> -    s->params.blk = 0;
>>> -    s->params.shared = 0;
>> Hmm you don't seem to have replaced this with anything.
>> I think that's a behavioural change; the trick COLO did (I'm not sure if this
>> is still the way it works) is that they initiate the first migration
>> with block migration enabled so that the two hosts (with non-shared storage)
>> get sync'd storage, and then at the completion of that first migration
>> they then switch into the checkpointing mode where they're only
>> doing updates - that's why it gets switched off at this point
>> prior to the 1st checkpoint.
>
> Weird, really.
>
> I did't catch that.
>
> Will investigate.

Yes, Dave is right, for non-shared disk, we need to enable block migration for first cycle,
to sync the disks of two sides. After that, qemu will go into COLO state which we need to
disable block migration.

Thanks,
Hailiang


> Thanks.
>
> .
>

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

[Juan Quintela]

Hailiang Zhang <zhang.zhanghailiang@huawei.com> wrote:
> Hi,
>
>>> Hmm you don't seem to have replaced this with anything.
>>> I think that's a behavioural change; the trick COLO did (I'm not sure if this
>>> is still the way it works) is that they initiate the first migration
>>> with block migration enabled so that the two hosts (with non-shared storage)
>>> get sync'd storage, and then at the completion of that first migration
>>> they then switch into the checkpointing mode where they're only
>>> doing updates - that's why it gets switched off at this point
>>> prior to the 1st checkpoint.
>>
>> Weird, really.
>>
>> I did't catch that.
>>
>> Will investigate.
>
> Yes, Dave is right, for non-shared disk, we need to enable block
> migration for first cycle,
> to sync the disks of two sides. After that, qemu will go into COLO
> state which we need to
> disable block migration.

v2 posted.

My understanding is that it maintains the sematic, please test/comment.

Thanks, Juan.

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

[Hailiang Zhang]

On 2017/5/12 0:33, Juan Quintela wrote:
> Hailiang Zhang <zhang.zhanghailiang@huawei.com> wrote:
>> Hi,
>>
>>>> Hmm you don't seem to have replaced this with anything.
>>>> I think that's a behavioural change; the trick COLO did (I'm not sure if this
>>>> is still the way it works) is that they initiate the first migration
>>>> with block migration enabled so that the two hosts (with non-shared storage)
>>>> get sync'd storage, and then at the completion of that first migration
>>>> they then switch into the checkpointing mode where they're only
>>>> doing updates - that's why it gets switched off at this point
>>>> prior to the 1st checkpoint.
>>> Weird, really.
>>>
>>> I did't catch that.
>>>
>>> Will investigate.
>> Yes, Dave is right, for non-shared disk, we need to enable block
>> migration for first cycle,
>> to sync the disks of two sides. After that, qemu will go into COLO
>> state which we need to
>> disable block migration.
> v2 posted.
>
> My understanding is that it maintains the sematic, please test/comment.

Yes, it is right now, i have reviewed it, thanks.

> Thanks, Juan.
>
> .
>

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

On 04/25/2017 05:30 AM, 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>
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c             | 17 ++---------------
>  migration/colo.c              |  3 ---
>  migration/migration.c         |  8 +++++---
>  migration/savevm.c            |  2 --
>  5 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 618ab0e..2917baa 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -42,8 +42,7 @@
>  extern int only_migratable;
>  
>  struct MigrationParams {
> -    bool blk;
> -    bool shared;
> +    bool unused; /* C don't allow empty structs */

s/don't/doesn't/

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Juan Quintela]

Not used anymore after moving block migration to use capabilities.

Signed-off-by: Juan Quintela <quintela@redhat.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 2917baa..3495162 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -41,10 +41,6 @@
 /* for vl.c */
 extern int only_migratable;
 
-struct MigrationParams {
-    bool unused; /* C don'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 */
@@ -134,12 +130,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 {
@@ -229,7 +223,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 9452dec..4396d7e 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 5c6c2f0..75e8807 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -333,7 +333,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     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 9b96f1a..f094079 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1102,7 +1102,7 @@ bool migration_is_idle(void)
     return false;
 }
 
-MigrationState *migrate_init(const MigrationParams *params)
+MigrationState *migrate_init(void)
 {
     MigrationState *s = migrate_get_current();
 
@@ -1116,7 +1116,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;
@@ -1215,7 +1214,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) ||
@@ -1233,7 +1231,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);
@@ -1966,7 +1964,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 102b11d..97c6908 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 3/3] migration: Remove old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) 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>

> ---
>  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 2917baa..3495162 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -41,10 +41,6 @@
>  /* for vl.c */
>  extern int only_migratable;
>  
> -struct MigrationParams {
> -    bool unused; /* C don'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 */
> @@ -134,12 +130,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 {
> @@ -229,7 +223,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 9452dec..4396d7e 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 5c6c2f0..75e8807 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -333,7 +333,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      }
>  
>      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 9b96f1a..f094079 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1102,7 +1102,7 @@ bool migration_is_idle(void)
>      return false;
>  }
>  
> -MigrationState *migrate_init(const MigrationParams *params)
> +MigrationState *migrate_init(void)
>  {
>      MigrationState *s = migrate_get_current();
>  
> @@ -1116,7 +1116,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;
> @@ -1215,7 +1214,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) ||
> @@ -1233,7 +1231,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);
> @@ -1966,7 +1964,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 102b11d..97c6908 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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2 0/3] Remove old MigrationParams

[Juan Quintela]

Hi

Changes from v1:

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

Please review, Juan.

[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

Please, review.


*** BLURB HERE ***

Juan Quintela (3):
  migration: Create block capabilities for shared and enable
  migration: Remove use of old MigrationParams
  migration: Remove old MigrationParams

 include/migration/block.h     |  3 +++
 include/migration/migration.h | 14 +++++-------
 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         | 52 ++++++++++++++++++++++++++++++++++++-------
 migration/savevm.c            | 18 +++------------
 qapi-schema.json              |  7 +++++-
 10 files changed, 68 insertions(+), 55 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Those two capabilities were added through the command line.  Notice that
we just created them.  This is just the boilerplate.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

--

Make migrate_set_block_* take a boolean argument.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/block.h     |  3 +++
 include/migration/migration.h |  3 +++
 migration/migration.c         | 36 ++++++++++++++++++++++++++++++++++++
 qapi-schema.json              |  7 ++++++-
 4 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/migration/block.h b/include/migration/block.h
index 41a1ac8..5b3f1a5 100644
--- a/include/migration/block.h
+++ b/include/migration/block.h
@@ -20,4 +20,7 @@ uint64_t blk_mig_bytes_transferred(void);
 uint64_t blk_mig_bytes_remaining(void);
 uint64_t blk_mig_bytes_total(void);
 
+void migrate_set_block_shared(MigrationState *s, bool value);
+void migrate_set_block_enabled(MigrationState *s, bool value);
+
 #endif /* MIGRATION_BLOCK_H */
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 0c9b6af..30c2913 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -305,6 +305,9 @@ bool migrate_colo_enabled(void);
 
 int64_t xbzrle_cache_resize(int64_t new_size);
 
+bool migrate_use_block_enabled(void);
+bool migrate_use_block_shared(void);
+
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
diff --git a/migration/migration.c b/migration/migration.c
index fe62f15..2f981aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1195,6 +1195,16 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
+void migrate_set_block_shared(MigrationState *s, bool value)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
+}
+
+void migrate_set_block_enabled(MigrationState *s, bool value)
+{
+    s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = value;
+}
+
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
@@ -1224,6 +1234,14 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
     s = migrate_init(&params);
 
+    if (has_blk && blk) {
+        migrate_set_block_enabled(s, true);
+    }
+
+    if (has_inc && inc) {
+        migrate_set_block_shared(s, true);
+    }
+
     if (strstart(uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
@@ -1419,6 +1437,24 @@ int64_t migrate_xbzrle_cache_size(void)
     return s->xbzrle_cache_size;
 }
 
+bool migrate_use_block_enabled(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED];
+}
+
+bool migrate_use_block_shared(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/qapi-schema.json b/qapi-schema.json
index 5728b7f..109852e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -894,11 +894,16 @@
 # @release-ram: if enabled, qemu will free the migrated ram pages on the source
 #        during postcopy-ram migration. (since 2.9)
 #
+# @block-enabled: enable block migration (Since 2.10)
+#
+# @block-shared: enable block shared migration (Since 2.10)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
-           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
+           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+           'block-enabled', 'block-shared' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On 05/11/2017 11:32 AM, Juan Quintela wrote:
> Those two capabilities were added through the command line.  Notice that
> we just created them.  This is just the boilerplate.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> --
> 
> Make migrate_set_block_* take a boolean argument.

Question - do we support the orthogonal selection of all 4 combinations
under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
together), or are there only 3 actual states? If the latter, should we
represent this as a single enum-valued property, rather than as two
independent boolean properties?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>> Those two capabilities were added through the command line.  Notice that
>> we just created them.  This is just the boilerplate.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 
>> --
>> 
>> Make migrate_set_block_* take a boolean argument.
>
> Question - do we support the orthogonal selection of all 4 combinations
> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
> together), or are there only 3 actual states? If the latter, should we
> represent this as a single enum-valued property, rather than as two
> independent boolean properties?

{ 'enum': 'MigrationCapability',
  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
           'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }


My understanding is that we can only have boolean capabilities here.
Or, how could we put a non-boolean capability?

There are three states as far as I can see.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Eric Blake <eblake@redhat.com> wrote:
> > On 05/11/2017 11:32 AM, Juan Quintela wrote:
> >> Those two capabilities were added through the command line.  Notice that
> >> we just created them.  This is just the boilerplate.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> 
> >> --
> >> 
> >> Make migrate_set_block_* take a boolean argument.
> >
> > Question - do we support the orthogonal selection of all 4 combinations
> > under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
> > together), or are there only 3 actual states? If the latter, should we
> > represent this as a single enum-valued property, rather than as two
> > independent boolean properties?
> 
> { 'enum': 'MigrationCapability',
>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
> 
> 
> My understanding is that we can only have boolean capabilities here.
> Or, how could we put a non-boolean capability?

Lets keep this simple and stick with the booleans.

Dave

> There are three states as far as I can see.
> 
> Later, Juan.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Eric Blake <eblake@redhat.com> wrote:
>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>> Those two capabilities were added through the command line.  Notice that
>>>> we just created them.  This is just the boilerplate.
>>>>
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>
>>>> --
>>>>
>>>> Make migrate_set_block_* take a boolean argument.
>>>
>>> Question - do we support the orthogonal selection of all 4 combinations
>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>> together), or are there only 3 actual states? If the latter, should we
>>> represent this as a single enum-valued property, rather than as two
>>> independent boolean properties?
>>
>> { 'enum': 'MigrationCapability',
>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>
>>
>> My understanding is that we can only have boolean capabilities here.
>> Or, how could we put a non-boolean capability?

If we want a non-boolean, then we make it a migration parameter rather
than a migration capability.  There may be other advantages to using
MigrationParameter instead of MigrationCapability (such as making it
easier to figure out whether the parameter settings are persistent or
apply per-migration).

> 
> Lets keep this simple and stick with the booleans.
> 
> Dave
> 
>> There are three states as far as I can see.

I'll leave it up to you as maintainers which way you prefer, I'm just
offering the potential design tradeoffs for simplicity of booleans (but
complexity in an unused state) vs. simplicity of design (but complexity
in code).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> Eric Blake <eblake@redhat.com> wrote:
>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>> Those two capabilities were added through the command line.  Notice that
>>>>> we just created them.  This is just the boilerplate.
>>>>>
>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>
>>>>> --
>>>>>
>>>>> Make migrate_set_block_* take a boolean argument.
>>>>
>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>> together), or are there only 3 actual states? If the latter, should we
>>>> represent this as a single enum-valued property, rather than as two
>>>> independent boolean properties?
>>>
>>> { 'enum': 'MigrationCapability',
>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>
>>>
>>> My understanding is that we can only have boolean capabilities here.
>>> Or, how could we put a non-boolean capability?
>
> If we want a non-boolean, then we make it a migration parameter rather
> than a migration capability.  There may be other advantages to using
> MigrationParameter instead of MigrationCapability (such as making it
> easier to figure out whether the parameter settings are persistent or
> apply per-migration).

What makes a migration knob a MigrationCapability rather than a
MigrationParameter?  Type bool, or is there more to it?

>> 
>> Lets keep this simple and stick with the booleans.
>> 
>> Dave
>> 
>>> There are three states as far as I can see.

Begs the question how the fourth state behaves.  Documentation is of no
help:

    diff --git a/qapi-schema.json b/qapi-schema.json
    index 5728b7f..109852e 100644
    --- a/qapi-schema.json
    +++ b/qapi-schema.json
    @@ -894,11 +894,16 @@
     # @release-ram: if enabled, qemu will free the migrated ram pages on the source
     #        during postcopy-ram migration. (since 2.9)
     #
    +# @block-enabled: enable block migration (Since 2.10)
    +#
    +# @block-shared: enable block shared migration (Since 2.10)
    +#
     # Since: 1.2
     ##

Please explain all four states clearly there.

> I'll leave it up to you as maintainers which way you prefer, I'm just
> offering the potential design tradeoffs for simplicity of booleans (but
> complexity in an unused state) vs. simplicity of design (but complexity
> in code).

For what it's worth, I dislike entangled booleans.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>>> * Juan Quintela (quintela@redhat.com) wrote:
>>>> Eric Blake <eblake@redhat.com> wrote:
>>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>>> Those two capabilities were added through the command line.  Notice that
>>>>>> we just created them.  This is just the boilerplate.
>>>>>>
>>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> Make migrate_set_block_* take a boolean argument.
>>>>>
>>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>>> together), or are there only 3 actual states? If the latter, should we
>>>>> represent this as a single enum-valued property, rather than as two
>>>>> independent boolean properties?
>>>>
>>>> { 'enum': 'MigrationCapability',
>>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>>
>>>>
>>>> My understanding is that we can only have boolean capabilities here.
>>>> Or, how could we put a non-boolean capability?
>>
>> If we want a non-boolean, then we make it a migration parameter rather
>> than a migration capability.  There may be other advantages to using
>> MigrationParameter instead of MigrationCapability (such as making it
>> easier to figure out whether the parameter settings are persistent or
>> apply per-migration).
>
> What makes a migration knob a MigrationCapability rather than a
> MigrationParameter?  Type bool, or is there more to it?

I didn't started this, but *my* undersanding:

Migration capability: we have to set this up before migration starts.
It is like a property that we can't change later.  We can't add a
compression thread in the middle of migration (notice that we "could"
but current code can't).

So, block enabled migration is a capability.

Block shared on the other hand is weird.  *My* understading of the code
is that we have a qcow2 overlay on top of the base image, and:

- without shared: we migrate all the block device
- with shared: we migrate only the top overlay

So, thisk could be a parameter of block migration.  If anyone understand
better than me, please stand up.

>>> Lets keep this simple and stick with the booleans.
>>> 
>>> Dave
>>> 
>>>> There are three states as far as I can see.
>
> Begs the question how the fourth state behaves.  Documentation is of no
> help:
>
>     diff --git a/qapi-schema.json b/qapi-schema.json
>     index 5728b7f..109852e 100644
>     --- a/qapi-schema.json
>     +++ b/qapi-schema.json
>     @@ -894,11 +894,16 @@
>      # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>      #        during postcopy-ram migration. (since 2.9)
>      #
>     +# @block-enabled: enable block migration (Since 2.10)
>     +#
>     +# @block-shared: enable block shared migration (Since 2.10)
>     +#
>      # Since: 1.2
>      ##
>
> Please explain all four states clearly there.

Is the previous explanation enough?

>> I'll leave it up to you as maintainers which way you prefer, I'm just
>> offering the potential design tradeoffs for simplicity of booleans (but
>> complexity in an unused state) vs. simplicity of design (but complexity
>> in code).
>
> For what it's worth, I dislike entangled booleans.

Some here, but except if we put shared as a migration parameter, I don't
know what to do here.

{ 'enum': 'MigrationParameter',
  'data': ['compress-level', 'compress-threads', 'decompress-threads',
           'cpu-throttle-initial', 'cpu-throttle-increment',
           'tls-creds', 'tls-hostname', 'max-bandwidth',
           'downtime-limit', 'x-checkpoint-delay' ] }


I don't see either how to define that block_shared will be a boolean parameter.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>>>> * Juan Quintela (quintela@redhat.com) wrote:
>>>>> Eric Blake <eblake@redhat.com> wrote:
>>>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>>>> Those two capabilities were added through the command line.  Notice that
>>>>>>> we just created them.  This is just the boilerplate.
>>>>>>>
>>>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Make migrate_set_block_* take a boolean argument.
>>>>>>
>>>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>>>> together), or are there only 3 actual states? If the latter, should we
>>>>>> represent this as a single enum-valued property, rather than as two
>>>>>> independent boolean properties?
>>>>>
>>>>> { 'enum': 'MigrationCapability',
>>>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>>>
>>>>>
>>>>> My understanding is that we can only have boolean capabilities here.
>>>>> Or, how could we put a non-boolean capability?
>>>
>>> If we want a non-boolean, then we make it a migration parameter rather
>>> than a migration capability.  There may be other advantages to using
>>> MigrationParameter instead of MigrationCapability (such as making it
>>> easier to figure out whether the parameter settings are persistent or
>>> apply per-migration).
>>
>> What makes a migration knob a MigrationCapability rather than a
>> MigrationParameter?  Type bool, or is there more to it?
>
> I didn't started this, but *my* undersanding:
>
> Migration capability: we have to set this up before migration starts.
> It is like a property that we can't change later.  We can't add a
> compression thread in the middle of migration (notice that we "could"
> but current code can't).
>
> So, block enabled migration is a capability.
>
> Block shared on the other hand is weird.  *My* understading of the code
> is that we have a qcow2 overlay on top of the base image, and:
>
> - without shared: we migrate all the block device
> - with shared: we migrate only the top overlay

Plausible.  The thing that's "shared" is the backing file, between
migration source and destination.

However, we can't change "shared" in the middle of migration, either.

> So, thisk could be a parameter of block migration.  If anyone understand
> better than me, please stand up.
>
>>>> Lets keep this simple and stick with the booleans.
>>>> 
>>>> Dave
>>>> 
>>>>> There are three states as far as I can see.
>>
>> Begs the question how the fourth state behaves.  Documentation is of no
>> help:
>>
>>     diff --git a/qapi-schema.json b/qapi-schema.json
>>     index 5728b7f..109852e 100644
>>     --- a/qapi-schema.json
>>     +++ b/qapi-schema.json
>>     @@ -894,11 +894,16 @@
>>      # @release-ram: if enabled, qemu will free the migrated ram pages on the source
>>      #        during postcopy-ram migration. (since 2.9)
>>      #
>>     +# @block-enabled: enable block migration (Since 2.10)
>>     +#
>>     +# @block-shared: enable block shared migration (Since 2.10)
>>     +#
>>      # Since: 1.2
>>      ##
>>
>> Please explain all four states clearly there.
>
> Is the previous explanation enough?

The new comments can serve as reminders for readers who already know
what "block migration" and "block shared migration" is.  Surely we can
do better for readers who don't, and I'd like you to try.

Note that the comments aren't just for QAPI schema readers, but also for
QMP documentation readers (generated docs/qemu-qmp-ref.*).

>>> I'll leave it up to you as maintainers which way you prefer, I'm just
>>> offering the potential design tradeoffs for simplicity of booleans (but
>>> complexity in an unused state) vs. simplicity of design (but complexity
>>> in code).
>>
>> For what it's worth, I dislike entangled booleans.
>
> Some here, but except if we put shared as a migration parameter, I don't
> know what to do here.
>
> { 'enum': 'MigrationParameter',
>   'data': ['compress-level', 'compress-threads', 'decompress-threads',
>            'cpu-throttle-initial', 'cpu-throttle-increment',
>            'tls-creds', 'tls-hostname', 'max-bandwidth',
>            'downtime-limit', 'x-checkpoint-delay' ] }
>
>
> I don't see either how to define that block_shared will be a boolean parameter.

Here:

  { 'struct': 'MigrationParameters',
    'data': { '*compress-level': 'int',
              '*compress-threads': 'int',
              '*decompress-threads': 'int',
              '*cpu-throttle-initial': 'int',
              '*cpu-throttle-increment': 'int',
              '*tls-creds': 'str',
              '*tls-hostname': 'str',
              '*max-bandwidth': 'int',
              '*downtime-limit': 'int',
              '*x-checkpoint-delay': 'int'} }

How enum MigrationParameter shadows the member names of
MigrationParameters is weird.  Wouldn't be necessary if QAPI provided
object type introspection to C (it provides it to QMP).

MigrationParameter is used only for HMP commands "migrate_set_parameter"
and "info migrate_parameters".  The way they're coded doesn't really
profit from QAPI; string literals would be simpler.  Yes, they'd have to
shadow MigrationParameters, but since they'd replace MigrationParameter,
we'd be roughly even.

Re: [Qemu-devel] [PATCH 1/3] migration: Create block capabilities for shared and enable

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 05/15/2017 04:46 AM, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> Eric Blake <eblake@redhat.com> wrote:
>>>> On 05/11/2017 11:32 AM, Juan Quintela wrote:
>>>>> Those two capabilities were added through the command line.  Notice that
>>>>> we just created them.  This is just the boilerplate.
>>>>>
>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>
>>>>> --
>>>>>
>>>>> Make migrate_set_block_* take a boolean argument.
>>>>
>>>> Question - do we support the orthogonal selection of all 4 combinations
>>>> under HMP 'migrate' (no argument, -b alone, -i alone, -b and -i
>>>> together), or are there only 3 actual states? If the latter, should we
>>>> represent this as a single enum-valued property, rather than as two
>>>> independent boolean properties?
>>>
>>> { 'enum': 'MigrationCapability',
>>>   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>>            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
>>>
>>>
>>> My understanding is that we can only have boolean capabilities here.
>>> Or, how could we put a non-boolean capability?
>
> If we want a non-boolean, then we make it a migration parameter rather
> than a migration capability.  There may be other advantages to using
> MigrationParameter instead of MigrationCapability (such as making it
> easier to figure out whether the parameter settings are persistent or
> apply per-migration).

Block migration is a capability as far as I can see/understand.  The
block shared bit could be a parameter, though.  Not sure if that would
be better/worse.

>
>> 
>> Lets keep this simple and stick with the booleans.
>> 
>> Dave
>> 
>>> There are three states as far as I can see.
>
> I'll leave it up to you as maintainers which way you prefer, I'm just
> offering the potential design tradeoffs for simplicity of booleans (but
> complexity in an unused state) vs. simplicity of design (but complexity
> in code).

As I expect to deprecate the old interface, I think that the best thing
to do is to use two capabilities or a capability(block enabled) and a
parameter (block shared).

What do you think?

Later, Juan.