Re: [PATCH v5 19/23] migration/multifd: Prepare multifd sync for mapped-ram migration

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Feb 28, 2024 at 12:21:23PM -0300, Fabiano Rosas wrote:
>> The mapped-ram migration can be performed live or non-live, but it is
>> always asynchronous, i.e. the source machine and the destination
>> machine are not migrating at the same time. We only need some pieces
>> of the multifd sync operations.
>> 
>> multifd_send_sync_main()
>> ------------------------
>>   Issued by the ram migration code on the migration thread, causes the
>>   multifd send channels to synchronize with the migration thread and
>>   makes the sending side emit a packet with the MULTIFD_FLUSH flag.
>> 
>>   With mapped-ram we want to maintain the sync on the sending side
>>   because that provides ordering between the rounds of dirty pages when
>>   migrating live.
>
> IIUC as I used to comment, we should probably only need that sync after
> each full iteration, which is find_dirty_block().
>
> I think keeping the setup/complete sync is fine, and that can be discussed
> separately.  However IMHO we should still avoid the sync in
> ram_save_iterate() always, or on new qemu + old machine types (where
> flush_after_each_section=true) fixed-ram could suffer perf issues, IIUC.
>
> So I assume at a minimum below would still be preferred?
>
> @@ -3257,7 +3257,8 @@ 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()) {
> +        if (migrate_multifd() && migrate_multifd_flush_after_each_section() &&
> +            !migrate_mapped_ram()) {
>              ret = multifd_send_sync_main();
>              if (ret < 0) {
>                  return ret;
>

I think I forgot this. I'll amend it.

>> 
>> MULTIFD_FLUSH
>> -------------
>>   On the receiving side, the presence of the MULTIFD_FLUSH flag on a
>>   packet causes the receiving channels to start synchronizing with the
>>   main thread.
>> 
>>   We're not using packets with mapped-ram, so there's no MULTIFD_FLUSH
>>   flag and therefore no channel sync on the receiving side.
>> 
>> multifd_recv_sync_main()
>> ------------------------
>>   Issued by the migration thread when the ram migration flag
>>   RAM_SAVE_FLAG_MULTIFD_FLUSH is received, causes the migration thread
>>   on the receiving side to start synchronizing with the recv
>>   channels. Due to compatibility, this is also issued when
>>   RAM_SAVE_FLAG_EOS is received.
>> 
>>   For mapped-ram we only need to synchronize the channels at the end of
>>   migration to avoid doing cleanup before the channels have finished
>>   their IO.
>
> Did you forget to add the sync at parse_ramblocks() for mapped-ram?
>

Ugh, I messed it up. I'll fix it.

>> 
>> Make sure the multifd syncs are only issued at the appropriate times.
>> 
>> Note that due to pre-existing backward compatibility issues, we have
>> the multifd_flush_after_each_section property that can cause a sync to
>> happen at EOS. Since the EOS flag is needed on the stream, allow
>> mapped-ram to just ignore it.
>
> Skipping EOS makes sense, but I suggest do that without invalid_flags.  See
> below.
>
>> 
>> Also emit an error if any other unexpected flags are found on the
>> stream.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> - skipped all FLUSH flags
>> - added invalid flags
>> - skipped EOS
>> ---
>>  migration/ram.c | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 18620784c6..250dcd110c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1368,8 +1368,11 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
>>                  if (ret < 0) {
>>                      return ret;
>>                  }
>> -                qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>> -                qemu_fflush(f);
>> +
>> +                if (!migrate_mapped_ram()) {
>> +                    qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>> +                    qemu_fflush(f);
>> +                }
>>              }
>>              /*
>>               * If memory migration starts over, we will meet a dirtied page
>> @@ -3111,7 +3114,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_mapped_ram()) {
>>          qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>>      }
>>  
>> @@ -3334,7 +3338,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>>          }
>>      }
>>  
>> -    if (migrate_multifd() && !migrate_multifd_flush_after_each_section()) {
>> +    if (migrate_multifd() && !migrate_multifd_flush_after_each_section() &&
>> +        !migrate_mapped_ram()) {
>>          qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>>      }
>>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>> @@ -4137,6 +4142,12 @@ static int ram_load_precopy(QEMUFile *f)
>>          invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
>>      }
>>  
>> +    if (migrate_mapped_ram()) {
>> +        invalid_flags |= (RAM_SAVE_FLAG_EOS | RAM_SAVE_FLAG_HOOK |
>> +                          RAM_SAVE_FLAG_MULTIFD_FLUSH | RAM_SAVE_FLAG_PAGE |
>> +                          RAM_SAVE_FLAG_XBZRLE | RAM_SAVE_FLAG_ZERO);
>
> IMHO EOS cannot be accounted as "invalid" here because it always exists.
> Rather than this trick (then explicitly ignore it below... which is even
> hackier, IMHO), we can avoid setting EOS in invalid_flags, but explicitly
> ignore EOS in below code to bypass it for mapped-ram:
>
> @@ -4301,7 +4302,12 @@ static int ram_load_precopy(QEMUFile *f)
>          case RAM_SAVE_FLAG_EOS:
>              /* normal exit */
>              if (migrate_multifd() &&
> -                migrate_multifd_flush_after_each_section()) {
> +                migrate_multifd_flush_after_each_section() &&
> +                /*
> +                 * Mapped-ram migration flushes once and for all after
> +                 * parsing ramblocks.  Always ignore EOS for it.
> +                 */
> +                !migrate_mapped_ram()) {
>                  multifd_recv_sync_main();
>              }
>              break;

I thought we were already spraying too many migrate_mapped_ram() checks
all over the code. But wat you said makes sense, I'll change it.

>> +    }
>> +
>>      while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
>>          ram_addr_t addr;
>>          void *host = NULL, *host_bak = NULL;
>> @@ -4158,6 +4169,13 @@ static int ram_load_precopy(QEMUFile *f)
>>          addr &= TARGET_PAGE_MASK;
>>  
>>          if (flags & invalid_flags) {
>> +            if (invalid_flags & RAM_SAVE_FLAG_EOS) {
>> +                /* EOS is always present, just ignore it */
>> +                continue;
>> +            }
>> +
>> +            error_report("Unexpected RAM flags: %d", flags & invalid_flags);
>> +
>>              if (flags & invalid_flags & RAM_SAVE_FLAG_COMPRESS_PAGE) {
>>                  error_report("Received an unexpected compressed page");
>>              }
>> -- 
>> 2.35.3
>>