| Message ID | 20180103054043.25719-5-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Peter Xu <peterx@redhat.com> wrote: > It was used either to: > > 1. store initial timestamp of migration start, and > 2. store total time used by last migration > > Let's provide two parameters for each of them. Mix use of the two is > slightly misleading. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> If you have to respin, I would like to use the names: start_time and total_time, i.e. without the mig_ preffix, because they are in an struct that is clearly named migration O:-) For the rest, good cleanup. Later, Juan. > --- > migration/migration.c | 13 +++++++------ > migration/migration.h | 5 ++++- > 2 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 20f7565527..b684c2005d 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -613,7 +613,7 @@ MigrationInfo *qmp_query_migrate(Error **errp) > info->has_status = true; > info->has_total_time = true; > info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) > - - s->total_time; > + - s->mig_start_time; > info->has_expected_downtime = true; > info->expected_downtime = s->expected_downtime; > info->has_setup_time = true; > @@ -629,7 +629,7 @@ MigrationInfo *qmp_query_migrate(Error **errp) > case MIGRATION_STATUS_COMPLETED: > info->has_status = true; > info->has_total_time = true; > - info->total_time = s->total_time; > + info->total_time = s->mig_total_time; > info->has_downtime = true; > info->downtime = s->downtime; > info->has_setup_time = true; > @@ -1270,7 +1270,8 @@ MigrationState *migrate_init(void) > > migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP); > > - s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > + s->mig_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > + s->mig_total_time = 0; > return s; > } > > @@ -2293,13 +2294,13 @@ static void *migration_thread(void *opaque) > qemu_mutex_lock_iothread(); > if (s->state == MIGRATION_STATUS_COMPLETED) { > uint64_t transferred_bytes = qemu_ftell(s->to_dst_file); > - s->total_time = end_time - s->total_time; > + s->mig_total_time = end_time - s->mig_start_time; > if (!entered_postcopy) { > s->downtime = end_time - start_time; > } > - if (s->total_time) { > + if (s->mig_total_time) { > s->mbps = (((double) transferred_bytes * 8.0) / > - ((double) s->total_time)) / 1000; > + ((double) s->mig_total_time)) / 1000; > } > runstate_set(RUN_STATE_POSTMIGRATE); > } else { > diff --git a/migration/migration.h b/migration/migration.h > index 663415fe48..ac74a12713 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -103,7 +103,10 @@ struct MigrationState > } rp_state; > > double mbps; > - int64_t total_time; > + /* Timestamp when recent migration starts (ms) */ > + int64_t mig_start_time; > + /* Total time used by latest migration (ms) */ > + int64_t mig_total_time; > int64_t downtime; > int64_t expected_downtime; > bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
On Wed, Jan 03, 2018 at 09:58:10AM +0100, Juan Quintela wrote: > Peter Xu <peterx@redhat.com> wrote: > > It was used either to: > > > > 1. store initial timestamp of migration start, and > > 2. store total time used by last migration > > > > Let's provide two parameters for each of them. Mix use of the two is > > slightly misleading. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> Thanks! > > If you have to respin, I would like to use the names: (I think it very possible :-) > > start_time and total_time, i.e. without the mig_ preffix, because they > are in an struct that is clearly named migration O:-) Oh, it's my bad (or good?) habit of keeping some prefix so that cscope won't mix these variables with others. I think the problem is that cscope is always using a global namespace for variables. Considering this do you still like me to change? :) Any suggestions on better usage of cscope would be greatly welcomed too! (Sure I can rename that! It's not a big deal)
Peter Xu <peterx@redhat.com> wrote: > On Wed, Jan 03, 2018 at 09:58:10AM +0100, Juan Quintela wrote: >> Peter Xu <peterx@redhat.com> wrote: >> > It was used either to: >> > >> > 1. store initial timestamp of migration start, and >> > 2. store total time used by last migration >> > >> > Let's provide two parameters for each of them. Mix use of the two is >> > slightly misleading. >> > >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> > > Thanks! > >> >> If you have to respin, I would like to use the names: > > (I think it very possible :-) > >> >> start_time and total_time, i.e. without the mig_ preffix, because they >> are in an struct that is clearly named migration O:-) > > Oh, it's my bad (or good?) habit of keeping some prefix so that cscope > won't mix these variables with others. I think the problem is that > cscope is always using a global namespace for variables. Considering > this do you still like me to change? :) Any suggestions on better > usage of cscope would be greatly welcomed too! I only use cscope very ocassionally, so I can't comment about its usage. As said, I put the reviewed-by anyways. But if you dont want to use generic names like start_time/total_time, then please use the full name: - migration_start_time - migration_total_time It is only used a couple of times, and clearer to read. I normally only put _prefixes_ if context don't make clear what the variable means. If I need *context* I tend to use the full name of things, not abbreviations. But yes, not all the code is coherent/consistent. Later, Juan. > > (Sure I can rename that! It's not a big deal)
On Wed, Jan 03, 2018 at 10:20:39AM +0100, Juan Quintela wrote: > Peter Xu <peterx@redhat.com> wrote: > > On Wed, Jan 03, 2018 at 09:58:10AM +0100, Juan Quintela wrote: > >> Peter Xu <peterx@redhat.com> wrote: > >> > It was used either to: > >> > > >> > 1. store initial timestamp of migration start, and > >> > 2. store total time used by last migration > >> > > >> > Let's provide two parameters for each of them. Mix use of the two is > >> > slightly misleading. > >> > > >> > Signed-off-by: Peter Xu <peterx@redhat.com> > >> > >> Reviewed-by: Juan Quintela <quintela@redhat.com> > > > > Thanks! > > > >> > >> If you have to respin, I would like to use the names: > > > > (I think it very possible :-) > > > >> > >> start_time and total_time, i.e. without the mig_ preffix, because they > >> are in an struct that is clearly named migration O:-) > > > > Oh, it's my bad (or good?) habit of keeping some prefix so that cscope > > won't mix these variables with others. I think the problem is that > > cscope is always using a global namespace for variables. Considering > > this do you still like me to change? :) Any suggestions on better > > usage of cscope would be greatly welcomed too! > > I only use cscope very ocassionally, so I can't comment about its usage. > As said, I put the reviewed-by anyways. But if you dont want to use > generic names like start_time/total_time, then please use the full name: > > - migration_start_time > - migration_total_time > > It is only used a couple of times, and clearer to read. I normally only > put _prefixes_ if context don't make clear what the variable means. If > I need *context* I tend to use the full name of things, not > abbreviations. But yes, not all the code is coherent/consistent. I'll use the shorter ones. Thanks!
diff --git a/migration/migration.c b/migration/migration.c index 20f7565527..b684c2005d 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -613,7 +613,7 @@ MigrationInfo *qmp_query_migrate(Error **errp) info->has_status = true; info->has_total_time = true; info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - - s->total_time; + - s->mig_start_time; info->has_expected_downtime = true; info->expected_downtime = s->expected_downtime; info->has_setup_time = true; @@ -629,7 +629,7 @@ MigrationInfo *qmp_query_migrate(Error **errp) case MIGRATION_STATUS_COMPLETED: info->has_status = true; info->has_total_time = true; - info->total_time = s->total_time; + info->total_time = s->mig_total_time; info->has_downtime = true; info->downtime = s->downtime; info->has_setup_time = true; @@ -1270,7 +1270,8 @@ MigrationState *migrate_init(void) migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP); - s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); + s->mig_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); + s->mig_total_time = 0; return s; } @@ -2293,13 +2294,13 @@ static void *migration_thread(void *opaque) qemu_mutex_lock_iothread(); if (s->state == MIGRATION_STATUS_COMPLETED) { uint64_t transferred_bytes = qemu_ftell(s->to_dst_file); - s->total_time = end_time - s->total_time; + s->mig_total_time = end_time - s->mig_start_time; if (!entered_postcopy) { s->downtime = end_time - start_time; } - if (s->total_time) { + if (s->mig_total_time) { s->mbps = (((double) transferred_bytes * 8.0) / - ((double) s->total_time)) / 1000; + ((double) s->mig_total_time)) / 1000; } runstate_set(RUN_STATE_POSTMIGRATE); } else { diff --git a/migration/migration.h b/migration/migration.h index 663415fe48..ac74a12713 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -103,7 +103,10 @@ struct MigrationState } rp_state; double mbps; - int64_t total_time; + /* Timestamp when recent migration starts (ms) */ + int64_t mig_start_time; + /* Total time used by latest migration (ms) */ + int64_t mig_total_time; int64_t downtime; int64_t expected_downtime; bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
It was used either to: 1. store initial timestamp of migration start, and 2. store total time used by last migration Let's provide two parameters for each of them. Mix use of the two is slightly misleading. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 13 +++++++------ migration/migration.h | 5 ++++- 2 files changed, 11 insertions(+), 7 deletions(-)