Re: [Qemu-devel] [PATCH 2/2] migration: extend migration_bitmap

[Li Zhijian]

On 06/26/2015 05:05 PM, Juan Quintela wrote:
> Li Zhijian <address@hidden> wrote:
> > Prevously, if we hotplug a device(e.g. device_add e1000) during
> > migration is processing in source side, qemu will add a new ram
> > block but migration_bitmap is not extended.
> > In this case, migration_bitmap will overflow and lead qemu abort
> > unexpectedly.
> >
> > Signed-off-by: Li Zhijian <address@hidden>
> > Signed-off-by: Wen Congyang <address@hidden>
> Just curious, how are you testing this?
> because you need a way of doing the hot-plug "kind of" atomically on
> both source and destination, no?
>
>
> > ---
> >   exec.c                  |  7 ++++++-
> >   include/exec/exec-all.h |  1 +
> >   migration/ram.c         | 16 ++++++++++++++++
> >   3 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/exec.c b/exec.c
> > index f7883d2..04d5c05 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1401,6 +1401,11 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, 
> > Error **errp)
> >           }
> >       }
> >   
> > +    new_ram_size = MAX(old_ram_size,
> > +              (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS);
> > +    if (new_ram_size > old_ram_size) {
> > +        migration_bitmap_extend(old_ram_size, new_ram_size);
> > +    }
> >       /* Keep the list sorted from biggest to smallest block.  Unlike QTAILQ,
> >        * QLIST (which has an RCU-friendly variant) does not have insertion at
> >        * tail, so save the last element in last_block.
> > @@ -1435,7 +1440,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, 
> > Error **errp)
> >               ram_list.dirty_memory[i] =
> >                   bitmap_zero_extend(ram_list.dirty_memory[i],
> >                                      old_ram_size, new_ram_size);
> > -       }
> > +        }
> Whitespace noise
>
> >       }
> >       cpu_physical_memory_set_dirty_range(new_block->offset,
> >                                           new_block->used_length,
> > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > index 2573e8c..dd9be44 100644
> > --- a/include/exec/exec-all.h
> > +++ b/include/exec/exec-all.h
> > @@ -385,4 +385,5 @@ static inline bool cpu_can_do_io(CPUState *cpu)
> >       return cpu->can_do_io != 0;
> >   }
> >   
> > +void migration_bitmap_extend(ram_addr_t old, ram_addr_t new);
> >   #endif
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 4754aa9..70dd8da 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -1063,6 +1063,22 @@ static void reset_ram_globals(void)
> >   
> >   #define MAX_WAIT 50 /* ms, half buffered_file limit */
> >   
> > +void migration_bitmap_extend(ram_addr_t old, ram_addr_t new)
> > +{
> > +    qemu_mutex_lock(&migration_bitmap_mutex);
> > +    if (migration_bitmap) {
> > +        unsigned long *old_bitmap = migration_bitmap, *bitmap;
> > +        bitmap = bitmap_new(new);
> > +        bitmap_set(bitmap, old, new - old);
> > +        memcpy(bitmap, old_bitmap,
> > +               BITS_TO_LONGS(old) * sizeof(unsigned long));
> Shouldn't the last two sentences be reversed? memcpy could "potentially"
> overwrote part of the bits setted on bitmap_set.  (notice the
> potentially part, my guess is that we never get a bitmap that is not
> word aligned, but well ....)

reverse the last two sentences would be better.
i will fix it in next version.

Thanks
Li Zhijian

> My understanding of the rest look right.
>
> Later, Juan.
> .