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

[Hailiang Zhang]

On 2017/1/31 18:04, Dr. David Alan Gilbert wrote:
> * zhanghailiang (address@hidden) 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 <address@hidden>
> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >   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.

OK, will fix it in next version, thanks.

> 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 / address@hidden / Manchester, UK
>
> .