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

login
register
mail settings
Submitter Yoshiaki Tamura
Date April 6, 2010, 12:51 a.m.
Message ID <1270515084-24120-3-git-send-email-tamura.yoshiaki@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/49473/
State New
Headers show

Comments

Yoshiaki Tamura - April 6, 2010, 12:51 a.m.
Replaces byte-based phys_ram_dirty bitmap with three 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>
Signed-off-by: OHMURA Kei <ohmura.kei@lab.ntt.co.jp>
---
 exec.c |   32 +++++++++++++++++++++++++++-----
 1 files changed, 27 insertions(+), 5 deletions(-)
Avi Kivity - April 12, 2010, 8:01 a.m.
On 04/06/2010 03:51 AM, Yoshiaki Tamura wrote:
> Replaces byte-based phys_ram_dirty bitmap with three bit-based phys_ram_dirty
> bitmap.  On allocation, it sets all bits in the bitmap.
>
>
>    

> index c74b0a4..9733892 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_FLAGS];
>   static int in_migration;
>
>   typedef struct RAMBlock {
> @@ -2825,10 +2825,32 @@ 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);
> +/* temporarily copy from qemu-kvm.git/qemu-kvm.h */
> +#define ALIGN(x, y)  (((x)+(y)-1)&  ~((y)-1))
> +#define BITMAP_SIZE(m) (ALIGN(((m)>>TARGET_PAGE_BITS), HOST_LONG_BITS) / 8)
>    

Please put in some header file, maybe qemu-common.h.

> +
> +    if (BITMAP_SIZE(last_ram_offset + size) != BITMAP_SIZE(last_ram_offset)) {
> +        phys_ram_dirty[MASTER_DIRTY_FLAG] =
> +            qemu_realloc(phys_ram_dirty[MASTER_DIRTY_FLAG],
> +                         BITMAP_SIZE(last_ram_offset + size));
> +        phys_ram_dirty[VGA_DIRTY_FLAG]
> +            = qemu_realloc(phys_ram_dirty[VGA_DIRTY_FLAG],
> +                           BITMAP_SIZE(last_ram_offset + size));
> +        phys_ram_dirty[CODE_DIRTY_FLAG] =
> +            qemu_realloc(phys_ram_dirty[CODE_DIRTY_FLAG],
> +                         BITMAP_SIZE(last_ram_offset + size));
> +        phys_ram_dirty[MIGRATION_DIRTY_FLAG] =
> +            qemu_realloc(phys_ram_dirty[MIGRATION_DIRTY_FLAG],
> +                         BITMAP_SIZE(last_ram_offset + size));
> +        memset((uint8_t *)phys_ram_dirty[MASTER_DIRTY_FLAG] +
> +               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> +        memset((uint8_t *)phys_ram_dirty[VGA_DIRTY_FLAG] +
> +               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> +        memset((uint8_t *)phys_ram_dirty[CODE_DIRTY_FLAG] +
> +               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> +        memset((uint8_t *)phys_ram_dirty[MIGRATION_DIRTY_FLAG] +
> +               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
> +    }
>
>    

Should be nicer as a loop calling a helper to allocate each bitmap.  
This patch won't work by itself, will it?  I think you need to merge it 
with the succeeding patches.
Yoshiaki Tamura - April 12, 2010, 9:39 a.m.
Avi Kivity wrote:
> On 04/06/2010 03:51 AM, Yoshiaki Tamura wrote:
>> Replaces byte-based phys_ram_dirty bitmap with three bit-based
>> phys_ram_dirty
>> bitmap. On allocation, it sets all bits in the bitmap.
>>
>>
>
>> index c74b0a4..9733892 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_FLAGS];
>> static int in_migration;
>>
>> typedef struct RAMBlock {
>> @@ -2825,10 +2825,32 @@ 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);
>> +/* temporarily copy from qemu-kvm.git/qemu-kvm.h */
>> +#define ALIGN(x, y) (((x)+(y)-1)& ~((y)-1))
>> +#define BITMAP_SIZE(m) (ALIGN(((m)>>TARGET_PAGE_BITS),
>> HOST_LONG_BITS) / 8)
>
> Please put in some header file, maybe qemu-common.h.

OK.  BTW, is qemu-kvm.h planned to go upstream?

>> +
>> + if (BITMAP_SIZE(last_ram_offset + size) !=
>> BITMAP_SIZE(last_ram_offset)) {
>> + phys_ram_dirty[MASTER_DIRTY_FLAG] =
>> + qemu_realloc(phys_ram_dirty[MASTER_DIRTY_FLAG],
>> + BITMAP_SIZE(last_ram_offset + size));
>> + phys_ram_dirty[VGA_DIRTY_FLAG]
>> + = qemu_realloc(phys_ram_dirty[VGA_DIRTY_FLAG],
>> + BITMAP_SIZE(last_ram_offset + size));
>> + phys_ram_dirty[CODE_DIRTY_FLAG] =
>> + qemu_realloc(phys_ram_dirty[CODE_DIRTY_FLAG],
>> + BITMAP_SIZE(last_ram_offset + size));
>> + phys_ram_dirty[MIGRATION_DIRTY_FLAG] =
>> + qemu_realloc(phys_ram_dirty[MIGRATION_DIRTY_FLAG],
>> + BITMAP_SIZE(last_ram_offset + size));
>> + memset((uint8_t *)phys_ram_dirty[MASTER_DIRTY_FLAG] +
>> + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
>> + memset((uint8_t *)phys_ram_dirty[VGA_DIRTY_FLAG] +
>> + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
>> + memset((uint8_t *)phys_ram_dirty[CODE_DIRTY_FLAG] +
>> + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
>> + memset((uint8_t *)phys_ram_dirty[MIGRATION_DIRTY_FLAG] +
>> + BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
>> + }
>>
>
> Should be nicer as a loop calling a helper to allocate each bitmap. This
> patch won't work by itself, will it? I think you need to merge it with
> the succeeding patches.

Yeah.  I originally wrote as you suggested, but I needed to skip 0x03 because of 
the reason written below.

> +/* Use DIRTY_FLAG as indexes of bit-based phys_ram_dirty.
> +   0x03 is empty to make it compatible with byte-based bitmap. */
> +#define MASTER_DIRTY_FLAG    0x00
>  #define VGA_DIRTY_FLAG       0x01
>  #define CODE_DIRTY_FLAG      0x02
> -#define MIGRATION_DIRTY_FLAG 0x08
> +#define MIGRATION_DIRTY_FLAG 0x04

If you think if (i != 0x03) is better, I would modify it to a loop.

And sorry, you need to merge the succeeding patches to make it work.
Avi Kivity - April 12, 2010, 10:17 a.m.
On 04/12/2010 12:39 PM, Yoshiaki Tamura wrote:
>> Please put in some header file, maybe qemu-common.h.
>
>
> OK.  BTW, is qemu-kvm.h planned to go upstream?

No.  Use kvm.h for kvm specific symbols (qemu-kvm.h includes it).

>>
>> Should be nicer as a loop calling a helper to allocate each bitmap. This
>> patch won't work by itself, will it? I think you need to merge it with
>> the succeeding patches.
>
> Yeah.  I originally wrote as you suggested, but I needed to skip 0x03 
> because of the reason written below.
>
>> +/* Use DIRTY_FLAG as indexes of bit-based phys_ram_dirty.
>> +   0x03 is empty to make it compatible with byte-based bitmap. */
>> +#define MASTER_DIRTY_FLAG    0x00
>>  #define VGA_DIRTY_FLAG       0x01
>>  #define CODE_DIRTY_FLAG      0x02
>> -#define MIGRATION_DIRTY_FLAG 0x08
>> +#define MIGRATION_DIRTY_FLAG 0x04
>
> If you think if (i != 0x03) is better, I would modify it to a loop.

You can have *_DIRTY_FLAG (1, 2, 4, 8) and *_DIRTY_IDX (0, 1, 2, 3); use 
_FLAG for compatibility with byte-based maps, and _IDX for multiple bitmaps.

Alternatively, have only _IDX, and modify the callers to shift when 
necessary.  At the end of the patchset, we'll only use bitmaps, so the 
shifts will be all gone.

Patch

diff --git a/exec.c b/exec.c
index c74b0a4..9733892 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_FLAGS];
 static int in_migration;
 
 typedef struct RAMBlock {
@@ -2825,10 +2825,32 @@  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);
+/* temporarily copy from qemu-kvm.git/qemu-kvm.h */
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
+#define BITMAP_SIZE(m) (ALIGN(((m)>>TARGET_PAGE_BITS), HOST_LONG_BITS) / 8)
+
+    if (BITMAP_SIZE(last_ram_offset + size) != BITMAP_SIZE(last_ram_offset)) {
+        phys_ram_dirty[MASTER_DIRTY_FLAG] =
+            qemu_realloc(phys_ram_dirty[MASTER_DIRTY_FLAG],
+                         BITMAP_SIZE(last_ram_offset + size));
+        phys_ram_dirty[VGA_DIRTY_FLAG] 
+            = qemu_realloc(phys_ram_dirty[VGA_DIRTY_FLAG],
+                           BITMAP_SIZE(last_ram_offset + size));
+        phys_ram_dirty[CODE_DIRTY_FLAG] =
+            qemu_realloc(phys_ram_dirty[CODE_DIRTY_FLAG],
+                         BITMAP_SIZE(last_ram_offset + size));
+        phys_ram_dirty[MIGRATION_DIRTY_FLAG] =
+            qemu_realloc(phys_ram_dirty[MIGRATION_DIRTY_FLAG],
+                         BITMAP_SIZE(last_ram_offset + size));
+        memset((uint8_t *)phys_ram_dirty[MASTER_DIRTY_FLAG] +
+               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
+        memset((uint8_t *)phys_ram_dirty[VGA_DIRTY_FLAG] +
+               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
+        memset((uint8_t *)phys_ram_dirty[CODE_DIRTY_FLAG] +
+               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
+        memset((uint8_t *)phys_ram_dirty[MIGRATION_DIRTY_FLAG] +
+               BITMAP_SIZE(last_ram_offset), 0xff, BITMAP_SIZE(size));
+    }
 
     last_ram_offset += size;