| Message ID | 1299790066-768-1-git-send-email-vpalatin@chromium.org |
|---|---|
| State | New |
| Headers | show |
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>
* 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
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); > } > }
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); } }
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(-)