Re: [Qemu-devel] [PATCH 04/15] memory: MemoryRegion topology must be stable when updating

[Blue Swirl]

On Thu, Aug 9, 2012 at 7:28 AM, liu ping fan <address@hidden> wrote:
> On Thu, Aug 9, 2012 at 3:17 AM, Blue Swirl <address@hidden> wrote:
>> On Wed, Aug 8, 2012 at 6:25 AM, Liu Ping Fan <address@hidden> wrote:
>>> From: Liu Ping Fan <address@hidden>
>>>
>>> Using mem_map_lock to protect among updaters. So we can get the intact
>>> snapshot of mem topology -- FlatView & radix-tree.
>>>
>>> Signed-off-by: Liu Ping Fan <address@hidden>
>>> ---
>>>  exec.c   |    3 +++
>>>  memory.c |   22 ++++++++++++++++++++++
>>>  memory.h |    2 ++
>>>  3 files changed, 27 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 8244d54..0e29ef9 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -210,6 +210,8 @@ static unsigned phys_map_nodes_nb, 
>>> phys_map_nodes_nb_alloc;
>>>     The bottom level has pointers to MemoryRegionSections.  */
>>>  static PhysPageEntry phys_map = { .ptr = PHYS_MAP_NODE_NIL, .is_leaf = 0 };
>>>
>>> +QemuMutex mem_map_lock;
>>> +
>>>  static void io_mem_init(void);
>>>  static void memory_map_init(void);
>>>
>>> @@ -637,6 +639,7 @@ void cpu_exec_init_all(void)
>>>  #if !defined(CONFIG_USER_ONLY)
>>>      memory_map_init();
>>>      io_mem_init();
>>> +    qemu_mutex_init(&mem_map_lock);
>>
>> I'd move this and the mutex to memory.c since there are no other uses.
>> The mutex could be static then.
>>
> But the init entry is in exec.c, not memory.c.

Memory subsystem does not have an init function of its own, this can
be the start of it.

>
> Regards,
> pingfan
>
>>>  #endif
>>>  }
>>>
>>> diff --git a/memory.c b/memory.c
>>> index aab4a31..5986532 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -761,7 +761,9 @@ void memory_region_transaction_commit(void)
>>>      assert(memory_region_transaction_depth);
>>>      --memory_region_transaction_depth;
>>>      if (!memory_region_transaction_depth && memory_region_update_pending) {
>>> +        qemu_mutex_lock(&mem_map_lock);
>>>          memory_region_update_topology(NULL);
>>> +        qemu_mutex_unlock(&mem_map_lock);
>>>      }
>>>  }
>>>
>>> @@ -1069,8 +1071,10 @@ void memory_region_set_log(MemoryRegion *mr, bool 
>>> log, unsigned client)
>>>  {
>>>      uint8_t mask = 1 << client;
>>>
>>> +    qemu_mutex_lock(&mem_map_lock);
>>>      mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask);
>>>      memory_region_update_topology(mr);
>>> +    qemu_mutex_unlock(&mem_map_lock);
>>>  }
>>>
>>>  bool memory_region_get_dirty(MemoryRegion *mr, target_phys_addr_t addr,
>>> @@ -1103,8 +1107,10 @@ void memory_region_sync_dirty_bitmap(MemoryRegion 
>>> *mr)
>>>  void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
>>>  {
>>>      if (mr->readonly != readonly) {
>>> +        qemu_mutex_lock(&mem_map_lock);
>>>          mr->readonly = readonly;
>>>          memory_region_update_topology(mr);
>>> +        qemu_mutex_unlock(&mem_map_lock);
>>>      }
>>>  }
>>>
>>> @@ -1112,7 +1118,9 @@ void 
>>> memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
>>>  {
>>>      if (mr->readable != readable) {
>>>          mr->readable = readable;
>>> +        qemu_mutex_lock(&mem_map_lock);
>>>          memory_region_update_topology(mr);
>>> +        qemu_mutex_unlock(&mem_map_lock);
>>>      }
>>>  }
>>>
>>> @@ -1206,6 +1214,7 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>>>      };
>>>      unsigned i;
>>>
>>> +    qemu_mutex_lock(&mem_map_lock);
>>>      for (i = 0; i < mr->ioeventfd_nb; ++i) {
>>>          if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i])) {
>>>              break;
>>> @@ -1218,6 +1227,7 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>>>              sizeof(*mr->ioeventfds) * (mr->ioeventfd_nb-1 - i));
>>>      mr->ioeventfds[i] = mrfd;
>>>      memory_region_update_topology(mr);
>>> +    qemu_mutex_unlock(&mem_map_lock);
>>>  }
>>>
>>>  void memory_region_del_eventfd(MemoryRegion *mr,
>>> @@ -1236,6 +1246,7 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>>>      };
>>>      unsigned i;
>>>
>>> +    qemu_mutex_lock(&mem_map_lock);
>>>      for (i = 0; i < mr->ioeventfd_nb; ++i) {
>>>          if (memory_region_ioeventfd_equal(mrfd, mr->ioeventfds[i])) {
>>>              break;
>>> @@ -1248,6 +1259,7 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>>>      mr->ioeventfds = g_realloc(mr->ioeventfds,
>>>                                    sizeof(*mr->ioeventfds)*mr->ioeventfd_nb 
>>> + 1);
>>>      memory_region_update_topology(mr);
>>> +    qemu_mutex_unlock(&mem_map_lock);
>>>  }
>>>
>>>  static void memory_region_add_subregion_common(MemoryRegion *mr,
>>> @@ -1259,6 +1271,8 @@ static void 
>>> memory_region_add_subregion_common(MemoryRegion *mr,
>>>      assert(!subregion->parent);
>>>      subregion->parent = mr;
>>>      subregion->addr = offset;
>>> +
>>> +    qemu_mutex_lock(&mem_map_lock);
>>>      QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
>>>          if (subregion->may_overlap || other->may_overlap) {
>>>              continue;
>>> @@ -1289,6 +1303,7 @@ static void 
>>> memory_region_add_subregion_common(MemoryRegion *mr,
>>>      QTAILQ_INSERT_TAIL(&mr->subregions, subregion, subregions_link);
>>>  done:
>>>      memory_region_update_topology(mr);
>>> +    qemu_mutex_unlock(&mem_map_lock);
>>>  }
>>>
>>>
>>> @@ -1316,8 +1331,11 @@ void memory_region_del_subregion(MemoryRegion *mr,
>>>  {
>>>      assert(subregion->parent == mr);
>>>      subregion->parent = NULL;
>>> +
>>> +    qemu_mutex_lock(&mem_map_lock);
>>>      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>>>      memory_region_update_topology(mr);
>>> +    qemu_mutex_unlock(&mem_map_lock);
>>>  }
>>>
>>>  void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
>>> @@ -1325,8 +1343,10 @@ void memory_region_set_enabled(MemoryRegion *mr, 
>>> bool enabled)
>>>      if (enabled == mr->enabled) {
>>>          return;
>>>      }
>>> +    qemu_mutex_lock(&mem_map_lock);
>>>      mr->enabled = enabled;
>>>      memory_region_update_topology(NULL);
>>> +    qemu_mutex_unlock(&mem_map_lock);
>>>  }
>>>
>>>  void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr)
>>> @@ -1361,7 +1381,9 @@ void memory_region_set_alias_offset(MemoryRegion *mr, 
>>> target_phys_addr_t offset)
>>>          return;
>>>      }
>>>
>>> +    qemu_mutex_lock(&mem_map_lock);
>>>      memory_region_update_topology(mr);
>>> +    qemu_mutex_unlock(&mem_map_lock);
>>>  }
>>>
>>>  ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
>>> diff --git a/memory.h b/memory.h
>>> index 740c48e..fe6aefa 100644
>>> --- a/memory.h
>>> +++ b/memory.h
>>> @@ -25,6 +25,7 @@
>>>  #include "iorange.h"
>>>  #include "ioport.h"
>>>  #include "int128.h"
>>> +#include "qemu-thread.h"
>>>
>>>  typedef struct MemoryRegionOps MemoryRegionOps;
>>>  typedef struct MemoryRegion MemoryRegion;
>>> @@ -207,6 +208,7 @@ struct MemoryListener {
>>>      QTAILQ_ENTRY(MemoryListener) link;
>>>  };
>>>
>>> +extern QemuMutex mem_map_lock;
>>>  /**
>>>   * memory_region_init: Initialize a memory region
>>>   *
>>> --
>>> 1.7.4.4
>>>