[Qemu-devel] [PATCH v3 0/6] Introduce bit-based phys_ram_dirty, and bit-based dirty page checker.

[Qemu-devel] [PATCH v3 0/6] Introduce bit-based phys_ram_dirty, and bit-based dirty page checker.

[Yoshiaki Tamura]

The dirty and non-dirty pages are checked one by one.  When most of the memory
is not dirty, checking the dirty and non-dirty pages by multiple page size
should be much faster than checking them one by one.  We introduced bit-based
phys_ram_dirty for VGA, CODE, MIGRATION, MASTER, and
cpu_physical_memory_get_dirty_range() for this purpose.

Changes from v2 to v3 are:

- Change FLAGS value to (1,2,4,8), and add IDX (0,1,2,3)
- Use ffs to convert FLAGS to IDX.
- Add a helper function which takes IDX.
- Change the behavior of MASTER as a buffer.
- Change dirty bitmap access to a loop.
- Add brace after if ()
- Move some macros to qemu-common.h.

Yoshiaki Tamura (6):
  Modify DIRTY_FLAG value and DIRTY_IDX introduce to use as indexes of
    bit-based phys_ram_dirty.
  Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and
    MASTER.
  Modifies wrapper functions for byte-based phys_ram_dirty bitmap to
    bit-based     phys_ram_dirty bitmap.
  Introduce cpu_physical_memory_get_dirty_range().
  Use cpu_physical_memory_set_dirty_range() to update phys_ram_dirty.
  Use cpu_physical_memory_get_dirty_range() to check multiple dirty
    pages.

 arch_init.c   |   54 ++++++++++++++---------
 bswap.h       |    2 +
 cpu-all.h     |  131 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 exec.c        |   83 ++++++++++++++++++++++++++++++++++--
 kvm-all.c     |   33 +++++++--------
 qemu-common.h |    3 +
 6 files changed, 243 insertions(+), 63 deletions(-)

[Qemu-devel] [PATCH v3 1/6] Modify DIRTY_FLAG value and DIRTY_IDX introduce to use as indexes of bit-based phys_ram_dirty.

[Yoshiaki Tamura]

It uses ffs() to convert DIRTY_FLAG to DIRTY_IDX.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 cpu-all.h |   30 ++++++++++++++++++++++++++----
 1 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index f8bfa66..8c2d678 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -37,6 +37,9 @@
 
 #include "softfloat.h"
 
+/* to use ffs in flag_to_idx() */
+#include <strings.h>
+
 #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
 #define BSWAP_NEEDED
 #endif
@@ -853,7 +856,6 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
 /* memory API */
 
 extern int phys_ram_fd;
-extern uint8_t *phys_ram_dirty;
 extern ram_addr_t ram_size;
 extern ram_addr_t last_ram_offset;
 
@@ -878,9 +880,29 @@ extern int mem_prealloc;
 /* Set if TLB entry is an IO callback.  */
 #define TLB_MMIO        (1 << 5)
 
-#define VGA_DIRTY_FLAG       0x01
-#define CODE_DIRTY_FLAG      0x02
-#define MIGRATION_DIRTY_FLAG 0x08
+/* Use DIRTY_IDX as indexes of bit-based phys_ram_dirty. */
+#define MASTER_DIRTY_IDX    0
+#define VGA_DIRTY_IDX       1
+#define CODE_DIRTY_IDX      2
+#define MIGRATION_DIRTY_IDX 3
+#define NUM_DIRTY_IDX       4
+
+#define MASTER_DIRTY_FLAG    (1 << MASTER_DIRTY_IDX)
+#define VGA_DIRTY_FLAG       (1 << VGA_DIRTY_IDX)
+#define CODE_DIRTY_FLAG      (1 << CODE_DIRTY_IDX)
+#define MIGRATION_DIRTY_FLAG (1 << MIGRATION_DIRTY_IDX)
+
+extern unsigned long *phys_ram_dirty[NUM_DIRTY_IDX];
+
+static inline int flag_to_idx(int flag)
+{
+    return ffs(flag) - 1;
+}
+
+static inline int idx_to_flag(int idx)
+{
+    return 1 << idx;
+}
 
 /* read dirty bit (return 0 or 1) */
 static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
-- 
1.7.0.31.g1df487

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

[Yoshiaki Tamura]

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)) {
+        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 */
-- 
1.7.0.31.g1df487

[Qemu-devel] [PATCH v3 3/6] Modifies wrapper functions for byte-based phys_ram_dirty bitmap to bit-based phys_ram_dirty bitmap.

[Yoshiaki Tamura]

MASTER works as a buffer, and upon get_diry() or get_dirty_flags(), it calls
cpu_physical_memory_sync_master() to update VGA and MIGRATION.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 cpu-all.h |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index 8c2d678..2478887 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -907,43 +907,109 @@ static inline int idx_to_flag(int idx)
 /* read dirty bit (return 0 or 1) */
 static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
 {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
+    unsigned long mask;
+    ram_addr_t index = (addr >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+    int offset = (addr >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+ 
+    mask = 1UL << offset;
+    return (phys_ram_dirty[MASTER_DIRTY_IDX][index] & mask) == mask;
+}
+
+static inline void cpu_physical_memory_sync_master(ram_addr_t index)
+{
+    if (phys_ram_dirty[MASTER_DIRTY_IDX][index]) {
+        phys_ram_dirty[VGA_DIRTY_IDX][index]
+            |=  phys_ram_dirty[MASTER_DIRTY_IDX][index];
+        phys_ram_dirty[MIGRATION_DIRTY_IDX][index]
+            |=  phys_ram_dirty[MASTER_DIRTY_IDX][index];
+        phys_ram_dirty[MASTER_DIRTY_IDX][index] = 0UL;
+    }
 }
 
 static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
 {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS];
+     unsigned long mask;
+     ram_addr_t index = (addr >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+     int offset = (addr >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+     int ret = 0, i;
+ 
+     mask = 1UL << offset;
+     cpu_physical_memory_sync_master(index);
+
+     for (i = VGA_DIRTY_IDX; i <= MIGRATION_DIRTY_IDX; i++) {
+         if (phys_ram_dirty[i][index] & mask) {
+             ret |= idx_to_flag(i);
+         }
+     }
+ 
+     return ret;
+}
+
+static inline int cpu_physical_memory_get_dirty_idx(ram_addr_t addr,
+                                                    int dirty_idx)
+{
+    unsigned long mask;
+    ram_addr_t index = (addr >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+    int offset = (addr >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+
+    mask = 1UL << offset;
+    cpu_physical_memory_sync_master(index);
+    return (phys_ram_dirty[dirty_idx][index] & mask) == mask;
 }
 
 static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
                                                 int dirty_flags)
 {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
+    return cpu_physical_memory_get_dirty_idx(addr, flag_to_idx(dirty_flags));
 }
 
 static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
 {
-    phys_ram_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
+    unsigned long mask;
+    ram_addr_t index = (addr >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+    int offset = (addr >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+
+    mask = 1UL << offset;
+    phys_ram_dirty[MASTER_DIRTY_IDX][index] |= mask;
 }
 
-static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
-                                                      int dirty_flags)
+static inline void cpu_physical_memory_set_dirty_range(ram_addr_t addr,
+                                                       unsigned long mask)
 {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
+    ram_addr_t index = (addr >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+
+    phys_ram_dirty[MASTER_DIRTY_IDX][index] |= mask;
+}
+
+static inline void cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
+                                                       int dirty_flags)
+{
+    unsigned long mask;
+    ram_addr_t index = (addr >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+    int offset = (addr >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+
+    mask = 1UL << offset;
+    phys_ram_dirty[MASTER_DIRTY_IDX][index] |= mask;
+
+    if (dirty_flags & CODE_DIRTY_FLAG) {
+        phys_ram_dirty[CODE_DIRTY_IDX][index] |= mask;
+    }
 }
 
 static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
-                                                        int length,
+                                                        unsigned long length,
                                                         int dirty_flags)
 {
-    int i, mask, len;
-    uint8_t *p;
+    ram_addr_t addr = start, index;
+    unsigned long mask;
+    int offset, i;
 
-    len = length >> TARGET_PAGE_BITS;
-    mask = ~dirty_flags;
-    p = phys_ram_dirty + (start >> TARGET_PAGE_BITS);
-    for (i = 0; i < len; i++)
-        p[i] &= mask;
+    for (i = 0;  i < length; i += TARGET_PAGE_SIZE) {
+        index = ((addr + i) >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+        offset = ((addr + i) >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+        mask = ~(1UL << offset);
+        phys_ram_dirty[flag_to_idx(dirty_flags)][index] &= mask;
+     }
 }
 
 void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end,
-- 
1.7.0.31.g1df487

[Qemu-devel] [PATCH v3 4/6] Introduce cpu_physical_memory_get_dirty_range().

[Yoshiaki Tamura]

It checks the first row and puts dirty addr in the array.  If the first row is
empty, it skips to the first non-dirty row or the end addr, and put the length
in the first entry of the array.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
Signed-off-by: OHMURA Kei <ohmura.kei@lab.ntt.co.jp>
---
 cpu-all.h |    4 +++
 exec.c    |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index 2478887..fcfffe8 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -1012,6 +1012,10 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
      }
 }
 
+int cpu_physical_memory_get_dirty_range(ram_addr_t start, ram_addr_t end, 
+                                        ram_addr_t *dirty_rams, int length,
+                                        int dirty_flags);
+
 void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end,
                                      int dirty_flags);
 void cpu_tlb_update_dirty(CPUState *env);
diff --git a/exec.c b/exec.c
index b85cb26..54f497b 100644
--- a/exec.c
+++ b/exec.c
@@ -2045,6 +2045,73 @@ static inline void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry,
     }
 }
 
+/* It checks the first row and puts dirty addrs in the array.
+   If the first row is empty, it skips to the first non-dirty row
+   or the end addr, and put the length in the first entry of the array. */
+int cpu_physical_memory_get_dirty_range(ram_addr_t start, ram_addr_t end, 
+                                        ram_addr_t *dirty_rams, int length,
+                                        int dirty_flag)
+{
+    unsigned long p = 0, page_number;
+    ram_addr_t addr;
+    ram_addr_t s_idx = (start >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+    ram_addr_t e_idx = (end >> TARGET_PAGE_BITS) / HOST_LONG_BITS;
+    int i, j, offset, dirty_idx = flag_to_idx(dirty_flag);
+
+    /* mask bits before the start addr */
+    offset = (start >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+    cpu_physical_memory_sync_master(s_idx);
+    p |= phys_ram_dirty[dirty_idx][s_idx] & ~((1UL << offset) - 1);
+
+    if (s_idx == e_idx) {
+        /* mask bits after the end addr */
+        offset = (end >> TARGET_PAGE_BITS) & (HOST_LONG_BITS - 1);
+        p &= (1UL << offset) - 1;
+    }
+
+    if (p == 0) {
+        /* when the row is empty */
+        ram_addr_t skip;
+        if (s_idx == e_idx)
+            skip = end;
+        else {
+            /* skip empty rows */
+            while (s_idx < e_idx) {
+                s_idx++;
+                cpu_physical_memory_sync_master(s_idx);
+
+                if (phys_ram_dirty[dirty_idx][s_idx] != 0) {
+                    break;
+                }
+            }
+            skip = (s_idx * HOST_LONG_BITS * TARGET_PAGE_SIZE);
+        }
+        dirty_rams[0] = skip - start;
+        i = 0;
+
+    } else if (p == ~0UL) {
+        /* when the row is fully dirtied */
+        addr = start;
+        for (i = 0; i < length; i++) {
+            dirty_rams[i] = addr;
+            addr += TARGET_PAGE_SIZE;
+        }
+    } else {
+        /* when the row is partially dirtied */
+        i = 0;
+        do {
+            j = ffsl(p) - 1;
+            p &= ~(1UL << j);
+            page_number = s_idx * HOST_LONG_BITS + j;
+            addr = page_number * TARGET_PAGE_SIZE;
+            dirty_rams[i] = addr;
+            i++;
+        } while (p != 0 && i < length);
+    }
+
+    return i;
+}
+
 /* Note: start and end must be within the same ram block.  */
 void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end,
                                      int dirty_flags)
-- 
1.7.0.31.g1df487

[Qemu-devel] [PATCH v3 5/6] Use cpu_physical_memory_set_dirty_range() to update phys_ram_dirty.

[Yoshiaki Tamura]

Modifies kvm_physical_sync_dirty_bitmap to use
cpu_physical_memory_set_dirty_range() to update the row of the bit-based
phys_ram_dirty bitmap at once.

Signed-off-by: OHMURA Kei <ohmura.kei@lab.ntt.co.jp>
Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 bswap.h   |    2 ++
 kvm-all.c |   33 +++++++++++++++------------------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/bswap.h b/bswap.h
index aace9b7..956f3fa 100644
--- a/bswap.h
+++ b/bswap.h
@@ -205,8 +205,10 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
 
 #ifdef HOST_WORDS_BIGENDIAN
 #define cpu_to_32wu cpu_to_be32wu
+#define leul_to_cpu(v) le ## HOST_LONG_BITS ## _to_cpu(v)
 #else
 #define cpu_to_32wu cpu_to_le32wu
+#define leul_to_cpu(v) (v)
 #endif
 
 #undef le_bswap
diff --git a/kvm-all.c b/kvm-all.c
index 7aa5e57..db762ff 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -282,11 +282,6 @@ static int kvm_set_migration_log(int enable)
     return 0;
 }
 
-static int test_le_bit(unsigned long nr, unsigned char *addr)
-{
-    return (addr[nr >> 3] >> (nr & 7)) & 1;
-}
-
 /**
  * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
  * This function updates qemu's dirty bitmap using cpu_physical_memory_set_dirty().
@@ -299,9 +294,9 @@ static int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
 					  target_phys_addr_t end_addr)
 {
     KVMState *s = kvm_state;
-    unsigned long size, allocated_size = 0;
-    target_phys_addr_t phys_addr;
-    ram_addr_t addr;
+    unsigned long size, page_number, addr, addr1, *bitmap,
+        allocated_size = 0;
+    unsigned int i, len;
     KVMDirtyLog d;
     KVMSlot *mem;
     int ret = 0;
@@ -313,7 +308,8 @@ static int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
             break;
         }
 
-        size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
+        size = ((mem->memory_size >> TARGET_PAGE_BITS) + HOST_LONG_BITS - 1) /
+            HOST_LONG_BITS * HOST_LONG_SIZE;
         if (!d.dirty_bitmap) {
             d.dirty_bitmap = qemu_malloc(size);
         } else if (size > allocated_size) {
@@ -330,17 +326,18 @@ static int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
             break;
         }
 
-        for (phys_addr = mem->start_addr, addr = mem->phys_offset;
-             phys_addr < mem->start_addr + mem->memory_size;
-             phys_addr += TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
-            unsigned char *bitmap = (unsigned char *)d.dirty_bitmap;
-            unsigned nr = (phys_addr - mem->start_addr) >> TARGET_PAGE_BITS;
-
-            if (test_le_bit(nr, bitmap)) {
-                cpu_physical_memory_set_dirty(addr);
+        bitmap = (unsigned long *)d.dirty_bitmap;
+        len = size / HOST_LONG_SIZE;
+        for (i = 0; i < len; i++) {
+            if (bitmap[i] != 0) {
+                page_number = i * HOST_LONG_BITS;
+                addr1 = page_number * TARGET_PAGE_SIZE;
+                addr = mem->phys_offset + addr1;
+                cpu_physical_memory_set_dirty_range(addr, 
+                    leul_to_cpu(bitmap[i]));
             }
         }
-        start_addr = phys_addr;
+        start_addr = mem->start_addr + mem->memory_size;
     }
     qemu_free(d.dirty_bitmap);
 
-- 
1.7.0.31.g1df487

[Qemu-devel] [PATCH v3 6/6] Use cpu_physical_memory_get_dirty_range() to check multiple dirty pages.

[Yoshiaki Tamura]

Modifies ram_save_block() and ram_save_remaining() to use
cpu_physical_memory_get_dirty_range() to check multiple dirty and non-dirty
pages at once.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
Signed-off-by: OHMURA Kei <ohmura.kei@lab.ntt.co.jp>
---
 arch_init.c |   54 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index cfc03ea..245a082 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -108,31 +108,37 @@ static int ram_save_block(QEMUFile *f)
     static ram_addr_t current_addr = 0;
     ram_addr_t saved_addr = current_addr;
     ram_addr_t addr = 0;
-    int found = 0;
+    ram_addr_t dirty_rams[HOST_LONG_BITS];
+    int i, found = 0;
 
     while (addr < last_ram_offset) {
-        if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
+        if ((found = cpu_physical_memory_get_dirty_range(
+                 current_addr, last_ram_offset, dirty_rams, HOST_LONG_BITS,
+                 MIGRATION_DIRTY_FLAG))) {
             uint8_t *p;
 
-            cpu_physical_memory_reset_dirty(current_addr,
-                                            current_addr + TARGET_PAGE_SIZE,
-                                            MIGRATION_DIRTY_FLAG);
+            for (i = 0; i < found; i++) {
+                ram_addr_t page_addr = dirty_rams[i];
+                cpu_physical_memory_reset_dirty(page_addr,
+                                                page_addr + TARGET_PAGE_SIZE,
+                                                MIGRATION_DIRTY_FLAG);
 
-            p = qemu_get_ram_ptr(current_addr);
+                p = qemu_get_ram_ptr(page_addr);
 
-            if (is_dup_page(p, *p)) {
-                qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
-                qemu_put_byte(f, *p);
-            } else {
-                qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_PAGE);
-                qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
+                if (is_dup_page(p, *p)) {
+                    qemu_put_be64(f, page_addr | RAM_SAVE_FLAG_COMPRESS);
+                    qemu_put_byte(f, *p);
+                } else {
+                    qemu_put_be64(f, page_addr | RAM_SAVE_FLAG_PAGE);
+                    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
+                }
             }
 
-            found = 1;
             break;
+        } else {
+            addr += dirty_rams[0];
+            current_addr = (saved_addr + addr) % last_ram_offset;
         }
-        addr += TARGET_PAGE_SIZE;
-        current_addr = (saved_addr + addr) % last_ram_offset;
     }
 
     return found;
@@ -142,12 +148,18 @@ static uint64_t bytes_transferred;
 
 static ram_addr_t ram_save_remaining(void)
 {
-    ram_addr_t addr;
+    ram_addr_t addr = 0;
     ram_addr_t count = 0;
+    ram_addr_t dirty_rams[HOST_LONG_BITS];
+    int found = 0;
 
-    for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
-        if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
-            count++;
+    while (addr < last_ram_offset) {
+        if ((found = cpu_physical_memory_get_dirty_range(addr, last_ram_offset,
+            dirty_rams, HOST_LONG_BITS, MIGRATION_DIRTY_FLAG))) {
+            count += found;
+            addr = dirty_rams[found - 1] + TARGET_PAGE_SIZE;
+        } else {
+            addr += dirty_rams[0];
         }
     }
 
-- 
1.7.0.31.g1df487

[Qemu-devel] Re: [PATCH v3 1/6] Modify DIRTY_FLAG value and DIRTY_IDX introduce to use as indexes of bit-based phys_ram_dirty.

[Avi Kivity]

On 04/19/2010 12:43 PM, Yoshiaki Tamura wrote:
> It uses ffs() to convert DIRTY_FLAG to DIRTY_IDX.
>
> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
> ---
>   cpu-all.h |   30 ++++++++++++++++++++++++++----
>   1 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/cpu-all.h b/cpu-all.h
> index f8bfa66..8c2d678 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -37,6 +37,9 @@
>
>   #include "softfloat.h"
>
> +/* to use ffs in flag_to_idx() */
> +#include<strings.h>
> +
>   #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
>   #define BSWAP_NEEDED
>   #endif
> @@ -853,7 +856,6 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
>   /* memory API */
>
>   extern int phys_ram_fd;
> -extern uint8_t *phys_ram_dirty;
>   extern ram_addr_t ram_size;
>   extern ram_addr_t last_ram_offset;
>
> @@ -878,9 +880,29 @@ extern int mem_prealloc;
>   /* Set if TLB entry is an IO callback.  */
>   #define TLB_MMIO        (1<<  5)
>
> -#define VGA_DIRTY_FLAG       0x01
> -#define CODE_DIRTY_FLAG      0x02
> -#define MIGRATION_DIRTY_FLAG 0x08
> +/* Use DIRTY_IDX as indexes of bit-based phys_ram_dirty. */
> +#define MASTER_DIRTY_IDX    0
> +#define VGA_DIRTY_IDX       1
> +#define CODE_DIRTY_IDX      2
> +#define MIGRATION_DIRTY_IDX 3
> +#define NUM_DIRTY_IDX       4
> +
> +#define MASTER_DIRTY_FLAG    (1<<  MASTER_DIRTY_IDX)
> +#define VGA_DIRTY_FLAG       (1<<  VGA_DIRTY_IDX)
> +#define CODE_DIRTY_FLAG      (1<<  CODE_DIRTY_IDX)
> +#define MIGRATION_DIRTY_FLAG (1<<  MIGRATION_DIRTY_IDX)
> +
> +extern unsigned long *phys_ram_dirty[NUM_DIRTY_IDX];
>    

Please modify the bitmap definition in the patch where you start using 
it; this patch won't compile by itself, breaking bisection.

> +
> +static inline int flag_to_idx(int flag)
> +{
> +    return ffs(flag) - 1;
> +}
> +
> +static inline int idx_to_flag(int idx)
> +{
> +    return 1<<  idx;
> +}
>
>    

These should not be in global scope - it is not clear they refer to 
dirty bitmaps.  Please rename or move to local scope.

Alternatively, convert the callers to use *_DIRTY_IDX.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Avi Kivity]

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

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v3 0/6] Introduce bit-based phys_ram_dirty, and bit-based dirty page checker.

[Avi Kivity]

On 04/19/2010 12:43 PM, Yoshiaki Tamura wrote:
> The dirty and non-dirty pages are checked one by one.  When most of the memory
> is not dirty, checking the dirty and non-dirty pages by multiple page size
> should be much faster than checking them one by one.  We introduced bit-based
> phys_ram_dirty for VGA, CODE, MIGRATION, MASTER, and
> cpu_physical_memory_get_dirty_range() for this purpose.
>
>    

Apart from my minor comments to patches 1 and 2 this looks ready to merge.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v3 1/6] Modify DIRTY_FLAG value and DIRTY_IDX introduce to use as indexes of bit-based phys_ram_dirty.

[Yoshiaki Tamura]

Avi Kivity wrote:
> On 04/19/2010 12:43 PM, Yoshiaki Tamura wrote:
>> It uses ffs() to convert DIRTY_FLAG to DIRTY_IDX.
>>
>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>> ---
>> cpu-all.h | 30 ++++++++++++++++++++++++++----
>> 1 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/cpu-all.h b/cpu-all.h
>> index f8bfa66..8c2d678 100644
>> --- a/cpu-all.h
>> +++ b/cpu-all.h
>> @@ -37,6 +37,9 @@
>>
>> #include "softfloat.h"
>>
>> +/* to use ffs in flag_to_idx() */
>> +#include<strings.h>
>> +
>> #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
>> #define BSWAP_NEEDED
>> #endif
>> @@ -853,7 +856,6 @@ target_phys_addr_t
>> cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
>> /* memory API */
>>
>> extern int phys_ram_fd;
>> -extern uint8_t *phys_ram_dirty;
>> extern ram_addr_t ram_size;
>> extern ram_addr_t last_ram_offset;
>>
>> @@ -878,9 +880,29 @@ extern int mem_prealloc;
>> /* Set if TLB entry is an IO callback. */
>> #define TLB_MMIO (1<< 5)
>>
>> -#define VGA_DIRTY_FLAG 0x01
>> -#define CODE_DIRTY_FLAG 0x02
>> -#define MIGRATION_DIRTY_FLAG 0x08
>> +/* Use DIRTY_IDX as indexes of bit-based phys_ram_dirty. */
>> +#define MASTER_DIRTY_IDX 0
>> +#define VGA_DIRTY_IDX 1
>> +#define CODE_DIRTY_IDX 2
>> +#define MIGRATION_DIRTY_IDX 3
>> +#define NUM_DIRTY_IDX 4
>> +
>> +#define MASTER_DIRTY_FLAG (1<< MASTER_DIRTY_IDX)
>> +#define VGA_DIRTY_FLAG (1<< VGA_DIRTY_IDX)
>> +#define CODE_DIRTY_FLAG (1<< CODE_DIRTY_IDX)
>> +#define MIGRATION_DIRTY_FLAG (1<< MIGRATION_DIRTY_IDX)
>> +
>> +extern unsigned long *phys_ram_dirty[NUM_DIRTY_IDX];
>
> Please modify the bitmap definition in the patch where you start using
> it; this patch won't compile by itself, breaking bisection.

Sorry about that.
I would merge this with 2/6.

>
>> +
>> +static inline int flag_to_idx(int flag)
>> +{
>> + return ffs(flag) - 1;
>> +}
>> +
>> +static inline int idx_to_flag(int idx)
>> +{
>> + return 1<< idx;
>> +}
>>
>
> These should not be in global scope - it is not clear they refer to
> dirty bitmaps. Please rename or move to local scope.
>
> Alternatively, convert the callers to use *_DIRTY_IDX.

OK.  Let me rename it to, dirty_flag_to_idx() and dirty_idx_to_flag() respectively.

[Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Yoshiaki Tamura]

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

[Qemu-devel] Re: [PATCH v3 0/6] Introduce bit-based phys_ram_dirty, and bit-based dirty page checker.

[Yoshiaki Tamura]

Avi Kivity wrote:
> On 04/19/2010 12:43 PM, Yoshiaki Tamura wrote:
>> The dirty and non-dirty pages are checked one by one. When most of the
>> memory
>> is not dirty, checking the dirty and non-dirty pages by multiple page
>> size
>> should be much faster than checking them one by one. We introduced
>> bit-based
>> phys_ram_dirty for VGA, CODE, MIGRATION, MASTER, and
>> cpu_physical_memory_get_dirty_range() for this purpose.
>>
>
> Apart from my minor comments to patches 1 and 2 this looks ready to merge.

Thank you for reviewing.
I would apply your comments, and post as v4.

Re: [Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Yoshiaki Tamura]

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

Re: [Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Yoshiaki Tamura]

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.

Re: [Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v3 2/6] Introduce bit-based phys_ram_dirty for VGA, CODE, MIGRATION and MASTER.

[Yoshiaki Tamura]

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));