diff mbox

[v5,04/10] migration: let MigrationState be a qdev

Message ID 1498193206-18007-5-git-send-email-peterx@redhat.com
State New
Headers show

Commit Message

Peter Xu June 23, 2017, 4:46 a.m. UTC 
Let the old man "MigrationState" join the object family. Direct benefit
is that we can start to use all the property features derived from
current QDev, like: HW_COMPAT_* bits, command line setup for migration
parameters (so will never need to set them up each time using HMP/QMP,
this is really, really attractive for test writters), etc.

I see no reason to disallow this happen yet. So let's start from this
one, to see whether it would be anything good.

Now we init the MigrationState struct statically in main() to make sure
it's initialized after global properties are applied, since we'll use
them during creation of the object.

No functional change at all.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h |  1 +
 migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
 migration/migration.h    | 19 ++++++++++++
 vl.c                     |  6 ++++
 4 files changed, 81 insertions(+), 23 deletions(-)

Comments

Eduardo Habkost June 23, 2017, 10:18 p.m. UTC | #1 
On Fri, Jun 23, 2017 at 12:46:40PM +0800, Peter Xu wrote:
> Let the old man "MigrationState" join the object family. Direct benefit
> is that we can start to use all the property features derived from
> current QDev, like: HW_COMPAT_* bits, command line setup for migration
> parameters (so will never need to set them up each time using HMP/QMP,
> this is really, really attractive for test writters), etc.
> 
> I see no reason to disallow this happen yet. So let's start from this
> one, to see whether it would be anything good.
> 
> Now we init the MigrationState struct statically in main() to make sure
> it's initialized after global properties are applied, since we'll use
> them during creation of the object.
> 
> No functional change at all.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/misc.h |  1 +
>  migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
>  migration/migration.h    | 19 ++++++++++++
>  vl.c                     |  6 ++++
>  4 files changed, 81 insertions(+), 23 deletions(-)
> 
[...]
> diff --git a/vl.c b/vl.c
> index cdd2ec8..9b04ba7 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
>       */
>      register_global_properties(current_machine);
>  
> +    /*
> +     * Migration object can only be created after global properties
> +     * are applied correctly.
> +     */
> +    migration_object_init();
> +

Do we really need this? Can't be we just do:

    if (!current_migration) {
        current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
    }

inside migration_get_current()?

>      /* This checkpoint is required by replay to separate prior clock
>         reading from the other reads, because timer polling functions query
>         clock values from the log. */
> -- 
> 2.7.4
> 
>
Peter Xu June 26, 2017, 2:50 a.m. UTC | #2 
On Fri, Jun 23, 2017 at 07:18:19PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 12:46:40PM +0800, Peter Xu wrote:
> > Let the old man "MigrationState" join the object family. Direct benefit
> > is that we can start to use all the property features derived from
> > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > parameters (so will never need to set them up each time using HMP/QMP,
> > this is really, really attractive for test writters), etc.
> > 
> > I see no reason to disallow this happen yet. So let's start from this
> > one, to see whether it would be anything good.
> > 
> > Now we init the MigrationState struct statically in main() to make sure
> > it's initialized after global properties are applied, since we'll use
> > them during creation of the object.
> > 
> > No functional change at all.
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/migration/misc.h |  1 +
> >  migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
> >  migration/migration.h    | 19 ++++++++++++
> >  vl.c                     |  6 ++++
> >  4 files changed, 81 insertions(+), 23 deletions(-)
> > 
> [...]
> > diff --git a/vl.c b/vl.c
> > index cdd2ec8..9b04ba7 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
> >       */
> >      register_global_properties(current_machine);
> >  
> > +    /*
> > +     * Migration object can only be created after global properties
> > +     * are applied correctly.
> > +     */
> > +    migration_object_init();
> > +
> 
> Do we really need this? Can't be we just do:
> 
>     if (!current_migration) {
>         current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
>     }
> 
> inside migration_get_current()?

I did this change on purpose (after AccelClass.global_props is
introduced). The comment above tried to explain it but looks like it's
still not clear enough... The reason is that currently the creation of
migration object is depending on the global properties, so we need to
create the object after register_global_properties(), while the old
migrate_get_current() cannot really be sure of this ordering: it just
creates the object on the first call of the function, but the first
call can be even before register_global_properties(). If so, we'll
have a problem (e.g. Xen compat properties will be missing).

Now this restriction is strictly followed if we create the migrate
object here. If anyone calls migrate_get_current() before
register_global_properties(), there will be an assert, and that should
be treated as a programming error.

Thanks,
Eduardo Habkost June 27, 2017, 12:54 a.m. UTC | #3 
On Mon, Jun 26, 2017 at 10:50:35AM +0800, Peter Xu wrote:
> On Fri, Jun 23, 2017 at 07:18:19PM -0300, Eduardo Habkost wrote:
> > On Fri, Jun 23, 2017 at 12:46:40PM +0800, Peter Xu wrote:
> > > Let the old man "MigrationState" join the object family. Direct benefit
> > > is that we can start to use all the property features derived from
> > > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > > parameters (so will never need to set them up each time using HMP/QMP,
> > > this is really, really attractive for test writters), etc.
> > > 
> > > I see no reason to disallow this happen yet. So let's start from this
> > > one, to see whether it would be anything good.
> > > 
> > > Now we init the MigrationState struct statically in main() to make sure
> > > it's initialized after global properties are applied, since we'll use
> > > them during creation of the object.
> > > 
> > > No functional change at all.
> > > 
> > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  include/migration/misc.h |  1 +
> > >  migration/migration.c    | 78 ++++++++++++++++++++++++++++++++++--------------
> > >  migration/migration.h    | 19 ++++++++++++
> > >  vl.c                     |  6 ++++
> > >  4 files changed, 81 insertions(+), 23 deletions(-)
> > > 
> > [...]
> > > diff --git a/vl.c b/vl.c
> > > index cdd2ec8..9b04ba7 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -4596,6 +4596,12 @@ int main(int argc, char **argv, char **envp)
> > >       */
> > >      register_global_properties(current_machine);
> > >  
> > > +    /*
> > > +     * Migration object can only be created after global properties
> > > +     * are applied correctly.
> > > +     */
> > > +    migration_object_init();
> > > +
> > 
> > Do we really need this? Can't be we just do:
> > 
> >     if (!current_migration) {
> >         current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
> >     }
> > 
> > inside migration_get_current()?
> 
> I did this change on purpose (after AccelClass.global_props is
> introduced). The comment above tried to explain it but looks like it's
> still not clear enough... The reason is that currently the creation of
> migration object is depending on the global properties, so we need to
> create the object after register_global_properties(), while the old
> migrate_get_current() cannot really be sure of this ordering: it just
> creates the object on the first call of the function, but the first
> call can be even before register_global_properties(). If so, we'll
> have a problem (e.g. Xen compat properties will be missing).
> 
> Now this restriction is strictly followed if we create the migrate
> object here. If anyone calls migrate_get_current() before
> register_global_properties(), there will be an assert, and that should
> be treated as a programming error.

Makes sense to me.  Thanks!
diff mbox

Patch

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 65c7070..2d36cf5 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -45,6 +45,7 @@  void savevm_skip_section_footers(void);
 void savevm_skip_configuration(void);
 
 /* migration/migration.c */
+void migration_object_init(void);
 void qemu_start_incoming_migration(const char *uri, Error **errp);
 bool migration_is_idle(void);
 void add_migration_state_change_notifier(Notifier *notify);
diff --git a/migration/migration.c b/migration/migration.c
index f588329..2c25927 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -98,32 +98,21 @@  enum mig_rp_message_type {
    migrations at once.  For now we don't need to add
    dynamic creation of migration */
 
+static MigrationState *current_migration;
+
+void migration_object_init(void)
+{
+    /* This can only be called once. */
+    assert(!current_migration);
+    current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
+}
+
 /* For outgoing */
 MigrationState *migrate_get_current(void)
 {
-    static bool once;
-    static MigrationState current_migration = {
-        .state = MIGRATION_STATUS_NONE,
-        .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
-        .mbps = -1,
-        .parameters = {
-            .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
-            .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
-            .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
-            .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
-            .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
-            .max_bandwidth = MAX_THROTTLE,
-            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
-            .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
-        },
-    };
-
-    if (!once) {
-        current_migration.parameters.tls_creds = g_strdup("");
-        current_migration.parameters.tls_hostname = g_strdup("");
-        once = true;
-    }
-    return &current_migration;
+    /* This can only be called after the object created. */
+    assert(current_migration);
+    return current_migration;
 }
 
 MigrationIncomingState *migration_incoming_get_current(void)
@@ -1987,3 +1976,46 @@  void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static void migration_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = false;
+}
+
+static void migration_instance_init(Object *obj)
+{
+    MigrationState *ms = MIGRATION_OBJ(obj);
+
+    ms->state = MIGRATION_STATUS_NONE;
+    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
+    ms->mbps = -1;
+    ms->parameters = (MigrationParameters) {
+        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
+        .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
+        .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
+        .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
+        .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+        .max_bandwidth = MAX_THROTTLE,
+        .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
+        .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
+    };
+    ms->parameters.tls_creds = g_strdup("");
+    ms->parameters.tls_hostname = g_strdup("");
+}
+
+static const TypeInfo migration_type = {
+    .name = TYPE_MIGRATION,
+    .parent = TYPE_DEVICE,
+    .class_init = migration_class_init,
+    .class_size = sizeof(MigrationClass),
+    .instance_size = sizeof(MigrationState),
+    .instance_init = migration_instance_init,
+};
+
+static void register_migration_types(void)
+{
+    type_register_static(&migration_type);
+}
+
+type_init(register_migration_types);
diff --git a/migration/migration.h b/migration/migration.h
index d9a268a..3fca364 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -19,6 +19,7 @@ 
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
+#include "hw/qdev.h"
 
 /* State for the incoming migration */
 struct MigrationIncomingState {
@@ -62,8 +63,26 @@  struct MigrationIncomingState {
 MigrationIncomingState *migration_incoming_get_current(void);
 void migration_incoming_state_destroy(void);
 
+#define TYPE_MIGRATION "migration"
+
+#define MIGRATION_CLASS(klass) \
+    OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
+#define MIGRATION_OBJ(obj) \
+    OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
+#define MIGRATION_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
+
+typedef struct MigrationClass {
+    /*< private >*/
+    DeviceClass parent_class;
+} MigrationClass;
+
 struct MigrationState
 {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
diff --git a/vl.c b/vl.c
index cdd2ec8..9b04ba7 100644
--- a/vl.c
+++ b/vl.c
@@ -4596,6 +4596,12 @@  int main(int argc, char **argv, char **envp)
      */
     register_global_properties(current_machine);
 
+    /*
+     * Migration object can only be created after global properties
+     * are applied correctly.
+     */
+    migration_object_init();
+
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
        clock values from the log. */