Re: [PATCH] migration: Don't serialize migration while can't switchover

[Joao Martins <joao.m.martins@oracle.com>]

On 27/02/2024 07:41, Peter Xu wrote:
> On Thu, Feb 22, 2024 at 05:56:27PM +0200, Avihai Horon wrote:
>> This bug was observed in several VFIO migration scenarios where some
>> workload on the VM prevented RAM from ever reaching a hard zero, not
>> allowing VFIO initial pre-copy data to be sent, and thus destination
>> could not ack switchover. Note that the same scenario, but without
>> switchover-ack, would converge.
>>
>> Fix it by not serializing device data sending during pre-copy iterative
>> phase if switchover was not acked yet.
> 
> I am still not fully convinced that it's even legal that one device can
> consume all iterator's bandwidth, ignoring the rest..  Though again it's
> not about this patch, but about commit 90697be889.
> 
> I'm thinking whether we should allow each device to have its own portion of
> chance to push data for each call to qemu_savevm_state_iterate(),
> irrelevant of vfio's switchover-ack capability.
> 

I guess that this means the only change needed is (...)

>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index d612c8a9020..3a012796375 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1386,7 +1386,7 @@ int qemu_savevm_state_resume_prepare(MigrationState *s)
>>   *   0 : We haven't finished, caller have to go again
>>   *   1 : We have finished, we can go to complete phase
>>   */
>> -int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
>> +int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy, bool can_switchover)
>>  {
>>      SaveStateEntry *se;
>>      int ret = 1;
>> @@ -1430,12 +1430,20 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
>>                           "%d(%s): %d",
>>                           se->section_id, se->idstr, ret);
>>              qemu_file_set_error(f, ret);
>> +            return ret;
>>          }
>> -        if (ret <= 0) {
>> -            /* Do not proceed to the next vmstate before this one reported
>> -               completion of the current stage. This serializes the migration
>> -               and reduces the probability that a faster changing state is
>> -               synchronized over and over again. */
>> +
>> +        if (ret == 0 && can_switchover) {
>> +            /*
>> +             * Do not proceed to the next vmstate before this one reported
>> +             * completion of the current stage. This serializes the migration
>> +             * and reduces the probability that a faster changing state is
>> +             * synchronized over and over again.
>> +             * Do it only if migration can switchover. If migration can't
>> +             * switchover yet, do proceed to let other devices send their data
>> +             * too, as this may be required for switchover to be acked and
>> +             * migration to converge.
>> +             */
>>              break;
>>          }
>>      }

(...) is here to have:

if (ret < 0) {
	break;
}

? Or you were thinking in some heuristic?

	Joao