Re: [Qemu-devel] [PATCH v1 2/6] mem: make global dispatch table ready for rcu

[Paolo Bonzini]

Il 29/05/2013 04:11, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <address@hidden>
> 
> Currently, phys_node_map and phys_sections are shared by all
> of the AddressSpaceDispatch. When updating mem topology, all
> AddressSpaceDispatch will rebuild dispatch tables sequentially
> on them. In order to use rcu-style, introducing next_node_map
> and next_phys_sections, so that when rebuilding, the new dispatch
> tables will locate on next_*. After all AddressSpaceDispatch
> finished, we can switch to next_* and drop the previous stuff.
> 
> This patch still work in the scenario that readers and writer
> can not run in parellel (ie, protected by biglock), so no extra
> lock method needed till now, and the rcu style consistent issue
> will be left to the following patches.
> 
> Signed-off-by: Liu Ping Fan <address@hidden>
> ---
>  exec.c |  202 ++++++++++++++++++++++++++++++++++-----------------------------
>  1 files changed, 109 insertions(+), 93 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 6934c2d..e5335f5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -80,16 +80,33 @@ int use_icount;
>  
>  #if !defined(CONFIG_USER_ONLY)
>  
> -static MemoryRegionSection *phys_sections;
> -static unsigned phys_sections_nb, phys_sections_nb_alloc;
>  #define phys_section_unassigned 0
>  #define phys_section_notdirty 1
>  #define phys_section_rom 2
>  #define phys_section_watch 3
>  
> -/* Simple allocator for PhysPageEntry nodes */
> -static PhysPageEntry (*phys_map_nodes)[L2_SIZE];
> -static unsigned phys_map_nodes_nb, phys_map_nodes_nb_alloc;
> +typedef PhysPageEntry Node[L2_SIZE];
> +
> +typedef struct AllocInfo {
> +    unsigned phys_sections_nb;
> +    unsigned phys_sections_nb_alloc;
> +    unsigned phys_map_nodes_nb;
> +    unsigned phys_map_nodes_nb_alloc;
> +    /* Only used for release purpose */
> +    Node *map;

Node *nodes;

> +    MemoryRegionSection *sections;
> +} AllocInfo;
> +
> +/* For performance, fetch page map related pointers directly, other than
> + * hiding them inside AllocInfo
> + */
> +static MemoryRegionSection *cur_phys_sections;
> +static Node *cur_map_nodes;
> +static AllocInfo *cur_alloc_info;
> +
> +static MemoryRegionSection *next_phys_sections;
> +static Node *next_map_nodes;
> +static AllocInfo *next_alloc_info;

No need for the "next" variables, just make it "static AllocInfo *next_map".
>  
>  #define PHYS_MAP_NODE_NIL (((uint16_t)~0) >> 1)
>  
> @@ -104,13 +121,15 @@ static MemoryRegion io_mem_watch;
>  
>  static void phys_map_node_reserve(unsigned nodes)
>  {
> -    if (phys_map_nodes_nb + nodes > phys_map_nodes_nb_alloc) {
> -        typedef PhysPageEntry Node[L2_SIZE];
> -        phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc * 2, 16);
> -        phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc,
> -                                      phys_map_nodes_nb + nodes);
> -        phys_map_nodes = g_renew(Node, phys_map_nodes,
> -                                 phys_map_nodes_nb_alloc);
> +    AllocInfo *info = next_alloc_info;
> +
> +    if (info->phys_map_nodes_nb + nodes > info->phys_map_nodes_nb_alloc) {
> +        info->phys_map_nodes_nb_alloc = MAX(info->phys_map_nodes_nb_alloc * 
> 2,
> +                                            16);
> +        info->phys_map_nodes_nb_alloc = MAX(info->phys_map_nodes_nb_alloc,
> +                                      info->phys_map_nodes_nb + nodes);
> +        next_map_nodes = g_renew(Node, next_map_nodes,
> +                                 info->phys_map_nodes_nb_alloc);
>      }
>  }
>  
> @@ -118,23 +137,18 @@ static uint16_t phys_map_node_alloc(void)
>  {
>      unsigned i;
>      uint16_t ret;
> +    AllocInfo *info = next_alloc_info;
>  
> -    ret = phys_map_nodes_nb++;
> +    ret = info->phys_map_nodes_nb++;
>      assert(ret != PHYS_MAP_NODE_NIL);
> -    assert(ret != phys_map_nodes_nb_alloc);
> +    assert(ret != info->phys_map_nodes_nb_alloc);
>      for (i = 0; i < L2_SIZE; ++i) {
> -        phys_map_nodes[ret][i].is_leaf = 0;
> -        phys_map_nodes[ret][i].ptr = PHYS_MAP_NODE_NIL;
> +        next_map_nodes[ret][i].is_leaf = 0;
> +        next_map_nodes[ret][i].ptr = PHYS_MAP_NODE_NIL;
>      }
>      return ret;
>  }
>  
> -static void phys_map_nodes_reset(void)
> -{
> -    phys_map_nodes_nb = 0;
> -}
> -
> -
>  static void phys_page_set_level(PhysPageEntry *lp, hwaddr *index,
>                                  hwaddr *nb, uint16_t leaf,
>                                  int level)
> @@ -145,7 +159,7 @@ static void phys_page_set_level(PhysPageEntry *lp, hwaddr 
> *index,
>  
>      if (!lp->is_leaf && lp->ptr == PHYS_MAP_NODE_NIL) {
>          lp->ptr = phys_map_node_alloc();
> -        p = phys_map_nodes[lp->ptr];
> +        p = next_map_nodes[lp->ptr];
>          if (level == 0) {
>              for (i = 0; i < L2_SIZE; i++) {
>                  p[i].is_leaf = 1;
> @@ -153,7 +167,7 @@ static void phys_page_set_level(PhysPageEntry *lp, hwaddr 
> *index,
>              }
>          }
>      } else {
> -        p = phys_map_nodes[lp->ptr];
> +        p = next_map_nodes[lp->ptr];
>      }
>      lp = &p[(*index >> (level * L2_BITS)) & (L2_SIZE - 1)];
>  
> @@ -180,20 +194,29 @@ static void phys_page_set(AddressSpaceDispatch *d,
>      phys_page_set_level(&d->phys_map, &index, &nb, leaf, P_L2_LEVELS - 1);
>  }
>  
> -static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr 
> index)
> +static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr 
> index, bool cur)
>  {
>      PhysPageEntry lp = d->phys_map;
>      PhysPageEntry *p;
>      int i;
> +    Node *map_nodes;
> +    MemoryRegionSection *sections;
>  
> +    if (likely(cur)) {
> +        map_nodes = cur_map_nodes;
> +        sections = cur_phys_sections;
> +    } else {
> +        map_nodes = next_map_nodes;
> +        sections = next_phys_sections;
> +    }

Just pass the AllocInfo here.

>      for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
>          if (lp.ptr == PHYS_MAP_NODE_NIL) {
> -            return &phys_sections[phys_section_unassigned];
> +            return &sections[phys_section_unassigned];
>          }
> -        p = phys_map_nodes[lp.ptr];
> +        p = map_nodes[lp.ptr];
>          lp = p[(index >> (i * L2_BITS)) & (L2_SIZE - 1)];
>      }
> -    return &phys_sections[lp.ptr];
> +    return &sections[lp.ptr];
>  }
>  
>  bool memory_region_is_unassigned(MemoryRegion *mr)
> @@ -205,7 +228,7 @@ bool memory_region_is_unassigned(MemoryRegion *mr)
>  static MemoryRegionSection *address_space_lookup_region(AddressSpace *as,
>                                                          hwaddr addr)
>  {
> -    return phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS);
> +    return phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS, true);
>  }
>  
>  MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
> @@ -213,7 +236,7 @@ MemoryRegionSection *address_space_translate(AddressSpace 
> *as, hwaddr addr,
>                                               bool is_write)
>  {
>      IOMMUTLBEntry iotlb;
> -    MemoryRegionSection *section;
> +    MemoryRegionSection *section, *base = cur_phys_sections;
>      hwaddr len = *plen;
>  
>      for (;;) {
> @@ -235,7 +258,7 @@ MemoryRegionSection *address_space_translate(AddressSpace 
> *as, hwaddr addr,
>                  | (addr & iotlb.addr_mask));
>          len = MIN(len, (addr | iotlb.addr_mask) - addr + 1);
>          if (!(iotlb.perm & (1 << is_write))) {
> -            section = &phys_sections[phys_section_unassigned];
> +            section = &base[phys_section_unassigned];
>              break;
>          }
>  
> @@ -677,7 +700,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
>              iotlb |= phys_section_rom;
>          }
>      } else {
> -        iotlb = section - phys_sections;
> +        iotlb = section - cur_phys_sections;
>          iotlb += xlat;
>      }
>  
> @@ -710,67 +733,31 @@ typedef struct subpage_t {
>  static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
>                               uint16_t section);
>  static subpage_t *subpage_init(hwaddr base);
> -static void destroy_page_desc(uint16_t section_index)
> -{
> -    MemoryRegionSection *section = &phys_sections[section_index];
> -    MemoryRegion *mr = section->mr;
> -
> -    if (mr->subpage) {
> -        subpage_t *subpage = container_of(mr, subpage_t, iomem);
> -        memory_region_destroy(&subpage->iomem);
> -        g_free(subpage);
> -    }
> -}
> -
> -static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
> -{
> -    unsigned i;
> -    PhysPageEntry *p;
> -
> -    if (lp->ptr == PHYS_MAP_NODE_NIL) {
> -        return;
> -    }
> -
> -    p = phys_map_nodes[lp->ptr];
> -    for (i = 0; i < L2_SIZE; ++i) {
> -        if (!p[i].is_leaf) {
> -            destroy_l2_mapping(&p[i], level - 1);
> -        } else {
> -            destroy_page_desc(p[i].ptr);
> -        }
> -    }
> -    lp->is_leaf = 0;
> -    lp->ptr = PHYS_MAP_NODE_NIL;
> -}
> -
> -static void destroy_all_mappings(AddressSpaceDispatch *d)
> -{
> -    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
> -    phys_map_nodes_reset();
> -}
>  
>  static uint16_t phys_section_add(MemoryRegionSection *section)
>  {
> +    AllocInfo *info = next_alloc_info;
>      /* The physical section number is ORed with a page-aligned
>       * pointer to produce the iotlb entries.  Thus it should
>       * never overflow into the page-aligned value.
>       */
> -    assert(phys_sections_nb < TARGET_PAGE_SIZE);
> +    assert(info->phys_sections_nb < TARGET_PAGE_SIZE);
>  
> -    if (phys_sections_nb == phys_sections_nb_alloc) {
> -        phys_sections_nb_alloc = MAX(phys_sections_nb_alloc * 2, 16);
> -        phys_sections = g_renew(MemoryRegionSection, phys_sections,
> -                                phys_sections_nb_alloc);
> +    if (info->phys_sections_nb == info->phys_sections_nb_alloc) {
> +        info->phys_sections_nb_alloc = MAX(info->phys_sections_nb_alloc * 2,
> +                                            16);
> +        next_phys_sections = g_renew(MemoryRegionSection, next_phys_sections,
> +                                info->phys_sections_nb_alloc);
>      }
> -    phys_sections[phys_sections_nb] = *section;
> +    next_phys_sections[info->phys_sections_nb] = *section;
>      memory_region_ref(section->mr);
> -    return phys_sections_nb++;
> +    return info->phys_sections_nb++;
>  }
>  
> -static void phys_sections_clear(void)
> +static void phys_sections_clear(unsigned cnt, MemoryRegionSection *mrs)
>  {
> -    while (phys_sections_nb > 0) {
> -        MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
> +    while (cnt > 0) {
> +        MemoryRegionSection *section = &mrs[--cnt];
>          memory_region_unref(section->mr);
>      }
>  }
> @@ -780,7 +767,8 @@ static void register_subpage(AddressSpaceDispatch *d, 
> MemoryRegionSection *secti
>      subpage_t *subpage;
>      hwaddr base = section->offset_within_address_space
>          & TARGET_PAGE_MASK;
> -    MemoryRegionSection *existing = phys_page_find(d, base >> 
> TARGET_PAGE_BITS);
> +    MemoryRegionSection *existing = phys_page_find(d,
> +                                    base >> TARGET_PAGE_BITS, false);
>      MemoryRegionSection subsection = {
>          .offset_within_address_space = base,
>          .size = TARGET_PAGE_SIZE,
> @@ -1572,13 +1560,13 @@ static uint64_t subpage_read(void *opaque, hwaddr 
> addr,
>      unsigned int idx = SUBPAGE_IDX(addr);
>      uint64_t val;
>  
> -    MemoryRegionSection *section;
> +    MemoryRegionSection *section, *base = cur_phys_sections;
>  #if defined(DEBUG_SUBPAGE)
>      printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d\n", 
> __func__,
>             mmio, len, addr, idx);
>  #endif
>  
> -    section = &phys_sections[mmio->sub_section[idx]];
> +    section = &base[mmio->sub_section[idx]];
>      addr += mmio->base;
>      addr -= section->offset_within_address_space;
>      addr += section->offset_within_region;
> @@ -1591,14 +1579,14 @@ static void subpage_write(void *opaque, hwaddr addr,
>  {
>      subpage_t *mmio = opaque;
>      unsigned int idx = SUBPAGE_IDX(addr);
> -    MemoryRegionSection *section;
> +    MemoryRegionSection *section, *base = cur_phys_sections;
>  #if defined(DEBUG_SUBPAGE)
>      printf("%s: subpage %p len %d addr " TARGET_FMT_plx
>             " idx %d value %"PRIx64"\n",
>             __func__, mmio, len, addr, idx, value);
>  #endif
>  
> -    section = &phys_sections[mmio->sub_section[idx]];
> +    section = &base[mmio->sub_section[idx]];
>      addr += mmio->base;
>      addr -= section->offset_within_address_space;
>      addr += section->offset_within_region;
> @@ -1610,14 +1598,14 @@ static bool subpage_accepts(void *opaque, hwaddr addr,
>  {
>      subpage_t *mmio = opaque;
>      unsigned int idx = SUBPAGE_IDX(addr);
> -    MemoryRegionSection *section;
> +    MemoryRegionSection *section, *base = cur_phys_sections;;
>  #if defined(DEBUG_SUBPAGE)
>      printf("%s: subpage %p %c len %d addr " TARGET_FMT_plx
>             " idx %d\n", __func__, mmio,
>             is_write ? 'w' : 'r', len, addr, idx);
>  #endif
>  
> -    section = &phys_sections[mmio->sub_section[idx]];
> +    section = &base[mmio->sub_section[idx]];
>      addr += mmio->base;
>      addr -= section->offset_within_address_space;
>      addr += section->offset_within_region;
> @@ -1676,8 +1664,8 @@ static int subpage_register (subpage_t *mmio, uint32_t 
> start, uint32_t end,
>      printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n", 
> __func__,
>             mmio, start, end, idx, eidx, memory);
>  #endif
> -    if (memory_region_is_ram(phys_sections[section].mr)) {
> -        MemoryRegionSection new_section = phys_sections[section];
> +    if (memory_region_is_ram(next_phys_sections[section].mr)) {
> +        MemoryRegionSection new_section = next_phys_sections[section];
>          new_section.mr = &io_mem_subpage_ram;
>          section = phys_section_add(&new_section);
>      }
> @@ -1721,7 +1709,7 @@ static uint16_t dummy_section(MemoryRegion *mr)
>  
>  MemoryRegion *iotlb_to_region(hwaddr index)
>  {
> -    return phys_sections[index & ~TARGET_PAGE_MASK].mr;
> +    return cur_phys_sections[index & ~TARGET_PAGE_MASK].mr;
>  }
>  
>  static void io_mem_init(void)
> @@ -1741,7 +1729,6 @@ static void mem_begin(MemoryListener *listener)
>  {
>      AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, 
> listener);
>  
> -    destroy_all_mappings(d);
>      d->phys_map.ptr = PHYS_MAP_NODE_NIL;
>  }
>  
> @@ -1749,7 +1736,7 @@ static void core_begin(MemoryListener *listener)
>  {
>      uint16_t n;
>  
> -    phys_sections_clear();
> +    next_alloc_info = g_malloc0(sizeof(AllocInfo));
>      n = dummy_section(&io_mem_unassigned);
>      assert(phys_section_unassigned == n);
>      n = dummy_section(&io_mem_notdirty);
> @@ -1760,6 +1747,35 @@ static void core_begin(MemoryListener *listener)
>      assert(phys_section_watch == n);
>  }
>  
> +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);
> +}
> +
> +/* This listener's commit run after the other AddressSpaceDispatch 
> listeners'.
> + * It means that AddressSpaceDispatch's deleter has finished, so it can be
> + * the place for call_rcu()
> + */
> +static void core_commit(MemoryListener *listener)
> +{
> +    AllocInfo *info = cur_alloc_info;
> +    info->map = cur_map_nodes;
> +    info->sections = cur_phys_sections;
> +
> +    /* Fix me, in future, will be protected by wr seqlock when in parellel
> +     * with reader
> +     */
> +    cur_map_nodes = next_map_nodes;
> +    cur_phys_sections = next_phys_sections;
> +    cur_alloc_info = next_alloc_info;
> +
> +    /* Fix me, will changed to call_rcu */
> +    release_dispatch_map(info);
> +}
> +
>  static void tcg_commit(MemoryListener *listener)
>  {
>      CPUArchState *env;
> @@ -1802,6 +1818,7 @@ static void io_region_del(MemoryListener *listener,
>  
>  static MemoryListener core_memory_listener = {
>      .begin = core_begin,
> +    .commit = core_commit,
>      .log_global_start = core_log_global_start,
>      .log_global_stop = core_log_global_stop,
>      .priority = 1,
> @@ -1837,7 +1854,6 @@ void address_space_destroy_dispatch(AddressSpace *as)
>      AddressSpaceDispatch *d = as->dispatch;
>  
>      memory_listener_unregister(&d->listener);
> -    destroy_l2_mapping(&d->phys_map, P_L2_LEVELS - 1);
>      g_free(d);
>      as->dispatch = NULL;
>  }
>