Re: [Qemu-devel] [PATCH 1/2] savevm: split save/find loadvm_handlers entry into two helper functions

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> COLO's checkpoint process is based on migration process,
> everytime we do checkpoint we will repeat the process of savevm and loadvm.
> 
> So we will call qemu_loadvm_section_start_full() repeatedly, It will
> add all migration sections information into loadvm_handlers list everytime,
> which will lead to memory leak.
> 
> To fix it, we split the process of saving and finding section entry into two
> helper functions, we will check if section info was exist in loadvm_handlers
> list before save it.
> 
> This modifications have no side effect for normal migration.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/savevm.c | 55 +++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f9c06e9..92b3d6c 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1805,6 +1805,37 @@ void loadvm_free_handlers(MigrationIncomingState *mis)
>      }
>  }
>  
> +static LoadStateEntry *loadvm_save_section_entry(MigrationIncomingState *mis,

Can you change that to loadvm_add_section_entry please;  it's a bit
confusing using 'save' in a function name that's part of the 'load' path.

Dave

> +                                                 SaveStateEntry *se,
> +                                                 uint32_t section_id,
> +                                                 uint32_t version_id)
> +{
> +    LoadStateEntry *le;
> +
> +    /* Add entry */
> +    le = g_malloc0(sizeof(*le));
> +
> +    le->se = se;
> +    le->section_id = section_id;
> +    le->version_id = version_id;
> +    QLIST_INSERT_HEAD(&mis->loadvm_handlers, le, entry);
> +    return le;
> +}
> +
> +static LoadStateEntry *loadvm_find_section_entry(MigrationIncomingState *mis,
> +                                                 uint32_t section_id)
> +{
> +    LoadStateEntry *le;
> +
> +    QLIST_FOREACH(le, &mis->loadvm_handlers, entry) {
> +        if (le->section_id == section_id) {
> +            break;
> +        }
> +    }
> +
> +    return le;
> +}
> +
>  static int
>  qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
>  {
> @@ -1847,15 +1878,12 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
>          return -EINVAL;
>      }
>  
> -    /* Add entry */
> -    le = g_malloc0(sizeof(*le));
> -
> -    le->se = se;
> -    le->section_id = section_id;
> -    le->version_id = version_id;
> -    QLIST_INSERT_HEAD(&mis->loadvm_handlers, le, entry);
> -
> -    ret = vmstate_load(f, le->se, le->version_id);
> +     /* Check if we have saved this section info before, if not, save it */
> +    le = loadvm_find_section_entry(mis, section_id);
> +    if (!le) {
> +        le = loadvm_save_section_entry(mis, se, section_id, version_id);
> +    }
> +    ret = vmstate_load(f, se, version_id);
>      if (ret < 0) {
>          error_report("error while loading state for instance 0x%x of"
>                       " device '%s'", instance_id, idstr);
> @@ -1878,12 +1906,9 @@ qemu_loadvm_section_part_end(QEMUFile *f, MigrationIncomingState *mis)
>      section_id = qemu_get_be32(f);
>  
>      trace_qemu_loadvm_state_section_partend(section_id);
> -    QLIST_FOREACH(le, &mis->loadvm_handlers, entry) {
> -        if (le->section_id == section_id) {
> -            break;
> -        }
> -    }
> -    if (le == NULL) {
> +
> +    le = loadvm_find_section_entry(mis, section_id);
> +    if (!le) {
>          error_report("Unknown savevm section %d", section_id);
>          return -EINVAL;
>      }
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK