| Message ID | 20170714042306.GH27284@pxdev.xzpeter.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jul 14, 2017 at 12:23:06PM +0800, Peter Xu wrote: > On Wed, Jul 12, 2017 at 08:02:40PM +0100, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > We have the MigrationState as QDev now (which seems crazy). Let's > > > continue to benefit. > > > > > > This series is exporting all migration capabilities/params as global > > > parameters. Then we can do something like this: > > > > > > qemu -global migration.postcopy-ram=true \ > > > -global migration.max-bandwidth=4096 > > > > > > The values will be inited just like we typed these values into HMP > > > monitor. It'll simplify lots of migration scripts. > > > > > > The changes are fairly straightforward. One tiny loss is that we still > > > don't support: > > > > > > -global migration.max-bandwidth=1g > > > > > > ...just like what we did in HMP: > > > > > > migrate_set_speed 1g > > > > > > ...while we need to use: > > > > > > -global migration.max-bandwidth=1073741824 > > > > > > However that should only be used in scripts, and that's good enough > > > imho. > > > > > > These properties should only be used for debugging/testing purpose, > > > and we should not guarantee any interface compatibility for them (just > > > like HMP). > > > > I guess the sanity checks in qmp_migrate_set_parameters and > > qmp_migrate_set_capabilities aren't run? > > Indeed I missed this point. Before that: Eduardo, do you think below > change would make any sense? > > diff --git a/qom/object.c b/qom/object.c > index 5f6fdfa..ca077a9 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti) > > static void object_post_init_with_type(Object *obj, TypeImpl *ti) > { > - if (ti->instance_post_init) { > - ti->instance_post_init(obj); > - } > - > if (type_has_parent(ti)) { > object_post_init_with_type(obj, type_get_parent(ti)); > } > + > + if (ti->instance_post_init) { > + ti->instance_post_init(obj); > + } > } > > static void object_initialize_with_type(void *data, size_t size, TypeImpl *type) > > IMHO it'll make more sense if we call the parent post_init first (just > like most of the other object hooks). Or is current ordering intended? I'm not sure. The point of post_init was to allow the parent class to perform actions after all the children classes have done their tasks. However, if you can prove this won't break existing code and you propose an amendment to ObjectClass::instance_post_init documentation, we can consider changing the ordering. (A quick look at arm_cpu_post_init() seems to indicate the order you propose will be more useful for TYPE_ARM_CPU too.) > > If we can have above change, it'll be much easier for me to do the > check that Dave mentioned, after the global properties applied (I can > then just provide post_init hook for the migration object). > > > > > Also, have you done any checks on multiple migrations; e.g. it's good > > to check that the flags/state are all preserved if we do a migrate, that > > migrate fails or is cancelled and then you do a 2nd migrate. > > I think so. They'll be applied only when creating the object, and > they'll be there even after a e.g. migrate_cancel. Thanks, > > -- > Peter Xu
On Fri, Jul 14, 2017 at 12:57:15PM -0300, Eduardo Habkost wrote: > On Fri, Jul 14, 2017 at 12:23:06PM +0800, Peter Xu wrote: > > On Wed, Jul 12, 2017 at 08:02:40PM +0100, Dr. David Alan Gilbert wrote: > > > * Peter Xu (peterx@redhat.com) wrote: > > > > We have the MigrationState as QDev now (which seems crazy). Let's > > > > continue to benefit. > > > > > > > > This series is exporting all migration capabilities/params as global > > > > parameters. Then we can do something like this: > > > > > > > > qemu -global migration.postcopy-ram=true \ > > > > -global migration.max-bandwidth=4096 > > > > > > > > The values will be inited just like we typed these values into HMP > > > > monitor. It'll simplify lots of migration scripts. > > > > > > > > The changes are fairly straightforward. One tiny loss is that we still > > > > don't support: > > > > > > > > -global migration.max-bandwidth=1g > > > > > > > > ...just like what we did in HMP: > > > > > > > > migrate_set_speed 1g > > > > > > > > ...while we need to use: > > > > > > > > -global migration.max-bandwidth=1073741824 > > > > > > > > However that should only be used in scripts, and that's good enough > > > > imho. > > > > > > > > These properties should only be used for debugging/testing purpose, > > > > and we should not guarantee any interface compatibility for them (just > > > > like HMP). > > > > > > I guess the sanity checks in qmp_migrate_set_parameters and > > > qmp_migrate_set_capabilities aren't run? > > > > Indeed I missed this point. Before that: Eduardo, do you think below > > change would make any sense? > > > > diff --git a/qom/object.c b/qom/object.c > > index 5f6fdfa..ca077a9 100644 > > --- a/qom/object.c > > +++ b/qom/object.c > > @@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti) > > > > static void object_post_init_with_type(Object *obj, TypeImpl *ti) > > { > > - if (ti->instance_post_init) { > > - ti->instance_post_init(obj); > > - } > > - > > if (type_has_parent(ti)) { > > object_post_init_with_type(obj, type_get_parent(ti)); > > } > > + > > + if (ti->instance_post_init) { > > + ti->instance_post_init(obj); > > + } > > } > > > > static void object_initialize_with_type(void *data, size_t size, TypeImpl *type) > > > > IMHO it'll make more sense if we call the parent post_init first (just > > like most of the other object hooks). Or is current ordering intended? > > I'm not sure. The point of post_init was to allow the parent > class to perform actions after all the children classes have done > their tasks. > > However, if you can prove this won't break existing code and you > propose an amendment to ObjectClass::instance_post_init > documentation, we can consider changing the ordering. > > (A quick look at arm_cpu_post_init() seems to indicate the order > you propose will be more useful for TYPE_ARM_CPU too.) Exactly. That's one of the reason I asked. I do think it makes more sense to post_init on parent first, since child always contains extra things to handle, and it may possibly want to override something of the parent rather than in a reversed order. So as you may have already seen, it should not break anything, instead it may benefit us for at least this series and possibly arm_cpu_post_init() as well in the future. Let me propose a patch for it then. Thanks!
diff --git a/qom/object.c b/qom/object.c index 5f6fdfa..ca077a9 100644 --- a/qom/object.c +++ b/qom/object.c @@ -347,13 +347,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti) static void object_post_init_with_type(Object *obj, TypeImpl *ti) { - if (ti->instance_post_init) { - ti->instance_post_init(obj); - } - if (type_has_parent(ti)) { object_post_init_with_type(obj, type_get_parent(ti)); } + + if (ti->instance_post_init) { + ti->instance_post_init(obj); + } } static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)