[PATCH v2 0/2] bugfix: Decrease dirty bitmap blocks after we remove ramblock

[PATCH v2 0/2] bugfix: Decrease dirty bitmap blocks after we remove ramblock

[Keqian Zhu]

Keqian Zhu (2):
  ramlist: Make dirty bitmap blocks of ramlist resizable
  ramlist: Resize dirty bitmap blocks after remove ramblock

 softmmu/physmem.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

-- 
2.23.0

[PATCH v2 1/2] ramlist: Make dirty bitmap blocks of ramlist resizable

[Keqian Zhu]

When we remove a ramblock, we should decrease the dirty bitmap blocks
of ramlist to avoid memory leakage. This patch rebuilds dirty_memory_
extend to support both "extend" and "decrease".

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
 softmmu/physmem.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 3027747c03..3e4f29f126 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1816,17 +1816,19 @@ void qemu_ram_msync(RAMBlock *block, ram_addr_t start, ram_addr_t length)
 }
 
 /* Called with ram_list.mutex held */
-static void dirty_memory_extend(ram_addr_t old_ram_size,
+static void dirty_memory_resize(ram_addr_t old_ram_size,
                                 ram_addr_t new_ram_size)
 {
     ram_addr_t old_num_blocks = DIV_ROUND_UP(old_ram_size,
                                              DIRTY_MEMORY_BLOCK_SIZE);
     ram_addr_t new_num_blocks = DIV_ROUND_UP(new_ram_size,
                                              DIRTY_MEMORY_BLOCK_SIZE);
+    ram_addr_t cpy_num_blocks = MIN(old_num_blocks, new_num_blocks);
+    bool extend = new_num_blocks > old_num_blocks;
     int i;
 
-    /* Only need to extend if block count increased */
-    if (new_num_blocks <= old_num_blocks) {
+    /* Only need to resize if block count changed */
+    if (new_num_blocks == old_num_blocks) {
         return;
     }
 
@@ -1839,15 +1841,26 @@ static void dirty_memory_extend(ram_addr_t old_ram_size,
         new_blocks = g_malloc(sizeof(*new_blocks) +
                               sizeof(new_blocks->blocks[0]) * new_num_blocks);
 
-        if (old_num_blocks) {
+        if (cpy_num_blocks) {
             memcpy(new_blocks->blocks, old_blocks->blocks,
-                   old_num_blocks * sizeof(old_blocks->blocks[0]));
+                   cpy_num_blocks * sizeof(old_blocks->blocks[0]));
         }
 
-        for (j = old_num_blocks; j < new_num_blocks; j++) {
-            new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
+        if (extend) {
+            for (j = cpy_num_blocks; j < new_num_blocks; j++) {
+                new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
+            }
+        } else {
+            for (j = cpy_num_blocks; j < old_num_blocks; j++) {
+                /* We are safe to free it, for that it is out-of-use */
+                g_free(old_blocks->blocks[j]);
+            }
         }
 
+        if (!new_num_blocks) {
+            g_free(new_blocks);
+            new_blocks = NULL;
+        }
         qatomic_rcu_set(&ram_list.dirty_memory[i], new_blocks);
 
         if (old_blocks) {
@@ -1894,7 +1907,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared)
     new_ram_size = MAX(old_ram_size,
               (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS);
     if (new_ram_size > old_ram_size) {
-        dirty_memory_extend(old_ram_size, new_ram_size);
+        dirty_memory_resize(old_ram_size, new_ram_size);
     }
     /* Keep the list sorted from biggest to smallest block.  Unlike QTAILQ,
      * QLIST (which has an RCU-friendly variant) does not have insertion at
-- 
2.23.0

Re: [PATCH v2 1/2] ramlist: Make dirty bitmap blocks of ramlist resizable

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On Mon, Nov 30, 2020 at 09:11:03PM +0800, Keqian Zhu wrote:
> @@ -1839,15 +1841,26 @@ static void dirty_memory_extend(ram_addr_t old_ram_size,
>          new_blocks = g_malloc(sizeof(*new_blocks) +
>                                sizeof(new_blocks->blocks[0]) * new_num_blocks);
>  
> -        if (old_num_blocks) {
> +        if (cpy_num_blocks) {
>              memcpy(new_blocks->blocks, old_blocks->blocks,
> -                   old_num_blocks * sizeof(old_blocks->blocks[0]));
> +                   cpy_num_blocks * sizeof(old_blocks->blocks[0]));
>          }
>  
> -        for (j = old_num_blocks; j < new_num_blocks; j++) {
> -            new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
> +        if (extend) {
> +            for (j = cpy_num_blocks; j < new_num_blocks; j++) {
> +                new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
> +            }
> +        } else {
> +            for (j = cpy_num_blocks; j < old_num_blocks; j++) {
> +                /* We are safe to free it, for that it is out-of-use */
> +                g_free(old_blocks->blocks[j]);

This looks unsafe because this code uses Read Copy Update (RCU):

  old_blocks = qatomic_rcu_read(&ram_list.dirty_memory[i]);

Other threads may still be accessing old_blocks so we cannot modify it
immediately. Changes need to be deferred until the next RCU period.
g_free_rcu() needs to be used to do this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] ramlist: Make dirty bitmap blocks of ramlist resizable

[Keqian Zhu]

On 2020/12/17 18:05, Stefan Hajnoczi wrote:
> On Mon, Nov 30, 2020 at 09:11:03PM +0800, Keqian Zhu wrote:
>> @@ -1839,15 +1841,26 @@ static void dirty_memory_extend(ram_addr_t old_ram_size,
>>          new_blocks = g_malloc(sizeof(*new_blocks) +
>>                                sizeof(new_blocks->blocks[0]) * new_num_blocks);
>>  
>> -        if (old_num_blocks) {
>> +        if (cpy_num_blocks) {
>>              memcpy(new_blocks->blocks, old_blocks->blocks,
>> -                   old_num_blocks * sizeof(old_blocks->blocks[0]));
>> +                   cpy_num_blocks * sizeof(old_blocks->blocks[0]));
>>          }
>>  
>> -        for (j = old_num_blocks; j < new_num_blocks; j++) {
>> -            new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
>> +        if (extend) {
>> +            for (j = cpy_num_blocks; j < new_num_blocks; j++) {
>> +                new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
>> +            }
>> +        } else {
>> +            for (j = cpy_num_blocks; j < old_num_blocks; j++) {
>> +                /* We are safe to free it, for that it is out-of-use */
>> +                g_free(old_blocks->blocks[j]);
> 
> This looks unsafe because this code uses Read Copy Update (RCU):
> 
>   old_blocks = qatomic_rcu_read(&ram_list.dirty_memory[i]);
> 
> Other threads may still be accessing old_blocks so we cannot modify it
> immediately. Changes need to be deferred until the next RCU period.
> g_free_rcu() needs to be used to do this.
> 
Hi Stefan,

You are right. I was thinking about the VM life cycle before. We shrink the dirty_memory
when we are removing unused ramblock. However we can not rely on this.

I also notice that "Organization into blocks allows dirty memory to grow (but not shrink)
under RCU". Why "but not shrink"? Any thoughts?

[...]
 * Organization into blocks allows dirty memory to grow (but not shrink) under
 * RCU.  When adding new RAMBlocks requires the dirty memory to grow, a new
 * DirtyMemoryBlocks array is allocated with pointers to existing blocks kept
 * the same.  Other threads can safely access existing blocks while dirty
 * memory is being grown.  When no threads are using the old DirtyMemoryBlocks
 * anymore it is freed by RCU (but the underlying blocks stay because they are
 * pointed to from the new DirtyMemoryBlocks).
 */
#define DIRTY_MEMORY_BLOCK_SIZE ((ram_addr_t)256 * 1024 * 8)
typedef struct {
    struct rcu_head rcu;
    unsigned long *blocks[];
} DirtyMemoryBlocks;
[...]

Thanks,
Keqian

Re: [PATCH v2 1/2] ramlist: Make dirty bitmap blocks of ramlist resizable

[Keqian Zhu]

[...]

>>> -        for (j = old_num_blocks; j < new_num_blocks; j++) {
>>> -            new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
>>> +        if (extend) {
>>> +            for (j = cpy_num_blocks; j < new_num_blocks; j++) {
>>> +                new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
>>> +            }
>>> +        } else {
>>> +            for (j = cpy_num_blocks; j < old_num_blocks; j++) {
>>> +                /* We are safe to free it, for that it is out-of-use */
>>> +                g_free(old_blocks->blocks[j]);
>>
>> This looks unsafe because this code uses Read Copy Update (RCU):
>>
>>   old_blocks = qatomic_rcu_read(&ram_list.dirty_memory[i]);
>>
>> Other threads may still be accessing old_blocks so we cannot modify it
>> immediately. Changes need to be deferred until the next RCU period.
>> g_free_rcu() needs to be used to do this.
>>
> Hi Stefan,
> 
> You are right. I was thinking about the VM life cycle before. We shrink the dirty_memory
> when we are removing unused ramblock. However we can not rely on this.
> 
> I also notice that "Organization into blocks allows dirty memory to grow (but not shrink)
> under RCU". Why "but not shrink"? Any thoughts?
Hi,

After my analysis, it's both unsafe to grow or shrink under RCU.

ram_list.blocks and ram_list.dirty_memory[X] are closely related and
both protected by RCU. For the lockless RCU readers, we can't promise they
always see consistent version of the two structures.

For grow, a reader may see un-growed @dirty_memory and growed @blocks, causing out-of-bound access.
For shrink, a reader may see shrinked @dirty_memory and un-shrinked @blocks, causing out-of-bound access too.

I think it's a design problem, RCU can just protect one structure, not two.

Thanks,
Keqian.
> 
> [...]
>  * Organization into blocks allows dirty memory to grow (but not shrink) under
>  * RCU.  When adding new RAMBlocks requires the dirty memory to grow, a new
>  * DirtyMemoryBlocks array is allocated with pointers to existing blocks kept
>  * the same.  Other threads can safely access existing blocks while dirty
>  * memory is being grown.  When no threads are using the old DirtyMemoryBlocks
>  * anymore it is freed by RCU (but the underlying blocks stay because they are
>  * pointed to from the new DirtyMemoryBlocks).
>  */
> #define DIRTY_MEMORY_BLOCK_SIZE ((ram_addr_t)256 * 1024 * 8)
> typedef struct {
>     struct rcu_head rcu;
>     unsigned long *blocks[];
> } DirtyMemoryBlocks;
> [...]
> 
> Thanks,
> Keqian
> 
> 
> .
> 

Re: [PATCH v2 1/2] ramlist: Make dirty bitmap blocks of ramlist resizable

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2015 bytes --]

On Sat, Dec 26, 2020 at 03:11:53PM +0800, Keqian Zhu wrote:
> 
> [...]
> 
> >>> -        for (j = old_num_blocks; j < new_num_blocks; j++) {
> >>> -            new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
> >>> +        if (extend) {
> >>> +            for (j = cpy_num_blocks; j < new_num_blocks; j++) {
> >>> +                new_blocks->blocks[j] = bitmap_new(DIRTY_MEMORY_BLOCK_SIZE);
> >>> +            }
> >>> +        } else {
> >>> +            for (j = cpy_num_blocks; j < old_num_blocks; j++) {
> >>> +                /* We are safe to free it, for that it is out-of-use */
> >>> +                g_free(old_blocks->blocks[j]);
> >>
> >> This looks unsafe because this code uses Read Copy Update (RCU):
> >>
> >>   old_blocks = qatomic_rcu_read(&ram_list.dirty_memory[i]);
> >>
> >> Other threads may still be accessing old_blocks so we cannot modify it
> >> immediately. Changes need to be deferred until the next RCU period.
> >> g_free_rcu() needs to be used to do this.
> >>
> > Hi Stefan,
> > 
> > You are right. I was thinking about the VM life cycle before. We shrink the dirty_memory
> > when we are removing unused ramblock. However we can not rely on this.
> > 
> > I also notice that "Organization into blocks allows dirty memory to grow (but not shrink)
> > under RCU". Why "but not shrink"? Any thoughts?
> Hi,
> 
> After my analysis, it's both unsafe to grow or shrink under RCU.
> 
> ram_list.blocks and ram_list.dirty_memory[X] are closely related and
> both protected by RCU. For the lockless RCU readers, we can't promise they
> always see consistent version of the two structures.
> 
> For grow, a reader may see un-growed @dirty_memory and growed @blocks, causing out-of-bound access.

Growth is safe because other threads only access pre-existing ranges
(below the old maximum size). They will only start accessing the newly
added ranges after resize.

Did you find a code path where this constraint is violated?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] ramlist: Resize dirty bitmap blocks after remove ramblock

[Keqian Zhu]

Use the new "dirty_memory_resize" interface to reduce dirty bitmap
blocks after we remove a ramblock from ramlist.

This bug is found by ASAN after executing several qmp commands about
object-add/object-del of memory-backend-ram. After applying this patch,
the memory leak is not reported anymore.

=================================================================
==qemu-system-aarch64==1720167==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 2359296 byte(s) in 9 object(s) allocated from:
    #0 0xfffeedf3e938 in __interceptor_calloc (/lib64/libasan.so.5+0xee938)
    #1 0xaaaaf6f1e740 in bitmap_new /qemu/include/qemu/bitmap.h:101
    #2 0xaaaaf6f1e81c in dirty_memory_extend ../exec.c:2212
    #3 0xaaaaf6f22524 in ram_block_add ../exec.c:2261
    #4 0xaaaaf6f22988 in qemu_ram_alloc_internal ../exec.c:2434
    #5 0xaaaaf6f8ae70 in memory_region_init_ram_shared_nomigrate ../softmmu/memory.c:1513
    #6 0xaaaaf66edee0 in ram_backend_memory_alloc ../backends/hostmem-ram.c:30
    #7 0xaaaaf660d03c in host_memory_backend_memory_complete ../backends/hostmem.c:333
    #8 0xaaaaf70f6968 in user_creatable_complete ../qom/object_interfaces.c:23
    #9 0xaaaaf70f6dac in user_creatable_add_type ../qom/object_interfaces.c:93
    #10 0xaaaaf70f7030 in user_creatable_add_dict ../qom/object_interfaces.c:134
    #11 0xaaaaf7340174 in do_qmp_dispatch_bh ../qapi/qmp-dispatch.c:110
    #12 0xaaaaf732da30 in aio_bh_poll ../util/async.c:164
    #13 0xaaaaf735c9a8 in aio_dispatch ../util/aio-posix.c:381
    #14 0xaaaaf732d2ec in aio_ctx_dispatch ../util/async.c:306
    #15 0xfffeecb029d8 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x529d8)
    #16 0xaaaaf733bb78 in os_host_main_loop_wait ../util/main-loop.c:244
    #17 0xaaaaf733beac in main_loop_wait ../util/main-loop.c:520
    #18 0xaaaaf70802a4 in qemu_main_loop ../softmmu/vl.c:1677
    #19 0xaaaaf655786c in main ../softmmu/main.c:50
    #20 0xfffeec043f5c in __libc_start_main (/lib64/libc.so.6+0x23f5c)
    #21 0xaaaaf656a198  (/qemu/build/qemu-system-aarch64+0x9ba198)
SUMMARY: AddressSanitizer: 2359296 byte(s) leaked in 9 allocation(s).

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>

----
little concern:
According to code, my bugfix can solve two problems:

1. Lose reference to dirty bitmap of deleted ramblock, because the reference is
   covered by dirty bitmap of newly added ramblock.
2. All dirty bitmap is not freed before qemu exit.

However, ASAN do not report memory leak for point 2.
Any thoughts or explanations?
---
 softmmu/physmem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 3e4f29f126..8c5f910677 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2132,6 +2132,8 @@ static void reclaim_ramblock(RAMBlock *block)
 
 void qemu_ram_free(RAMBlock *block)
 {
+    ram_addr_t old_ram_size, new_ram_size;
+
     if (!block) {
         return;
     }
@@ -2141,12 +2143,18 @@ void qemu_ram_free(RAMBlock *block)
     }
 
     qemu_mutex_lock_ramlist();
+
+    old_ram_size = last_ram_page();
     QLIST_REMOVE_RCU(block, next);
+    new_ram_size = last_ram_page();
+    dirty_memory_resize(old_ram_size, new_ram_size);
+
     ram_list.mru_block = NULL;
     /* Write list before version */
     smp_wmb();
     ram_list.version++;
     call_rcu(block, reclaim_ramblock, rcu);
+
     qemu_mutex_unlock_ramlist();
 }
 
-- 
2.23.0

Ping: [PATCH v2 0/2] bugfix: Decrease dirty bitmap blocks after we remove ramblock

[zhukeqian]

Hi folks, kindly ping ...

This bugfix can save several MBs memory, waiting for review, thanks.

Keqian.

On 2020/11/30 21:11, Keqian Zhu wrote:
> Keqian Zhu (2):
>   ramlist: Make dirty bitmap blocks of ramlist resizable
>   ramlist: Resize dirty bitmap blocks after remove ramblock
> 
>  softmmu/physmem.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 

Re: Ping: [PATCH v2 0/2] bugfix: Decrease dirty bitmap blocks after we remove ramblock

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 302 bytes --]

On Thu, Dec 03, 2020 at 10:02:05PM +0800, zhukeqian wrote:
> Hi folks, kindly ping ...
> 
> This bugfix can save several MBs memory, waiting for review, thanks.

Paolo: Please take a look when you get a chance.

I left a comment because Patch 1 doesn't seem to take RCU into account.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]