| Message ID | 1271670198-12793-3-git-send-email-tamura.yoshiaki@lab.ntt.co.jp |
|---|---|
| State | New |
| Headers | show |
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; > >
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; >> >
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; >>> >> > > > >
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.
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.
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.
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));
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 */
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(-)