| Message ID | 1372444009-11544-26-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2013-06-28 20:26, Paolo Bonzini wrote: > This lets us get a consistent (phys_map, nodes, sections) using > RCU. After this patch, cur_map is not used anymore except for freeing > it at the end of the topology update. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > exec.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/exec.c b/exec.c > index 0d852ee..581f0c4 100644 > --- a/exec.c > +++ b/exec.c > @@ -94,11 +94,15 @@ struct PhysPageEntry { > uint16_t ptr : 15; > }; > > +typedef PhysPageEntry Node[L2_SIZE]; > + > struct AddressSpaceDispatch { > /* This is a multi-level map on the physical address space. > * The bottom level has pointers to MemoryRegionSections. > */ > PhysPageEntry phys_map; > + Node *nodes; > + MemoryRegionSection *sections; Why not sticking the whole current PhysPageMap into here? Wouldn't that also allow to overcome prev_map completely (next patch)? Jan
Il 02/07/2013 16:42, Jan Kiszka ha scritto: >> > +typedef PhysPageEntry Node[L2_SIZE]; >> > + >> > struct AddressSpaceDispatch { >> > /* This is a multi-level map on the physical address space. >> > * The bottom level has pointers to MemoryRegionSections. >> > */ >> > PhysPageEntry phys_map; >> > + Node *nodes; >> > + MemoryRegionSection *sections; > Why not sticking the whole current PhysPageMap into here? Wouldn't that > also allow to overcome prev_map completely (next patch)? (BTW, this is also why patch 21 has two separate arguments to phys_page_find). Sticking a pointer would add one useless pointer chase. Storing it by value makes it unclear who owns the PhysPageMap and doesn't say that AddressSpaceDispatch cannot extend the vectors. In the end, this seemed like a good compromise. Paolo
On 2013-07-02 17:08, Paolo Bonzini wrote: > Il 02/07/2013 16:42, Jan Kiszka ha scritto: >>>> +typedef PhysPageEntry Node[L2_SIZE]; >>>> + >>>> struct AddressSpaceDispatch { >>>> /* This is a multi-level map on the physical address space. >>>> * The bottom level has pointers to MemoryRegionSections. >>>> */ >>>> PhysPageEntry phys_map; >>>> + Node *nodes; >>>> + MemoryRegionSection *sections; >> Why not sticking the whole current PhysPageMap into here? Wouldn't that >> also allow to overcome prev_map completely (next patch)? > > (BTW, this is also why patch 21 has two separate arguments to > phys_page_find). > > Sticking a pointer would add one useless pointer chase. Storing it by > value makes it unclear who owns the PhysPageMap and doesn't say that > AddressSpaceDispatch cannot extend the vectors. In the end, this seemed > like a good compromise. Well, PhysPageMap is still globally owned. And as long as this is not broken up, you are likely right. So let's wait with this for the next refactoring round. Jan
diff --git a/exec.c b/exec.c index 0d852ee..581f0c4 100644 --- a/exec.c +++ b/exec.c @@ -94,11 +94,15 @@ struct PhysPageEntry { uint16_t ptr : 15; }; +typedef PhysPageEntry Node[L2_SIZE]; + struct AddressSpaceDispatch { /* This is a multi-level map on the physical address space. * The bottom level has pointers to MemoryRegionSections. */ PhysPageEntry phys_map; + Node *nodes; + MemoryRegionSection *sections; AddressSpace *as; }; @@ -115,8 +119,6 @@ typedef struct subpage_t { #define PHYS_SECTION_ROM 2 #define PHYS_SECTION_WATCH 3 -typedef PhysPageEntry Node[L2_SIZE]; - typedef struct PhysPageMap { unsigned sections_nb; unsigned sections_nb_alloc; @@ -238,14 +240,15 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpace *as, hwaddr addr, bool resolve_subpage) { + AddressSpaceDispatch *d = as->dispatch; MemoryRegionSection *section; subpage_t *subpage; - section = phys_page_find(as->dispatch->phys_map, addr >> TARGET_PAGE_BITS, - cur_map.nodes, cur_map.sections); + section = phys_page_find(d->phys_map, addr >> TARGET_PAGE_BITS, + d->nodes, d->sections); if (resolve_subpage && section->mr->subpage) { subpage = container_of(section->mr, subpage_t, iomem); - section = &cur_map.sections[subpage->sub_section[SUBPAGE_IDX(addr)]]; + section = &d->sections[subpage->sub_section[SUBPAGE_IDX(addr)]]; } return section; } @@ -744,7 +747,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env, iotlb |= PHYS_SECTION_ROM; } } else { - iotlb = section - cur_map.sections; + iotlb = section - address_space_memory.dispatch->sections; iotlb += xlat; } @@ -1701,7 +1704,7 @@ static uint16_t dummy_section(MemoryRegion *mr) MemoryRegion *iotlb_to_region(hwaddr index) { - return cur_map.sections[index & ~TARGET_PAGE_MASK].mr; + return address_space_memory.dispatch->sections[index & ~TARGET_PAGE_MASK].mr; } static void io_mem_init(void) @@ -1728,11 +1731,14 @@ static void mem_begin(MemoryListener *listener) static void mem_commit(MemoryListener *listener) { AddressSpace *as = container_of(listener, AddressSpace, dispatch_listener); - AddressSpaceDispatch *d = as->dispatch; + AddressSpaceDispatch *cur = as->dispatch; + AddressSpaceDispatch *next = as->next_dispatch; - /* cur_map will soon be switched to next_map, too. */ - as->dispatch = as->next_dispatch; - g_free(d); + next->nodes = next_map.nodes; + next->sections = next_map.sections; + + as->dispatch = next; + g_free(cur); } static void core_begin(MemoryListener *listener)
This lets us get a consistent (phys_map, nodes, sections) using RCU. After this patch, cur_map is not used anymore except for freeing it at the end of the topology update. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- exec.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)