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

[Steven Sistare <steven.sistare@oracle.com>]

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.

- Steve

>After all IIUC that's before
> QMP is available, so there's no legal way to set it, then no legal way that
> it overwrites an user input?
> 
>>   
>>       assert(mode >= 0 && mode < MIG_MODE__MAX);
>>       return mode;
>> -- 
>> 1.8.3.1
>>
>