Re: [Qemu-devel] [RFC v1 2/3] ramlist: apply fine grain lock for ram_list

[liu ping fan]

On Sat, Nov 10, 2012 at 9:54 AM, Paolo Bonzini <address@hidden> wrote:
> Il 09/11/2012 04:14, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <address@hidden>
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  cpu-all.h |    1 +
>>  exec.c    |   46 +++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 40 insertions(+), 7 deletions(-)
>
> The problem here is that the ram_list is a pretty critical bit for TCG.
>
This patch does not touch the MRU, so you mean the expense of lock? If
it is, I think, we can define empty lock ops for TCG, and later, when
we adopt urcu, we can come back to fix the TCG problem.
> The migration thread series has patches that split the list in two: a
> MRU-accessed list that uses the BQL, and another that uses a separate lock.
>
I read the thread, but I think we can not protect RAMBlock w/o a
unified lock.  When ram device's refcnt->0, and call
qemu_ram_free_from_ptr(), it can be with/without QBL.  So we need to
sync the two lists: ->blocks and ->blocks_mru on the same lock,
otherwise, a destroyed RAMBlock will be exposed.

Thanks and regards,
Pingfan
> address_space_map could use the latter list.  In order to improve
> performance further, we could sort the list from the biggest to the
> smallest region, like KVM does in the kernel.
>
> Paolo
>
>> diff --git a/cpu-all.h b/cpu-all.h
>> index 6aa7e58..d3ead99 100644
>> --- a/cpu-all.h
>> +++ b/cpu-all.h
>> @@ -498,6 +498,7 @@ typedef struct RAMBlock {
>>  } RAMBlock;
>>
>>  typedef struct RAMList {
>> +    QemuMutex lock;
>>      uint8_t *phys_dirty;
>>      QLIST_HEAD(, RAMBlock) blocks;
>>  } RAMList;
>> diff --git a/exec.c b/exec.c
>> index fe84718..e5f1c0f 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2444,6 +2444,7 @@ static ram_addr_t find_ram_offset(ram_addr_t size)
>>      if (QLIST_EMPTY(&ram_list.blocks))
>>          return 0;
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          ram_addr_t end, next = RAM_ADDR_MAX;
>>
>> @@ -2459,6 +2460,7 @@ static ram_addr_t find_ram_offset(ram_addr_t size)
>>              mingap = next - end;
>>          }
>>      }
>> +    qemu_mutex_unlock(&ram_list.lock);
>>
>>      if (offset == RAM_ADDR_MAX) {
>>          fprintf(stderr, "Failed to find gap of requested size: %" PRIu64 
>> "\n",
>> @@ -2474,8 +2476,10 @@ ram_addr_t last_ram_offset(void)
>>      RAMBlock *block;
>>      ram_addr_t last = 0;
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next)
>>          last = MAX(last, block->offset + block->length);
>> +    qemu_mutex_unlock(&ram_list.lock);
>>
>>      return last;
>>  }
>> @@ -2503,6 +2507,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char 
>> *name, DeviceState *dev)
>>      RAMBlock *new_block, *block;
>>
>>      new_block = NULL;
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          if (block->offset == addr) {
>>              new_block = block;
>> @@ -2528,6 +2533,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char 
>> *name, DeviceState *dev)
>>              abort();
>>          }
>>      }
>> +    qemu_mutex_unlock(&ram_list.lock);
>>  }
>>
>>  static int memory_try_enable_merging(void *addr, size_t len)
>> @@ -2582,12 +2588,6 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, 
>> void *host,
>>      }
>>      new_block->length = size;
>>
>> -    QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
>> -
>> -    ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
>> -                                       last_ram_offset() >> 
>> TARGET_PAGE_BITS);
>> -    memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
>> -           0, size >> TARGET_PAGE_BITS);
>>      cpu_physical_memory_set_dirty_range(new_block->offset, size, 0xff);
>>
>>      qemu_ram_setup_dump(new_block->host, size);
>> @@ -2596,6 +2596,15 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, 
>> void *host,
>>      if (kvm_enabled())
>>          kvm_setup_guest_memory(new_block->host, size);
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>> +    QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
>> +
>> +    ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
>> +                                       last_ram_offset() >> 
>> TARGET_PAGE_BITS);
>> +    memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
>> +           0, size >> TARGET_PAGE_BITS);
>> +    qemu_mutex_unlock(&ram_list.lock);
>> +
>>      return new_block->offset;
>>  }
>>
>> @@ -2608,19 +2617,23 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
>>  {
>>      RAMBlock *block;
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          if (addr == block->offset) {
>>              QLIST_REMOVE(block, next);
>>              g_free(block);
>> +            qemu_mutex_unlock(&ram_list.lock);
>>              return;
>>          }
>>      }
>> +    qemu_mutex_unlock(&ram_list.lock);
>>  }
>>
>>  void qemu_ram_free(ram_addr_t addr)
>>  {
>>      RAMBlock *block;
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          if (addr == block->offset) {
>>              QLIST_REMOVE(block, next);
>> @@ -2649,10 +2662,11 @@ void qemu_ram_free(ram_addr_t addr)
>>  #endif
>>              }
>>              g_free(block);
>> +            qemu_mutex_unlock(&ram_list.lock);
>>              return;
>>          }
>>      }
>> -
>> +    qemu_mutex_unlock(&ram_list.lock);
>>  }
>>
>>  #ifndef _WIN32
>> @@ -2663,6 +2677,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
>>      int flags;
>>      void *area, *vaddr;
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          offset = addr - block->offset;
>>          if (offset < block->length) {
>> @@ -2711,9 +2726,11 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t 
>> length)
>>                  memory_try_enable_merging(vaddr, length);
>>                  qemu_ram_setup_dump(vaddr, length);
>>              }
>> +            qemu_mutex_unlock(&ram_list.lock);
>>              return;
>>          }
>>      }
>> +    qemu_mutex_unlock(&ram_list.lock);
>>  }
>>  #endif /* !_WIN32 */
>>
>> @@ -2729,6 +2746,7 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>>  {
>>      RAMBlock *block;
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          if (addr - block->offset < block->length) {
>>              /* Move this entry to to start of the list.  */
>> @@ -2742,15 +2760,18 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>>                   * In that case just map until the end of the page.
>>                   */
>>                  if (block->offset == 0) {
>> +                    qemu_mutex_unlock(&ram_list.lock);
>>                      return xen_map_cache(addr, 0, 0);
>>                  } else if (block->host == NULL) {
>>                      block->host =
>>                          xen_map_cache(block->offset, block->length, 1);
>>                  }
>>              }
>> +            qemu_mutex_unlock(&ram_list.lock);
>>              return block->host + (addr - block->offset);
>>          }
>>      }
>> +    qemu_mutex_unlock(&ram_list.lock);
>>
>>      fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
>>      abort();
>> @@ -2765,6 +2786,7 @@ void *qemu_safe_ram_ptr(ram_addr_t addr)
>>  {
>>      RAMBlock *block;
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          if (addr - block->offset < block->length) {
>>              if (xen_enabled()) {
>> @@ -2773,15 +2795,18 @@ void *qemu_safe_ram_ptr(ram_addr_t addr)
>>                   * In that case just map until the end of the page.
>>                   */
>>                  if (block->offset == 0) {
>> +                    qemu_mutex_unlock(&ram_list.lock);
>>                      return xen_map_cache(addr, 0, 0);
>>                  } else if (block->host == NULL) {
>>                      block->host =
>>                          xen_map_cache(block->offset, block->length, 1);
>>                  }
>>              }
>> +            qemu_mutex_unlock(&ram_list.lock);
>>              return block->host + (addr - block->offset);
>>          }
>>      }
>> +    qemu_mutex_unlock(&ram_list.lock);
>>
>>      fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
>>      abort();
>> @@ -2801,13 +2826,16 @@ void *qemu_ram_ptr_length(ram_addr_t addr, 
>> ram_addr_t *size)
>>      } else {
>>          RAMBlock *block;
>>
>> +        qemu_mutex_lock(&ram_list.lock);
>>          QLIST_FOREACH(block, &ram_list.blocks, next) {
>>              if (addr - block->offset < block->length) {
>>                  if (addr - block->offset + *size > block->length)
>>                      *size = block->length - addr + block->offset;
>> +                qemu_mutex_lock(&ram_list.lock);
>>                  return block->host + (addr - block->offset);
>>              }
>>          }
>> +        qemu_mutex_unlock(&ram_list.lock);
>>
>>          fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
>>          abort();
>> @@ -2829,6 +2857,7 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t 
>> *ram_addr)
>>          return 0;
>>      }
>>
>> +    qemu_mutex_lock(&ram_list.lock);
>>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>>          /* This case append when the block is not mapped. */
>>          if (block->host == NULL) {
>> @@ -2836,9 +2865,11 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t 
>> *ram_addr)
>>          }
>>          if (host - block->host < block->length) {
>>              *ram_addr = block->offset + (host - block->host);
>> +            qemu_mutex_unlock(&ram_list.lock);
>>              return 0;
>>          }
>>      }
>> +    qemu_mutex_unlock(&ram_list.lock);
>>
>>      return -1;
>>  }
>> @@ -3318,6 +3349,7 @@ static void memory_map_init(void)
>>      address_space_io.name = "I/O";
>>
>>      qemu_mutex_init(&bounce.lock);
>> +    qemu_mutex_unlock(&ram_list.lock);
>>
>>      memory_listener_register(&core_memory_listener, &address_space_memory);
>>      memory_listener_register(&io_memory_listener, &address_space_io);
>>
>