Re: [Qemu-devel] [PATCH V3 06/11] memory: make mmio dispatch able to be out of biglock

[Avi Kivity <avi@redhat.com>]

On 09/11/2012 10:51 AM, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Without biglock, we try to protect the mr by increase refcnt.
> If we can inc refcnt, go backward and resort to biglock.
> 
> Another point is memory radix-tree can be flushed by another
> thread, so we should get the copy of terminal mr to survive
> from such issue.
> 
>  void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                              int len, int is_write)
>  {
> @@ -3413,14 +3479,35 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>      uint8_t *ptr;
>      uint32_t val;
>      target_phys_addr_t page;
> -    MemoryRegionSection *section;
> +    MemoryRegionSection *section, obj_mrs;
> +    int ret;
> +    int need_biglock = 0;
>  
> +    /* Will finally removed after all mr->ops implement ref/unref() */
> +try_big_lock:
> +    if (need_biglock == 1) {
> +        qemu_mutex_lock_iothread();
> +    }
>      while (len > 0) {
>          page = addr & TARGET_PAGE_MASK;
>          l = (page + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> +
> +        qemu_mutex_lock(&mem_map_lock);
>          section = phys_page_find(page >> TARGET_PAGE_BITS);
> +        if (need_biglock == 0) {
> +            obj_mrs = mrs_get_terminal(section, addr);
> +            ret = mrs_ref(&obj_mrs);
> +            if (ret <= 0) {
> +                need_biglock = 1;
> +                qemu_mutex_unlock(&mem_map_lock);
> +                goto try_big_lock;
> +            }
> +            /* rely on local variable */
> +            section = &obj_mrs;
> +        }
> +        qemu_mutex_unlock(&mem_map_lock);
>  

If on the first pass we find that we need the big lock, we may find on
the second pass that we don't need it since the memory map has changed.
 So on the second pass we might actually need to drop the big lock.

Could code it like

  section, need_lock = lookup(...)
  if need_lock:
     lock(big_lock)
     section, need_lock = lookup(...)
     if not need_lock:
        unlock(big_lock)
  dispatch(section)
  if need_lock:
     unlock(big_lock)

It's ugly as hell, so we'll to apologize to readers in a big comment
explaining what's going on.


-- 
error compiling committee.c: too many arguments to function