Re: [Qemu-devel] [PATCH] migration: use call_rcu instead of synchronize_rcu

[Juan Quintela]

Li Zhijian <address@hidden> wrote:
> from commit 2ff6403, we make a mistake to call synchronize_rcu()
> within rcu_read_lock()/rcu_read_unlock()
>
> Signed-off-by: Li Zhijian <address@hidden>
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: Li Zhijian <address@hidden>

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