[Qemu-devel] [PATCH v3 0/3] memory: some little optimizations

[Qemu-devel] [PATCH v3 0/3] memory: some little optimizations

[Gonglei]

Please see the details in each patch.

v3 -> v2:
 - rebase on the master
 - resend because Delivery has failed to these recipients or groups: qemu-devel@nongnu.org

v2 -> v1:
 - rework an argument's name for clearity in patch 1. [Fam]
 - fix some typos in patch 2. [Fam]
 - touche block->fd directly and remove the wrappers. [Paolo]

Gonglei (3):
  memory: drop find_ram_block()
  exec: adjust rcu_read_lock requirement
  memory: drop some wrappers that waste cpu cycle


Gonglei (3):
  memory: drop find_ram_block()
  exec: adjust rcu_read_lock requirement
  memory: drop some wrappers that waste cpu cycle

 exec.c                    | 71 ++++++-----------------------------------------
 hw/misc/ivshmem.c         |  8 ++++--
 hw/virtio/vhost-user.c    | 13 +++++----
 include/exec/cpu-common.h |  4 +--
 include/exec/ram_addr.h   |  6 ++--
 memory.c                  |  4 +--
 migration/ram.c           |  2 +-
 migration/savevm.c        |  4 +--
 8 files changed, 31 insertions(+), 81 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 1/3] memory: drop find_ram_block()

[Gonglei]

On the one hand, we have already qemu_get_ram_block() whose function
is similar. On the other hand, we can directly use mr->ram_block but
searching RAMblock by ram_addr which is a kind of waste.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 exec.c                    | 34 +++++++---------------------------
 include/exec/cpu-common.h |  4 ++--
 include/exec/ram_addr.h   |  2 +-
 memory.c                  |  2 +-
 migration/ram.c           |  2 +-
 migration/savevm.c        |  4 ++--
 6 files changed, 14 insertions(+), 34 deletions(-)

diff --git a/exec.c b/exec.c
index c4f9036..c840bc7 100644
--- a/exec.c
+++ b/exec.c
@@ -1407,34 +1407,18 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
     }
 }
 
-/* Called within an RCU critical section, or while the ramlist lock
- * is held.
- */
-static RAMBlock *find_ram_block(ram_addr_t addr)
-{
-    RAMBlock *block;
-
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
-        if (block->offset == addr) {
-            return block;
-        }
-    }
-
-    return NULL;
-}
-
 const char *qemu_ram_get_idstr(RAMBlock *rb)
 {
     return rb->idstr;
 }
 
 /* Called with iothread lock held.  */
-void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
+void qemu_ram_set_idstr(RAMBlock *new_block, const char *name, DeviceState *dev)
 {
-    RAMBlock *new_block, *block;
+    RAMBlock *block;
 
     rcu_read_lock();
-    new_block = find_ram_block(addr);
+
     assert(new_block);
     assert(!new_block->idstr[0]);
 
@@ -1448,7 +1432,8 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
     pstrcat(new_block->idstr, sizeof(new_block->idstr), name);
 
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
-        if (block != new_block && !strcmp(block->idstr, new_block->idstr)) {
+        if (block != new_block &&
+            !strcmp(block->idstr, new_block->idstr)) {
             fprintf(stderr, "RAMBlock \"%s\" already registered, abort!\n",
                     new_block->idstr);
             abort();
@@ -1458,17 +1443,14 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
 }
 
 /* Called with iothread lock held.  */
-void qemu_ram_unset_idstr(ram_addr_t addr)
+void qemu_ram_unset_idstr(RAMBlock *block)
 {
-    RAMBlock *block;
-
     /* FIXME: arch_init.c assumes that this is not called throughout
      * migration.  Ignore the problem since hot-unplug during migration
      * does not work anyway.
      */
 
     rcu_read_lock();
-    block = find_ram_block(addr);
     if (block) {
         memset(block->idstr, 0, sizeof(block->idstr));
     }
@@ -1492,10 +1474,8 @@ static int memory_try_enable_merging(void *addr, size_t len)
  * resize callback to update device state and/or add assertions to detect
  * misuse, if necessary.
  */
-int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp)
+int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
 {
-    RAMBlock *block = find_ram_block(base);
-
     assert(block);
 
     newsize = HOST_PAGE_ALIGN(newsize);
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 9e839e5..187df3b 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -65,8 +65,8 @@ MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
 RAMBlock *qemu_ram_block_by_name(const char *name);
 RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
                                    ram_addr_t *ram_addr, ram_addr_t *offset);
-void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev);
-void qemu_ram_unset_idstr(ram_addr_t addr);
+void qemu_ram_set_idstr(RAMBlock *block, const char *name, DeviceState *dev);
+void qemu_ram_unset_idstr(RAMBlock *block);
 const char *qemu_ram_get_idstr(RAMBlock *rb);
 
 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 5adf7a4..5b6e1b8 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -110,7 +110,7 @@ void qemu_set_ram_fd(ram_addr_t addr, int fd);
 void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
 void qemu_ram_free(RAMBlock *block);
 
-int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp);
+int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
 
 #define DIRTY_CLIENTS_ALL     ((1 << DIRTY_MEMORY_NUM) - 1)
 #define DIRTY_CLIENTS_NOCODE  (DIRTY_CLIENTS_ALL & ~(1 << DIRTY_MEMORY_CODE))
diff --git a/memory.c b/memory.c
index f76f85d..239e6da 100644
--- a/memory.c
+++ b/memory.c
@@ -1671,7 +1671,7 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
 {
     assert(mr->ram_block);
 
-    qemu_ram_resize(memory_region_get_ram_addr(mr), newsize, errp);
+    qemu_ram_resize(mr->ram_block, newsize, errp);
 }
 
 static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpace *as)
diff --git a/migration/ram.c b/migration/ram.c
index 3f05738..28b5dd8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2476,7 +2476,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
                     if (length != block->used_length) {
                         Error *local_err = NULL;
 
-                        ret = qemu_ram_resize(block->offset, length,
+                        ret = qemu_ram_resize(block, length,
                                               &local_err);
                         if (local_err) {
                             error_report_err(local_err);
diff --git a/migration/savevm.c b/migration/savevm.c
index 16ba443..3a2e843 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2228,13 +2228,13 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
 {
-    qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK,
+    qemu_ram_set_idstr(mr->ram_block,
                        memory_region_name(mr), dev);
 }
 
 void vmstate_unregister_ram(MemoryRegion *mr, DeviceState *dev)
 {
-    qemu_ram_unset_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
+    qemu_ram_unset_idstr(mr->ram_block);
 }
 
 void vmstate_register_ram_global(MemoryRegion *mr)
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 2/3] exec: adjust rcu_read_lock requirement

[Gonglei]

qemu_ram_unset_idstr() doesn't need rcu lock anymore,
meanwhile make the range of rcu lock in
qemu_ram_set_idstr() as small as possible.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 exec.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/exec.c b/exec.c
index c840bc7..117c9a8 100644
--- a/exec.c
+++ b/exec.c
@@ -1417,8 +1417,6 @@ void qemu_ram_set_idstr(RAMBlock *new_block, const char *name, DeviceState *dev)
 {
     RAMBlock *block;
 
-    rcu_read_lock();
-
     assert(new_block);
     assert(!new_block->idstr[0]);
 
@@ -1431,6 +1429,7 @@ void qemu_ram_set_idstr(RAMBlock *new_block, const char *name, DeviceState *dev)
     }
     pstrcat(new_block->idstr, sizeof(new_block->idstr), name);
 
+    rcu_read_lock();
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
         if (block != new_block &&
             !strcmp(block->idstr, new_block->idstr)) {
@@ -1449,12 +1448,9 @@ void qemu_ram_unset_idstr(RAMBlock *block)
      * migration.  Ignore the problem since hot-unplug during migration
      * does not work anyway.
      */
-
-    rcu_read_lock();
     if (block) {
         memset(block->idstr, 0, sizeof(block->idstr));
     }
-    rcu_read_unlock();
 }
 
 static int memory_try_enable_merging(void *addr, size_t len)
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 3/3] memory: drop some wrappers that waste cpu cycle

[Gonglei]

For better performance, we can use RAMblock
directly stored in memory_region at present.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 exec.c                  | 33 ++-------------------------------
 hw/misc/ivshmem.c       |  8 +++++---
 hw/virtio/vhost-user.c  | 13 ++++++++-----
 include/exec/ram_addr.h |  4 +---
 memory.c                |  2 +-
 5 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/exec.c b/exec.c
index 117c9a8..f8de928 100644
--- a/exec.c
+++ b/exec.c
@@ -1812,38 +1812,9 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
 }
 #endif /* !_WIN32 */
 
-int qemu_get_ram_fd(ram_addr_t addr)
+void *qemu_get_ram_block_host_ptr(RAMBlock *ram_block)
 {
-    RAMBlock *block;
-    int fd;
-
-    rcu_read_lock();
-    block = qemu_get_ram_block(addr);
-    fd = block->fd;
-    rcu_read_unlock();
-    return fd;
-}
-
-void qemu_set_ram_fd(ram_addr_t addr, int fd)
-{
-    RAMBlock *block;
-
-    rcu_read_lock();
-    block = qemu_get_ram_block(addr);
-    block->fd = fd;
-    rcu_read_unlock();
-}
-
-void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
-{
-    RAMBlock *block;
-    void *ptr;
-
-    rcu_read_lock();
-    block = qemu_get_ram_block(addr);
-    ptr = ramblock_ptr(block, 0);
-    rcu_read_unlock();
-    return ptr;
+    return ramblock_ptr(ram_block, 0);
 }
 
 /* Return a host pointer to ram allocated with qemu_ram_alloc.
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index e40f23b..1e930fa 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -533,7 +533,9 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
     }
     memory_region_init_ram_ptr(&s->server_bar2, OBJECT(s),
                                "ivshmem.bar2", size, ptr);
-    qemu_set_ram_fd(memory_region_get_ram_addr(&s->server_bar2), fd);
+    assert(s->server_bar2.ram_block);
+    s->server_bar2.ram_block->fd = fd;
+
     s->ivshmem_bar2 = &s->server_bar2;
 }
 
@@ -939,8 +941,8 @@ static void ivshmem_exit(PCIDevice *dev)
                 error_report("Failed to munmap shared memory %s",
                              strerror(errno));
             }
-
-            fd = qemu_get_ram_fd(memory_region_get_ram_addr(s->ivshmem_bar2));
+            assert(s->ivshmem_bar2->ram_block);
+            fd = s->ivshmem_bar2->ram_block->fd;
             close(fd);
         }
 
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5914e85..5082e04 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -248,17 +248,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
     for (i = 0; i < dev->mem->nregions; ++i) {
         struct vhost_memory_region *reg = dev->mem->regions + i;
         ram_addr_t ram_addr;
+        MemoryRegion *mr;
 
         assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-        qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
+        mr = qemu_ram_addr_from_host((void *)(uintptr_t)reg->userspace_addr,
                                 &ram_addr);
-        fd = qemu_get_ram_fd(ram_addr);
+        fd = mr->ram_block->fd;
         if (fd > 0) {
             msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
             msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
             msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
             msg.payload.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
-                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
+              (uintptr_t) qemu_get_ram_block_host_ptr(mr->ram_block);
             assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
             fds[fd_num++] = fd;
         }
@@ -622,11 +623,13 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
 
     mr = qemu_ram_addr_from_host((void *)(uintptr_t)start1, &ram_addr);
     assert(mr);
-    mfd = qemu_get_ram_fd(ram_addr);
+    assert(mr->ram_block);
+    mfd = mr->ram_block->fd;
 
     mr = qemu_ram_addr_from_host((void *)(uintptr_t)start2, &ram_addr);
     assert(mr);
-    rfd = qemu_get_ram_fd(ram_addr);
+    assert(mr->ram_block);
+    rfd = mr->ram_block->fd;
 
     return mfd == rfd;
 }
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 5b6e1b8..9a01d4a 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -105,9 +105,7 @@ RAMBlock *qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t max_size,
                                                     uint64_t length,
                                                     void *host),
                                     MemoryRegion *mr, Error **errp);
-int qemu_get_ram_fd(ram_addr_t addr);
-void qemu_set_ram_fd(ram_addr_t addr, int fd);
-void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
+void *qemu_get_ram_block_host_ptr(RAMBlock *ram_block);
 void qemu_ram_free(RAMBlock *block);
 
 int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
diff --git a/memory.c b/memory.c
index 239e6da..9cff7e2 100644
--- a/memory.c
+++ b/memory.c
@@ -1641,7 +1641,7 @@ int memory_region_get_fd(MemoryRegion *mr)
 
     assert(mr->ram_block);
 
-    return qemu_get_ram_fd(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
+    return mr->ram_block->fd;
 }
 
 void *memory_region_get_ram_ptr(MemoryRegion *mr)
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 3/3] memory: drop some wrappers that waste cpu cycle

[Fam Zheng]

On Thu, 05/12 18:07, Gonglei wrote:
> For better performance, we can use RAMblock
> directly stored in memory_region at present.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  exec.c                  | 33 ++-------------------------------
>  hw/misc/ivshmem.c       |  8 +++++---
>  hw/virtio/vhost-user.c  | 13 ++++++++-----
>  include/exec/ram_addr.h |  4 +---
>  memory.c                |  2 +-
>  5 files changed, 17 insertions(+), 43 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 117c9a8..f8de928 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1812,38 +1812,9 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
>  }
>  #endif /* !_WIN32 */
>  
> -int qemu_get_ram_fd(ram_addr_t addr)
> +void *qemu_get_ram_block_host_ptr(RAMBlock *ram_block)
>  {
> -    RAMBlock *block;
> -    int fd;
> -
> -    rcu_read_lock();
> -    block = qemu_get_ram_block(addr);
> -    fd = block->fd;
> -    rcu_read_unlock();
> -    return fd;
> -}
> -
> -void qemu_set_ram_fd(ram_addr_t addr, int fd)
> -{
> -    RAMBlock *block;
> -
> -    rcu_read_lock();
> -    block = qemu_get_ram_block(addr);
> -    block->fd = fd;
> -    rcu_read_unlock();
> -}
> -
> -void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> -{
> -    RAMBlock *block;
> -    void *ptr;
> -
> -    rcu_read_lock();
> -    block = qemu_get_ram_block(addr);
> -    ptr = ramblock_ptr(block, 0);
> -    rcu_read_unlock();
> -    return ptr;
> +    return ramblock_ptr(ram_block, 0);
>  }
>  
>  /* Return a host pointer to ram allocated with qemu_ram_alloc.
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index e40f23b..1e930fa 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -533,7 +533,9 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
>      }
>      memory_region_init_ram_ptr(&s->server_bar2, OBJECT(s),
>                                 "ivshmem.bar2", size, ptr);
> -    qemu_set_ram_fd(memory_region_get_ram_addr(&s->server_bar2), fd);
> +    assert(s->server_bar2.ram_block);
> +    s->server_bar2.ram_block->fd = fd;
> +
>      s->ivshmem_bar2 = &s->server_bar2;
>  }
>  
> @@ -939,8 +941,8 @@ static void ivshmem_exit(PCIDevice *dev)
>                  error_report("Failed to munmap shared memory %s",
>                               strerror(errno));
>              }
> -
> -            fd = qemu_get_ram_fd(memory_region_get_ram_addr(s->ivshmem_bar2));

Maybe this is okay but personally I think it is cleaner to add a
qemu_{set,get}_ramblock_fd pair.

Fam