Re: [PATCH 5/5] multifd: Only sync once each full round of memory

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Tue, Jul 05, 2022 at 06:13:40PM +0100, Dr. David Alan Gilbert wrote:
>> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> > > On Tue, Jul 05, 2022 at 05:11:46PM +0200, Juan Quintela wrote:
>> > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > > > > * Juan Quintela (quintela@redhat.com) wrote:
>> > > > >> We need to add a new flag to mean to sync at that point.
>> > > > >> Notice that we still synchronize at the end of setup and at the end of
>> > > > >> complete stages.
>> > > > >> 
>> > > > >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> > > > >> ---
>> > > > >>  migration/migration.c |  2 +-
>> > > > >>  migration/ram.c       | 42 ++++++++++++++++++++++++++++++------------
>> > > > >>  2 files changed, 31 insertions(+), 13 deletions(-)
>> > > > >> 
>> > > > >> diff --git a/migration/migration.c b/migration/migration.c
>> > > > >> index 3f79df0b70..6627787fc2 100644
>> > > > >> --- a/migration/migration.c
>> > > > >> +++ b/migration/migration.c
>> > > > >> @@ -4283,7 +4283,7 @@ static Property migration_properties[] = {
>> > > > >>                        DEFAULT_MIGRATE_ANNOUNCE_STEP),
>> > > > >>      /* We will change to false when we introduce the new mechanism */
>> > > > >>      DEFINE_PROP_BOOL("multifd-sync-each-iteration", MigrationState,
>> > > > >> -                      multifd_sync_each_iteration, true),
>> > > > >> +                      multifd_sync_each_iteration, false),
>> > > > >>  
>> > > > >>      /* Migration capabilities */
>> > > > >>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>> > > > >> diff --git a/migration/ram.c b/migration/ram.c
>> > > > >> index 2c7289edad..6792986565 100644
>> > > > >> --- a/migration/ram.c
>> > > > >> +++ b/migration/ram.c
>> > > > >> @@ -81,6 +81,7 @@
>> > > > >>  #define RAM_SAVE_FLAG_XBZRLE   0x40
>> > > > >>  /* 0x80 is reserved in migration.h start with 0x100 next */
>> > > > >>  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
>> > > > >> +#define RAM_SAVE_FLAG_MULTIFD_SYNC     0x200
>> > > > >
>> > > > > Note this is the very last usable flag!
>> > > > 
>> > > > We can recover two flags right now:
>> > > > 
>> > > > RAM_SAVE_FLAG_FULL is not used anymore.
>> > > > 0x80 is free since years ago.
>> > > > 
>> > > > Once multifd is default, there are some other that could go.
>> > 
>> > I have suggested that a few times in the past.
>> > 
>> > > Non-multifd migration isn't likely to go away any time soon, given
>> > > distros desire to support migration between QEMU's with quite
>> > > significantly different versions. So feels like quite a long time
>> > > before we might reclaim more flags.
>> > > 
>> > > > > We could do with avoiding using them as flags where we dont need to.
>> > > > 
>> > > > I can't really think on another way to do it.  The other thing that I
>> > > > can do is just reuse one of the flags that don't make sense for multifd
>> > > > (RAM_SAVE_FLAG_ZERO after zero pages patch,
>> > > > RAM_SAVE_FLAG_XBZRLE/COMPRESS_PAGE).
>> > > 
>> > > Re-using flags based on use context differences feels like a recipe
>> > > to confuse people.
>> > 
>> > Note that most of these things aren't really 'flags'; in the sense that
>> > only a few of them are actually combinable; so we should start using
>> > combinations to mean things new.
>> 
>> IOW, treat the field as an enum of valid values instead, and just
>> define enum entries for the few valid combinations, giving us many
>> more values to play with ?
>
> Right; some care needs to be taken with the ones that were interpreted
> as flags; but since you're not going to send the new values to an old
> qemu, you've got quite a bit of flexibility.

Rigth now no combinations are allowed, so we are free to play with that
combination thing.  Reception side code is:

        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
        case RAM_SAVE_FLAG_MEM_SIZE:
        ....
        break;

        case RAM_SAVE_FLAG_ZERO:
            ...
            break;

        case RAM_SAVE_FLAG_PAGE:
            ....
            break;

        case RAM_SAVE_FLAG_COMPRESS_PAGE:
            ....
            break;

        case RAM_SAVE_FLAG_XBZRLE:
            ....
            break;
        case RAM_SAVE_FLAG_MULTIFD_SYNC:
            ...
            break;
        case RAM_SAVE_FLAG_EOS:
            ....
            break;
        default:
            if (flags & RAM_SAVE_FLAG_HOOK) {
                   .....
            }
        }

So the only value that is a flag is the CONTINUE one, there are not
other combinations with other flags.

Yes, the RAM_SAVE_FLAG_HOOK is as weird as it can be.

Later, Juan.