Re: [PATCH] migration: Fix qmp_query_migrate mbps value

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Feb 19, 2024 at 04:44:57PM -0300, Fabiano Rosas wrote:
>> The QMP command query_migrate might see incorrect throughput numbers
>> if it runs after we've set the migration completion status but before
>> migration_calculate_complete() has updated s->total_time and s->mbps.
>> 
>> The migration status would show COMPLETED, but the throughput value
>> would be the one from the last iteration and not the one from the
>> whole migration. This will usually be a larger value due to the time
>> period being smaller (one iteration).
>> 
>> Move migration_calculate_complete() earlier so that the status
>> MIGRATION_STATUS_COMPLETED is only emitted after the final counters
>> update.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1182405776
>> ---
>>  migration/migration.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index ab21de2cad..7486d59da0 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -102,6 +102,7 @@ static int migration_maybe_pause(MigrationState *s,
>>                                   int new_state);
>>  static void migrate_fd_cancel(MigrationState *s);
>>  static bool close_return_path_on_source(MigrationState *s);
>> +static void migration_calculate_complete(MigrationState *s);
>>  
>>  static void migration_downtime_start(MigrationState *s)
>>  {
>> @@ -2746,6 +2747,7 @@ static void migration_completion(MigrationState *s)
>>          migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>>                            MIGRATION_STATUS_COLO);
>>      } else {
>> +        migration_calculate_complete(s);
>>          migrate_set_state(&s->state, current_active_state,
>>                            MIGRATION_STATUS_COMPLETED);
>>      }
>> @@ -2784,6 +2786,7 @@ static void bg_migration_completion(MigrationState *s)
>>          goto fail;
>>      }
>>  
>> +    migration_calculate_complete(s);
>>      migrate_set_state(&s->state, current_active_state,
>>                        MIGRATION_STATUS_COMPLETED);
>>      return;
>> @@ -2993,12 +2996,15 @@ static void migration_calculate_complete(MigrationState *s)
>>      int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>>      int64_t transfer_time;
>>  
>> +    /* QMP could read from these concurrently */
>> +    bql_lock();
>>      migration_downtime_end(s);
>>      s->total_time = end_time - s->start_time;
>>      transfer_time = s->total_time - s->setup_time;
>>      if (transfer_time) {
>>          s->mbps = ((double) bytes * 8.0) / transfer_time / 1000;
>>      }
>> +    bql_unlock();
>
> The lock is not needed?
>
> AFAIU that was needed because of things like runstate_set() rather than
> setting of these fields.
>

Don't we need to keep the total_time and mbps update atomic? Otherwise
query-migrate might see (say) total_time=0 and mbps=<correct value> or
total_time=<correct value> and mbps=<previous value>.

Also, what orders s->mbps update before the s->state update? I'd say we
should probably hold the lock around the whole total_time,mbps,state
update.

I'm not entirely sure, what do you think?

> See migration_update_counters() where it also updates mbps without holding
> a lock.

Here it might be less important since it's the middle of the migration,
there will proabably be more than one query-migrate which would see the
correct values.

>
> Other than that it looks all good.
>
>>  }
>>  
>>  static void update_iteration_initial_status(MigrationState *s)
>> @@ -3145,7 +3151,6 @@ static void migration_iteration_finish(MigrationState *s)
>>      bql_lock();
>>      switch (s->state) {
>>      case MIGRATION_STATUS_COMPLETED:
>> -        migration_calculate_complete(s);
>>          runstate_set(RUN_STATE_POSTMIGRATE);
>>          break;
>>      case MIGRATION_STATUS_COLO:
>> @@ -3189,9 +3194,6 @@ static void bg_migration_iteration_finish(MigrationState *s)
>>      bql_lock();
>>      switch (s->state) {
>>      case MIGRATION_STATUS_COMPLETED:
>> -        migration_calculate_complete(s);
>> -        break;
>> -
>>      case MIGRATION_STATUS_ACTIVE:
>>      case MIGRATION_STATUS_FAILED:
>>      case MIGRATION_STATUS_CANCELLED:
>> -- 
>> 2.35.3
>>