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