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

[Peter Xu]

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.

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.

>              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.

-- 
Peter Xu