hw/core/qdev-properties.c | 32 +++++++++++++++++++++++++++ include/hw/qdev-properties.h | 3 +++ migration/migration.c | 52 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 77 insertions(+), 10 deletions(-)
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).
Please review. Thanks.
Peter Xu (3):
qdev: provide DEFINE_PROP_INT64()
migration: export parameters to props
migration: export capabilities to props
hw/core/qdev-properties.c | 32 +++++++++++++++++++++++++++
include/hw/qdev-properties.h | 3 +++
migration/migration.c | 52 +++++++++++++++++++++++++++++++++++---------
3 files changed, 77 insertions(+), 10 deletions(-)
--
2.7.4
* 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? 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. Dave > Please review. Thanks. > > Peter Xu (3): > qdev: provide DEFINE_PROP_INT64() > migration: export parameters to props > migration: export capabilities to props > > hw/core/qdev-properties.c | 32 +++++++++++++++++++++++++++ > include/hw/qdev-properties.h | 3 +++ > migration/migration.c | 52 +++++++++++++++++++++++++++++++++++--------- > 3 files changed, 77 insertions(+), 10 deletions(-) > > -- > 2.7.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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?
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: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
--
Eduardo
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!
--
Peter Xu
On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote: [...] > These properties should only be used for debugging/testing purpose, > and we should not guarantee any interface compatibility for them (just > like HMP). If we don't guarantee compatibility, the property names need to be prefixed with "x-". -- Eduardo
On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote: > On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote: > [...] > > These properties should only be used for debugging/testing purpose, > > and we should not guarantee any interface compatibility for them (just > > like HMP). > > If we don't guarantee compatibility, the property names need to > be prefixed with "x-". Indeed. Sorry I missed that. But I'd say it is slightly awkward to add "x-" for all these (for me, "x-" means more like "this is not stable and experimental, use it carefully", while this does not suite for this series). Maybe I can just remove this sentence in commit log (I think I am just a little bit frightened by the compatibility problems)... Thanks, -- Peter Xu
On Fri, Jul 14, 2017 at 01:04:23PM +0800, Peter Xu wrote: > On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote: > > On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote: > > [...] > > > These properties should only be used for debugging/testing purpose, > > > and we should not guarantee any interface compatibility for them (just > > > like HMP). > > > > If we don't guarantee compatibility, the property names need to > > be prefixed with "x-". > > Indeed. Sorry I missed that. > > But I'd say it is slightly awkward to add "x-" for all these (for me, > "x-" means more like "this is not stable and experimental, use it > carefully", while this does not suite for this series). Maybe I can > just remove this sentence in commit log (I think I am just a little > bit frightened by the compatibility problems)... "x-" in property names doesn't mean "experimental", but just "not part of the stable interface". If you have the tiniest doubt about command-line compatibility, I think it won't hurt to use "x-". -- Eduardo
* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Fri, Jul 14, 2017 at 01:04:23PM +0800, Peter Xu wrote:
> > On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote:
> > > On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote:
> > > [...]
> > > > These properties should only be used for debugging/testing purpose,
> > > > and we should not guarantee any interface compatibility for them (just
> > > > like HMP).
> > >
> > > If we don't guarantee compatibility, the property names need to
> > > be prefixed with "x-".
> >
> > Indeed. Sorry I missed that.
> >
> > But I'd say it is slightly awkward to add "x-" for all these (for me,
> > "x-" means more like "this is not stable and experimental, use it
> > carefully", while this does not suite for this series). Maybe I can
> > just remove this sentence in commit log (I think I am just a little
> > bit frightened by the compatibility problems)...
>
> "x-" in property names doesn't mean "experimental", but just "not
> part of the stable interface". If you have the tiniest doubt
> about command-line compatibility, I think it won't hurt to use
> "x-".
We have:
DEFINE_PROP_MIG_CAP("xbzrle", MIGRATION_CAPABILITY_XBZRLE),
so we could easily do:
DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
to ensure that all of the things we expose as props here have
that added fealing of uncertainty but don't change what migration
parameters see.
(It's a shame we have to have those lists manually, there are so
many manual places for each parameter and capability)
Dave
>
> --
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jul 14, 2017 at 05:32:10PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Fri, Jul 14, 2017 at 01:04:23PM +0800, Peter Xu wrote:
> > > On Wed, Jul 12, 2017 at 04:05:58PM -0300, Eduardo Habkost wrote:
> > > > On Wed, Jul 12, 2017 at 02:53:40PM +0800, Peter Xu wrote:
> > > > [...]
> > > > > These properties should only be used for debugging/testing purpose,
> > > > > and we should not guarantee any interface compatibility for them (just
> > > > > like HMP).
> > > >
> > > > If we don't guarantee compatibility, the property names need to
> > > > be prefixed with "x-".
> > >
> > > Indeed. Sorry I missed that.
> > >
> > > But I'd say it is slightly awkward to add "x-" for all these (for me,
> > > "x-" means more like "this is not stable and experimental, use it
> > > carefully", while this does not suite for this series). Maybe I can
> > > just remove this sentence in commit log (I think I am just a little
> > > bit frightened by the compatibility problems)...
> >
> > "x-" in property names doesn't mean "experimental", but just "not
> > part of the stable interface". If you have the tiniest doubt
> > about command-line compatibility, I think it won't hurt to use
> > "x-".
>
> We have:
> DEFINE_PROP_MIG_CAP("xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>
> so we could easily do:
> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>
> to ensure that all of the things we expose as props here have
> that added fealing of uncertainty but don't change what migration
> parameters see.
I was thinking to ask either you or Juan on what you would like for
this, looks like I got the answer now. :-) Let me add "x-" to them.
>
>
> (It's a shame we have to have those lists manually, there are so
> many manual places for each parameter and capability)
Temporarily I think it's still okay. But indeed you are right.
--
Peter Xu
© 2016 - 2024 Red Hat, Inc.