Message ID | 1436432462-28023-1-git-send-email-lizhijian@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
Li Zhijian <lizhijian@cn.fujitsu.com> wrote: > from commit 2ff6403, we make a mistake to call synchronize_rcu() > within rcu_read_lock()/rcu_read_unlock() > > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> Hi my understanding is that commit d09a6fde1590ca3a45b608b6873a680f208dfeb5 from Paolo already fixed this, right? Not that I am against the change. Should I still pull it? Thanks, Juan. > --- > migration/ram.c | 49 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 32 insertions(+), 17 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index c696814..51635e9 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -221,7 +221,11 @@ static RAMBlock *last_seen_block; > /* This is the last block from where we have sent data */ > static RAMBlock *last_sent_block; > static ram_addr_t last_offset; > -static unsigned long *migration_bitmap; > +struct rcu_bitmap { > + struct rcu_head rcu; > + unsigned long *bitmap; > +}; > +static struct rcu_bitmap *migration_bitmap; > static QemuMutex migration_bitmap_mutex; > static uint64_t migration_dirty_pages; > static uint32_t last_version; > @@ -504,7 +508,7 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr, > unsigned long nr = base + (start >> TARGET_PAGE_BITS); > uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr)); > unsigned long size = base + (mr_size >> TARGET_PAGE_BITS); > - unsigned long *bitmap; > + struct rcu_bitmap *bitmap; > > unsigned long next; > > @@ -512,11 +516,11 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr, > if (ram_bulk_stage && nr > base) { > next = nr + 1; > } else { > - next = find_next_bit(bitmap, size, nr); > + next = find_next_bit(bitmap->bitmap, size, nr); > } > > if (next < size) { > - clear_bit(next, bitmap); > + clear_bit(next, bitmap->bitmap); > migration_dirty_pages--; > } > return (next - base) << TARGET_PAGE_BITS; > @@ -525,10 +529,10 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr, > /* Called with rcu_read_lock() to protect migration_bitmap */ > static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length) > { > - unsigned long *bitmap; > + struct rcu_bitmap *bitmap; > bitmap = atomic_rcu_read(&migration_bitmap); > migration_dirty_pages += > - cpu_physical_memory_sync_dirty_bitmap(bitmap, start, length); > + cpu_physical_memory_sync_dirty_bitmap(bitmap->bitmap, start, length); > } > > > @@ -1024,17 +1028,29 @@ void free_xbzrle_decoded_buf(void) > xbzrle_decoded_buf = NULL; > } > > +static struct rcu_bitmap *rcu_bitmap_new(long bits) > +{ > + struct rcu_bitmap *bitmap = g_malloc0(sizeof(*bitmap)); > + bitmap->bitmap = bitmap_new(bits); > + return bitmap; > +} > + > +static void rcu_bitmap_destroy(struct rcu_bitmap *bitmap) > +{ > + g_free(bitmap->bitmap); > + g_free(bitmap); > +} > + > static void migration_end(void) > { > /* caller have hold iothread lock or is in a bh, so there is > * no writing race against this migration_bitmap > */ > - unsigned long *bitmap = migration_bitmap; > + struct rcu_bitmap *bitmap = migration_bitmap; > atomic_rcu_set(&migration_bitmap, NULL); > if (bitmap) { > memory_global_dirty_log_stop(); > - synchronize_rcu(); > - g_free(bitmap); > + call_rcu(bitmap, rcu_bitmap_destroy, rcu); > } > > XBZRLE_cache_lock(); > @@ -1071,8 +1087,8 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) > * no writing race against this migration_bitmap > */ > if (migration_bitmap) { > - unsigned long *old_bitmap = migration_bitmap, *bitmap; > - bitmap = bitmap_new(new); > + struct rcu_bitmap *old_bitmap = migration_bitmap, *bitmap; > + bitmap = rcu_bitmap_new(new); > > /* prevent migration_bitmap content from being set bit > * by migration_bitmap_sync_range() at the same time. > @@ -1080,13 +1096,12 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) > * at the same time. > */ > qemu_mutex_lock(&migration_bitmap_mutex); > - bitmap_copy(bitmap, old_bitmap, old); > - bitmap_set(bitmap, old, new - old); > + bitmap_copy(bitmap->bitmap, old_bitmap->bitmap, old); > + bitmap_set(bitmap->bitmap, old, new - old); > atomic_rcu_set(&migration_bitmap, bitmap); > qemu_mutex_unlock(&migration_bitmap_mutex); > migration_dirty_pages += new - old; > - synchronize_rcu(); > - g_free(old_bitmap); > + call_rcu(old_bitmap, rcu_bitmap_destroy, rcu); > } > } > > @@ -1145,8 +1160,8 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > reset_ram_globals(); > > ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS; > - migration_bitmap = bitmap_new(ram_bitmap_pages); > - bitmap_set(migration_bitmap, 0, ram_bitmap_pages); > + migration_bitmap = rcu_bitmap_new(ram_bitmap_pages); > + bitmap_set(migration_bitmap->bitmap, 0, ram_bitmap_pages); > > /* > * Count the total number of pages used by ram blocks not including any
On 15/07/2015 10:31, Juan Quintela wrote: > Li Zhijian <lizhijian@cn.fujitsu.com> wrote: >> > from commit 2ff6403, we make a mistake to call synchronize_rcu() >> > within rcu_read_lock()/rcu_read_unlock() >> > >> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> >> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > Hi > > my understanding is that commit > d09a6fde1590ca3a45b608b6873a680f208dfeb5 > > from Paolo already fixed this, right? > > Not that I am against the change. Should I still pull it? I think it's unnecessary complication. Paolo
diff --git a/migration/ram.c b/migration/ram.c index c696814..51635e9 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -221,7 +221,11 @@ static RAMBlock *last_seen_block; /* This is the last block from where we have sent data */ static RAMBlock *last_sent_block; static ram_addr_t last_offset; -static unsigned long *migration_bitmap; +struct rcu_bitmap { + struct rcu_head rcu; + unsigned long *bitmap; +}; +static struct rcu_bitmap *migration_bitmap; static QemuMutex migration_bitmap_mutex; static uint64_t migration_dirty_pages; static uint32_t last_version; @@ -504,7 +508,7 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr, unsigned long nr = base + (start >> TARGET_PAGE_BITS); uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr)); unsigned long size = base + (mr_size >> TARGET_PAGE_BITS); - unsigned long *bitmap; + struct rcu_bitmap *bitmap; unsigned long next; @@ -512,11 +516,11 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr, if (ram_bulk_stage && nr > base) { next = nr + 1; } else { - next = find_next_bit(bitmap, size, nr); + next = find_next_bit(bitmap->bitmap, size, nr); } if (next < size) { - clear_bit(next, bitmap); + clear_bit(next, bitmap->bitmap); migration_dirty_pages--; } return (next - base) << TARGET_PAGE_BITS; @@ -525,10 +529,10 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr, /* Called with rcu_read_lock() to protect migration_bitmap */ static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length) { - unsigned long *bitmap; + struct rcu_bitmap *bitmap; bitmap = atomic_rcu_read(&migration_bitmap); migration_dirty_pages += - cpu_physical_memory_sync_dirty_bitmap(bitmap, start, length); + cpu_physical_memory_sync_dirty_bitmap(bitmap->bitmap, start, length); } @@ -1024,17 +1028,29 @@ void free_xbzrle_decoded_buf(void) xbzrle_decoded_buf = NULL; } +static struct rcu_bitmap *rcu_bitmap_new(long bits) +{ + struct rcu_bitmap *bitmap = g_malloc0(sizeof(*bitmap)); + bitmap->bitmap = bitmap_new(bits); + return bitmap; +} + +static void rcu_bitmap_destroy(struct rcu_bitmap *bitmap) +{ + g_free(bitmap->bitmap); + g_free(bitmap); +} + static void migration_end(void) { /* caller have hold iothread lock or is in a bh, so there is * no writing race against this migration_bitmap */ - unsigned long *bitmap = migration_bitmap; + struct rcu_bitmap *bitmap = migration_bitmap; atomic_rcu_set(&migration_bitmap, NULL); if (bitmap) { memory_global_dirty_log_stop(); - synchronize_rcu(); - g_free(bitmap); + call_rcu(bitmap, rcu_bitmap_destroy, rcu); } XBZRLE_cache_lock(); @@ -1071,8 +1087,8 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) * no writing race against this migration_bitmap */ if (migration_bitmap) { - unsigned long *old_bitmap = migration_bitmap, *bitmap; - bitmap = bitmap_new(new); + struct rcu_bitmap *old_bitmap = migration_bitmap, *bitmap; + bitmap = rcu_bitmap_new(new); /* prevent migration_bitmap content from being set bit * by migration_bitmap_sync_range() at the same time. @@ -1080,13 +1096,12 @@ void migration_bitmap_extend(ram_addr_t old, ram_addr_t new) * at the same time. */ qemu_mutex_lock(&migration_bitmap_mutex); - bitmap_copy(bitmap, old_bitmap, old); - bitmap_set(bitmap, old, new - old); + bitmap_copy(bitmap->bitmap, old_bitmap->bitmap, old); + bitmap_set(bitmap->bitmap, old, new - old); atomic_rcu_set(&migration_bitmap, bitmap); qemu_mutex_unlock(&migration_bitmap_mutex); migration_dirty_pages += new - old; - synchronize_rcu(); - g_free(old_bitmap); + call_rcu(old_bitmap, rcu_bitmap_destroy, rcu); } } @@ -1145,8 +1160,8 @@ static int ram_save_setup(QEMUFile *f, void *opaque) reset_ram_globals(); ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS; - migration_bitmap = bitmap_new(ram_bitmap_pages); - bitmap_set(migration_bitmap, 0, ram_bitmap_pages); + migration_bitmap = rcu_bitmap_new(ram_bitmap_pages); + bitmap_set(migration_bitmap->bitmap, 0, ram_bitmap_pages); /* * Count the total number of pages used by ram blocks not including any