Re: [PATCH v1 1/8] migration: Implemented new parameter stream_content

Re: [PATCH v1 1/8] migration: Implemented new parameter stream_content

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>

You'd better change author of commit, to keep signed-off-by email and author email to be the same thing.

> 
> This new optional parameter contains inormation about migration
> stream parts to be sent (such as RAM, block, bitmap). This looks
> better than using capabilities to solve problem of dividing
> migration stream.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   migration/migration.c | 47 ++++++++++++++++++++++++++++++++++++++++++-
>   migration/migration.h |  2 ++
>   qapi/migration.json   | 21 ++++++++++++++++---
>   3 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 695f0f2900..4adcc87d1d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1334,6 +1334,12 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>       }
>   }
>   
> +static bool check_stream_parts(strList *stream_content_list)
> +{
> +    /* To be implemented in ext commits */
> +    return true;
> +}
> +
>   /*
>    * Check whether the parameters are valid. Error will be put into errp
>    * (if provided). Return true if valid, otherwise false.
> @@ -1482,7 +1488,12 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>           return false;
>       }
>   
> -    return true;
> +    if (params->has_stream_content_list &&
> +        !check_stream_parts(params->stream_content_list)) {
> +        error_setg(errp, "Invalid parts of stream given for stream-content");
> +    }
> +
> +   return true;
>   }
>   
>   static void migrate_params_test_apply(MigrateSetParameters *params,
> @@ -1581,6 +1592,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>           dest->has_block_bitmap_mapping = true;
>           dest->block_bitmap_mapping = params->block_bitmap_mapping;
>       }
> +
> +    if (params->has_stream_content_list) {
> +        dest->has_stream_content_list = true;
> +        dest->stream_content_list = params->stream_content_list;
> +    }
>   }
>   
>   static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> @@ -1703,6 +1719,13 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>               QAPI_CLONE(BitmapMigrationNodeAliasList,
>                          params->block_bitmap_mapping);
>       }
> +
> +    if (params->has_stream_content_list) {
> +        qapi_free_strList(s->parameters.stream_content_list);
> +        s->parameters.has_stream_content_list = true;
> +        s->parameters.stream_content_list =
> +            QAPI_CLONE(strList, params->stream_content_list);
> +    }
>   }
>   
>   void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> @@ -2620,6 +2643,28 @@ bool migrate_background_snapshot(void)
>       return s->enabled_capabilities[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT];
>   }
>   
> +/* Checks if stream-content parameter has section_name in list */
> +bool migrate_find_stream_content(const char *section_name)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    if (!s->parameters.has_stream_content_list) {
> +        return false;
> +    }
> +
> +    strList *list = s->parameters.stream_content_list;
> +
> +    for (; list; list = list->next) {
> +        if (!strcmp(list->value, section_name)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>   /* migration thread support */
>   /*
>    * Something bad happened to the RP stream, mark an error
> diff --git a/migration/migration.h b/migration/migration.h
> index 2de861df01..411c58e919 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -396,6 +396,8 @@ bool migrate_use_events(void);
>   bool migrate_postcopy_blocktime(void);
>   bool migrate_background_snapshot(void);
>   
> +bool migrate_find_stream_content(const char *section_name);
> +
>   /* Sending on the return path - generic and then for each message type */
>   void migrate_send_rp_shut(MigrationIncomingState *mis,
>                             uint32_t value);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 18e2610e88..80acf6dbc3 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -760,6 +760,12 @@
>   #                        block device name if there is one, and to their node name
>   #                        otherwise. (Since 5.2)
>   #
> +# @stream-content-list: Parameter control content of migration stream such as RAM,
> +#                       vmstate, block and dirty-bitmaps. This is optional parameter
> +#                       so migration will work correctly without it.
> +#                       This parameter takes string list as description of content
> +#                       and include that part of migration stream. (Since 7.0)

Should be 7.1.

I also think that simpler "stream-content" name would be enough

> +#
>   # Features:
>   # @unstable: Member @x-checkpoint-delay is experimental.
>   #
> @@ -780,7 +786,8 @@
>              'xbzrle-cache-size', 'max-postcopy-bandwidth',
>              'max-cpu-throttle', 'multifd-compression',
>              'multifd-zlib-level' ,'multifd-zstd-level',
> -           'block-bitmap-mapping' ] }
> +           'block-bitmap-mapping',
> +           'stream-content-list' ] }
>   
>   ##
>   # @MigrateSetParameters:
> @@ -925,6 +932,12 @@
>   #                        block device name if there is one, and to their node name
>   #                        otherwise. (Since 5.2)
>   #
> +# @stream-content-list: Parameter control content of migration stream such as RAM,
> +#                       vmstate, block and dirty-bitmaps. This is optional parameter
> +#                       so migration will work correctly without it.
> +#                       This parameter takes string list as description of content
> +#                       and include that part of migration stream. (Since 7.0)
> +#
>   # Features:
>   # @unstable: Member @x-checkpoint-delay is experimental.
>   #
> @@ -960,7 +973,8 @@
>               '*multifd-compression': 'MultiFDCompression',
>               '*multifd-zlib-level': 'uint8',
>               '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*stream-content-list': [ 'str' ] } }

Don't make it a list of strings. Better define enum, like

{ 'enum': 'StreamContentType',
   'data': [ 'block-dirty-bitmaps', 'ram', .. ] }

And make stream-content to be list of this new enum type

>   
>   ##
>   # @migrate-set-parameters:
> @@ -1158,7 +1172,8 @@
>               '*multifd-compression': 'MultiFDCompression',
>               '*multifd-zlib-level': 'uint8',
>               '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*stream-content-list': [ 'str' ] } }
>   
>   ##
>   # @query-migrate-parameters:


That's not a real problem, but IMHO it's better to add QAPI interface together with the feature itself (although some functions may be implemented in earlier patches). Otherwise, starting from this commit we do have a new documented QAPI interface, but it actually doesn't work until some future commit.


-- 
Best regards,
Vladimir

Re: [PATCH v1 2/8] migration: should_skip() implemented

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> For next changes it is convenient to make all decisions about
> sections skipping in one function.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   migration/savevm.c | 54 ++++++++++++++++++++++++----------------------
>   1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 02ed94c180..c68f187ef7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -943,6 +943,15 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se,
>       return vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
>   }
>   
> +static bool should_skip(SaveStateEntry *se)
> +{
> +    if (se->ops && se->ops->is_active && !se->ops->is_active(se->opaque)) {
> +        return true;
> +    }
> +
> +    return false;

that may be simply

    return se->ops && se->ops->is_active && !se->ops->is_active(se->opaque);

But may be in future commits the code will be more complicated and we prepare for it now, will see.

> +}
> +
>   /*
>    * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL)
>    */

[..]

>           trace_savevm_section_start(se->idstr, se->section_id);
>   
> @@ -1417,6 +1417,9 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>               trace_savevm_section_skip(se->idstr, se->section_id);
>               continue;
>           }
> +        if (should_skip(se)) {
> +            continue;
> +        }

Except for this and ...

>   
>           trace_savevm_section_start(se->idstr, se->section_id);
>   
> @@ -1522,10 +1525,8 @@ void qemu_savevm_state_pending(QEMUFile *f, uint64_t threshold_size,
>           if (!se->ops || !se->ops->save_live_pending) {
>               continue;
>           }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (should_skip(se)) {
> +            continue;
>           }
>           se->ops->save_live_pending(f, se->opaque, threshold_size,
>                                      res_precopy_only, res_compatible,
> @@ -1635,6 +1636,9 @@ int qemu_save_device_state(QEMUFile *f)
>           if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) {
>               continue;
>           }
> +        if (should_skip(se)) {
> +            continue;
> +        }

this all other chunks are simple refactoring.

Let's do no-logic-change refactoring in a separate commit, and these two changes in another one, with description what and why.

>   
>           save_section_header(f, se, QEMU_VM_SECTION_FULL);
>   
> @@ -2542,10 +2546,8 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
>           if (!se->ops || !se->ops->load_setup) {
>               continue;
>           }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (should_skip(se)) {
> +            continue;
>           }
>   
>           ret = se->ops->load_setup(f, se->opaque);


-- 
Best regards,
Vladimir

Re: [PATCH v1 3/8] migration: Add vmstate part of migration stream

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> Now we can disable and enable vmstate part by stream_content parameter.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   migration/migration.c | 10 ++++++++--
>   migration/savevm.c    | 13 +++++++++++++
>   2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4adcc87d1d..bbf9b6aad1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1334,9 +1334,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>       }
>   }
>   
> -static bool check_stream_parts(strList *stream_content_list)
> +static bool check_stream_parts(strList *stream_list)
>   {
> -    /* To be implemented in ext commits */
> +    for (; stream_list; stream_list = stream_list->next) {
> +        if (!strcmp(stream_list->value, "vmstate")) {
> +            continue;
> +        }
> +
> +        return false;
> +    }
>       return true;
>   }

When you move to enum for list elements in QAPI, this thing would be checked automatically, you will not have to check it by hand.

>   
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c68f187ef7..8f35c0c81e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -949,6 +949,19 @@ static bool should_skip(SaveStateEntry *se)
>           return true;
>       }
>   
> +    /*
> +     * Assume that any SaveStateEntry with non-null vmsd is
> +     * part of vmstate.
> +     * Vmstate is included by default so firstly check if
> +     * stream-content-list is enabled.
> +     */
> +
> +    if (se->vmsd &&
> +        migrate_get_current()->parameters.has_stream_content_list &&
> +        !migrate_find_stream_content("vmstate")) {

And in migrate_find_stream_content you do
     if (!s->parameters.has_stream_content_list) {
         return false;
     }

Seems better to do
     if (!s->parameters.has_stream_content_list) {
         return true;
     }

in migrate_find_stream_content(), and rename it to something like should_migrate_content() or just migrate_content().

Then here you don't need to check .has_stream_content_list.

> +        return true;
> +    }
> +
>       return false;
>   }
>   


-- 
Best regards,
Vladimir

Re: [PATCH v1 3/8] migration: Add vmstate part of migration stream

[Vladimir Sementsov-Ogievskiy]

23.03.2022 15:40, Vladimir Sementsov-Ogievskiy wrote:
> 23.03.2022 13:53, Nikita Lapshin wrote:
>> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
>>
>> Now we can disable and enable vmstate part by stream_content parameter.
>>
>> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
>> ---
>>   migration/migration.c | 10 ++++++++--
>>   migration/savevm.c    | 13 +++++++++++++
>>   2 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 4adcc87d1d..bbf9b6aad1 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1334,9 +1334,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>>       }
>>   }
>> -static bool check_stream_parts(strList *stream_content_list)
>> +static bool check_stream_parts(strList *stream_list)
>>   {
>> -    /* To be implemented in ext commits */
>> +    for (; stream_list; stream_list = stream_list->next) {
>> +        if (!strcmp(stream_list->value, "vmstate")) {
>> +            continue;
>> +        }
>> +
>> +        return false;
>> +    }
>>       return true;
>>   }
> 
> When you move to enum for list elements in QAPI, this thing would be checked automatically, you will not have to check it by hand.
> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index c68f187ef7..8f35c0c81e 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -949,6 +949,19 @@ static bool should_skip(SaveStateEntry *se)
>>           return true;
>>       }
>> +    /*
>> +     * Assume that any SaveStateEntry with non-null vmsd is
>> +     * part of vmstate.
>> +     * Vmstate is included by default so firstly check if
>> +     * stream-content-list is enabled.
>> +     */
>> +
>> +    if (se->vmsd &&
>> +        migrate_get_current()->parameters.has_stream_content_list &&
>> +        !migrate_find_stream_content("vmstate")) {
> 
> And in migrate_find_stream_content you do
>      if (!s->parameters.has_stream_content_list) {
>          return false;
>      }
> 
> Seems better to do
>      if (!s->parameters.has_stream_content_list) {
>          return true;
>      }
> 
> in migrate_find_stream_content(), and rename it to something like should_migrate_content() or just migrate_content().
> 
> Then here you don't need to check .has_stream_content_list.

Hmm, but that will work bad with dirty-bitmaps, as they are disabled by default.

So, this actually means that we have different default for different contents:

ram and and other device states are enabled by default, dirty-bitmaps are disabled by defaults. We can honestly realize these defaults in migrate_content().

> 
>> +        return true;
>> +    }
>> +
>>       return false;
>>   }
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH v1 3/8] migration: Add vmstate part of migration stream

[Vladimir Sementsov-Ogievskiy]

23.03.2022 16:21, Nikita Lapshin wrote:
> 
> On 3/23/22 15:49, Vladimir Sementsov-Ogievskiy wrote:
> 
>> 23.03.2022 15:40, Vladimir Sementsov-Ogievskiy wrote:
>>> 23.03.2022 13:53, Nikita Lapshin wrote:
>>>> From: Nikita Lapshin<nikita.lapshin@virtuozzo.com>
>>>> Now we can disable and enable vmstate part by stream_content parameter.
>>>> Signed-off-by: Nikita Lapshin<nikita.lapshin@openvz.org>
>>>> ---
>>>>    migration/migration.c | 10 ++++++++--
>>>>    migration/savevm.c    | 13 +++++++++++++
>>>>    2 files changed, 21 insertions(+), 2 deletions(-)
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 4adcc87d1d..bbf9b6aad1 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -1334,9 +1334,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>>>>        }
>>>>    }
>>>> -static bool check_stream_parts(strList *stream_content_list)
>>>> +static bool check_stream_parts(strList *stream_list)
>>>>    {
>>>> -    /* To be implemented in ext commits */
>>>> +    for (; stream_list; stream_list = stream_list->next) {
>>>> +        if (!strcmp(stream_list->value, "vmstate")) {
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        return false;
>>>> +    }
>>>>        return true;
>>>>    }
>>> When you move to enum for list elements in QAPI, this thing would be checked automatically, you will not have to check it by hand.
>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>> index c68f187ef7..8f35c0c81e 100644
>>>> --- a/migration/savevm.c
>>>> +++ b/migration/savevm.c
>>>> @@ -949,6 +949,19 @@ static bool should_skip(SaveStateEntry *se)
>>>>            return true;
>>>>        }
>>>> +    /*
>>>> +     * Assume that any SaveStateEntry with non-null vmsd is
>>>> +     * part of vmstate.
>>>> +     * Vmstate is included by default so firstly check if
>>>> +     * stream-content-list is enabled.
>>>> +     */
>>>> +
>>>> +    if (se->vmsd &&
>>>> +        migrate_get_current()->parameters.has_stream_content_list &&
>>>> +        !migrate_find_stream_content("vmstate")) {
>>> And in migrate_find_stream_content you do
>>>       if (!s->parameters.has_stream_content_list) {
>>>           return false;
>>>       }
>>> Seems better to do
>>>       if (!s->parameters.has_stream_content_list) {
>>>           return true;
>>>       }
>>> in migrate_find_stream_content(), and rename it to something like should_migrate_content() or just migrate_content().
>>> Then here you don't need to check .has_stream_content_list.
>> Hmm, but that will work bad with dirty-bitmaps, as they are disabled by default.
>> So, this actually means that we have different default for different contents:
>> ram and and other device states are enabled by default, dirty-bitmaps are disabled by defaults. We can honestly realize these defaults in migrate_content().
> 
> Yes, I agree that this logic looks quite strange. May be explicit checks can help.


I think, checking for deprecated capabilites should be in same function migrate_content() too.

> 
>>>> +        return true;
>>>> +    }
>>>> +
>>>>        return false;
>>>>    }
>>


-- 
Best regards,
Vladimir

Re: [PATCH v1 4/8] migration: Add dirty-bitmaps part of migration stream

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> This enable and disable dirty-bitmaps in migration stream.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   migration/migration.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index bbf9b6aad1..ad789915ce 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1337,7 +1337,8 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>   static bool check_stream_parts(strList *stream_list)
>   {
>       for (; stream_list; stream_list = stream_list->next) {
> -        if (!strcmp(stream_list->value, "vmstate")) {
> +        if (!strcmp(stream_list->value, "vmstate") ||
> +            !strcmp(stream_list->value, "dirty-bitmaps")) {
>               continue;
>           }
>   
> @@ -2501,7 +2502,8 @@ bool migrate_dirty_bitmaps(void)
>   
>       s = migrate_get_current();
>   
> -    return s->enabled_capabilities[MIGRATION_CAPABILITY_DIRTY_BITMAPS];
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_DIRTY_BITMAPS] ||
> +           migrate_find_stream_content("dirty-bitmaps");
>   }
>   
>   bool migrate_ignore_shared(void)

Probably we should restrict using dirty-bitmaps capability together with new stream-content parameter to be strict.


-- 
Best regards,
Vladimir

Re: [PATCH v1 5/8] migration: Add block part of migration stream

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> This enable and disable block in migration stream.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   migration/migration.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index ad789915ce..d81f3c6891 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1338,7 +1338,8 @@ static bool check_stream_parts(strList *stream_list)
>   {
>       for (; stream_list; stream_list = stream_list->next) {
>           if (!strcmp(stream_list->value, "vmstate") ||
> -            !strcmp(stream_list->value, "dirty-bitmaps")) {
> +            !strcmp(stream_list->value, "dirty-bitmaps") ||
> +            !strcmp(stream_list->value, "block")) {
>               continue;
>           }
>   
> @@ -2621,7 +2622,8 @@ bool migrate_use_block(void)
>   
>       s = migrate_get_current();
>   
> -    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK];
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK] ||
> +           migrate_find_stream_content("block");
>   }
>   
>   bool migrate_use_return_path(void)


Same as for bitmaps. Yes, I now think we should restrict using new stream-content parameter together with old capabilities, and deprecated this old capabilities.

-- 
Best regards,
Vladimir

Re: [PATCH v1 6/8] migration: Add RAM part of migration stream

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> 'ram' parameter enable RAM sections in migration stream. If it
> isn't specified it will be skipped.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   migration/migration.c | 17 ++++++++++++++++-
>   migration/migration.h |  1 +
>   migration/ram.c       |  6 ++++++
>   3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index d81f3c6891..6528b3ad41 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1339,7 +1339,8 @@ static bool check_stream_parts(strList *stream_list)
>       for (; stream_list; stream_list = stream_list->next) {
>           if (!strcmp(stream_list->value, "vmstate") ||
>               !strcmp(stream_list->value, "dirty-bitmaps") ||
> -            !strcmp(stream_list->value, "block")) {
> +            !strcmp(stream_list->value, "block") ||
> +            !strcmp(stream_list->value, "ram")) {
>               continue;
>           }
>   
> @@ -2653,6 +2654,20 @@ bool migrate_background_snapshot(void)
>       return s->enabled_capabilities[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT];
>   }
>   
> +bool migrate_ram(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    /*
> +     * By default RAM is enabled so if stream-content-list disabled
> +     * RAM will be passed.
> +     */
> +    return !s->parameters.has_stream_content_list ||
> +           migrate_find_stream_content("ram");
> +}

I think, better is avoid this extra function

> +
>   /* Checks if stream-content parameter has section_name in list */
>   bool migrate_find_stream_content(const char *section_name)
>   {
> diff --git a/migration/migration.h b/migration/migration.h
> index 411c58e919..5c43788a2b 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -395,6 +395,7 @@ int migrate_decompress_threads(void);
>   bool migrate_use_events(void);
>   bool migrate_postcopy_blocktime(void);
>   bool migrate_background_snapshot(void);
> +bool migrate_ram(void);
>   
>   bool migrate_find_stream_content(const char *section_name);
>   
> diff --git a/migration/ram.c b/migration/ram.c
> index 170e522a1f..ddc7abd08a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4263,6 +4263,11 @@ static int ram_resume_prepare(MigrationState *s, void *opaque)
>       return 0;
>   }
>   
> +static bool is_ram_active(void *opaque)
> +{
> +    return migrate_ram();

and here just call should_migrate_content("ram");

> +}
> +
>   static SaveVMHandlers savevm_ram_handlers = {
>       .save_setup = ram_save_setup,
>       .save_live_iterate = ram_save_iterate,
> @@ -4275,6 +4280,7 @@ static SaveVMHandlers savevm_ram_handlers = {
>       .load_setup = ram_load_setup,
>       .load_cleanup = ram_load_cleanup,
>       .resume_prepare = ram_resume_prepare,
> +    .is_active = is_ram_active,
>   };
>   
>   static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,


-- 
Best regards,
Vladimir

Re: [PATCH v1 7/8] migration: analyze-migration script changed

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:53, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> This script is used for RAM capabilities test. But it cannot work
> in case of no vm description in migration stream.
> So new flag is added to allow work this script with ram-only
> migration stream.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   scripts/analyze-migration.py | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index b82a1b0c58..80077a09bc 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -495,7 +495,7 @@ def __init__(self, filename):
>           self.filename = filename
>           self.vmsd_desc = None
>   
> -    def read(self, desc_only = False, dump_memory = False, write_memory = False):
> +    def read(self, ram_only, desc_only = False, dump_memory = False, write_memory = False):
>           # Read in the whole file
>           file = MigrationFile(self.filename)
>   
> @@ -509,7 +509,8 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>           if data != self.QEMU_VM_FILE_VERSION:
>               raise Exception("Invalid version number %d" % data)
>   
> -        self.load_vmsd_json(file)
> +        if not ram_only:
> +            self.load_vmsd_json(file)
>   
>           # Read sections
>           self.sections = collections.OrderedDict()
> @@ -518,7 +519,10 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>               return
>   
>           ramargs = {}
> -        ramargs['page_size'] = self.vmsd_desc['page_size']
> +        if ram_only:
> +            ramargs['page_size'] = 4096
> +        else:
> +            ramargs['page_size'] = self.vmsd_desc['page_size']
>           ramargs['dump_memory'] = dump_memory
>           ramargs['write_memory'] = write_memory
>           self.section_classes[('ram',0)][1] = ramargs
> @@ -579,6 +583,7 @@ def default(self, o):
>   parser.add_argument("-m", "--memory", help='dump RAM contents as well', action='store_true')
>   parser.add_argument("-d", "--dump", help='what to dump ("state" or "desc")', default='state')
>   parser.add_argument("-x", "--extract", help='extract contents into individual files', action='store_true')
> +parser.add_argument("--ram-only", help='parse migration dump containing only RAM', action='store_true')
>   args = parser.parse_args()
>   
>   jsonenc = JSONEncoder(indent=4, separators=(',', ': '))
> @@ -586,14 +591,14 @@ def default(self, o):
>   if args.extract:
>       dump = MigrationDump(args.file)

could this ram_only instead be stored into object, so that we do

dump = MigrationDump(args.file, ram_only=args.ram_only)

and don't update each read call?

>   
> -    dump.read(desc_only = True)
> +    dump.read(desc_only = True, ram_only = args.ram_only)
>       print("desc.json")
>       f = open("desc.json", "w")
>       f.truncate()
>       f.write(jsonenc.encode(dump.vmsd_desc))
>       f.close()
>   
> -    dump.read(write_memory = True)
> +    dump.read(write_memory = True, ram_only = args.ram_only)
>       dict = dump.getDict()
>       print("state.json")
>       f = open("state.json", "w")
> @@ -602,12 +607,12 @@ def default(self, o):
>       f.close()
>   elif args.dump == "state":
>       dump = MigrationDump(args.file)
> -    dump.read(dump_memory = args.memory)
> +    dump.read(dump_memory = args.memory, ram_only = args.ram_only)
>       dict = dump.getDict()
>       print(jsonenc.encode(dict))
>   elif args.dump == "desc":
>       dump = MigrationDump(args.file)
> -    dump.read(desc_only = True)
> +    dump.read(desc_only = True, ram_only = args.ram_only)
>       print(jsonenc.encode(dump.vmsd_desc))
>   else:
>       raise Exception("Please specify either -x, -d state or -d desc")


-- 
Best regards,
Vladimir

Re: [PATCH v1 7/8] migration: analyze-migration script changed

[Nikita Lapshin]

[-- Attachment #1: Type: text/plain, Size: 4431 bytes --]

On 3/23/22 16:57, Vladimir Sementsov-Ogievskiy wrote:

> 23.03.2022 13:53, Nikita Lapshin wrote:
>> From: Nikita Lapshin<nikita.lapshin@virtuozzo.com>
>> This script is used for RAM capabilities test. But it cannot work
>> in case of no vm description in migration stream.
>> So new flag is added to allow work this script with ram-only
>> migration stream.
>> Signed-off-by: Nikita Lapshin<nikita.lapshin@openvz.org>
>> ---
>>    scripts/analyze-migration.py | 19 ++++++++++++-------
>>    1 file changed, 12 insertions(+), 7 deletions(-)
>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
>> index b82a1b0c58..80077a09bc 100755
>> --- a/scripts/analyze-migration.py
>> +++ b/scripts/analyze-migration.py
>> @@ -495,7 +495,7 @@ def __init__(self, filename):
>>            self.filename = filename
>>            self.vmsd_desc = None
>>    
>> -    def read(self, desc_only = False, dump_memory = False, write_memory = False):
>> +    def read(self, ram_only, desc_only = False, dump_memory = False, write_memory = False):
>>            # Read in the whole file
>>            file = MigrationFile(self.filename)
>>    
>> @@ -509,7 +509,8 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>>            if data != self.QEMU_VM_FILE_VERSION:
>>                raise Exception("Invalid version number %d" % data)
>>    
>> -        self.load_vmsd_json(file)
>> +        if not ram_only:
>> +            self.load_vmsd_json(file)
>>    
>>            # Read sections
>>            self.sections = collections.OrderedDict()
>> @@ -518,7 +519,10 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>>                return
>>    
>>            ramargs = {}
>> -        ramargs['page_size'] = self.vmsd_desc['page_size']
>> +        if ram_only:
>> +            ramargs['page_size'] = 4096
>> +        else:
>> +            ramargs['page_size'] = self.vmsd_desc['page_size']
>>            ramargs['dump_memory'] = dump_memory
>>            ramargs['write_memory'] = write_memory
>>            self.section_classes[('ram',0)][1] = ramargs
>> @@ -579,6 +583,7 @@ def default(self, o):
>>    parser.add_argument("-m", "--memory", help='dump RAM contents as well', action='store_true')
>>    parser.add_argument("-d", "--dump", help='what to dump ("state" or "desc")', default='state')
>>    parser.add_argument("-x", "--extract", help='extract contents into individual files', action='store_true')
>> +parser.add_argument("--ram-only", help='parse migration dump containing only RAM', action='store_true')
>>    args = parser.parse_args()
>>    
>>    jsonenc = JSONEncoder(indent=4, separators=(',', ': '))
>> @@ -586,14 +591,14 @@ def default(self, o):
>>    if args.extract:
>>        dump = MigrationDump(args.file)
> could this ram_only instead be stored into object, so that we do
> dump = MigrationDump(args.file, ram_only=args.ram_only)
> and don't update each read call?

Yes, it could, don't see any problem with this.

>>    
>> -    dump.read(desc_only = True)
>> +    dump.read(desc_only = True, ram_only = args.ram_only)
>>        print("desc.json")
>>        f = open("desc.json", "w")
>>        f.truncate()
>>        f.write(jsonenc.encode(dump.vmsd_desc))
>>        f.close()
>>    
>> -    dump.read(write_memory = True)
>> +    dump.read(write_memory = True, ram_only = args.ram_only)
>>        dict = dump.getDict()
>>        print("state.json")
>>        f = open("state.json", "w")
>> @@ -602,12 +607,12 @@ def default(self, o):
>>        f.close()
>>    elif args.dump == "state":
>>        dump = MigrationDump(args.file)
>> -    dump.read(dump_memory = args.memory)
>> +    dump.read(dump_memory = args.memory, ram_only = args.ram_only)
>>        dict = dump.getDict()
>>        print(jsonenc.encode(dict))
>>    elif args.dump == "desc":
>>        dump = MigrationDump(args.file)
>> -    dump.read(desc_only = True)
>> +    dump.read(desc_only = True, ram_only = args.ram_only)
>>        print(jsonenc.encode(dump.vmsd_desc))
>>    else:
>>        raise Exception("Please specify either -x, -d state or -d desc")
>

[-- Attachment #2: Type: text/html, Size: 6555 bytes --]

Re: [PATCH v1 8/8] migration: Test for RAM and vmstate parts

[Vladimir Sementsov-Ogievskiy]

23.03.2022 13:54, Nikita Lapshin wrote:
> From: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> 
> All other parts works just like existed capabilities.
> Though RAM and vmstate are new so here is new test for
> that parts.
> 
> Signed-off-by: Nikita Lapshin <nikita.lapshin@openvz.org>
> ---
>   .../tests/migrate-ram-stream-content-test     | 96 +++++++++++++++++++
>   .../tests/migrate-ram-stream-content-test.out |  5 +
>   2 files changed, 101 insertions(+)
>   create mode 100755 tests/qemu-iotests/tests/migrate-ram-stream-content-test
>   create mode 100644 tests/qemu-iotests/tests/migrate-ram-stream-content-test.out
> 
> diff --git a/tests/qemu-iotests/tests/migrate-ram-stream-content-test b/tests/qemu-iotests/tests/migrate-ram-stream-content-test
> new file mode 100755
> index 0000000000..2855ca4a64
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/migrate-ram-stream-content-test
> @@ -0,0 +1,96 @@
> +#!/usr/bin/env python3
> +# group: rw migration
> +#
> +# Tests for 'no-ram' and 'ram-only' capabilities
> +#
> +# Copyright (c) 2021 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import json
> +import subprocess
> +import iotests
> +
> +img = os.path.join(iotests.test_dir, 'disk.img')
> +
> +class TestRamCapabilities(iotests.QMPTestCase):
> +    def setUp(self):
> +        iotests.qemu_img('create', '-f', iotests.imgfmt, img, '10M')
> +        self.vm = iotests.VM()
> +        self.vm.launch()
> +        self.vm.qmp('migrate-set-capabilities', capabilities=[
> +            {
> +                'capability': 'events',
> +                'state': True
> +            }
> +        ])
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(img)
> +
> +    def check_ram_only(self, output):
> +        str_json = output.decode()
> +        json_obj = json.loads(str_json)
> +
> +        success = False
> +        for key in json_obj:
> +            self.assertTrue("ram" in key)
> +            success = True
> +        self.assertTrue(success)

I'd write it like this:

self.assertTrue(len(json_obj) > 0)
self.assertTrue(all('ram' in key for key in json_obj))

> +
> +    def run_migration(self, no_ram, tmp_stream):
> +        if no_ram:
> +            output = self.vm.qmp('migrate-set-parameters',
> +                    stream_content_list = ['vmstate'])
> +        else:
> +            self.vm.qmp('migrate-set-parameters',
> +                    stream_content_list = ['ram'])

It would look better if just pass stream_content list argument to the function.

> +
> +        self.vm.qmp('migrate', uri='exec:cat>' + tmp_stream)
> +
> +        while True:
> +            event = self.vm.event_wait('MIGRATION')
> +
> +            if event['data']['status'] == 'completed':
> +                break
> +
> +
> +    def test_no_ram(self):
> +        with iotests.FilePath('tmp_stream') as tmp_stream:
> +            self.run_migration(True, tmp_stream)
> +            output = subprocess.run(
> +                ['../../../scripts/analyze-migration.py', '-f', tmp_stream],
> +                stdout=subprocess.PIPE,
> +                stderr=subprocess.STDOUT,
> +                check=False).stdout
> +
> +            self.assertFalse('ram' in output.decode())
> +
> +    def test_ram_only(self):
> +        with iotests.FilePath('tmp_stream') as tmp_stream:
> +            self.run_migration(False, tmp_stream)
> +            output = subprocess.run(
> +                ['../../../scripts/analyze-migration.py', '-f', tmp_stream,
> +                    '--ram-only'],
> +                stdout=subprocess.PIPE,
> +                stderr=subprocess.STDOUT,
> +                check=False).stdout
> +
> +            self.check_ram_only(output)


Hmm both test cases are mostly the same - may be refactored a bit mor eto not duplicate.


> +
> +if __name__ == '__main__':
> +    iotests.main(supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/tests/migrate-ram-stream-content-test.out b/tests/qemu-iotests/tests/migrate-ram-stream-content-test.out
> new file mode 100644
> index 0000000000..fbc63e62f8
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/migrate-ram-stream-content-test.out
> @@ -0,0 +1,5 @@
> +..
> +----------------------------------------------------------------------
> +Ran 2 tests
> +
> +OK

Looks good in general

-- 
Best regards,
Vladimir