Re: [PATCH V2 03/13] migration: save cpr mode

[Peter Xu <peterx@redhat.com>]

On Mon, Oct 07, 2024 at 03:31:09PM -0400, Steven Sistare wrote:
> On 10/7/2024 11:18 AM, Peter Xu wrote:
> > On Mon, Sep 30, 2024 at 12:40:34PM -0700, Steve Sistare wrote:
> > > Save the mode in CPR state, so the user does not need to explicitly specify
> > > it for the target.  Modify migrate_mode() so it returns the incoming mode on
> > > the target.
> > > 
> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > > ---
> > >   include/migration/cpr.h |  7 +++++++
> > >   migration/cpr.c         | 23 ++++++++++++++++++++++-
> > >   migration/migration.c   |  1 +
> > >   migration/options.c     |  9 +++++++--
> > >   4 files changed, 37 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> > > index e7b898b..ac7a63e 100644
> > > --- a/include/migration/cpr.h
> > > +++ b/include/migration/cpr.h
> > > @@ -8,9 +8,16 @@
> > >   #ifndef MIGRATION_CPR_H
> > >   #define MIGRATION_CPR_H
> > > +#include "qapi/qapi-types-migration.h"
> > > +
> > > +#define MIG_MODE_NONE           -1
> > > +
> > >   #define QEMU_CPR_FILE_MAGIC     0x51435052
> > >   #define QEMU_CPR_FILE_VERSION   0x00000001
> > > +MigMode cpr_get_incoming_mode(void);
> > > +void cpr_set_incoming_mode(MigMode mode);
> > > +
> > >   typedef int (*cpr_walk_fd_cb)(int fd);
> > >   void cpr_save_fd(const char *name, int id, int fd);
> > >   void cpr_delete_fd(const char *name, int id);
> > > diff --git a/migration/cpr.c b/migration/cpr.c
> > > index e50fc75..7514c4e 100644
> > > --- a/migration/cpr.c
> > > +++ b/migration/cpr.c
> > > @@ -21,10 +21,23 @@
> > >   typedef QLIST_HEAD(CprFdList, CprFd) CprFdList;
> > >   typedef struct CprState {
> > > +    MigMode mode;
> > >       CprFdList fds;
> > >   } CprState;
> > > -static CprState cpr_state;
> > > +static CprState cpr_state = {
> > > +    .mode = MIG_MODE_NONE,
> > > +};
> > > +
> > > +MigMode cpr_get_incoming_mode(void)
> > > +{
> > > +    return cpr_state.mode;
> > > +}
> > > +
> > > +void cpr_set_incoming_mode(MigMode mode)
> > > +{
> > > +    cpr_state.mode = mode;
> > > +}
> > >   /****************************************************************************/
> > > @@ -124,11 +137,19 @@ void cpr_resave_fd(const char *name, int id, int fd)
> > >   /*************************************************************************/
> > >   #define CPR_STATE "CprState"
> > > +static int cpr_state_presave(void *opaque)
> > > +{
> > > +    cpr_state.mode = migrate_mode();
> > > +    return 0;
> > > +}
> > > +
> > >   static const VMStateDescription vmstate_cpr_state = {
> > >       .name = CPR_STATE,
> > >       .version_id = 1,
> > >       .minimum_version_id = 1,
> > > +    .pre_save = cpr_state_presave,
> > >       .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT32(mode, CprState),
> > >           VMSTATE_QLIST_V(fds, CprState, 1, vmstate_cpr_fd, CprFd, next),
> > >           VMSTATE_END_OF_LIST()
> > >       }
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 834b0a2..df00e5c 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -416,6 +416,7 @@ void migration_incoming_state_destroy(void)
> > >           mis->postcopy_qemufile_dst = NULL;
> > >       }
> > > +    cpr_set_incoming_mode(MIG_MODE_NONE);
> > >       yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> > >   }
> > > diff --git a/migration/options.c b/migration/options.c
> > > index 147cd2b..cc85a84 100644
> > > --- a/migration/options.c
> > > +++ b/migration/options.c
> > > @@ -22,6 +22,7 @@
> > >   #include "qapi/qmp/qnull.h"
> > >   #include "sysemu/runstate.h"
> > >   #include "migration/colo.h"
> > > +#include "migration/cpr.h"
> > >   #include "migration/misc.h"
> > >   #include "migration.h"
> > >   #include "migration-stats.h"
> > > @@ -768,8 +769,12 @@ uint64_t migrate_max_postcopy_bandwidth(void)
> > >   MigMode migrate_mode(void)
> > >   {
> > > -    MigrationState *s = migrate_get_current();
> > > -    MigMode mode = s->parameters.mode;
> > > +    MigMode mode = cpr_get_incoming_mode();
> > > +
> > > +    if (mode == MIG_MODE_NONE) {
> > > +        MigrationState *s = migrate_get_current();
> > > +        mode = s->parameters.mode;
> > > +    }
> > 
> > Is this trying to avoid interfering with what user specified?
> 
> No.
> 
> > I can kind of get the point of it, but it'll also look pretty werid in this
> > case that user can set the mode but then when query before cpr-transfer
> > incoming completes it won't read what was set previously, but what was
> > migrated via the cpr channel.
> > 
> > And IIUC it is needed to migrate this mode in cpr stream so as to avoid
> > another new qemu cmdline on dest qemu.  If true this needs to be mentioned
> > in the commit message; so far it reads like it's optional, then it's not
> > clear why only cpr-mode needs to be migrated not other migration parameters.
> 
> The mode is needed on the incoming side early -- before migration_object_init,
> and before the monitor is started.  Thus the user cannot set it as a normal
> migration parameter.
> 
> > If that won't get right easily, I wonder whether we could just overwrite
> > parameters.mode directly by the cpr stream.
> 
> I considered that, but parameters.mode cannot be set before migration_object_init,
> and some code needs to know mode before that.

Ah OK...

I wonder whether it really helps in migrating this mode at all, knowing
that no other mode should be there but the cpr-transfer mode when with
-cpr-uri cmdline.

How about we use cpr_uri to detect early stage cpr transfer mode, then
after early load stage we unset cpr_uri and always stick with what user
specified (instead of special casing NONE mode)?  Then it looks like:

MigMode migrate_mode(void)
{
  /*
   * When cpr_uri set, it always means QEMU is currently in early
   * cpr-transfer loading stage.
   */ 
  if (cpr_uri) {
      return MIG_MODE_CPR_TRANSFER;
  }

  return migrate_get_current()->parameters.mode;
}

Then we don't need to migrate the mode either, which is good as it aligns
with other migration parameters.

Would this look slightly cleaner?

-- 
Peter Xu