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

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

[Li Zhijian]

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>
---
 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
-- 
2.1.4

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

[Juan Quintela]

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

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

[Paolo Bonzini]

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