Re: [Qemu-devel] [PATCH v1 4/6] mem: concenter the root of each AddressSpaceDispatch

[liu ping fan]

On Wed, May 29, 2013 at 3:03 PM, Paolo Bonzini <address@hidden> wrote:
> Il 29/05/2013 04:11, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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).
>
Implement based on separate AddressSpaceDispatch is broken. We should
walk through the ASD chain under the same mem topology.  Take the
following scenario:
(ASD-x is followed by ASD-y)
walk through ASD-x under topology-A
          ----before walk on ASD-y,  mem topology changes, and switch
from A to B
continue to walk ASD-y in B (but it should be A not B)

To elimate such issue, I concenter the
roots/phys_sections/phys_map_nodes, so we can switch from topology-A
to B  atomiclly.

Regards,
Pingfan
> 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;
>>  };
>>
>>
>