| Message ID | 1496980142-8986-3-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
Peter Xu <peterx@redhat.com> 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. > > No functional change at all. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
Peter Xu <peterx@redhat.com> writes: > 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. > > No functional change at all. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/migration/migration.h | 19 ++++++++++++++ > migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- > 2 files changed, 59 insertions(+), 21 deletions(-) > > diff --git a/include/migration/migration.h b/include/migration/migration.h > index 79b5484..bd0186c 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -21,6 +21,7 @@ > #include "qapi-types.h" > #include "exec/cpu-common.h" > #include "qemu/coroutine_int.h" > +#include "hw/qdev.h" > > #define QEMU_VM_FILE_MAGIC 0x5145564d > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 > @@ -49,6 +50,8 @@ enum mig_rp_message_type { > MIG_RP_MSG_MAX > }; > > +#define TYPE_MIGRATION "migration" > + > /* State for the incoming migration */ > struct MigrationIncomingState { > QEMUFile *from_src_file; > @@ -91,8 +94,24 @@ struct MigrationIncomingState { > MigrationIncomingState *migration_incoming_get_current(void); > void migration_incoming_state_destroy(void); > > +#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 >*/ Turning MigrationState into a QOM object so you can use QOM infrastructure makes sense. But why turn it into a device? It doesn't feel device-like to me. Would ObjectClass and Object instead of DeviceClass and DeviceState do? > size_t bytes_xfer; > size_t xfer_limit; > QemuThread thread; > diff --git a/migration/migration.c b/migration/migration.c > index 48c94c9..98b77e2 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -93,29 +93,13 @@ static bool deferred_incoming; > /* 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, > - }, > - }; > + static MigrationState *current_migration; You add an indirection... > > - if (!once) { > - current_migration.parameters.tls_creds = g_strdup(""); > - current_migration.parameters.tls_hostname = g_strdup(""); > - once = true; > + if (!current_migration) { > + current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION)); ... possibly just so you can use object_new(). Have you considered object_initialize()? > } > - return ¤t_migration; > + > + return current_migration; > } > > MigrationIncomingState *migration_incoming_get_current(void) > @@ -2123,3 +2107,38 @@ void migrate_fd_connect(MigrationState *s) > s->migration_thread_running = true; > } > > +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_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);
On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > 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. > > > > No functional change at all. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/migration/migration.h | 19 ++++++++++++++ > > migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- > > 2 files changed, 59 insertions(+), 21 deletions(-) > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > index 79b5484..bd0186c 100644 > > --- a/include/migration/migration.h > > +++ b/include/migration/migration.h > > @@ -21,6 +21,7 @@ > > #include "qapi-types.h" > > #include "exec/cpu-common.h" > > #include "qemu/coroutine_int.h" > > +#include "hw/qdev.h" > > > > #define QEMU_VM_FILE_MAGIC 0x5145564d > > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 > > @@ -49,6 +50,8 @@ enum mig_rp_message_type { > > MIG_RP_MSG_MAX > > }; > > > > +#define TYPE_MIGRATION "migration" > > + > > /* State for the incoming migration */ > > struct MigrationIncomingState { > > QEMUFile *from_src_file; > > @@ -91,8 +94,24 @@ struct MigrationIncomingState { > > MigrationIncomingState *migration_incoming_get_current(void); > > void migration_incoming_state_destroy(void); > > > > +#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 >*/ > > Turning MigrationState into a QOM object so you can use QOM > infrastructure makes sense. But why turn it into a device? It doesn't > feel device-like to me. Would ObjectClass and Object instead of > DeviceClass and DeviceState do? Yeah. That's the main reason for the series to be (was) RFC. I was trying to use the HW_COMPAT_* bits and -global, and that's QDev thing (IIUC you got that already :). I am just curious about why that is not for QObject from the very beginning, then it'll be easier. For now, IMHO QDev is okay as well for migration, as long as it's kept internally inside QEMU. But sure at least I should turn user_creatable to off. I'll investigate more to see how to make this a safer approach in next post. > > > size_t bytes_xfer; > > size_t xfer_limit; > > QemuThread thread; > > diff --git a/migration/migration.c b/migration/migration.c > > index 48c94c9..98b77e2 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -93,29 +93,13 @@ static bool deferred_incoming; > > /* 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, > > - }, > > - }; > > + static MigrationState *current_migration; > > You add an indirection... > > > > > - if (!once) { > > - current_migration.parameters.tls_creds = g_strdup(""); > > - current_migration.parameters.tls_hostname = g_strdup(""); > > - once = true; > > + if (!current_migration) { > > + current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION)); > > ... possibly just so you can use object_new(). Have you considered > object_initialize()? Yes, but even with object_initialize() I still need that indirection? Or, I can put the init into main() in some way to avoid the indirection. Thanks!
On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote: > On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote: > > Peter Xu <peterx@redhat.com> writes: > > > > > 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. > > > > > > No functional change at all. > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > include/migration/migration.h | 19 ++++++++++++++ > > > migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- > > > 2 files changed, 59 insertions(+), 21 deletions(-) > > > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > > index 79b5484..bd0186c 100644 > > > --- a/include/migration/migration.h > > > +++ b/include/migration/migration.h > > > @@ -21,6 +21,7 @@ > > > #include "qapi-types.h" > > > #include "exec/cpu-common.h" > > > #include "qemu/coroutine_int.h" > > > +#include "hw/qdev.h" > > > > > > #define QEMU_VM_FILE_MAGIC 0x5145564d > > > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 > > > @@ -49,6 +50,8 @@ enum mig_rp_message_type { > > > MIG_RP_MSG_MAX > > > }; > > > > > > +#define TYPE_MIGRATION "migration" > > > + > > > /* State for the incoming migration */ > > > struct MigrationIncomingState { > > > QEMUFile *from_src_file; > > > @@ -91,8 +94,24 @@ struct MigrationIncomingState { > > > MigrationIncomingState *migration_incoming_get_current(void); > > > void migration_incoming_state_destroy(void); > > > > > > +#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 >*/ > > > > Turning MigrationState into a QOM object so you can use QOM > > infrastructure makes sense. But why turn it into a device? It doesn't > > feel device-like to me. Would ObjectClass and Object instead of > > DeviceClass and DeviceState do? > > Yeah. That's the main reason for the series to be (was) RFC. > > I was trying to use the HW_COMPAT_* bits and -global, and that's QDev > thing (IIUC you got that already :). I am just curious about why that > is not for QObject from the very beginning, then it'll be easier. > > For now, IMHO QDev is okay as well for migration, as long as it's kept > internally inside QEMU. But sure at least I should turn user_creatable > to off. I'll investigate more to see how to make this a safer approach > in next post. We could allow non-device QOM objects use the global property system optionally. We could make qdev_prop_set_globals() work on any Object*, and let QOM classes call it on post_init if they want to be configurable by global properties too. Note that this would break qdev_prop_check_globals(), because it expects globals to work only on TYPE_DEVICE. We could address that by introducing a new interface type to indicate the type works with -global (something like INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?). Such a system would probably allow us to replace default_machine_opts, default_boot_order, default_display, default_ram_size (and probably many other compat fields) on MachineClass. It could also be used to implement -machine and -accel options by simply translating them to global properties (like we already do for -cpu). (This sounds like reinventing a QemuOpts-like system on top of global properties. Maybe that's a bad thing, maybe that's a good thing. I'm not sure.) > [...]
On Mon, Jun 12, 2017 at 01:13:49PM -0300, Eduardo Habkost wrote: > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote: > > On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote: > > > Peter Xu <peterx@redhat.com> writes: > > > > > > > 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. > > > > > > > > No functional change at all. > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > include/migration/migration.h | 19 ++++++++++++++ > > > > migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- > > > > 2 files changed, 59 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > > > index 79b5484..bd0186c 100644 > > > > --- a/include/migration/migration.h > > > > +++ b/include/migration/migration.h > > > > @@ -21,6 +21,7 @@ > > > > #include "qapi-types.h" > > > > #include "exec/cpu-common.h" > > > > #include "qemu/coroutine_int.h" > > > > +#include "hw/qdev.h" > > > > > > > > #define QEMU_VM_FILE_MAGIC 0x5145564d > > > > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 > > > > @@ -49,6 +50,8 @@ enum mig_rp_message_type { > > > > MIG_RP_MSG_MAX > > > > }; > > > > > > > > +#define TYPE_MIGRATION "migration" > > > > + > > > > /* State for the incoming migration */ > > > > struct MigrationIncomingState { > > > > QEMUFile *from_src_file; > > > > @@ -91,8 +94,24 @@ struct MigrationIncomingState { > > > > MigrationIncomingState *migration_incoming_get_current(void); > > > > void migration_incoming_state_destroy(void); > > > > > > > > +#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 >*/ > > > > > > Turning MigrationState into a QOM object so you can use QOM > > > infrastructure makes sense. But why turn it into a device? It doesn't > > > feel device-like to me. Would ObjectClass and Object instead of > > > DeviceClass and DeviceState do? > > > > Yeah. That's the main reason for the series to be (was) RFC. > > > > I was trying to use the HW_COMPAT_* bits and -global, and that's QDev > > thing (IIUC you got that already :). I am just curious about why that > > is not for QObject from the very beginning, then it'll be easier. > > > > For now, IMHO QDev is okay as well for migration, as long as it's kept > > internally inside QEMU. But sure at least I should turn user_creatable > > to off. I'll investigate more to see how to make this a safer approach > > in next post. > > We could allow non-device QOM objects use the global property > system optionally. > > We could make qdev_prop_set_globals() work on any Object*, and > let QOM classes call it on post_init if they want to be > configurable by global properties too. > > Note that this would break qdev_prop_check_globals(), because it > expects globals to work only on TYPE_DEVICE. We could address > that by introducing a new interface type to indicate the type > works with -global (something like > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?). > > Such a system would probably allow us to replace > default_machine_opts, default_boot_order, default_display, > default_ram_size (and probably many other compat fields) on > MachineClass. It could also be used to implement -machine and > -accel options by simply translating them to global properties > (like we already do for -cpu). > > (This sounds like reinventing a QemuOpts-like system on top of > global properties. Maybe that's a bad thing, maybe that's a good > thing. I'm not sure.) Thanks Eduardo for the reviews and suggestions. It'll obviously benefit us a lot if all the objects can use the global properties and the *_COMPAT_* legacy magic, while the tricky point is that, QObject will then depend on QDev? Generally speaking this sounds good to me. Before I do anything else, I believe I should wait to see how other QOM developers think about it. Thanks,
On Tue, Jun 13, 2017 at 11:52:15AM +0800, Peter Xu wrote: > On Mon, Jun 12, 2017 at 01:13:49PM -0300, Eduardo Habkost wrote: > > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote: > > > On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote: > > > > Peter Xu <peterx@redhat.com> writes: > > > > > > > > > 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. > > > > > > > > > > No functional change at all. > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > --- > > > > > include/migration/migration.h | 19 ++++++++++++++ > > > > > migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- > > > > > 2 files changed, 59 insertions(+), 21 deletions(-) > > > > > > > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > > > > index 79b5484..bd0186c 100644 > > > > > --- a/include/migration/migration.h > > > > > +++ b/include/migration/migration.h > > > > > @@ -21,6 +21,7 @@ > > > > > #include "qapi-types.h" > > > > > #include "exec/cpu-common.h" > > > > > #include "qemu/coroutine_int.h" > > > > > +#include "hw/qdev.h" > > > > > > > > > > #define QEMU_VM_FILE_MAGIC 0x5145564d > > > > > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 > > > > > @@ -49,6 +50,8 @@ enum mig_rp_message_type { > > > > > MIG_RP_MSG_MAX > > > > > }; > > > > > > > > > > +#define TYPE_MIGRATION "migration" > > > > > + > > > > > /* State for the incoming migration */ > > > > > struct MigrationIncomingState { > > > > > QEMUFile *from_src_file; > > > > > @@ -91,8 +94,24 @@ struct MigrationIncomingState { > > > > > MigrationIncomingState *migration_incoming_get_current(void); > > > > > void migration_incoming_state_destroy(void); > > > > > > > > > > +#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 >*/ > > > > > > > > Turning MigrationState into a QOM object so you can use QOM > > > > infrastructure makes sense. But why turn it into a device? It doesn't > > > > feel device-like to me. Would ObjectClass and Object instead of > > > > DeviceClass and DeviceState do? > > > > > > Yeah. That's the main reason for the series to be (was) RFC. > > > > > > I was trying to use the HW_COMPAT_* bits and -global, and that's QDev > > > thing (IIUC you got that already :). I am just curious about why that > > > is not for QObject from the very beginning, then it'll be easier. > > > > > > For now, IMHO QDev is okay as well for migration, as long as it's kept > > > internally inside QEMU. But sure at least I should turn user_creatable > > > to off. I'll investigate more to see how to make this a safer approach > > > in next post. > > > > We could allow non-device QOM objects use the global property > > system optionally. > > > > We could make qdev_prop_set_globals() work on any Object*, and > > let QOM classes call it on post_init if they want to be > > configurable by global properties too. > > > > Note that this would break qdev_prop_check_globals(), because it > > expects globals to work only on TYPE_DEVICE. We could address > > that by introducing a new interface type to indicate the type > > works with -global (something like > > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?). > > > > Such a system would probably allow us to replace > > default_machine_opts, default_boot_order, default_display, > > default_ram_size (and probably many other compat fields) on > > MachineClass. It could also be used to implement -machine and > > -accel options by simply translating them to global properties > > (like we already do for -cpu). > > > > (This sounds like reinventing a QemuOpts-like system on top of > > global properties. Maybe that's a bad thing, maybe that's a good > > thing. I'm not sure.) > > Thanks Eduardo for the reviews and suggestions. > > It'll obviously benefit us a lot if all the objects can use the global > properties and the *_COMPAT_* legacy magic, while the tricky point is > that, QObject will then depend on QDev? I don't think the dependency would be the main problem. The qdev global-property code would be moved outside qdev, to common code. The main problem I see is: it would make global properties too powerful and dangerous. Enabling global properties for all QOM types unconditionally would expose QEMU internals that are not supposed to be touched by the user. > > Generally speaking this sounds good to me. Before I do anything else, > I believe I should wait to see how other QOM developers think about > it. Thanks, > > -- > Peter Xu
Eduardo Habkost <ehabkost@redhat.com> writes: > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote: >> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote: >> > Peter Xu <peterx@redhat.com> writes: >> > >> > > 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. >> > > >> > > No functional change at all. >> > > >> > > Signed-off-by: Peter Xu <peterx@redhat.com> >> > > --- >> > > include/migration/migration.h | 19 ++++++++++++++ >> > > migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- >> > > 2 files changed, 59 insertions(+), 21 deletions(-) >> > > >> > > diff --git a/include/migration/migration.h b/include/migration/migration.h >> > > index 79b5484..bd0186c 100644 >> > > --- a/include/migration/migration.h >> > > +++ b/include/migration/migration.h >> > > @@ -21,6 +21,7 @@ >> > > #include "qapi-types.h" >> > > #include "exec/cpu-common.h" >> > > #include "qemu/coroutine_int.h" >> > > +#include "hw/qdev.h" >> > > >> > > #define QEMU_VM_FILE_MAGIC 0x5145564d >> > > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 >> > > @@ -49,6 +50,8 @@ enum mig_rp_message_type { >> > > MIG_RP_MSG_MAX >> > > }; >> > > >> > > +#define TYPE_MIGRATION "migration" >> > > + >> > > /* State for the incoming migration */ >> > > struct MigrationIncomingState { >> > > QEMUFile *from_src_file; >> > > @@ -91,8 +94,24 @@ struct MigrationIncomingState { >> > > MigrationIncomingState *migration_incoming_get_current(void); >> > > void migration_incoming_state_destroy(void); >> > > >> > > +#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 >*/ >> > >> > Turning MigrationState into a QOM object so you can use QOM >> > infrastructure makes sense. But why turn it into a device? It doesn't >> > feel device-like to me. Would ObjectClass and Object instead of >> > DeviceClass and DeviceState do? >> >> Yeah. That's the main reason for the series to be (was) RFC. >> >> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev >> thing (IIUC you got that already :). I am just curious about why that >> is not for QObject from the very beginning, then it'll be easier. >> >> For now, IMHO QDev is okay as well for migration, as long as it's kept >> internally inside QEMU. But sure at least I should turn user_creatable >> to off. I'll investigate more to see how to make this a safer approach >> in next post. > > We could allow non-device QOM objects use the global property > system optionally. > > We could make qdev_prop_set_globals() work on any Object*, and > let QOM classes call it on post_init if they want to be > configurable by global properties too. > > Note that this would break qdev_prop_check_globals(), because it > expects globals to work only on TYPE_DEVICE. We could address > that by introducing a new interface type to indicate the type > works with -global (something like > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?). > > Such a system would probably allow us to replace > default_machine_opts, default_boot_order, default_display, > default_ram_size (and probably many other compat fields) on > MachineClass. It could also be used to implement -machine and > -accel options by simply translating them to global properties > (like we already do for -cpu). > > (This sounds like reinventing a QemuOpts-like system on top of > global properties. Maybe that's a bad thing, maybe that's a good > thing. I'm not sure.) Not sure I get the connection to QemuOpts. Lifting the "adjust property default values on the command line" feature from qdev / TYPE_DEVICE to QOM / TYPE_OBJECT may make sense. But let's reconsider what exactly we're trying to accomplish for migration. The cover letter and commit message are too vague for me gue figure it out. Please describe how exactly you intend this series' features to be used. You mentioned -global and HW_COMPAT_* briefly; make sure to cover both. Concrete examples like "instead of <inconvenient way to do FOO> we can then do <better way>" would be nice. >> [...]
On Mon, Jun 19, 2017 at 11:09:58AM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote: > >> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote: > >> > Peter Xu <peterx@redhat.com> writes: > >> > > >> > > 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. > >> > > > >> > > No functional change at all. > >> > > > >> > > Signed-off-by: Peter Xu <peterx@redhat.com> > >> > > --- > >> > > include/migration/migration.h | 19 ++++++++++++++ > >> > > migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- > >> > > 2 files changed, 59 insertions(+), 21 deletions(-) > >> > > > >> > > diff --git a/include/migration/migration.h b/include/migration/migration.h > >> > > index 79b5484..bd0186c 100644 > >> > > --- a/include/migration/migration.h > >> > > +++ b/include/migration/migration.h > >> > > @@ -21,6 +21,7 @@ > >> > > #include "qapi-types.h" > >> > > #include "exec/cpu-common.h" > >> > > #include "qemu/coroutine_int.h" > >> > > +#include "hw/qdev.h" > >> > > > >> > > #define QEMU_VM_FILE_MAGIC 0x5145564d > >> > > #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 > >> > > @@ -49,6 +50,8 @@ enum mig_rp_message_type { > >> > > MIG_RP_MSG_MAX > >> > > }; > >> > > > >> > > +#define TYPE_MIGRATION "migration" > >> > > + > >> > > /* State for the incoming migration */ > >> > > struct MigrationIncomingState { > >> > > QEMUFile *from_src_file; > >> > > @@ -91,8 +94,24 @@ struct MigrationIncomingState { > >> > > MigrationIncomingState *migration_incoming_get_current(void); > >> > > void migration_incoming_state_destroy(void); > >> > > > >> > > +#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 >*/ > >> > > >> > Turning MigrationState into a QOM object so you can use QOM > >> > infrastructure makes sense. But why turn it into a device? It doesn't > >> > feel device-like to me. Would ObjectClass and Object instead of > >> > DeviceClass and DeviceState do? > >> > >> Yeah. That's the main reason for the series to be (was) RFC. > >> > >> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev > >> thing (IIUC you got that already :). I am just curious about why that > >> is not for QObject from the very beginning, then it'll be easier. > >> > >> For now, IMHO QDev is okay as well for migration, as long as it's kept > >> internally inside QEMU. But sure at least I should turn user_creatable > >> to off. I'll investigate more to see how to make this a safer approach > >> in next post. > > > > We could allow non-device QOM objects use the global property > > system optionally. > > > > We could make qdev_prop_set_globals() work on any Object*, and > > let QOM classes call it on post_init if they want to be > > configurable by global properties too. > > > > Note that this would break qdev_prop_check_globals(), because it > > expects globals to work only on TYPE_DEVICE. We could address > > that by introducing a new interface type to indicate the type > > works with -global (something like > > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?). > > > > Such a system would probably allow us to replace > > default_machine_opts, default_boot_order, default_display, > > default_ram_size (and probably many other compat fields) on > > MachineClass. It could also be used to implement -machine and > > -accel options by simply translating them to global properties > > (like we already do for -cpu). > > > > (This sounds like reinventing a QemuOpts-like system on top of > > global properties. Maybe that's a bad thing, maybe that's a good > > thing. I'm not sure.) > > Not sure I get the connection to QemuOpts. > > Lifting the "adjust property default values on the command line" feature > from qdev / TYPE_DEVICE to QOM / TYPE_OBJECT may make sense. > > But let's reconsider what exactly we're trying to accomplish for > migration. The cover letter and commit message are too vague for me gue > figure it out. Please describe how exactly you intend this series' > features to be used. You mentioned -global and HW_COMPAT_* briefly; > make sure to cover both. Concrete examples like "instead of > <inconvenient way to do FOO> we can then do <better way>" would be nice. Hi, Markus, I added some more explaination on the cover letter in the latter posts (latest v4). Please see whether that'll help, and just let me know if there is still things unclear in that. Thanks!
diff --git a/include/migration/migration.h b/include/migration/migration.h index 79b5484..bd0186c 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -21,6 +21,7 @@ #include "qapi-types.h" #include "exec/cpu-common.h" #include "qemu/coroutine_int.h" +#include "hw/qdev.h" #define QEMU_VM_FILE_MAGIC 0x5145564d #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002 @@ -49,6 +50,8 @@ enum mig_rp_message_type { MIG_RP_MSG_MAX }; +#define TYPE_MIGRATION "migration" + /* State for the incoming migration */ struct MigrationIncomingState { QEMUFile *from_src_file; @@ -91,8 +94,24 @@ struct MigrationIncomingState { MigrationIncomingState *migration_incoming_get_current(void); void migration_incoming_state_destroy(void); +#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/migration/migration.c b/migration/migration.c index 48c94c9..98b77e2 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -93,29 +93,13 @@ static bool deferred_incoming; /* 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, - }, - }; + static MigrationState *current_migration; - if (!once) { - current_migration.parameters.tls_creds = g_strdup(""); - current_migration.parameters.tls_hostname = g_strdup(""); - once = true; + if (!current_migration) { + current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION)); } - return ¤t_migration; + + return current_migration; } MigrationIncomingState *migration_incoming_get_current(void) @@ -2123,3 +2107,38 @@ void migrate_fd_connect(MigrationState *s) s->migration_thread_running = true; } +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_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);
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. No functional change at all. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/migration/migration.h | 19 ++++++++++++++ migration/migration.c | 61 ++++++++++++++++++++++++++++--------------- 2 files changed, 59 insertions(+), 21 deletions(-)