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

[Igor Mammedov]

On Fri, 26 Jun 2015 17:15:58 +0800
Wen Congyang <address@hidden> wrote:

> 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?
> 
> If we don't do hot-plug on destination, migration should fail. But in our
> test, the source qemu's memory is corrupted, and qemu quits unexpectedly.
> 
> We also do hot-plug on the destination before migration, and do hot-plug
> on the source during migration, the migration can success. I know the
> right way is that: do hot-plug at the same time, but my hand is too
> slow to do it.
other way could be to disable hotplug when migration is in progress.

> 
> This patchset just fixes the problem that will cause the source qemu's memory
> is corrupted.
> 
> Thanks
> Wen Congyang.
> 
> > 
> > 
> >> ---
> >>  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 ....)
> > 
> > My understanding of the rest look right.
> > 
> > Later, Juan.
> > .
> > 
> 
>