* [Qemu-devel] [PATCH 1/8] exec: always call qemu_get_ram_ptr within rcu_read_lock
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
@ 2015-12-16 10:59 ` Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 2/8] exec: make qemu_ram_ptr_length more similar to qemu_get_ram_ptr Paolo Bonzini
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 10:59 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
Simplify the code and document the assumption. The only caller
that is not within rcu_read_lock is memory_region_get_ram_ptr.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 22 +++++-----------------
include/exec/memory.h | 9 +++++++--
memory.c | 14 ++++++++++----
3 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/exec.c b/exec.c
index 0bf0a6e..1a5ea1f 100644
--- a/exec.c
+++ b/exec.c
@@ -1813,19 +1813,11 @@ void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
* or address_space_rw instead. For local memory (e.g. video ram) that the
* device owns, use memory_region_get_ram_ptr.
*
- * By the time this function returns, the returned pointer is not protected
- * by RCU anymore. If the caller is not within an RCU critical section and
- * does not hold the iothread lock, it must have other means of protecting the
- * pointer, such as a reference to the region that includes the incoming
- * ram_addr_t.
+ * Called within RCU critical section.
*/
void *qemu_get_ram_ptr(ram_addr_t addr)
{
- RAMBlock *block;
- void *ptr;
-
- rcu_read_lock();
- block = qemu_get_ram_block(addr);
+ RAMBlock *block = qemu_get_ram_block(addr);
if (xen_enabled() && block->host == NULL) {
/* We need to check if the requested address is in the RAM
@@ -1833,17 +1825,12 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
* In that case just map until the end of the page.
*/
if (block->offset == 0) {
- ptr = xen_map_cache(addr, 0, 0);
- goto unlock;
+ return xen_map_cache(addr, 0, 0);
}
block->host = xen_map_cache(block->offset, block->max_length, 1);
}
- ptr = ramblock_ptr(block, addr - block->offset);
-
-unlock:
- rcu_read_unlock();
- return ptr;
+ return ramblock_ptr(block, addr - block->offset);
}
/* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
@@ -1981,6 +1968,7 @@ MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
return block->mr;
}
+/* Called within RCU critical section. */
static void notdirty_mem_write(void *opaque, hwaddr ram_addr,
uint64_t val, unsigned size)
{
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0f07159..9bbd247 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -656,8 +656,13 @@ int memory_region_get_fd(MemoryRegion *mr);
* memory_region_get_ram_ptr: Get a pointer into a RAM memory region.
*
* Returns a host pointer to a RAM memory region (created with
- * memory_region_init_ram() or memory_region_init_ram_ptr()). Use with
- * care.
+ * memory_region_init_ram() or memory_region_init_ram_ptr()).
+ *
+ * Use with care; by the time this function returns, the returned pointer is
+ * not protected by RCU anymore. If the caller is not within an RCU critical
+ * section and does not hold the iothread lock, it must have other means of
+ * protecting the pointer, such as a reference to the region that includes
+ * the incoming ram_addr_t.
*
* @mr: the memory region being queried.
*/
diff --git a/memory.c b/memory.c
index e193658..c0770a6 100644
--- a/memory.c
+++ b/memory.c
@@ -1549,13 +1549,19 @@ int memory_region_get_fd(MemoryRegion *mr)
void *memory_region_get_ram_ptr(MemoryRegion *mr)
{
- if (mr->alias) {
- return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
+ void *ptr;
+ uint64_t offset = 0;
+
+ rcu_read_lock();
+ while (mr->alias) {
+ offset += mr->alias_offset;
+ mr = mr->alias;
}
-
assert(mr->ram_addr != RAM_ADDR_INVALID);
+ ptr = qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
+ rcu_read_unlock();
- return qemu_get_ram_ptr(mr->ram_addr & TARGET_PAGE_MASK);
+ return ptr + offset;
}
void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH 2/8] exec: make qemu_ram_ptr_length more similar to qemu_get_ram_ptr
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 1/8] exec: always call qemu_get_ram_ptr within rcu_read_lock Paolo Bonzini
@ 2015-12-16 10:59 ` Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 3/8] memory: reorder MemoryRegion fields Paolo Bonzini
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 10:59 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
Notably, use qemu_get_ram_block to enjoy the MRU optimization.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 46 +++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/exec.c b/exec.c
index 1a5ea1f..026a9f2 100644
--- a/exec.c
+++ b/exec.c
@@ -1836,36 +1836,33 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
/* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
* but takes a size argument.
*
- * By the time this function returns, the returned pointer is not protected
- * by RCU anymore. If the caller is not within an RCU critical section and
- * does not hold the iothread lock, it must have other means of protecting the
- * pointer, such as a reference to the region that includes the incoming
- * ram_addr_t.
+ * Called within RCU critical section.
*/
static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size)
{
- void *ptr;
+ RAMBlock *block;
+ ram_addr_t offset_inside_block;
if (*size == 0) {
return NULL;
}
- if (xen_enabled()) {
- return xen_map_cache(addr, *size, 1);
- } else {
- RAMBlock *block;
- rcu_read_lock();
- QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
- if (addr - block->offset < block->max_length) {
- if (addr - block->offset + *size > block->max_length)
- *size = block->max_length - addr + block->offset;
- ptr = ramblock_ptr(block, addr - block->offset);
- rcu_read_unlock();
- return ptr;
- }
+
+ block = qemu_get_ram_block(addr);
+ offset_inside_block = addr - block->offset;
+ *size = MIN(*size, block->max_length - offset_inside_block);
+
+ if (xen_enabled() && block->host == NULL) {
+ /* We need to check if the requested address is in the RAM
+ * because we don't want to map the entire memory in QEMU.
+ * In that case just map the requested area.
+ */
+ if (block->offset == 0) {
+ return xen_map_cache(addr, *size, 1);
}
- fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
- abort();
+ block->host = xen_map_cache(block->offset, block->max_length, 1);
}
+
+ return ramblock_ptr(block, offset_inside_block);
}
/*
@@ -2813,6 +2810,7 @@ void *address_space_map(AddressSpace *as,
hwaddr l, xlat, base;
MemoryRegion *mr, *this_mr;
ram_addr_t raddr;
+ void *ptr;
if (len == 0) {
return NULL;
@@ -2864,9 +2862,11 @@ void *address_space_map(AddressSpace *as,
}
memory_region_ref(mr);
- rcu_read_unlock();
*plen = done;
- return qemu_ram_ptr_length(raddr + base, plen);
+ ptr = qemu_ram_ptr_length(raddr + base, plen);
+ rcu_read_unlock();
+
+ return ptr;
}
/* Unmaps a memory region previously mapped by address_space_map().
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH 3/8] memory: reorder MemoryRegion fields
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 1/8] exec: always call qemu_get_ram_ptr within rcu_read_lock Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 2/8] exec: make qemu_ram_ptr_length more similar to qemu_get_ram_ptr Paolo Bonzini
@ 2015-12-16 10:59 ` Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 4/8] memory: avoid unnecessary object_ref/unref Paolo Bonzini
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 10:59 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
Order fields so that all fields accessed during a RAM read/write fit in
the same cache line.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/exec/memory.h | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9bbd247..5b1fd12 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -159,27 +159,32 @@ typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
struct MemoryRegion {
Object parent_obj;
+
/* All fields are private - violators will be prosecuted */
- const MemoryRegionOps *ops;
+
+ /* The following fields should fit in a cache line */
+ bool romd_mode;
+ bool ram;
+ bool subpage;
+ bool readonly; /* For RAM regions */
+ bool rom_device;
+ bool flush_coalesced_mmio;
+ bool global_locking;
+ uint8_t dirty_log_mask;
+ ram_addr_t ram_addr;
const MemoryRegionIOMMUOps *iommu_ops;
+
+ const MemoryRegionOps *ops;
void *opaque;
MemoryRegion *container;
Int128 size;
hwaddr addr;
void (*destructor)(MemoryRegion *mr);
- ram_addr_t ram_addr;
uint64_t align;
- bool subpage;
bool terminates;
- bool romd_mode;
- bool ram;
bool skip_dump;
- bool readonly; /* For RAM regions */
bool enabled;
- bool rom_device;
bool warning_printed; /* For reservations */
- bool flush_coalesced_mmio;
- bool global_locking;
uint8_t vga_logging_count;
MemoryRegion *alias;
hwaddr alias_offset;
@@ -189,7 +194,6 @@ struct MemoryRegion {
QTAILQ_ENTRY(MemoryRegion) subregions_link;
QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
const char *name;
- uint8_t dirty_log_mask;
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
NotifierList iommu_notify;
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH 4/8] memory: avoid unnecessary object_ref/unref
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
` (2 preceding siblings ...)
2015-12-16 10:59 ` [Qemu-devel] [PATCH 3/8] memory: reorder MemoryRegion fields Paolo Bonzini
@ 2015-12-16 10:59 ` Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 5/8] memory: split address_space_read and address_space_write Paolo Bonzini
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 10:59 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
For the common case of DMA into non-hotplugged RAM, it is unnecessary
but expensive to do object_ref/unref. Add back an owner field to
MemoryRegion, so that these memory regions can skip the reference
counting.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/exec/memory.h | 1 +
memory.c | 28 ++++++++++++----------------
2 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5b1fd12..24b7cba 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -172,6 +172,7 @@ struct MemoryRegion {
bool global_locking;
uint8_t dirty_log_mask;
ram_addr_t ram_addr;
+ Object *owner;
const MemoryRegionIOMMUOps *iommu_ops;
const MemoryRegionOps *ops;
diff --git a/memory.c b/memory.c
index c0770a6..1783300 100644
--- a/memory.c
+++ b/memory.c
@@ -908,20 +908,22 @@ void memory_region_init(MemoryRegion *mr,
const char *name,
uint64_t size)
{
- if (!owner) {
- owner = container_get(qdev_get_machine(), "/unattached");
- }
-
object_initialize(mr, sizeof(*mr), TYPE_MEMORY_REGION);
mr->size = int128_make64(size);
if (size == UINT64_MAX) {
mr->size = int128_2_64();
}
mr->name = g_strdup(name);
+ mr->owner = owner;
if (name) {
char *escaped_name = memory_region_escape_name(name);
char *name_array = g_strdup_printf("%s[*]", escaped_name);
+
+ if (!owner) {
+ owner = container_get(qdev_get_machine(), "/unattached");
+ }
+
object_property_add_child(owner, name_array, OBJECT(mr), &error_abort);
object_unref(OBJECT(mr));
g_free(name_array);
@@ -1341,24 +1343,18 @@ void memory_region_ref(MemoryRegion *mr)
* The memory region is a child of its owner. As long as the
* owner doesn't call unparent itself on the memory region,
* ref-ing the owner will also keep the memory region alive.
- * Memory regions without an owner are supposed to never go away,
- * but we still ref/unref them for debugging purposes.
+ * Memory regions without an owner are supposed to never go away;
+ * we do not ref/unref them because it slows down DMA sensibly.
*/
- Object *obj = OBJECT(mr);
- if (obj && obj->parent) {
- object_ref(obj->parent);
- } else {
- object_ref(obj);
+ if (mr && mr->owner) {
+ object_ref(mr->owner);
}
}
void memory_region_unref(MemoryRegion *mr)
{
- Object *obj = OBJECT(mr);
- if (obj && obj->parent) {
- object_unref(obj->parent);
- } else {
- object_unref(obj);
+ if (mr && mr->owner) {
+ object_unref(mr->owner);
}
}
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH 5/8] memory: split address_space_read and address_space_write
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
` (3 preceding siblings ...)
2015-12-16 10:59 ` [Qemu-devel] [PATCH 4/8] memory: avoid unnecessary object_ref/unref Paolo Bonzini
@ 2015-12-16 10:59 ` Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 6/8] memory: extract first iteration of " Paolo Bonzini
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 10:59 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
Rather than dispatching on is_write for every iteration, make
address_space_rw call one of the two functions. The amount of
duplicate logic is pretty small, and memory_access_is_direct can
be tweaked so that it inlines better in the callers.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 208 ++++++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 116 insertions(+), 92 deletions(-)
diff --git a/exec.c b/exec.c
index 026a9f2..44b04ba 100644
--- a/exec.c
+++ b/exec.c
@@ -395,11 +395,10 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
{
- if (memory_region_is_ram(mr)) {
- return !(is_write && mr->readonly);
- }
- if (memory_region_is_romd(mr)) {
- return !is_write;
+ if (is_write) {
+ return memory_region_is_ram(mr) && !mr->readonly;
+ } else {
+ return memory_region_is_ram(mr) || memory_region_is_romd(mr);
}
return false;
@@ -2496,8 +2495,8 @@ static bool prepare_mmio_access(MemoryRegion *mr)
return release_lock;
}
-MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- uint8_t *buf, int len, bool is_write)
+MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
+ const uint8_t *buf, int len)
{
hwaddr l;
uint8_t *ptr;
@@ -2510,87 +2509,47 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
rcu_read_lock();
while (len > 0) {
l = len;
- mr = address_space_translate(as, addr, &addr1, &l, is_write);
+ mr = address_space_translate(as, addr, &addr1, &l, true);
- if (is_write) {
- if (!memory_access_is_direct(mr, is_write)) {
- release_lock |= prepare_mmio_access(mr);
- l = memory_access_size(mr, l, addr1);
- /* XXX: could force current_cpu to NULL to avoid
- potential bugs */
- switch (l) {
- case 8:
- /* 64 bit write access */
- val = ldq_p(buf);
- result |= memory_region_dispatch_write(mr, addr1, val, 8,
- attrs);
- break;
- case 4:
- /* 32 bit write access */
- val = ldl_p(buf);
- result |= memory_region_dispatch_write(mr, addr1, val, 4,
- attrs);
- break;
- case 2:
- /* 16 bit write access */
- val = lduw_p(buf);
- result |= memory_region_dispatch_write(mr, addr1, val, 2,
- attrs);
- break;
- case 1:
- /* 8 bit write access */
- val = ldub_p(buf);
- result |= memory_region_dispatch_write(mr, addr1, val, 1,
- attrs);
- break;
- default:
- abort();
- }
- } else {
- addr1 += memory_region_get_ram_addr(mr);
- /* RAM case */
- ptr = qemu_get_ram_ptr(addr1);
- memcpy(ptr, buf, l);
- invalidate_and_set_dirty(mr, addr1, l);
+ if (!memory_access_is_direct(mr, true)) {
+ release_lock |= prepare_mmio_access(mr);
+ l = memory_access_size(mr, l, addr1);
+ /* XXX: could force current_cpu to NULL to avoid
+ potential bugs */
+ switch (l) {
+ case 8:
+ /* 64 bit write access */
+ val = ldq_p(buf);
+ result |= memory_region_dispatch_write(mr, addr1, val, 8,
+ attrs);
+ break;
+ case 4:
+ /* 32 bit write access */
+ val = ldl_p(buf);
+ result |= memory_region_dispatch_write(mr, addr1, val, 4,
+ attrs);
+ break;
+ case 2:
+ /* 16 bit write access */
+ val = lduw_p(buf);
+ result |= memory_region_dispatch_write(mr, addr1, val, 2,
+ attrs);
+ break;
+ case 1:
+ /* 8 bit write access */
+ val = ldub_p(buf);
+ result |= memory_region_dispatch_write(mr, addr1, val, 1,
+ attrs);
+ break;
+ default:
+ abort();
}
} else {
- if (!memory_access_is_direct(mr, is_write)) {
- /* I/O case */
- release_lock |= prepare_mmio_access(mr);
- l = memory_access_size(mr, l, addr1);
- switch (l) {
- case 8:
- /* 64 bit read access */
- result |= memory_region_dispatch_read(mr, addr1, &val, 8,
- attrs);
- stq_p(buf, val);
- break;
- case 4:
- /* 32 bit read access */
- result |= memory_region_dispatch_read(mr, addr1, &val, 4,
- attrs);
- stl_p(buf, val);
- break;
- case 2:
- /* 16 bit read access */
- result |= memory_region_dispatch_read(mr, addr1, &val, 2,
- attrs);
- stw_p(buf, val);
- break;
- case 1:
- /* 8 bit read access */
- result |= memory_region_dispatch_read(mr, addr1, &val, 1,
- attrs);
- stb_p(buf, val);
- break;
- default:
- abort();
- }
- } else {
- /* RAM case */
- ptr = qemu_get_ram_ptr(mr->ram_addr + addr1);
- memcpy(buf, ptr, l);
- }
+ addr1 += memory_region_get_ram_addr(mr);
+ /* RAM case */
+ ptr = qemu_get_ram_ptr(addr1);
+ memcpy(ptr, buf, l);
+ invalidate_and_set_dirty(mr, addr1, l);
}
if (release_lock) {
@@ -2607,18 +2566,83 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
return result;
}
-MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- const uint8_t *buf, int len)
-{
- return address_space_rw(as, addr, attrs, (uint8_t *)buf, len, true);
-}
-
MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
uint8_t *buf, int len)
{
- return address_space_rw(as, addr, attrs, buf, len, false);
+ hwaddr l;
+ uint8_t *ptr;
+ uint64_t val;
+ hwaddr addr1;
+ MemoryRegion *mr;
+ MemTxResult result = MEMTX_OK;
+ bool release_lock = false;
+
+ rcu_read_lock();
+ while (len > 0) {
+ l = len;
+ mr = address_space_translate(as, addr, &addr1, &l, false);
+
+ if (!memory_access_is_direct(mr, false)) {
+ /* I/O case */
+ release_lock |= prepare_mmio_access(mr);
+ l = memory_access_size(mr, l, addr1);
+ switch (l) {
+ case 8:
+ /* 64 bit read access */
+ result |= memory_region_dispatch_read(mr, addr1, &val, 8,
+ attrs);
+ stq_p(buf, val);
+ break;
+ case 4:
+ /* 32 bit read access */
+ result |= memory_region_dispatch_read(mr, addr1, &val, 4,
+ attrs);
+ stl_p(buf, val);
+ break;
+ case 2:
+ /* 16 bit read access */
+ result |= memory_region_dispatch_read(mr, addr1, &val, 2,
+ attrs);
+ stw_p(buf, val);
+ break;
+ case 1:
+ /* 8 bit read access */
+ result |= memory_region_dispatch_read(mr, addr1, &val, 1,
+ attrs);
+ stb_p(buf, val);
+ break;
+ default:
+ abort();
+ }
+ } else {
+ /* RAM case */
+ ptr = qemu_get_ram_ptr(mr->ram_addr + addr1);
+ memcpy(buf, ptr, l);
+ }
+
+ if (release_lock) {
+ qemu_mutex_unlock_iothread();
+ release_lock = false;
+ }
+
+ len -= l;
+ buf += l;
+ addr += l;
+ }
+ rcu_read_unlock();
+
+ return result;
}
+MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
+ uint8_t *buf, int len, bool is_write)
+{
+ if (is_write) {
+ return address_space_write(as, addr, attrs, (uint8_t *)buf, len);
+ } else {
+ return address_space_read(as, addr, attrs, (uint8_t *)buf, len);
+ }
+}
void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
int len, int is_write)
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH 6/8] memory: extract first iteration of address_space_read and address_space_write
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
` (4 preceding siblings ...)
2015-12-16 10:59 ` [Qemu-devel] [PATCH 5/8] memory: split address_space_read and address_space_write Paolo Bonzini
@ 2015-12-16 10:59 ` Paolo Bonzini
2015-12-16 10:59 ` [Qemu-devel] [PATCH 7/8] memory: inline a few small accessors Paolo Bonzini
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 10:59 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
We want to inline the case where there is only one iteration, because
then the compiler can also inline the memcpy. As a start, extract
everything after the first address_space_translate call.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 87 ++++++++++++++++++++++++++++++++++++++-------------
include/exec/memory.h | 6 ++++
2 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/exec.c b/exec.c
index 44b04ba..12313bd 100644
--- a/exec.c
+++ b/exec.c
@@ -2495,22 +2495,19 @@ static bool prepare_mmio_access(MemoryRegion *mr)
return release_lock;
}
-MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- const uint8_t *buf, int len)
+/* Called within RCU critical section. */
+static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs,
+ const uint8_t *buf,
+ int len, hwaddr addr1,
+ hwaddr l, MemoryRegion *mr)
{
- hwaddr l;
uint8_t *ptr;
uint64_t val;
- hwaddr addr1;
- MemoryRegion *mr;
MemTxResult result = MEMTX_OK;
bool release_lock = false;
- rcu_read_lock();
- while (len > 0) {
- l = len;
- mr = address_space_translate(as, addr, &addr1, &l, true);
-
+ for (;;) {
if (!memory_access_is_direct(mr, true)) {
release_lock |= prepare_mmio_access(mr);
l = memory_access_size(mr, l, addr1);
@@ -2560,28 +2557,50 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
len -= l;
buf += l;
addr += l;
+
+ if (!len) {
+ break;
+ }
+
+ l = len;
+ mr = address_space_translate(as, addr, &addr1, &l, true);
}
- rcu_read_unlock();
return result;
}
-MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- uint8_t *buf, int len)
+MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
+ const uint8_t *buf, int len)
{
hwaddr l;
- uint8_t *ptr;
- uint64_t val;
hwaddr addr1;
MemoryRegion *mr;
MemTxResult result = MEMTX_OK;
+
+ if (len > 0) {
+ rcu_read_lock();
+ l = len;
+ mr = address_space_translate(as, addr, &addr1, &l, true);
+ result = address_space_write_continue(as, addr, attrs, buf, len,
+ addr1, l, mr);
+ rcu_read_unlock();
+ }
+
+ return result;
+}
+
+/* Called within RCU critical section. */
+MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs, uint8_t *buf,
+ int len, hwaddr addr1, hwaddr l,
+ MemoryRegion *mr)
+{
+ uint8_t *ptr;
+ uint64_t val;
+ MemTxResult result = MEMTX_OK;
bool release_lock = false;
- rcu_read_lock();
- while (len > 0) {
- l = len;
- mr = address_space_translate(as, addr, &addr1, &l, false);
-
+ for (;;) {
if (!memory_access_is_direct(mr, false)) {
/* I/O case */
release_lock |= prepare_mmio_access(mr);
@@ -2628,8 +2647,34 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
len -= l;
buf += l;
addr += l;
+
+ if (!len) {
+ break;
+ }
+
+ l = len;
+ mr = address_space_translate(as, addr, &addr1, &l, false);
+ }
+
+ return result;
+}
+
+MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
+ uint8_t *buf, int len)
+{
+ hwaddr l;
+ hwaddr addr1;
+ MemoryRegion *mr;
+ MemTxResult result = MEMTX_OK;
+
+ if (len > 0) {
+ rcu_read_lock();
+ l = len;
+ mr = address_space_translate(as, addr, &addr1, &l, false);
+ result = address_space_read_continue(as, addr, attrs, buf, len,
+ addr1, l, mr);
+ rcu_read_unlock();
}
- rcu_read_unlock();
return result;
}
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 24b7cba..3680d6a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1366,6 +1366,12 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
int is_write, hwaddr access_len);
+/* Internal functions, part of the implementation of address_space_read. */
+MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs, uint8_t *buf,
+ int len, hwaddr addr1, hwaddr l,
+ MemoryRegion *mr);
+
#endif
#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH 7/8] memory: inline a few small accessors
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
` (5 preceding siblings ...)
2015-12-16 10:59 ` [Qemu-devel] [PATCH 6/8] memory: extract first iteration of " Paolo Bonzini
@ 2015-12-16 10:59 ` Paolo Bonzini
2015-12-16 11:00 ` [Qemu-devel] [PATCH 8/8] memory: try to inline constant-length reads Paolo Bonzini
2015-12-16 11:00 ` [Qemu-devel] [PATCH] " Paolo Bonzini
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 10:59 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
These are used in the address_space_* fast paths.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/exec/memory.h | 22 ++++++++++++++++++----
memory.c | 20 --------------------
2 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3680d6a..a0993e5 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -523,7 +523,10 @@ uint64_t memory_region_size(MemoryRegion *mr);
*
* @mr: the memory region being queried
*/
-bool memory_region_is_ram(MemoryRegion *mr);
+static inline bool memory_region_is_ram(MemoryRegion *mr)
+{
+ return mr->ram;
+}
/**
* memory_region_is_skip_dump: check whether a memory region should not be
@@ -563,7 +566,11 @@ static inline bool memory_region_is_romd(MemoryRegion *mr)
*
* @mr: the memory region being queried
*/
-bool memory_region_is_iommu(MemoryRegion *mr);
+static inline bool memory_region_is_iommu(MemoryRegion *mr)
+{
+ return mr->iommu_ops;
+}
+
/**
* memory_region_notify_iommu: notify a change in an IOMMU translation entry.
@@ -645,7 +652,11 @@ uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr);
*
* @mr: the memory region being queried
*/
-bool memory_region_is_rom(MemoryRegion *mr);
+static inline bool memory_region_is_rom(MemoryRegion *mr)
+{
+ return mr->ram && mr->readonly;
+}
+
/**
* memory_region_get_fd: Get a file descriptor backing a RAM memory region.
@@ -970,7 +981,10 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
* DO NOT USE THIS FUNCTION. This is a temporary workaround while the Xen
* code is being reworked.
*/
-ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr);
+static inline ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
+{
+ return mr->ram_addr;
+}
uint64_t memory_region_get_alignment(const MemoryRegion *mr);
/**
diff --git a/memory.c b/memory.c
index 1783300..7937463 100644
--- a/memory.c
+++ b/memory.c
@@ -1375,11 +1375,6 @@ const char *memory_region_name(const MemoryRegion *mr)
return mr->name;
}
-bool memory_region_is_ram(MemoryRegion *mr)
-{
- return mr->ram;
-}
-
bool memory_region_is_skip_dump(MemoryRegion *mr)
{
return mr->skip_dump;
@@ -1399,16 +1394,6 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
return memory_region_get_dirty_log_mask(mr) & (1 << client);
}
-bool memory_region_is_rom(MemoryRegion *mr)
-{
- return mr->ram && mr->readonly;
-}
-
-bool memory_region_is_iommu(MemoryRegion *mr)
-{
- return mr->iommu_ops;
-}
-
void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n)
{
notifier_list_add(&mr->iommu_notify, n);
@@ -1891,11 +1876,6 @@ void memory_region_set_alias_offset(MemoryRegion *mr, hwaddr offset)
memory_region_transaction_commit();
}
-ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
-{
- return mr->ram_addr;
-}
-
uint64_t memory_region_get_alignment(const MemoryRegion *mr)
{
return mr->align;
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH 8/8] memory: try to inline constant-length reads
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
` (6 preceding siblings ...)
2015-12-16 10:59 ` [Qemu-devel] [PATCH 7/8] memory: inline a few small accessors Paolo Bonzini
@ 2015-12-16 11:00 ` Paolo Bonzini
2015-12-16 11:00 ` [Qemu-devel] [PATCH] " Paolo Bonzini
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 11:00 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
memcpy can take a large amount of time for small reads and writes.
Handle the common case of reading s/g descriptors from memory (there
is no corresponding "write" case that is as common, because writes
often use address_space_st* functions) by inlining the relevant
parts of address_space_read into the caller.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 15 ++--------
include/exec/memory.h | 74 +++++++++++++++++++++++++++++++++++++------------
include/exec/ram_addr.h | 1 -
3 files changed, 59 insertions(+), 31 deletions(-)
diff --git a/exec.c b/exec.c
index 12313bd..18f2fab 100644
--- a/exec.c
+++ b/exec.c
@@ -393,17 +393,6 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
return section;
}
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
-{
- if (is_write) {
- return memory_region_is_ram(mr) && !mr->readonly;
- } else {
- return memory_region_is_ram(mr) || memory_region_is_romd(mr);
- }
-
- return false;
-}
-
/* Called from RCU critical section */
MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
hwaddr *xlat, hwaddr *plen,
@@ -2659,8 +2648,8 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
return result;
}
-MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- uint8_t *buf, int len)
+MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs, uint8_t *buf, int len)
{
hwaddr l;
hwaddr addr1;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index a0993e5..a1dfa20 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1234,23 +1234,7 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
MemTxAttrs attrs,
const uint8_t *buf, int len);
-/**
- * address_space_read: read from an address space.
- *
- * Return a MemTxResult indicating whether the operation succeeded
- * or failed (eg unassigned memory, device rejected the transaction,
- * IOMMU fault).
- *
- * @as: #AddressSpace to be accessed
- * @addr: address within that address space
- * @attrs: memory transaction attributes
- * @buf: buffer with the data transferred
- */
-MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- uint8_t *buf, int len);
-
-/**
- * address_space_ld*: load from an address space
+/* address_space_ld*: load from an address space
* address_space_st*: store to an address space
*
* These functions perform a load or store of the byte, word,
@@ -1385,6 +1369,62 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
MemTxAttrs attrs, uint8_t *buf,
int len, hwaddr addr1, hwaddr l,
MemoryRegion *mr);
+MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs, uint8_t *buf, int len);
+void *qemu_get_ram_ptr(ram_addr_t addr);
+
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+{
+ if (is_write) {
+ return memory_region_is_ram(mr) && !mr->readonly;
+ } else {
+ return memory_region_is_ram(mr) || memory_region_is_romd(mr);
+ }
+
+ return false;
+}
+
+/**
+ * address_space_read: read from an address space.
+ *
+ * Return a MemTxResult indicating whether the operation succeeded
+ * or failed (eg unassigned memory, device rejected the transaction,
+ * IOMMU fault).
+ *
+ * @as: #AddressSpace to be accessed
+ * @addr: address within that address space
+ * @attrs: memory transaction attributes
+ * @buf: buffer with the data transferred
+ */
+static inline __attribute__((__always_inline__))
+MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
+ uint8_t *buf, int len)
+{
+ MemTxResult result = MEMTX_OK;
+ hwaddr l, addr1;
+ void *ptr;
+ MemoryRegion *mr;
+
+ if (__builtin_constant_p(len)) {
+ if (len) {
+ rcu_read_lock();
+ l = len;
+ mr = address_space_translate(as, addr, &addr1, &l, false);
+ if (len == l && memory_access_is_direct(mr, false)) {
+ addr1 += memory_region_get_ram_addr(mr);
+ ptr = qemu_get_ram_ptr(addr1);
+ memcpy(buf, ptr, len);
+ } else {
+ result = address_space_read_continue(as, addr, attrs, buf, len,
+ addr1, l, mr);
+ }
+ rcu_read_unlock();
+ }
+ } else {
+ result = address_space_read_full(as, addr, attrs, buf, len);
+ }
+ return result;
+}
#endif
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 7115154..7f88eaa 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -73,7 +73,6 @@ ram_addr_t qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t max_size,
MemoryRegion *mr, Error **errp);
int qemu_get_ram_fd(ram_addr_t addr);
void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
-void *qemu_get_ram_ptr(ram_addr_t addr);
void qemu_ram_free(ram_addr_t addr);
void qemu_ram_free_from_ptr(ram_addr_t addr);
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Qemu-devel] [PATCH] memory: try to inline constant-length reads
2015-12-16 10:59 [Qemu-devel] [PATCH 0/8] Optimize address_space_read/write/map Paolo Bonzini
` (7 preceding siblings ...)
2015-12-16 11:00 ` [Qemu-devel] [PATCH 8/8] memory: try to inline constant-length reads Paolo Bonzini
@ 2015-12-16 11:00 ` Paolo Bonzini
8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2015-12-16 11:00 UTC (permalink / raw)
To: qemu-devel; +Cc: v.maffione
memcpy can take a large amount of time for small reads and writes.
Handle the common case of reading s/g descriptors from memory (there
is no corresponding "write" case that is as common, because writes
often use address_space_st* functions) by inlining the relevant
parts of address_space_read into the caller.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 15 ++--------
include/exec/memory.h | 74 +++++++++++++++++++++++++++++++++++++------------
include/exec/ram_addr.h | 1 -
3 files changed, 59 insertions(+), 31 deletions(-)
diff --git a/exec.c b/exec.c
index 7783012..744e4c7 100644
--- a/exec.c
+++ b/exec.c
@@ -393,17 +393,6 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
return section;
}
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
-{
- if (is_write) {
- return memory_region_is_ram(mr) && !mr->readonly;
- } else {
- return memory_region_is_ram(mr) || memory_region_is_romd(mr);
- }
-
- return false;
-}
-
/* Called from RCU critical section */
MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
hwaddr *xlat, hwaddr *plen,
@@ -2659,8 +2648,8 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
return result;
}
-MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- uint8_t *buf, int len)
+MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs, uint8_t *buf, int len)
{
hwaddr l;
hwaddr addr1;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index a0993e5..01f1004 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1234,23 +1234,7 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
MemTxAttrs attrs,
const uint8_t *buf, int len);
-/**
- * address_space_read: read from an address space.
- *
- * Return a MemTxResult indicating whether the operation succeeded
- * or failed (eg unassigned memory, device rejected the transaction,
- * IOMMU fault).
- *
- * @as: #AddressSpace to be accessed
- * @addr: address within that address space
- * @attrs: memory transaction attributes
- * @buf: buffer with the data transferred
- */
-MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
- uint8_t *buf, int len);
-
-/**
- * address_space_ld*: load from an address space
+/* address_space_ld*: load from an address space
* address_space_st*: store to an address space
*
* These functions perform a load or store of the byte, word,
@@ -1385,6 +1369,62 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
MemTxAttrs attrs, uint8_t *buf,
int len, hwaddr addr1, hwaddr l,
MemoryRegion *mr);
+MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs, uint8_t *buf, int len);
+void *qemu_get_ram_ptr(ram_addr_t addr);
+
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+{
+ if (is_write) {
+ return memory_region_is_ram(mr) && !mr->readonly;
+ } else {
+ return memory_region_is_ram(mr) || memory_region_is_romd(mr);
+ }
+
+ return false;
+}
+
+/**
+ * address_space_read: read from an address space.
+ *
+ * Return a MemTxResult indicating whether the operation succeeded
+ * or failed (eg unassigned memory, device rejected the transaction,
+ * IOMMU fault).
+ *
+ * @as: #AddressSpace to be accessed
+ * @addr: address within that address space
+ * @attrs: memory transaction attributes
+ * @buf: buffer with the data transferred
+ */
+static inline __attribute__((__always_inline__))
+MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
+ uint8_t *buf, int len)
+{
+ MemTxResult result = MEMTX_OK;
+ hwaddr l, addr1;
+ void *ptr;
+ MemoryRegion *mr;
+
+ if (__builtin_constant_p(len)) {
+ if (len) {
+ rcu_read_lock();
+ l = len;
+ mr = address_space_translate(as, addr, &addr1, &l, false);
+ if (len == l && memory_access_is_direct(mr, false)) {
+ addr1 += memory_region_get_ram_addr(mr);
+ ptr = qemu_get_ram_ptr(addr1);
+ memcpy(buf, ptr, len);
+ } else {
+ result = address_space_read_continue(as, addr, attrs, buf, len,
+ addr1, l, mr);
+ }
+ rcu_read_unlock();
+ }
+ } else {
+ result = address_space_read_full(as, addr, attrs, buf, len);
+ }
+ return result;
+}
#endif
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 7115154..7f88eaa 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -73,7 +73,6 @@ ram_addr_t qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t max_size,
MemoryRegion *mr, Error **errp);
int qemu_get_ram_fd(ram_addr_t addr);
void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
-void *qemu_get_ram_ptr(ram_addr_t addr);
void qemu_ram_free(ram_addr_t addr);
void qemu_ram_free_from_ptr(ram_addr_t addr);
--
2.5.0
^ permalink raw reply related [flat|nested] 10+ messages in thread