Li Zhijian <lizhij...@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 <lizhij...@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhij...@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
