[Qemu-devel] [PATCH] Fix performance regression in qemu_get_ram_ptr

[Qemu-devel] [PATCH] Fix performance regression in qemu_get_ram_ptr

[Vincent Palatin]

When the commit f471a17e9d869df3c6573f7ec02c4725676d6f3a converted the
ram_blocks structure to QLIST, it also removed the conditional check before
switching the current block at the beginning of the list.

In the common use case where ram_blocks has a few blocks with only one
frequently accessed (the main RAM), this has a performance impact as it
performs the useless list operations on each call (which are on a really
hot path).

On my machine emulation (ARM on amd64), this patch reduces the
percentage of CPU time spent in qemu_get_ram_ptr from 6.3% to 2.1% in the
profiling of a full boot.

Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
---
 exec.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index d611100..81f08b7 100644
--- a/exec.c
+++ b/exec.c
@@ -2957,8 +2957,11 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
 
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr - block->offset < block->length) {
-            QLIST_REMOVE(block, next);
-            QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
+            /* Move this entry to to start of the list.  */
+            if (block != QLIST_FIRST(&ram_list.blocks)) {
+                QLIST_REMOVE(block, next);
+                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
+            }
             return block->host + (addr - block->offset);
         }
     }
-- 
1.7.3.1

[Qemu-devel] Re: [PATCH] Fix performance regression in qemu_get_ram_ptr

[Alex Williamson]

On Thu, 2011-03-10 at 15:47 -0500, Vincent Palatin wrote:
> When the commit f471a17e9d869df3c6573f7ec02c4725676d6f3a converted the
> ram_blocks structure to QLIST, it also removed the conditional check before
> switching the current block at the beginning of the list.
> 
> In the common use case where ram_blocks has a few blocks with only one
> frequently accessed (the main RAM), this has a performance impact as it
> performs the useless list operations on each call (which are on a really
> hot path).
> 
> On my machine emulation (ARM on amd64), this patch reduces the
> percentage of CPU time spent in qemu_get_ram_ptr from 6.3% to 2.1% in the
> profiling of a full boot.
> 
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> ---
>  exec.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d611100..81f08b7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2957,8 +2957,11 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>  
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr - block->offset < block->length) {
> -            QLIST_REMOVE(block, next);
> -            QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +            /* Move this entry to to start of the list.  */
> +            if (block != QLIST_FIRST(&ram_list.blocks)) {
> +                QLIST_REMOVE(block, next);
> +                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +            }
>              return block->host + (addr - block->offset);
>          }
>      }

Looks good

Acked-by: Alex Williamson <alex.williamson@redhat.com>

[Qemu-devel] Re: [PATCH] Fix performance regression in qemu_get_ram_ptr

[Chris Wright]

* Vincent Palatin (vpalatin@chromium.org) wrote:
> When the commit f471a17e9d869df3c6573f7ec02c4725676d6f3a converted the
> ram_blocks structure to QLIST, it also removed the conditional check before
> switching the current block at the beginning of the list.

Nice catch.

> In the common use case where ram_blocks has a few blocks with only one
> frequently accessed (the main RAM), this has a performance impact as it
> performs the useless list operations on each call (which are on a really
> hot path).
> 
> On my machine emulation (ARM on amd64), this patch reduces the
> percentage of CPU time spent in qemu_get_ram_ptr from 6.3% to 2.1% in the
> profiling of a full boot.

Hopefully this is back on par with before the QLIST switchover.

> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>

Acked-by: Chris Wright <chrisw@redhat.com>

> ---
>  exec.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d611100..81f08b7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2957,8 +2957,11 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>  
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr - block->offset < block->length) {
> -            QLIST_REMOVE(block, next);
> -            QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +            /* Move this entry to to start of the list.  */
> +            if (block != QLIST_FIRST(&ram_list.blocks)) {
> +                QLIST_REMOVE(block, next);
> +                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +            }

Pretty close to self-documenting code now.  Not sure if it's subtle enough
to warrant change to the comment like:
 
  /* Move block to head of list if it's not there already */

thanks,
-chris

[Qemu-devel] Re: [PATCH] Fix performance regression in qemu_get_ram_ptr

[Anthony Liguori]

On 03/10/2011 02:47 PM, Vincent Palatin wrote:
> When the commit f471a17e9d869df3c6573f7ec02c4725676d6f3a converted the
> ram_blocks structure to QLIST, it also removed the conditional check before
> switching the current block at the beginning of the list.
>
> In the common use case where ram_blocks has a few blocks with only one
> frequently accessed (the main RAM), this has a performance impact as it
> performs the useless list operations on each call (which are on a really
> hot path).
>
> On my machine emulation (ARM on amd64), this patch reduces the
> percentage of CPU time spent in qemu_get_ram_ptr from 6.3% to 2.1% in the
> profiling of a full boot.
>
> Signed-off-by: Vincent Palatin<vpalatin@chromium.org>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   exec.c |    7 +++++--
>   1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index d611100..81f08b7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2957,8 +2957,11 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>
>       QLIST_FOREACH(block,&ram_list.blocks, next) {
>           if (addr - block->offset<  block->length) {
> -            QLIST_REMOVE(block, next);
> -            QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +            /* Move this entry to to start of the list.  */
> +            if (block != QLIST_FIRST(&ram_list.blocks)) {
> +                QLIST_REMOVE(block, next);
> +                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +            }
>               return block->host + (addr - block->offset);
>           }
>       }