Patchwork [v3,2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

login
register
mail settings
Submitter Yoshiaki Tamura
Date April 19, 2010, 9:43 a.m.
Message ID <1271670198-12793-3-git-send-email-tamura.yoshiaki@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/50439/
State New
Headers show

Comments

Yoshiaki Tamura - April 19, 2010, 9:43 a.m.
Replaces byte-based phys_ram_dirty bitmap with four bit-based phys_ram_dirty
bitmap.  On allocation, it sets all bits in the bitmap.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 exec.c        |   16 +++++++++++-----
 qemu-common.h |    3 +++
 2 files changed, 14 insertions(+), 5 deletions(-)
Avi Kivity - April 19, 2010, 10:17 a.m.
On 04/19/2010 12:43 PM, Yoshiaki Tamura wrote:
> Replaces byte-based phys_ram_dirty bitmap with four bit-based phys_ram_dirty
> bitmap.  On allocation, it sets all bits in the bitmap.
>
> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
> ---
>   exec.c        |   16 +++++++++++-----
>   qemu-common.h |    3 +++
>   2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index c74b0a4..b85cb26 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -110,7 +110,7 @@ uint8_t *code_gen_ptr;
>
>   #if !defined(CONFIG_USER_ONLY)
>   int phys_ram_fd;
> -uint8_t *phys_ram_dirty;
> +unsigned long *phys_ram_dirty[NUM_DIRTY_IDX];
>   static int in_migration;
>
>   typedef struct RAMBlock {
> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>       new_block->next = ram_blocks;
>       ram_blocks = new_block;
>
> -    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> -        (last_ram_offset + size)>>  TARGET_PAGE_BITS);
> -    memset(phys_ram_dirty + (last_ram_offset>>  TARGET_PAGE_BITS),
> -           0xff, size>>  TARGET_PAGE_BITS);
> +    if (BITMAP_SIZE(last_ram_offset + size) != BITMAP_SIZE(last_ram_offset)) {
>    

This check is unneeded - the code will work fine even if the bitmap size 
doesn't change.

> +        int i;
> +        for (i = MASTER_DIRTY_IDX; i<  NUM_DIRTY_IDX; i++) {
> +            phys_ram_dirty[i]
> +                = qemu_realloc(phys_ram_dirty[i],
> +                               BITMAP_SIZE(last_ram_offset + size));
> +            memset((uint8_t *)phys_ram_dirty[i] +
> +                   BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> +        }
> +    }
>
>       last_ram_offset += size;
>
>
Yoshiaki Tamura - April 19, 2010, 10:36 a.m.
Avi Kivity wrote:
> On 04/19/2010 12:43 PM, Yoshiaki Tamura wrote:
>> Replaces byte-based phys_ram_dirty bitmap with four bit-based
>> phys_ram_dirty
>> bitmap. On allocation, it sets all bits in the bitmap.
>>
>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>> ---
>> exec.c | 16 +++++++++++-----
>> qemu-common.h | 3 +++
>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index c74b0a4..b85cb26 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -110,7 +110,7 @@ uint8_t *code_gen_ptr;
>>
>> #if !defined(CONFIG_USER_ONLY)
>> int phys_ram_fd;
>> -uint8_t *phys_ram_dirty;
>> +unsigned long *phys_ram_dirty[NUM_DIRTY_IDX];
>> static int in_migration;
>>
>> typedef struct RAMBlock {
>> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>> new_block->next = ram_blocks;
>> ram_blocks = new_block;
>>
>> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
>> - (last_ram_offset + size)>> TARGET_PAGE_BITS);
>> - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS),
>> - 0xff, size>> TARGET_PAGE_BITS);
>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>> BITMAP_SIZE(last_ram_offset)) {
>
> This check is unneeded - the code will work fine even if the bitmap size
> doesn't change.

OK.  I'll remove it.

>
>> + int i;
>> + for (i = MASTER_DIRTY_IDX; i< NUM_DIRTY_IDX; i++) {
>> + phys_ram_dirty[i]
>> + = qemu_realloc(phys_ram_dirty[i],
>> + BITMAP_SIZE(last_ram_offset + size));
>> + memset((uint8_t *)phys_ram_dirty[i] +
>> + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
>> + }
>> + }
>>
>> last_ram_offset += size;
>>
>
Yoshiaki Tamura - April 19, 2010, 11:31 a.m.
2010/4/19 Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>:
> Avi Kivity wrote:
>>
>> On 04/19/2010 12:43 PM, Yoshiaki Tamura wrote:
>>>
>>> Replaces byte-based phys_ram_dirty bitmap with four bit-based
>>> phys_ram_dirty
>>> bitmap. On allocation, it sets all bits in the bitmap.
>>>
>>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>>> ---
>>> exec.c | 16 +++++++++++-----
>>> qemu-common.h | 3 +++
>>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index c74b0a4..b85cb26 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -110,7 +110,7 @@ uint8_t *code_gen_ptr;
>>>
>>> #if !defined(CONFIG_USER_ONLY)
>>> int phys_ram_fd;
>>> -uint8_t *phys_ram_dirty;
>>> +unsigned long *phys_ram_dirty[NUM_DIRTY_IDX];
>>> static int in_migration;
>>>
>>> typedef struct RAMBlock {
>>> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>>> new_block->next = ram_blocks;
>>> ram_blocks = new_block;
>>>
>>> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
>>> - (last_ram_offset + size)>> TARGET_PAGE_BITS);
>>> - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS),
>>> - 0xff, size>> TARGET_PAGE_BITS);
>>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>>> BITMAP_SIZE(last_ram_offset)) {
>>
>> This check is unneeded - the code will work fine even if the bitmap size
>> doesn't change.
>
> OK.  I'll remove it.

I have a problem here.
If I remove this check, glibc reports an error as below.

*** glibc detected *** /usr/local/qemu/bin/qemu-system-x86_64:
realloc(): invalid pointer: 0x0000000001f0e450 ***
======= Backtrace: =========
/lib64/libc.so.6[0x369fa75a96]
/lib64/libc.so.6(realloc+0x2a1)[0x369fa7b881]
/usr/local/qemu/bin/qemu-system-x86_64[0x437d93]
/usr/local/qemu/bin/qemu-system-x86_64[0x4f03f6]
/usr/local/qemu/bin/qemu-system-x86_64[0x5b052c]
/usr/local/qemu/bin/qemu-system-x86_64[0x5b0d8b]
/usr/local/qemu/bin/qemu-system-x86_64[0x41ec2b]
/lib64/libc.so.6(__libc_start_main+0xfd)[0x369fa1ea2d]
/usr/local/qemu/bin/qemu-system-x86_64[0x406479]
======= Memory map: ========

I reminded that I put this check to avoid reallocating same size to the bitmap.
qemu goes this routine at start up, and extends last_ram_offset at
small numbers.
The error above is reported at the extension phase.

Is this happening only on my environment (Fedora 11 x86_64)?

>>> + int i;
>>> + for (i = MASTER_DIRTY_IDX; i< NUM_DIRTY_IDX; i++) {
>>> + phys_ram_dirty[i]
>>> + = qemu_realloc(phys_ram_dirty[i],
>>> + BITMAP_SIZE(last_ram_offset + size));
>>> + memset((uint8_t *)phys_ram_dirty[i] +
>>> + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
>>> + }
>>> + }
>>>
>>> last_ram_offset += size;
>>>
>>
>
>
>
>
Avi Kivity - April 19, 2010, 11:38 a.m.
On 04/19/2010 02:31 PM, Yoshiaki Tamura wrote:
>
>>>> typedef struct RAMBlock {
>>>> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>>>> new_block->next = ram_blocks;
>>>> ram_blocks = new_block;
>>>>
>>>> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
>>>> - (last_ram_offset + size)>>  TARGET_PAGE_BITS);
>>>> - memset(phys_ram_dirty + (last_ram_offset>>  TARGET_PAGE_BITS),
>>>> - 0xff, size>>  TARGET_PAGE_BITS);
>>>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>>>> BITMAP_SIZE(last_ram_offset)) {
>>>>          
>>> This check is unneeded - the code will work fine even if the bitmap size
>>> doesn't change.
>>>        
>> OK.  I'll remove it.
>>      
> I have a problem here.
> If I remove this check, glibc reports an error as below.
>
> *** glibc detected *** /usr/local/qemu/bin/qemu-system-x86_64:
> realloc(): invalid pointer: 0x0000000001f0e450 ***
> ======= Backtrace: =========
> /lib64/libc.so.6[0x369fa75a96]
> /lib64/libc.so.6(realloc+0x2a1)[0x369fa7b881]
> /usr/local/qemu/bin/qemu-system-x86_64[0x437d93]
> /usr/local/qemu/bin/qemu-system-x86_64[0x4f03f6]
> /usr/local/qemu/bin/qemu-system-x86_64[0x5b052c]
> /usr/local/qemu/bin/qemu-system-x86_64[0x5b0d8b]
> /usr/local/qemu/bin/qemu-system-x86_64[0x41ec2b]
> /lib64/libc.so.6(__libc_start_main+0xfd)[0x369fa1ea2d]
> /usr/local/qemu/bin/qemu-system-x86_64[0x406479]
> ======= Memory map: ========
>
> I reminded that I put this check to avoid reallocating same size to the bitmap.
> qemu goes this routine at start up, and extends last_ram_offset at
> small numbers.
> The error above is reported at the extension phase.
>
>    

This probably means that an old bitmap pointer leaked somewhere, and we 
realloc() it after free?  Or perhaps a glibc bug.
Yoshiaki Tamura - April 19, 2010, 11:52 a.m.
Avi Kivity wrote:
> On 04/19/2010 02:31 PM, Yoshiaki Tamura wrote:
>>
>>>>> typedef struct RAMBlock {
>>>>> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>>>>> new_block->next = ram_blocks;
>>>>> ram_blocks = new_block;
>>>>>
>>>>> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
>>>>> - (last_ram_offset + size)>> TARGET_PAGE_BITS);
>>>>> - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS),
>>>>> - 0xff, size>> TARGET_PAGE_BITS);
>>>>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>>>>> BITMAP_SIZE(last_ram_offset)) {
>>>> This check is unneeded - the code will work fine even if the bitmap
>>>> size
>>>> doesn't change.
>>> OK. I'll remove it.
>> I have a problem here.
>> If I remove this check, glibc reports an error as below.
>>
>> *** glibc detected *** /usr/local/qemu/bin/qemu-system-x86_64:
>> realloc(): invalid pointer: 0x0000000001f0e450 ***
>> ======= Backtrace: =========
>> /lib64/libc.so.6[0x369fa75a96]
>> /lib64/libc.so.6(realloc+0x2a1)[0x369fa7b881]
>> /usr/local/qemu/bin/qemu-system-x86_64[0x437d93]
>> /usr/local/qemu/bin/qemu-system-x86_64[0x4f03f6]
>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b052c]
>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b0d8b]
>> /usr/local/qemu/bin/qemu-system-x86_64[0x41ec2b]
>> /lib64/libc.so.6(__libc_start_main+0xfd)[0x369fa1ea2d]
>> /usr/local/qemu/bin/qemu-system-x86_64[0x406479]
>> ======= Memory map: ========
>>
>> I reminded that I put this check to avoid reallocating same size to
>> the bitmap.
>> qemu goes this routine at start up, and extends last_ram_offset at
>> small numbers.
>> The error above is reported at the extension phase.
>>
>
> This probably means that an old bitmap pointer leaked somewhere, and we
> realloc() it after free? Or perhaps a glibc bug.

Original qemu doesn't have a code the frees phys_ram_dirty, and I didn't either.
Hmmm.
Avi Kivity - April 19, 2010, 11:56 a.m.
On 04/19/2010 02:52 PM, Yoshiaki Tamura wrote:
> Avi Kivity wrote:
>> On 04/19/2010 02:31 PM, Yoshiaki Tamura wrote:
>>>
>>>>>> typedef struct RAMBlock {
>>>>>> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>>>>>> new_block->next = ram_blocks;
>>>>>> ram_blocks = new_block;
>>>>>>
>>>>>> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
>>>>>> - (last_ram_offset + size)>> TARGET_PAGE_BITS);
>>>>>> - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS),
>>>>>> - 0xff, size>> TARGET_PAGE_BITS);
>>>>>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>>>>>> BITMAP_SIZE(last_ram_offset)) {
>>>>> This check is unneeded - the code will work fine even if the bitmap
>>>>> size
>>>>> doesn't change.
>>>> OK. I'll remove it.
>>> I have a problem here.
>>> If I remove this check, glibc reports an error as below.
>>>
>>> *** glibc detected *** /usr/local/qemu/bin/qemu-system-x86_64:
>>> realloc(): invalid pointer: 0x0000000001f0e450 ***
>>> ======= Backtrace: =========
>>> /lib64/libc.so.6[0x369fa75a96]
>>> /lib64/libc.so.6(realloc+0x2a1)[0x369fa7b881]
>>> /usr/local/qemu/bin/qemu-system-x86_64[0x437d93]
>>> /usr/local/qemu/bin/qemu-system-x86_64[0x4f03f6]
>>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b052c]
>>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b0d8b]
>>> /usr/local/qemu/bin/qemu-system-x86_64[0x41ec2b]
>>> /lib64/libc.so.6(__libc_start_main+0xfd)[0x369fa1ea2d]
>>> /usr/local/qemu/bin/qemu-system-x86_64[0x406479]
>>> ======= Memory map: ========
>>>
>>> I reminded that I put this check to avoid reallocating same size to
>>> the bitmap.
>>> qemu goes this routine at start up, and extends last_ram_offset at
>>> small numbers.
>>> The error above is reported at the extension phase.
>>>
>>
>> This probably means that an old bitmap pointer leaked somewhere, and we
>> realloc() it after free? Or perhaps a glibc bug.
>
> Original qemu doesn't have a code the frees phys_ram_dirty, and I 
> didn't either.
> Hmmm.

I meant, after we realloc() something we keep using the old pointer.  
realloc() is equivalent to free(), after all.

It might also be memory corruption - bits set outside the bitmap and 
hitting glibc malloc metadata.
Yoshiaki Tamura - April 19, 2010, 2:51 p.m.
2010/4/19 Avi Kivity <avi@redhat.com>:
> On 04/19/2010 02:52 PM, Yoshiaki Tamura wrote:
>>
>> Avi Kivity wrote:
>>>
>>> On 04/19/2010 02:31 PM, Yoshiaki Tamura wrote:
>>>>
>>>>>>> typedef struct RAMBlock {
>>>>>>> @@ -2825,10 +2825,16 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
>>>>>>> new_block->next = ram_blocks;
>>>>>>> ram_blocks = new_block;
>>>>>>>
>>>>>>> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
>>>>>>> - (last_ram_offset + size)>> TARGET_PAGE_BITS);
>>>>>>> - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS),
>>>>>>> - 0xff, size>> TARGET_PAGE_BITS);
>>>>>>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>>>>>>> BITMAP_SIZE(last_ram_offset)) {
>>>>>>
>>>>>> This check is unneeded - the code will work fine even if the bitmap
>>>>>> size
>>>>>> doesn't change.
>>>>>
>>>>> OK. I'll remove it.
>>>>
>>>> I have a problem here.
>>>> If I remove this check, glibc reports an error as below.
>>>>
>>>> *** glibc detected *** /usr/local/qemu/bin/qemu-system-x86_64:
>>>> realloc(): invalid pointer: 0x0000000001f0e450 ***
>>>> ======= Backtrace: =========
>>>> /lib64/libc.so.6[0x369fa75a96]
>>>> /lib64/libc.so.6(realloc+0x2a1)[0x369fa7b881]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x437d93]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x4f03f6]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b052c]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x5b0d8b]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x41ec2b]
>>>> /lib64/libc.so.6(__libc_start_main+0xfd)[0x369fa1ea2d]
>>>> /usr/local/qemu/bin/qemu-system-x86_64[0x406479]
>>>> ======= Memory map: ========
>>>>
>>>> I reminded that I put this check to avoid reallocating same size to
>>>> the bitmap.
>>>> qemu goes this routine at start up, and extends last_ram_offset at
>>>> small numbers.
>>>> The error above is reported at the extension phase.
>>>>
>>>
>>> This probably means that an old bitmap pointer leaked somewhere, and we
>>> realloc() it after free? Or perhaps a glibc bug.
>>
>> Original qemu doesn't have a code the frees phys_ram_dirty, and I didn't
>> either.
>> Hmmm.
>
> I meant, after we realloc() something we keep using the old pointer.
>  realloc() is equivalent to free(), after all.
>
> It might also be memory corruption - bits set outside the bitmap and hitting
> glibc malloc metadata.

The latter seems to be the problem.  I was calling memset() too
aggressively when BITMAP_SIZE didn't grow.  I think It should be as
following.

memset((uint8_t *)phys_ram_dirty[i] + BITMAP_SIZE(last_ram_offset), 0xff,
               BITMAP_SIZE(last_ram_offset + size) -
BITMAP_SIZE(last_ram_offset));

Patch

diff --git a/exec.c b/exec.c
index c74b0a4..b85cb26 100644
--- a/exec.c
+++ b/exec.c
@@ -110,7 +110,7 @@  uint8_t *code_gen_ptr;
 
 #if !defined(CONFIG_USER_ONLY)
 int phys_ram_fd;
-uint8_t *phys_ram_dirty;
+unsigned long *phys_ram_dirty[NUM_DIRTY_IDX];
 static int in_migration;
 
 typedef struct RAMBlock {
@@ -2825,10 +2825,16 @@  ram_addr_t qemu_ram_alloc(ram_addr_t size)
     new_block->next = ram_blocks;
     ram_blocks = new_block;
 
-    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
-        (last_ram_offset + size) >> TARGET_PAGE_BITS);
-    memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
-           0xff, size >> TARGET_PAGE_BITS);
+    if (BITMAP_SIZE(last_ram_offset + size) != BITMAP_SIZE(last_ram_offset)) {
+        int i;
+        for (i = MASTER_DIRTY_IDX; i < NUM_DIRTY_IDX; i++) {
+            phys_ram_dirty[i] 
+                = qemu_realloc(phys_ram_dirty[i],
+                               BITMAP_SIZE(last_ram_offset + size));
+            memset((uint8_t *)phys_ram_dirty[i] +
+                   BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
+        }
+    }
 
     last_ram_offset += size;
 
diff --git a/qemu-common.h b/qemu-common.h
index 4ba0cda..efe5b1f 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -285,6 +285,9 @@  static inline uint8_t from_bcd(uint8_t val)
     return ((val >> 4) * 10) + (val & 0x0f);
 }
 
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
+#define BITMAP_SIZE(m) (ALIGN(((m)>>TARGET_PAGE_BITS), HOST_LONG_BITS) / 8)
+
 #include "module.h"
 
 #endif /* dyngen-exec.h hack */