Re: [PATCH V4 05/11] migration: preserve suspended for bg_migration

[Steven Sistare]

On 8/30/2023 12:35 PM, Peter Xu wrote:
> On Tue, Aug 29, 2023 at 11:18:00AM -0700, Steve Sistare wrote:
>> Do not wake a suspended guest during bg_migration.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  migration/migration.c | 12 +++++-------
>>  softmmu/runstate.c    |  1 +
>>  2 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index a9ecb66..303d5a6 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3064,7 +3064,9 @@ static void bg_migration_vm_start_bh(void *opaque)
>>      qemu_bh_delete(s->vm_start_bh);
>>      s->vm_start_bh = NULL;
>>  
>> -    vm_start();
>> +    if (!runstate_check(RUN_STATE_SUSPENDED)) {
>> +        vm_start();
>> +    }
>>      s->downtime = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - 
>> s->downtime_start;
>>  }
>>  
>> @@ -3134,16 +3136,12 @@ static void *bg_migration_thread(void *opaque)
>>  
>>      qemu_mutex_lock_iothread();
>>  
>> -    /*
>> -     * If VM is currently in suspended state, then, to make a valid runstate
>> -     * transition in vm_stop_force_state() we need to wakeup it up.
>> -     */
>> -    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
>>      s->vm_old_state = runstate_get();
>>  
>>      global_state_store();
>>      /* Forcibly stop VM before saving state of vCPUs and devices */
>> -    if (vm_stop_force_state(RUN_STATE_PAUSED)) {
>> +    if (!runstate_check(RUN_STATE_SUSPENDED) &&
>> +        vm_stop_force_state(RUN_STATE_PAUSED)) {
> 
> IIUC we need the vm_stop even for SUSPENDED?  I think we need to make sure
> all backends are stopped before we start wr-protect the guest pages, so
> when wr-protect happens the guest pages should be in a consistent state.

Agreed, I stop everything for suspended in V5.

> I'd think it proper to juse reuse the helper you introduced in the previous
> patches, but maybe you have a reason to not do so (I didn't see it
> mentioned anywhere, though)?

This code is outgoing. The helper applies to the incoming side.

- Steve

>>          goto fail;
>>      }
>>      /*
>> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
>> index 21d7407..4417527 100644
>> --- a/softmmu/runstate.c
>> +++ b/softmmu/runstate.c
>> @@ -163,6 +163,7 @@ static const RunStateTransition 
>> runstate_transitions_def[] = {
>>      { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
>>      { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
>>      { RUN_STATE_SUSPENDED, RUN_STATE_COLO},
>> +    { RUN_STATE_SUSPENDED, RUN_STATE_PAUSED },
>>  
>>      { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>>      { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>> -- 
>> 1.8.3.1
>>
>