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

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

On 10/7/2024 4:10 PM, Peter Xu wrote:
> 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?

Sure.  Mode does not need to be sent in cpr_state.

- Steve