From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34569) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhaQY-0008IJ-Oe for qemu-devel@nongnu.org; Wed, 29 May 2013 03:04:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UhaQV-0003Pn-AG for qemu-devel@nongnu.org; Wed, 29 May 2013 03:04:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21277) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhaQV-0003PX-2w for qemu-devel@nongnu.org; Wed, 29 May 2013 03:04:03 -0400 Message-ID: <51A5A853.4040900@redhat.com> Date: Wed, 29 May 2013 09:03:47 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1369793469-30883-1-git-send-email-qemulist@gmail.com> <1369793469-30883-5-git-send-email-qemulist@gmail.com> In-Reply-To: <1369793469-30883-5-git-send-email-qemulist@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 4/6] mem: concenter the root of each AddressSpaceDispatch List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Ping Fan Cc: qemu-devel@nongnu.org, Anthony Liguori Il 29/05/2013 04:11, Liu Ping Fan ha scritto: > From: Liu Ping Fan > > All of AddressSpaceDispatch's roots are part of dispatch context, > along with cur_map_nodes, cur_phys_sections, and we should walk > through AddressSpaceDispatchs in the same dispatch context, ie > the same memory topology. Concenter the roots, so we can switch > to next more easily. > > Signed-off-by: Liu Ping Fan > --- > exec.c | 48 ++++++++++++++++++++++++++++++++++++--- > include/exec/memory-internal.h | 2 +- > 2 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/exec.c b/exec.c > index eb69a98..315960d 100644 > --- a/exec.c > +++ b/exec.c > @@ -49,6 +49,7 @@ > #include "translate-all.h" > > #include "exec/memory-internal.h" > +#include "qemu/bitops.h" > > //#define DEBUG_SUBPAGE > > @@ -95,6 +96,7 @@ typedef struct AllocInfo { > /* Only used for release purpose */ > Node *map; > MemoryRegionSection *sections; > + PhysPageEntry *roots; > } AllocInfo; I wouldn't put it here. I would put it in AddressSpaceDispatch (as next_phys_map/next_sections/next_nodes: initialize next_sections/next_nodes in the "begin" hook, switch under seqlock in the "commit" hook). This requires refcounting AllocInfo, but it removes the need for the ->idx indirection and the id management. Paolo > /* For performance, fetch page map related pointers directly, other than > @@ -102,10 +104,12 @@ typedef struct AllocInfo { > */ > static MemoryRegionSection *cur_phys_sections; > static Node *cur_map_nodes; > +static PhysPageEntry *cur_roots; > static AllocInfo *cur_alloc_info; > > static MemoryRegionSection *next_phys_sections; > static Node *next_map_nodes; > +static PhysPageEntry *next_roots; > static AllocInfo *next_alloc_info; > > #define PHYS_MAP_NODE_NIL (((uint16_t)~0) >> 1) > @@ -191,12 +195,13 @@ static void phys_page_set(AddressSpaceDispatch *d, > /* Wildly overreserve - it doesn't matter much. */ > phys_map_node_reserve(3 * P_L2_LEVELS); > > - phys_page_set_level(&d->phys_map, &index, &nb, leaf, P_L2_LEVELS - 1); > + phys_page_set_level(&next_roots[d->idx], &index, &nb, leaf, > + P_L2_LEVELS - 1); > } > > static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index, bool cur) > { > - PhysPageEntry lp = d->phys_map; > + PhysPageEntry lp; > PhysPageEntry *p; > int i; > Node *map_nodes; > @@ -205,9 +210,11 @@ static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index > if (likely(cur)) { > map_nodes = cur_map_nodes; > sections = cur_phys_sections; > + lp = cur_roots[d->idx]; > } else { > map_nodes = next_map_nodes; > sections = next_phys_sections; > + lp = next_roots[d->idx]; > } > for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) { > if (lp.ptr == PHYS_MAP_NODE_NIL) { > @@ -1729,7 +1736,8 @@ static void mem_begin(MemoryListener *listener) > { > AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, listener); > > - d->phys_map.ptr = PHYS_MAP_NODE_NIL; > + next_roots[d->idx] = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, > + is_leaf = 0 }; > } > > static void core_begin(MemoryListener *listener) > @@ -1737,6 +1745,7 @@ static void core_begin(MemoryListener *listener) > uint16_t n; > > next_alloc_info = g_malloc0(sizeof(AllocInfo)); > + next_roots = g_malloc0(2048*sizeof(PhysPageEntry)); > n = dummy_section(&io_mem_unassigned); > assert(phys_section_unassigned == n); > n = dummy_section(&io_mem_notdirty); > @@ -1752,6 +1761,7 @@ static void release_dispatch_map(AllocInfo *info) > phys_sections_clear(info->phys_sections_nb, info->sections); > g_free(info->map); > g_free(info->sections); > + g_free(info->roots); > g_free(info); > } > > @@ -1765,6 +1775,7 @@ static void core_commit(MemoryListener *listener) > AllocInfo *info = cur_alloc_info; > info->map = cur_map_nodes; > info->sections = cur_phys_sections; > + info->roots = cur_roots; > > /* Fix me, in future, will be protected by wr seqlock when in parellel > * with reader > @@ -1772,6 +1783,7 @@ static void core_commit(MemoryListener *listener) > cur_map_nodes = next_map_nodes; > cur_phys_sections = next_phys_sections; > cur_alloc_info = next_alloc_info; > + cur_roots = next_roots; > > /* since each CPU stores ram addresses in its TLB cache, we must > reset the modified entries */ > @@ -1826,11 +1838,36 @@ static MemoryListener io_memory_listener = { > .priority = 0, > }; > > +static MemoryListener tcg_memory_listener = { > + .commit = tcg_commit, > +}; Rebase artifact? > +#define MAX_IDX (1<<15) > +static unsigned long *address_space_id_map; > +static QemuMutex id_lock; > + > +static void address_space_release_id(int16_t idx) > +{ > + qemu_mutex_lock(&id_lock); > + clear_bit(idx, address_space_id_map); > + qemu_mutex_unlock(&id_lock); > +} > + > +static int16_t address_space_alloc_id() > +{ > + unsigned long idx; > + > + qemu_mutex_lock(&id_lock); > + idx = find_first_zero_bit(address_space_id_map, MAX_IDX/BITS_PER_LONG); > + set_bit(idx, address_space_id_map); > + qemu_mutex_unlock(&id_lock); > +} > + > void address_space_init_dispatch(AddressSpace *as) > { > AddressSpaceDispatch *d = g_new(AddressSpaceDispatch, 1); > > - d->phys_map = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .is_leaf = 0 }; > + d->idx = address_space_alloc_id(); > d->listener = (MemoryListener) { > .begin = mem_begin, > .region_add = mem_add, > @@ -1845,6 +1882,7 @@ void address_space_destroy_dispatch(AddressSpace *as) > { > AddressSpaceDispatch *d = as->dispatch; > > + address_space_release_id(d->idx); > memory_listener_unregister(&d->listener); > g_free(d); > as->dispatch = NULL; > @@ -1852,6 +1890,8 @@ void address_space_destroy_dispatch(AddressSpace *as) > > static void memory_map_init(void) > { > + qemu_mutex_init(&id_lock); > + address_space_id_map = g_malloc0(MAX_IDX/BITS_PER_BYTE); > system_memory = g_malloc(sizeof(*system_memory)); > memory_region_init(system_memory, "system", INT64_MAX); > address_space_init(&address_space_memory, system_memory, "memory"); > diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h > index 799c02a..54a3635 100644 > --- a/include/exec/memory-internal.h > +++ b/include/exec/memory-internal.h > @@ -36,7 +36,7 @@ struct AddressSpaceDispatch { > /* This is a multi-level map on the physical address space. > * The bottom level has pointers to MemoryRegionSections. > */ > - PhysPageEntry phys_map; > + int16_t idx; > MemoryListener listener; > }; > >