diff mbox

[1/3] memory: drop find_ram_block()

Message ID 1461123261-15408-2-git-send-email-arei.gonglei@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gonglei (Arei) April 20, 2016, 3:34 a.m. UTC 
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>
---
 exec.c                    | 37 +++++++++----------------------------
 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, 16 insertions(+), 35 deletions(-)

Comments

Fam Zheng April 29, 2016, 7:48 a.m. UTC | #1 
On Wed, 04/20 11:34, Gonglei wrote:
> 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>
> ---
>  exec.c                    | 37 +++++++++----------------------------
>  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, 16 insertions(+), 35 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index c4f9036..101f0f4 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1407,34 +1407,19 @@ 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 *block, const char *name, DeviceState *dev)
>  {
> -    RAMBlock *new_block, *block;
> +    RAMBlock *new_block = block;
> +    RAMBlock *old_block;
>  
>      rcu_read_lock();
> -    new_block = find_ram_block(addr);
> +
>      assert(new_block);
>      assert(!new_block->idstr[0]);
>  
> @@ -1447,8 +1432,9 @@ 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)) {
> +    QLIST_FOREACH_RCU(old_block, &ram_list.blocks, next) {
> +        if (old_block != new_block &&
> +            !strcmp(old_block->idstr, new_block->idstr)) {

Cosmetic point: maybe you don't need to touch these lines if you name the
function parameter to new_block.

Reviewed-by: Fam Zheng <famz@redhat.com>

>              fprintf(stderr, "RAMBlock \"%s\" already registered, abort!\n",
>                      new_block->idstr);
>              abort();
> @@ -1458,17 +1444,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 +1475,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
> 
>
Gonglei (Arei) April 29, 2016, 8:57 a.m. UTC | #2 
Hi,

> >
> > @@ -1447,8 +1432,9 @@ 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))
> {
> > +    QLIST_FOREACH_RCU(old_block, &ram_list.blocks, next) {
> > +        if (old_block != new_block &&
> > +            !strcmp(old_block->idstr, new_block->idstr)) {
> 
> Cosmetic point: maybe you don't need to touch these lines if you name the
> function parameter to new_block.
> 
Yes, we can.

> Reviewed-by: Fam Zheng <famz@redhat.com>
> 
Thanks.


Regards,
-Gonglei
diff mbox

Patch

diff --git a/exec.c b/exec.c
index c4f9036..101f0f4 100644
--- a/exec.c
+++ b/exec.c
@@ -1407,34 +1407,19 @@  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 *block, const char *name, DeviceState *dev)
 {
-    RAMBlock *new_block, *block;
+    RAMBlock *new_block = block;
+    RAMBlock *old_block;
 
     rcu_read_lock();
-    new_block = find_ram_block(addr);
+
     assert(new_block);
     assert(!new_block->idstr[0]);
 
@@ -1447,8 +1432,9 @@  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)) {
+    QLIST_FOREACH_RCU(old_block, &ram_list.blocks, next) {
+        if (old_block != new_block &&
+            !strcmp(old_block->idstr, new_block->idstr)) {
             fprintf(stderr, "RAMBlock \"%s\" already registered, abort!\n",
                     new_block->idstr);
             abort();
@@ -1458,17 +1444,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 +1475,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)