migration: cache memory region ram ptr

arch_init.c +5 -3

            fprintf(stderr, "Ack, bad migration stream!\n");
            return NULL;
        }
 
-        return memory_region_get_ram_ptr(block->mr) + offset;
+        return ram_ptr + offset;
    }
 
    len = qemu_get_byte(f);

@@ -1007,8 +1014,10 @@ static inline void *host_from_stream_offset(QEMUFile *f,
    id[len] = 0;
 
    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        if (!strncmp(id, block->idstr, sizeof(id)))
-            return memory_region_get_ram_ptr(block->mr) + offset;
+        if (!strncmp(id, block->idstr, sizeof(id))) {
+            ram_ptr = memory_region_get_ram_ptr(block->mr);
+            return ram_ptr + offset;
+        }
    }
 
    fprintf(stderr, "Can't find block %s!\n", id);

Signature · 1 line

1.7.9.5

Hi,
The patch is correct. There is a small improved point.

arch_init.c static inline void *host_from_stream_offset(QEMUFile *f, · line 998

Quoted 6 lines

            /* In doubt sent page as normal */
            bytes_sent = -1;

arch_init.c +0 -0

@@ -990,16 +996,17 @@ static inline void *host_from_stream_offset(QEMUFile *f,
                                            int flags)
 {
    static RAMBlock *block = NULL;

       RAMBlock *block = NULL;

arch_init.c static inline void *host_from_stream_offset(QEMUFile *f, · line 1004

Quoted 8 lines

arch_init.c +2 -1

+    static uint8_t *ram_ptr;
    char id[256];
    uint8_t len;
 
    if (flags & RAM_SAVE_FLAG_CONTINUE) {
-        if (!block) {
+        if (!block || !ram_ptr) {

             if (!ram_ptr) {

Best regards
ChenLiang

Quoted 26 lines

arch_init.c +5 -3

            fprintf(stderr, "Ack, bad migration stream!\n");
            return NULL;
        }
 
-        return memory_region_get_ram_ptr(block->mr) + offset;
+        return ram_ptr + offset;
    }
 
    len = qemu_get_byte(f);

@@ -1007,8 +1014,10 @@ static inline void *host_from_stream_offset(QEMUFile *f,
    id[len] = 0;
 
    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        if (!strncmp(id, block->idstr, sizeof(id)))
-            return memory_region_get_ram_ptr(block->mr) + offset;
+        if (!strncmp(id, block->idstr, sizeof(id))) {
+            ram_ptr = memory_region_get_ram_ptr(block->mr);
+            return ram_ptr + offset;
+        }
    }
 
    fprintf(stderr, "Can't find block %s!\n", id);

Signature · 1 line

1.7.9.5

we currently look up the ram ptr for each single page. Cache the pointer while we operate on the same block.

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

arch_init.c +16 -7

index 582b716..ce338aa 100644
@@ -594,13 +594,19 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
                 ram_bulk_stage = false;
             }
         } else {
+            static uint8_t *ram_ptr;
             int ret;
             uint8_t *p;
             bool send_async = true;
-            int cont = (block == last_sent_block) ?
-                RAM_SAVE_FLAG_CONTINUE : 0;
+            int cont = 0;
 
-            p = memory_region_get_ram_ptr(mr) + offset;
+            if (block != last_sent_block) {
+                ram_ptr = memory_region_get_ram_ptr(mr);
+            } else {
+                cont = RAM_SAVE_FLAG_CONTINUE;
+            }
+
+            p = ram_ptr + offset;
 
             /* In doubt sent page as normal */
             bytes_sent = -1;

@@ -990,16 +996,17 @@ static inline void *host_from_stream_offset(QEMUFile *f,
                                             int flags)
 {
     static RAMBlock *block = NULL;
+    static uint8_t *ram_ptr;
     char id[256];
     uint8_t len;
 
     if (flags & RAM_SAVE_FLAG_CONTINUE) {
-        if (!block) {
+        if (!block || !ram_ptr) {
             fprintf(stderr, "Ack, bad migration stream!\n");
             return NULL;
         }
 
-        return memory_region_get_ram_ptr(block->mr) + offset;
+        return ram_ptr + offset;
     }
 
     len = qemu_get_byte(f);

@@ -1007,8 +1014,10 @@ static inline void *host_from_stream_offset(QEMUFile *f,
     id[len] = 0;
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        if (!strncmp(id, block->idstr, sizeof(id)))
-            return memory_region_get_ram_ptr(block->mr) + offset;
+        if (!strncmp(id, block->idstr, sizeof(id))) {
+            ram_ptr = memory_region_get_ram_ptr(block->mr);
+            return ram_ptr + offset;
+        }
     }
 
     fprintf(stderr, "Can't find block %s!\n", id);

Il 10/05/2014 12:51, Peter Lieven ha scritto:

Quoted 2 lines

we currently look up the ram ptr for each single page. Cache the pointer while we operate on the same block.

Why don't you instead cache the result in the MemoryRegion, so that memory_region_get_ram_ptr becomes a simple, inline field access?

Paolo

Quoted 66 lines

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

arch_init.c +16 -7

index 582b716..ce338aa 100644
@@ -594,13 +594,19 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
                 ram_bulk_stage = false;
             }
         } else {
+            static uint8_t *ram_ptr;
             int ret;
             uint8_t *p;
             bool send_async = true;
-            int cont = (block == last_sent_block) ?
-                RAM_SAVE_FLAG_CONTINUE : 0;
+            int cont = 0;
 
-            p = memory_region_get_ram_ptr(mr) + offset;
+            if (block != last_sent_block) {
+                ram_ptr = memory_region_get_ram_ptr(mr);
+            } else {
+                cont = RAM_SAVE_FLAG_CONTINUE;
+            }
+
+            p = ram_ptr + offset;
 
             /* In doubt sent page as normal */
             bytes_sent = -1;

@@ -990,16 +996,17 @@ static inline void *host_from_stream_offset(QEMUFile *f,
                                             int flags)
 {
     static RAMBlock *block = NULL;
+    static uint8_t *ram_ptr;
     char id[256];
     uint8_t len;
 
     if (flags & RAM_SAVE_FLAG_CONTINUE) {
-        if (!block) {
+        if (!block || !ram_ptr) {
             fprintf(stderr, "Ack, bad migration stream!\n");
             return NULL;
         }
 
-        return memory_region_get_ram_ptr(block->mr) + offset;
+        return ram_ptr + offset;
     }
 
     len = qemu_get_byte(f);

@@ -1007,8 +1014,10 @@ static inline void *host_from_stream_offset(QEMUFile *f,
     id[len] = 0;
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        if (!strncmp(id, block->idstr, sizeof(id)))
-            return memory_region_get_ram_ptr(block->mr) + offset;
+        if (!strncmp(id, block->idstr, sizeof(id))) {
+            ram_ptr = memory_region_get_ram_ptr(block->mr);
+            return ram_ptr + offset;
+        }
     }
 
     fprintf(stderr, "Can't find block %s!\n", id);

Il 10/05/2014 12:51, Peter Lieven ha scritto:

Quoted 2 lines

we currently look up the ram ptr for each single page. Cache the pointer while we operate on the same block.

Why don't you instead cache the result in the MemoryRegion, so that memory_region_get_ram_ptr becomes a simple, inline field access?

Paolo

Quoted 66 lines

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

arch_init.c +16 -7

index 582b716..ce338aa 100644
@@ -594,13 +594,19 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
                 ram_bulk_stage = false;
             }
         } else {
+            static uint8_t *ram_ptr;
             int ret;
             uint8_t *p;
             bool send_async = true;
-            int cont = (block == last_sent_block) ?
-                RAM_SAVE_FLAG_CONTINUE : 0;
+            int cont = 0;
 
-            p = memory_region_get_ram_ptr(mr) + offset;
+            if (block != last_sent_block) {
+                ram_ptr = memory_region_get_ram_ptr(mr);
+            } else {
+                cont = RAM_SAVE_FLAG_CONTINUE;
+            }
+
+            p = ram_ptr + offset;
 
             /* In doubt sent page as normal */
             bytes_sent = -1;

@@ -990,16 +996,17 @@ static inline void *host_from_stream_offset(QEMUFile *f,
                                             int flags)
 {
     static RAMBlock *block = NULL;
+    static uint8_t *ram_ptr;
     char id[256];
     uint8_t len;
 
     if (flags & RAM_SAVE_FLAG_CONTINUE) {
-        if (!block) {
+        if (!block || !ram_ptr) {
             fprintf(stderr, "Ack, bad migration stream!\n");
             return NULL;
         }
 
-        return memory_region_get_ram_ptr(block->mr) + offset;
+        return ram_ptr + offset;
     }
 
     len = qemu_get_byte(f);

@@ -1007,8 +1014,10 @@ static inline void *host_from_stream_offset(QEMUFile *f,
     id[len] = 0;
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        if (!strncmp(id, block->idstr, sizeof(id)))
-            return memory_region_get_ram_ptr(block->mr) + offset;
+        if (!strncmp(id, block->idstr, sizeof(id))) {
+            ram_ptr = memory_region_get_ram_ptr(block->mr);
+            return ram_ptr + offset;
+        }
     }
 
     fprintf(stderr, "Can't find block %s!\n", id);

Am 10.05.2014 17:33, schrieb Paolo Bonzini:

Quoted 4 lines

Il 10/05/2014 12:51, Peter Lieven ha scritto:

Quoted 2 lines

we currently look up the ram ptr for each single page. Cache the pointer while we operate on the same block.

Why don't you instead cache the result in the MemoryRegion, so that memory_region_get_ram_ptr becomes a simple, inline field access?

This seems to work. Wondering if it has other side implications. Basic tests like booting vServers and migration work. What about XEN?

include/exec/memory.h +1 -0

index 1d55ad9..3003875 100644
@@ -161,6 +161,7 @@ struct MemoryRegion {
     unsigned ioeventfd_nb;
     MemoryRegionIoeventfd *ioeventfds;
     NotifierList iommu_notify;
+    void *ram_ptr;
 };
 
 /**

memory.c +7 -1

index 3f1df23..78d4032 100644
@@ -862,6 +862,7 @@ void memory_region_init(MemoryRegion *mr,
     mr->ioeventfd_nb = 0;
     mr->ioeventfds = NULL;
     mr->flush_coalesced_mmio = false;
+    mr->ram_ptr = NULL;
 }
 
 static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,

@@ -1249,7 +1250,12 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
 
     assert(mr->terminates);
 
-    return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
+    if (mr->ram_ptr) {
+        return mr->ram_ptr;
+    }
+
+    mr->ram_ptr = qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
+    return mr->ram_ptr;
 }
 
 static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpace *as)

Peter

Quoted 68 lines

Paolo

Quoted 66 lines

Signed-off-by Peter Lieven <[email protected]>

---
 arch_init.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

arch_init.c +16 -7

index 582b716..ce338aa 100644
@@ -594,13 +594,19 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
                 ram_bulk_stage = false;
             }
         } else {
+            static uint8_t *ram_ptr;
             int ret;
             uint8_t *p;
             bool send_async = true;
-            int cont = (block == last_sent_block) ?
-                RAM_SAVE_FLAG_CONTINUE : 0;
+            int cont = 0;
 
-            p = memory_region_get_ram_ptr(mr) + offset;
+            if (block != last_sent_block) {
+                ram_ptr = memory_region_get_ram_ptr(mr);
+            } else {
+                cont = RAM_SAVE_FLAG_CONTINUE;
+            }
+
+            p = ram_ptr + offset;
 
             /* In doubt sent page as normal */
             bytes_sent = -1;

@@ -990,16 +996,17 @@ static inline void *host_from_stream_offset(QEMUFile *f,
                                             int flags)
 {
     static RAMBlock *block = NULL;
+    static uint8_t *ram_ptr;
     char id[256];
     uint8_t len;
 
     if (flags & RAM_SAVE_FLAG_CONTINUE) {
-        if (!block) {
+        if (!block || !ram_ptr) {
             fprintf(stderr, "Ack, bad migration stream!\n");
             return NULL;
         }
 
-        return memory_region_get_ram_ptr(block->mr) + offset;
+        return ram_ptr + offset;
     }
 
     len = qemu_get_byte(f);

@@ -1007,8 +1014,10 @@ static inline void *host_from_stream_offset(QEMUFile *f,
     id[len] = 0;
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        if (!strncmp(id, block->idstr, sizeof(id)))
-            return memory_region_get_ram_ptr(block->mr) + offset;
+        if (!strncmp(id, block->idstr, sizeof(id))) {
+            ram_ptr = memory_region_get_ram_ptr(block->mr);
+            return ram_ptr + offset;
+        }
     }
 
     fprintf(stderr, "Can't find block %s!\n", id);

What about XEN?

Il 10/05/2014 18:32, Peter Lieven ha scritto:

Quoted 2 lines

What about XEN?

You're right, Xen wouldn't work. Your original patch would not break it just because Xen doesn't use migration (but the code would be broken).

You would have to cache qemu_get_ram_block rather than qemu_get_ram_ptr, move RAMBlock to memory-internal.h, and split the RAMBlock + ram_addr_t => void * conversion out of qemu_get_ram_ptr and into a separate function (to be used by memory_region_get_ram_ptr).

I'm not sure of the benefit of your patch though. qemu_get_ram_block already has a 1-item cache, are you seeing a low hit rate there? Or any other profiling that shows qemu_get_ram_ptr as hot?

Paolo