| Message ID | 1443172180-1005-2-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 09/25/2015 05:09 PM, Denis V. Lunev wrote: > we can omit calling of bitmap_set in migration_bitmap_extend and > ram_save_setup just after bitmap_new, which properly zeroes memory > inside. This patch is wrong. bitmap_set() is set all bits of the memory to 1, not 0. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Igor Redko <redkoi@virtuozzo.com> > CC: Anna Melekhova <annam@virtuozzo.com> > CC: Juan Quintela <quintela@redhat.com> > CC: Amit Shah <amit.shah@redhat.com> > CC: Wen Congyang <wency@cn.fujitsu.com> > --- > migration/ram.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 7f007e6..a712c68 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1081,7 +1081,6 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) > */ > qemu_mutex_lock(&migration_bitmap_mutex); > bitmap_copy(bitmap, old_bitmap, old); > - bitmap_set(bitmap, old, new - old); > atomic_rcu_set(&migration_bitmap, bitmap); > qemu_mutex_unlock(&migration_bitmap_mutex); > migration_dirty_pages += new - old; > @@ -1146,7 +1145,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > > ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS; > migration_bitmap = bitmap_new(ram_bitmap_pages); > - bitmap_set(migration_bitmap, 0, ram_bitmap_pages); > > /* > * Count the total number of pages used by ram blocks not including any >
On 09/25/2015 12:24 PM, Wen Congyang wrote: > On 09/25/2015 05:09 PM, Denis V. Lunev wrote: >> we can omit calling of bitmap_set in migration_bitmap_extend and >> ram_save_setup just after bitmap_new, which properly zeroes memory >> inside. > This patch is wrong. bitmap_set() is set all bits of the memory to 1, > not 0. > OK, then I'll replace g_try_malloc0 with g_try_malloc in the next patch to avoid double memset >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Igor Redko <redkoi@virtuozzo.com> >> CC: Anna Melekhova <annam@virtuozzo.com> >> CC: Juan Quintela <quintela@redhat.com> >> CC: Amit Shah <amit.shah@redhat.com> >> CC: Wen Congyang <wency@cn.fujitsu.com> >> --- >> migration/ram.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 7f007e6..a712c68 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -1081,7 +1081,6 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) >> */ >> qemu_mutex_lock(&migration_bitmap_mutex); >> bitmap_copy(bitmap, old_bitmap, old); >> - bitmap_set(bitmap, old, new - old); >> atomic_rcu_set(&migration_bitmap, bitmap); >> qemu_mutex_unlock(&migration_bitmap_mutex); >> migration_dirty_pages += new - old; >> @@ -1146,7 +1145,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> >> ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS; >> migration_bitmap = bitmap_new(ram_bitmap_pages); >> - bitmap_set(migration_bitmap, 0, ram_bitmap_pages); >> >> /* >> * Count the total number of pages used by ram blocks not including any >>
On 09/25/2015 05:31 PM, Denis V. Lunev wrote: > On 09/25/2015 12:24 PM, Wen Congyang wrote: >> On 09/25/2015 05:09 PM, Denis V. Lunev wrote: >>> we can omit calling of bitmap_set in migration_bitmap_extend and >>> ram_save_setup just after bitmap_new, which properly zeroes memory >>> inside. >> This patch is wrong. bitmap_set() is set all bits of the memory to 1, >> not 0. >> > > OK, then I'll replace g_try_malloc0 with g_try_malloc in the next patch to avoid > double memset No, bitmap_new() is called in many places. Some caller needs zero memory, and some caller doesn't need... > >>> Signed-off-by: Denis V. Lunev <den@openvz.org> >>> CC: Igor Redko <redkoi@virtuozzo.com> >>> CC: Anna Melekhova <annam@virtuozzo.com> >>> CC: Juan Quintela <quintela@redhat.com> >>> CC: Amit Shah <amit.shah@redhat.com> >>> CC: Wen Congyang <wency@cn.fujitsu.com> >>> --- >>> migration/ram.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/migration/ram.c b/migration/ram.c >>> index 7f007e6..a712c68 100644 >>> --- a/migration/ram.c >>> +++ b/migration/ram.c >>> @@ -1081,7 +1081,6 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) >>> */ >>> qemu_mutex_lock(&migration_bitmap_mutex); >>> bitmap_copy(bitmap, old_bitmap, old); >>> - bitmap_set(bitmap, old, new - old); >>> atomic_rcu_set(&migration_bitmap, bitmap); >>> qemu_mutex_unlock(&migration_bitmap_mutex); >>> migration_dirty_pages += new - old; >>> @@ -1146,7 +1145,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >>> ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS; >>> migration_bitmap = bitmap_new(ram_bitmap_pages); >>> - bitmap_set(migration_bitmap, 0, ram_bitmap_pages); >>> /* >>> * Count the total number of pages used by ram blocks not including any >>> > > . >
On 09/25/2015 12:37 PM, Wen Congyang wrote: > On 09/25/2015 05:31 PM, Denis V. Lunev wrote: >> On 09/25/2015 12:24 PM, Wen Congyang wrote: >>> On 09/25/2015 05:09 PM, Denis V. Lunev wrote: >>>> we can omit calling of bitmap_set in migration_bitmap_extend and >>>> ram_save_setup just after bitmap_new, which properly zeroes memory >>>> inside. >>> This patch is wrong. bitmap_set() is set all bits of the memory to 1, >>> not 0. >>> >> OK, then I'll replace g_try_malloc0 with g_try_malloc in the next patch to avoid >> double memset > No, bitmap_new() is called in many places. Some caller needs zero memory, and > some caller doesn't need... > you are correct with your proposal with double allocation for the next patch. With a specific function like I have did in v2 g_try_malloc0 can be replaced. Anyway, I am fine with your proposal, this is not a (very) big deal. EINPROGRESS Den
diff --git a/migration/ram.c b/migration/ram.c index 7f007e6..a712c68 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1081,7 +1081,6 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) */ qemu_mutex_lock(&migration_bitmap_mutex); bitmap_copy(bitmap, old_bitmap, old); - bitmap_set(bitmap, old, new - old); atomic_rcu_set(&migration_bitmap, bitmap); qemu_mutex_unlock(&migration_bitmap_mutex); migration_dirty_pages += new - old; @@ -1146,7 +1145,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque) ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS; migration_bitmap = bitmap_new(ram_bitmap_pages); - bitmap_set(migration_bitmap, 0, ram_bitmap_pages); /* * Count the total number of pages used by ram blocks not including any
we can omit calling of bitmap_set in migration_bitmap_extend and ram_save_setup just after bitmap_new, which properly zeroes memory inside. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Igor Redko <redkoi@virtuozzo.com> CC: Anna Melekhova <annam@virtuozzo.com> CC: Juan Quintela <quintela@redhat.com> CC: Amit Shah <amit.shah@redhat.com> CC: Wen Congyang <wency@cn.fujitsu.com> --- migration/ram.c | 2 -- 1 file changed, 2 deletions(-)