| Message ID | 1372210541-28092-16-git-send-email-mrhines@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 06/25/2013 07:35 PM, mrhines@linux.vnet.ibm.com wrote: > From: "Michael R. Hines" <mrhines@us.ibm.com> > > Using the previous patches, we're now able to timestamp the SETUP > state. Once we have this time, let the user know about it in the > schema. > > Reviewed-by: Juan Quintela <quintela@redhat.com> > Signed-off-by: Michael R. Hines <mrhines@us.ibm.com> Usually, Reviewed-by lines are listed _after_ S-o-b lines - signature lines are typically chronological, but the patch has to be signed before a review can have any weight at getting the patch into a pull request :) > --- > hmp.c | 4 ++++ > include/migration/migration.h | 1 + > migration.c | 9 +++++++++ > qapi-schema.json | 9 ++++++++- > 4 files changed, 22 insertions(+), 1 deletion(-) > > +++ b/qapi-schema.json > @@ -578,6 +578,12 @@ > # expected downtime in milliseconds for the guest in last walk > # of the dirty bitmap. (since 1.3) > # > +# @setup-time: #optional amount of setup time spent _before_ the iterations > +# begin but _after_ the QMP command is issued. This is designed to In what units? One can easily assume milliseconds, based on the docs of other elapsed time parameters, but being explicit never hurts. > +# provide an accounting of any activities (such as RDMA pinning) which > +# may be expensive, but do not actually occur during the iterative > +# migration rounds themselves. (since 1.6) > +# > # Since: 0.14.0 > ## > { 'type': 'MigrationInfo', > @@ -586,7 +592,8 @@ > '*xbzrle-cache': 'XBZRLECacheStats', > '*total-time': 'int', > '*expected-downtime': 'int', > - '*downtime': 'int'} } > + '*downtime': 'int', > + '*setup-time' : 'int'} } We typically don't have space before ':' (as seen in the other lines just above). I can live with the patch as-is, but if you respin the series for other things, then fix those two things before adding Reviewed-by: Eric Blake <eblake@redhat.com>
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 28/06/2013 00:58, Eric Blake ha scritto: >> Using the previous patches, we're now able to timestamp the >> SETUP state. Once we have this time, let the user know about it >> in the schema. >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: >> Michael R. Hines <mrhines@us.ibm.com> > > Usually, Reviewed-by lines are listed _after_ S-o-b lines - > signature lines are typically chronological, but the patch has to > be signed before a review can have any weight at getting the patch > into a pull request :) Hmm, that's not how I understood it. The last line in the message should be S-o-b. If _I_ collect the Reviewed-bys when committing, I'll do Signed-off-by: Michael R. Hines <mrhines@us.ibm.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> But since Michael is the one collecting the tags from previous submissions of the patch series, he's placing it right. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRzTf4AAoJEBvWZb6bTYby33kP/A1oAqs6djatLmvKXwFa4uq/ XgDvn8TZq/TP+I8a3v8rvMiw8MbLYP+YBKvuwKmUb/WKHmdvDFzZeNRRI/DBVOb6 c8qMlc6sGCZW1R8+aBq1lYJf/sXlXFdE+owL4gVfujJzHdFn1N9hllWXFGxqnXDk G7mJHPsyFA2B2W+4rJvfJozvJHsT9IviA+iXcAZMqGPH22VvOzcBodeE6Y9kYTmN a3cRUkygY7wi4rTsxqbc1oQ1575FUL3xt7QPx2B47vT6C04n/mC5fCMHbgwS0oiJ +YRQap6Lnnd3FgKwpXh4lJamA7n+HT/3H5GC2nlr86MmR/sU6+zeYmksKgDhs8L1 l8QcWc+aQ0urfN29oUxhNChilI/0TWlcTIe8XyxgEpPeZ9KM4rCBVMIui0YYjaUK B4mrxpqi1s9dP4Tx4VEgr46LIx2aTen5vhurFlfHVvLse9UqX8QUwmymZJTw+OIv jLL37XfN2L2vfS8TTyr+a1Z1lrcyi0E5nY1IEm6UnH1wZB/kRdh9F5DgkPjB7tob 2GlJBBH9oNs2ohHfS3/4WdYq8zPx5rSlSqy0PfctgtIHR7+LblxLP4wcTDKm+apM yx3bfEDEyBta0js+SME0nU2gVWfTJEJBNSnARax5Sxe06/NShOtJ9vqOSUlMk4SY v5R7ZkeDsKYuKBvDHRel =n0+a -----END PGP SIGNATURE-----
On 06/27/2013 06:58 PM, Eric Blake wrote: > On 06/25/2013 07:35 PM, mrhines@linux.vnet.ibm.com wrote: >> From: "Michael R. Hines" <mrhines@us.ibm.com> >> >> Using the previous patches, we're now able to timestamp the SETUP >> state. Once we have this time, let the user know about it in the >> schema. >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com> > Usually, Reviewed-by lines are listed _after_ S-o-b lines - signature > lines are typically chronological, but the patch has to be signed before > a review can have any weight at getting the patch into a pull request :) > >> --- >> hmp.c | 4 ++++ >> include/migration/migration.h | 1 + >> migration.c | 9 +++++++++ >> qapi-schema.json | 9 ++++++++- >> 4 files changed, 22 insertions(+), 1 deletion(-) >> >> +++ b/qapi-schema.json >> @@ -578,6 +578,12 @@ >> # expected downtime in milliseconds for the guest in last walk >> # of the dirty bitmap. (since 1.3) >> # >> +# @setup-time: #optional amount of setup time spent _before_ the iterations >> +# begin but _after_ the QMP command is issued. This is designed to > In what units? One can easily assume milliseconds, based on the docs of > other elapsed time parameters, but being explicit never hurts. > >> +# provide an accounting of any activities (such as RDMA pinning) which >> +# may be expensive, but do not actually occur during the iterative >> +# migration rounds themselves. (since 1.6) >> +# >> # Since: 0.14.0 >> ## >> { 'type': 'MigrationInfo', >> @@ -586,7 +592,8 @@ >> '*xbzrle-cache': 'XBZRLECacheStats', >> '*total-time': 'int', >> '*expected-downtime': 'int', >> - '*downtime': 'int'} } >> + '*downtime': 'int', >> + '*setup-time' : 'int'} } > We typically don't have space before ':' (as seen in the other lines > just above). I can live with the patch as-is, but if you respin the > series for other things, then fix those two things before adding > > Reviewed-by: Eric Blake <eblake@redhat.com> > Good catches.....thank you.
On 06/28/2013 03:15 AM, Paolo Bonzini wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Il 28/06/2013 00:58, Eric Blake ha scritto: >>> Using the previous patches, we're now able to timestamp the >>> SETUP state. Once we have this time, let the user know about it >>> in the schema. >>> >>> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: >>> Michael R. Hines <mrhines@us.ibm.com> >> Usually, Reviewed-by lines are listed _after_ S-o-b lines - >> signature lines are typically chronological, but the patch has to >> be signed before a review can have any weight at getting the patch >> into a pull request :) > Hmm, that's not how I understood it. The last line in the message > should be S-o-b. If _I_ collect the Reviewed-bys when committing, I'll do > > Signed-off-by: Michael R. Hines <mrhines@us.ibm.com> > Reviewed-by: Juan Quintela <quintela@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > But since Michael is the one collecting the tags from previous > submissions of the patch series, he's placing it right. > > Paolo > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2.0.19 (GNU/Linux) > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > iQIcBAEBAgAGBQJRzTf4AAoJEBvWZb6bTYby33kP/A1oAqs6djatLmvKXwFa4uq/ > XgDvn8TZq/TP+I8a3v8rvMiw8MbLYP+YBKvuwKmUb/WKHmdvDFzZeNRRI/DBVOb6 > c8qMlc6sGCZW1R8+aBq1lYJf/sXlXFdE+owL4gVfujJzHdFn1N9hllWXFGxqnXDk > G7mJHPsyFA2B2W+4rJvfJozvJHsT9IviA+iXcAZMqGPH22VvOzcBodeE6Y9kYTmN > a3cRUkygY7wi4rTsxqbc1oQ1575FUL3xt7QPx2B47vT6C04n/mC5fCMHbgwS0oiJ > +YRQap6Lnnd3FgKwpXh4lJamA7n+HT/3H5GC2nlr86MmR/sU6+zeYmksKgDhs8L1 > l8QcWc+aQ0urfN29oUxhNChilI/0TWlcTIe8XyxgEpPeZ9KM4rCBVMIui0YYjaUK > B4mrxpqi1s9dP4Tx4VEgr46LIx2aTen5vhurFlfHVvLse9UqX8QUwmymZJTw+OIv > jLL37XfN2L2vfS8TTyr+a1Z1lrcyi0E5nY1IEm6UnH1wZB/kRdh9F5DgkPjB7tob > 2GlJBBH9oNs2ohHfS3/4WdYq8zPx5rSlSqy0PfctgtIHR7+LblxLP4wcTDKm+apM > yx3bfEDEyBta0js+SME0nU2gVWfTJEJBNSnARax5Sxe06/NShOtJ9vqOSUlMk4SY > v5R7ZkeDsKYuKBvDHRel > =n0+a > -----END PGP SIGNATURE----- > Oh. ok. Understood =)
On 06/28/2013 01:15 AM, Paolo Bonzini wrote: > Il 28/06/2013 00:58, Eric Blake ha scritto: >>> Using the previous patches, we're now able to timestamp the >>> SETUP state. Once we have this time, let the user know about it >>> in the schema. >>> >>> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: >>> Michael R. Hines <mrhines@us.ibm.com> > >> Usually, Reviewed-by lines are listed _after_ S-o-b lines - >> signature lines are typically chronological, but the patch has to >> be signed before a review can have any weight at getting the patch >> into a pull request :) > > Hmm, that's not how I understood it. The last line in the message > should be S-o-b. If _I_ collect the Reviewed-bys when committing, I'll do > > Signed-off-by: Michael R. Hines <mrhines@us.ibm.com> > Reviewed-by: Juan Quintela <quintela@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> That's what I meant by signatures being chronological. The _first_ line is Michael's s-o-b, since he wrote it; on any of Michael's respins where he adds signatures collected during the review process (which will only be reviewed-by or tested-by - here a review from Juan), those come next; then when the maintainer incorporates the patch into a pull request, further signatures are collected (any reviewed-by that were not incorporated by Michael because no respin was required, followed by the final s-o-b saying the maintainer modified the commit message as part of incorporating into the pull request). > But since Michael is the one collecting the tags from previous > submissions of the patch series, he's placing it right. Having Michael's s-o-b last, after reviewed-by picked up from earlier revisions, is not chronological. Maintainers add reviewed-by before their own s-o-b, but only because their s-o-b is a secondary s-o-b; I still don't see why the original author would add reviewed-by before their (lone) s-o-b.
On 06/28/2013 09:32 AM, Eric Blake wrote: > On 06/28/2013 01:15 AM, Paolo Bonzini wrote: >> Il 28/06/2013 00:58, Eric Blake ha scritto: >>>> Using the previous patches, we're now able to timestamp the >>>> SETUP state. Once we have this time, let the user know about it >>>> in the schema. >>>> >>>> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: >>>> Michael R. Hines <mrhines@us.ibm.com> >>> Usually, Reviewed-by lines are listed _after_ S-o-b lines - >>> signature lines are typically chronological, but the patch has to >>> be signed before a review can have any weight at getting the patch >>> into a pull request :) >> Hmm, that's not how I understood it. The last line in the message >> should be S-o-b. If _I_ collect the Reviewed-bys when committing, I'll do >> >> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > That's what I meant by signatures being chronological. The _first_ line > is Michael's s-o-b, since he wrote it; on any of Michael's respins where > he adds signatures collected during the review process (which will only > be reviewed-by or tested-by - here a review from Juan), those come next; > then when the maintainer incorporates the patch into a pull request, > further signatures are collected (any reviewed-by that were not > incorporated by Michael because no respin was required, followed by the > final s-o-b saying the maintainer modified the commit message as part of > incorporating into the pull request). > >> But since Michael is the one collecting the tags from previous >> submissions of the patch series, he's placing it right. > Having Michael's s-o-b last, after reviewed-by picked up from earlier > revisions, is not chronological. Maintainers add reviewed-by before > their own s-o-b, but only because their s-o-b is a secondary s-o-b; I > still don't see why the original author would add reviewed-by before > their (lone) s-o-b. > I don't mind prioritizing the reviewers. Everyone is putting in way more review time than they probably otherwise want to. =) Besides, the author's name is all over the email list - so it's obvious that the email sender will have an s-o-b. It's less obvious who reviewed the patch. - Michael
diff --git a/hmp.c b/hmp.c index 148a3fb..5f52f17 100644 --- a/hmp.c +++ b/hmp.c @@ -164,6 +164,10 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict) monitor_printf(mon, "downtime: %" PRIu64 " milliseconds\n", info->downtime); } + if (info->has_setup_time) { + monitor_printf(mon, "setup: %" PRIu64 " milliseconds\n", + info->setup_time); + } } if (info->has_ram) { diff --git a/include/migration/migration.h b/include/migration/migration.h index b5e413a..71dbe54 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -49,6 +49,7 @@ struct MigrationState int64_t dirty_bytes_rate; bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; int64_t xbzrle_cache_size; + int64_t setup_time; }; void process_incoming_migration(QEMUFile *f); diff --git a/migration.c b/migration.c index a199a67..892302a 100644 --- a/migration.c +++ b/migration.c @@ -191,6 +191,7 @@ MigrationInfo *qmp_query_migrate(Error **errp) case MIG_STATE_SETUP: info->has_status = true; info->status = g_strdup("setup"); + info->has_total_time = false; break; case MIG_STATE_ACTIVE: info->has_status = true; @@ -200,6 +201,8 @@ MigrationInfo *qmp_query_migrate(Error **errp) - s->total_time; info->has_expected_downtime = true; info->expected_downtime = s->expected_downtime; + info->has_setup_time = true; + info->setup_time = s->setup_time; info->has_ram = true; info->ram = g_malloc0(sizeof(*info->ram)); @@ -231,6 +234,8 @@ MigrationInfo *qmp_query_migrate(Error **errp) info->total_time = s->total_time; info->has_downtime = true; info->downtime = s->downtime; + info->has_setup_time = true; + info->setup_time = s->setup_time; info->has_ram = true; info->ram = g_malloc0(sizeof(*info->ram)); @@ -522,6 +527,7 @@ static void *migration_thread(void *opaque) { MigrationState *s = opaque; int64_t initial_time = qemu_get_clock_ms(rt_clock); + int64_t setup_start = qemu_get_clock_ms(host_clock); int64_t initial_bytes = 0; int64_t max_size = 0; int64_t start_time = initial_time; @@ -530,8 +536,11 @@ static void *migration_thread(void *opaque) DPRINTF("beginning savevm\n"); qemu_savevm_state_begin(s->file, &s->params); + s->setup_time = qemu_get_clock_ms(host_clock) - setup_start; migrate_set_state(s, MIG_STATE_SETUP, MIG_STATE_ACTIVE); + DPRINTF("setup complete\n"); + while (s->state == MIG_STATE_ACTIVE) { int64_t current_time; uint64_t pending_size; diff --git a/qapi-schema.json b/qapi-schema.json index a30a728..0ad7257 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -578,6 +578,12 @@ # expected downtime in milliseconds for the guest in last walk # of the dirty bitmap. (since 1.3) # +# @setup-time: #optional amount of setup time spent _before_ the iterations +# begin but _after_ the QMP command is issued. This is designed to +# provide an accounting of any activities (such as RDMA pinning) which +# may be expensive, but do not actually occur during the iterative +# migration rounds themselves. (since 1.6) +# # Since: 0.14.0 ## { 'type': 'MigrationInfo', @@ -586,7 +592,8 @@ '*xbzrle-cache': 'XBZRLECacheStats', '*total-time': 'int', '*expected-downtime': 'int', - '*downtime': 'int'} } + '*downtime': 'int', + '*setup-time' : 'int'} } ## # @query-migrate