Re: [RFC PATCH v3 19/30] migration/ram: Ignore multifd flush when doing fixed-ram migration

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Nov 27, 2023 at 05:26:01PM -0300, Fabiano Rosas wrote:
>> Some functionalities of multifd are incompatible with the 'fixed-ram'
>> migration format.
>> 
>> The MULTIFD_FLUSH flag in particular is not used because in fixed-ram
>> there is no sinchronicity between migration source and destination so
>> there is not need for a sync packet. In fact, fixed-ram disables
>> packets in multifd as a whole.
>> 
>> However, we still need to sync the migration thread with the multifd
>> channels at key moments:
>> 
>> - between iterations, to avoid a slow channel being overrun by a fast
>> channel in the subsequent iteration;
>> 
>> - at ram_save_complete, to make sure all data has been transferred
>>   before finishing migration;
>
> [1]
>
>> 
>> Make sure RAM_SAVE_FLAG_MULTIFD_FLUSH is only emitted for fixed-ram at
>> those key moments.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/ram.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 08604222f2..ad6abd1761 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1363,7 +1363,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
>>          pss->page = 0;
>>          pss->block = QLIST_NEXT_RCU(pss->block, next);
>>          if (!pss->block) {
>> -            if (migrate_multifd() &&
>> +            if (migrate_multifd() && !migrate_fixed_ram() &&
>>                  !migrate_multifd_flush_after_each_section()) {
>>                  QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
>>                  int ret = multifd_send_sync_main(f);
>> @@ -3112,7 +3112,8 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>          return ret;
>>      }
>>  
>> -    if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
>> +    if (migrate_multifd() && !migrate_multifd_flush_after_each_section()
>> +        && !migrate_fixed_ram()) {
>>          qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>>      }
>>  
>> @@ -3242,8 +3243,11 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>  out:
>>      if (ret >= 0
>>          && migration_is_setup_or_active(migrate_get_current()->state)) {
>> -        if (migrate_multifd() && migrate_multifd_flush_after_each_section()) {
>> -            ret = multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
>> +        if (migrate_multifd() &&
>> +            (migrate_multifd_flush_after_each_section() ||
>> +             migrate_fixed_ram())) {
>> +            ret = multifd_send_sync_main(
>> +                rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
>
> Why you want this one?  ram_save_iterate() can be called tens for each
> second iiuc.
>

AIUI, this is a requirement for live migration, so that we're not
sending the new version of the page while the old version is still in
transit.

> There's one more?  ram_save_complete():
>
>     if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
>         qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>     }
>
> IIUC that's the one you referred to at [1] above, not sure why you modified
> the code in ram_save_iterate() instead.
>

I mentioned it in the commit message as well:

" - between iterations, to avoid a slow channel being overrun by a fast
 channel in the subsequent iteration;"

>>              if (ret < 0) {
>>                  return ret;
>>              }
>> -- 
>> 2.35.3
>> 
>
> Since the file migration added its whole new code in
> multifd_send_sync_main(), now I'm hesitating whether we should just provide
> multifd_file_sync_threads(), put file sync there, and call explicitly,
> like:
>
>   if (migrate_multifd()) {
>     if (migrate_is_file()) {
>        multifd_file_sync_threads();
>     } else if (migrate_multifd_flush_after_each_section()) {
>        multifd_send_sync_main();
>     }
>   }
>
> It'll be much clearer that file goes into its own path and we don't need to
> worry on fat eyes of those if clauses.  diff should be similar.

Hm, it could be a good idea indeed. Let me experiment with it.