[Qemu-devel] [RFC v1 0/3] make address_space_map() safe without biglock's protection

[Qemu-devel] [RFC v1 0/3] make address_space_map() safe without biglock's protection

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

No access google. 
For background, please refer to discussion for " [RFC PATCH v3 05/19] Implement dimm device abstraction "

Liu Ping Fan (3):
  bouce buffer has fine grain lock
  ramlist: apply fine grain lock for ram_list
  make address_space_map safe

 cpu-all.h         |    1 +
 cpu-common.h      |    8 +++-
 cputlb.c          |    4 +-
 dma-helpers.c     |    4 +-
 dma.h             |    5 ++-
 exec.c            |  112 ++++++++++++++++++++++++++++++++++++++++++-----------
 memory.h          |    4 +-
 target-i386/kvm.c |    4 +-
 8 files changed, 110 insertions(+), 32 deletions(-)

-- 
1.7.4.4

[Qemu-devel] [RFC v1 1/3] bouce buffer has fine grain lock

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 exec.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index 73d5242..fe84718 100644
--- a/exec.c
+++ b/exec.c
@@ -3296,6 +3296,15 @@ void address_space_destroy_dispatch(AddressSpace *as)
     as->dispatch = NULL;
 }
 
+typedef struct {
+    QemuMutex lock;
+    void *buffer;
+    target_phys_addr_t addr;
+    target_phys_addr_t len;
+} BounceBuffer;
+
+static BounceBuffer bounce;
+
 static void memory_map_init(void)
 {
     system_memory = g_malloc(sizeof(*system_memory));
@@ -3308,6 +3317,8 @@ static void memory_map_init(void)
     address_space_init(&address_space_io, system_io);
     address_space_io.name = "I/O";
 
+    qemu_mutex_init(&bounce.lock);
+
     memory_listener_register(&core_memory_listener, &address_space_memory);
     memory_listener_register(&io_memory_listener, &address_space_io);
     memory_listener_register(&tcg_memory_listener, &address_space_memory);
@@ -3671,14 +3682,6 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
     }
 }
 
-typedef struct {
-    void *buffer;
-    target_phys_addr_t addr;
-    target_phys_addr_t len;
-} BounceBuffer;
-
-static BounceBuffer bounce;
-
 typedef struct MapClient {
     void *opaque;
     void (*callback)(void *opaque);
@@ -3747,6 +3750,7 @@ void *address_space_map(AddressSpace *as,
         section = &mr_obj;
 
         if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
+            qemu_mutex_lock(&bounce.lock);
             if (todo || bounce.buffer) {
                 break;
             }
@@ -3807,6 +3811,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
     }
     qemu_vfree(bounce.buffer);
     bounce.buffer = NULL;
+    qemu_mutex_unlock(&bounce.lock);
     cpu_notify_map_clients();
 }
 
-- 
1.7.4.4

Re: [Qemu-devel] [RFC v1 1/3] bouce buffer has fine grain lock

[Paolo Bonzini]

Il 09/11/2012 04:14, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  exec.c |   21 +++++++++++++--------
>  1 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 73d5242..fe84718 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3296,6 +3296,15 @@ void address_space_destroy_dispatch(AddressSpace *as)
>      as->dispatch = NULL;
>  }
>  
> +typedef struct {
> +    QemuMutex lock;
> +    void *buffer;
> +    target_phys_addr_t addr;
> +    target_phys_addr_t len;
> +} BounceBuffer;
> +
> +static BounceBuffer bounce;
> +
>  static void memory_map_init(void)
>  {
>      system_memory = g_malloc(sizeof(*system_memory));
> @@ -3308,6 +3317,8 @@ static void memory_map_init(void)
>      address_space_init(&address_space_io, system_io);
>      address_space_io.name = "I/O";
>  
> +    qemu_mutex_init(&bounce.lock);
> +
>      memory_listener_register(&core_memory_listener, &address_space_memory);
>      memory_listener_register(&io_memory_listener, &address_space_io);
>      memory_listener_register(&tcg_memory_listener, &address_space_memory);
> @@ -3671,14 +3682,6 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>      }
>  }
>  
> -typedef struct {
> -    void *buffer;
> -    target_phys_addr_t addr;
> -    target_phys_addr_t len;
> -} BounceBuffer;
> -
> -static BounceBuffer bounce;
> -
>  typedef struct MapClient {
>      void *opaque;
>      void (*callback)(void *opaque);
> @@ -3747,6 +3750,7 @@ void *address_space_map(AddressSpace *as,
>          section = &mr_obj;
>  
>          if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
> +            qemu_mutex_lock(&bounce.lock);
>              if (todo || bounce.buffer) {
>                  break;
>              }

"todo" must be checked before the if.

Also, you do not need to keep the lock after address_space_map exits.
In fact, it can be released as soon as bounce.buffer is written to.
After that point, bounce will not be touched (the lock only serves to
serialize writes to bounce.buffer).  That is,

     if (todo) {
         break;
     }
     qemu_mutex_lock(&bounce.lock);
     if (bounce.buffer) {
         qemu_mutex_unlock(&bounce.lock);
         break;
     }
     bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
     qemu_mutex_unlock(&bounce.lock);


Of course, this must be documented.

Paolo

> @@ -3807,6 +3811,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
>      }
>      qemu_vfree(bounce.buffer);
>      bounce.buffer = NULL;
> +    qemu_mutex_unlock(&bounce.lock);
>      cpu_notify_map_clients();
>  }
>  
> 

Re: [Qemu-devel] [RFC v1 1/3] bouce buffer has fine grain lock

[liu ping fan]

On Sat, Nov 10, 2012 at 9:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/11/2012 04:14, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  exec.c |   21 +++++++++++++--------
>>  1 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 73d5242..fe84718 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3296,6 +3296,15 @@ void address_space_destroy_dispatch(AddressSpace *as)
>>      as->dispatch = NULL;
>>  }
>>
>> +typedef struct {
>> +    QemuMutex lock;
>> +    void *buffer;
>> +    target_phys_addr_t addr;
>> +    target_phys_addr_t len;
>> +} BounceBuffer;
>> +
>> +static BounceBuffer bounce;
>> +
>>  static void memory_map_init(void)
>>  {
>>      system_memory = g_malloc(sizeof(*system_memory));
>> @@ -3308,6 +3317,8 @@ static void memory_map_init(void)
>>      address_space_init(&address_space_io, system_io);
>>      address_space_io.name = "I/O";
>>
>> +    qemu_mutex_init(&bounce.lock);
>> +
>>      memory_listener_register(&core_memory_listener, &address_space_memory);
>>      memory_listener_register(&io_memory_listener, &address_space_io);
>>      memory_listener_register(&tcg_memory_listener, &address_space_memory);
>> @@ -3671,14 +3682,6 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>>      }
>>  }
>>
>> -typedef struct {
>> -    void *buffer;
>> -    target_phys_addr_t addr;
>> -    target_phys_addr_t len;
>> -} BounceBuffer;
>> -
>> -static BounceBuffer bounce;
>> -
>>  typedef struct MapClient {
>>      void *opaque;
>>      void (*callback)(void *opaque);
>> @@ -3747,6 +3750,7 @@ void *address_space_map(AddressSpace *as,
>>          section = &mr_obj;
>>
>>          if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
>> +            qemu_mutex_lock(&bounce.lock);
>>              if (todo || bounce.buffer) {
>>                  break;
>>              }
>
> "todo" must be checked before the if.
>
Applied, thanks.

> Also, you do not need to keep the lock after address_space_map exits.
> In fact, it can be released as soon as bounce.buffer is written to.
> After that point, bounce will not be touched (the lock only serves to
> serialize writes to bounce.buffer).  That is,
>
But w/o the lock, the content in bounce.buffer for threadA, can be
flushed with thread B.
>      if (todo) {
>          break;
>      }
>      qemu_mutex_lock(&bounce.lock);
>      if (bounce.buffer) {
>          qemu_mutex_unlock(&bounce.lock);
>          break;
>      }
>      bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
>      qemu_mutex_unlock(&bounce.lock);
>
As above.

Regards,
Pingfan
>
> Of course, this must be documented.
>
> Paolo
>
>> @@ -3807,6 +3811,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
>>      }
>>      qemu_vfree(bounce.buffer);
>>      bounce.buffer = NULL;
>> +    qemu_mutex_unlock(&bounce.lock);
>>      cpu_notify_map_clients();
>>  }
>>
>>
>

Re: [Qemu-devel] [RFC v1 1/3] bouce buffer has fine grain lock

[Paolo Bonzini]

Il 12/11/2012 07:23, liu ping fan ha scritto:
>> > Also, you do not need to keep the lock after address_space_map exits.
>> > In fact, it can be released as soon as bounce.buffer is written to.
>> > After that point, bounce will not be touched (the lock only serves to
>> > serialize writes to bounce.buffer).  That is,
>> >
> But w/o the lock, the content in bounce.buffer for threadA, can be
> flushed with thread B.

It doesn't matter _which_ thread calls address_space_unmap.  As long as
it's only one, you do not need a lock.  In fact, it is perfectly fine
for thread A to call address_space_map and pass the address to thread B.
 Thread B will later call address_space_unmap.

The important thing is that only one thread will call
address_space_unmap with buffer == bounce.buffer.  So you do not need a
lock to serialize the writes in address_space_unmap.

See thread-pool.c for a similar trick:

    /* Moving state out of THREAD_QUEUED is protected by lock.  After
     * that, only the worker thread can write to it.  Reads and writes
     * of state and ret are ordered with memory barriers.
     */
    enum ThreadState state;
    int ret;

...

        qemu_mutex_lock(&lock);
        ...
        req->state = THREAD_ACTIVE;
        qemu_mutex_unlock(&lock);

        req->ret = req->func(req->arg);
        smp_wmb();
        req->state = THREAD_DONE;

Paolo

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

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpu-all.h |    1 +
 exec.c    |   46 +++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 40 insertions(+), 7 deletions(-)

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);
-- 
1.7.4.4

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

[Paolo Bonzini]

Il 09/11/2012 04:14, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  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.

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.

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);
> 

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 <pbonzini@redhat.com> wrote:
> Il 09/11/2012 04:14, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  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);
>>
>

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

[Paolo Bonzini]

Il 12/11/2012 07:22, liu ping fan ha scritto:
> On Sat, Nov 10, 2012 at 9:54 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 09/11/2012 04:14, Liu Ping Fan ha scritto:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  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?

Yes.

One alternative is to remove the MRU, but add a 1-item cache to speed up
the common case.  Then the case where you use the cache can be placed
(later) in an RCU critical section.

>> 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.

Note that you would also split between unmap (which does QLIST_REMOVE)
and free (which actually frees the block).  qemu_ram_free_from_ptr()
would really become qemu_ram_unmap_from_ptr(), and could do part of the
work asynchronously---which makes it free to take and release locks as
needed.  I don't think it is problematic to delay the freeing of the
blocks_mru item which requires BQL.

Paolo

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

[liu ping fan]

On Mon, Nov 12, 2012 at 4:48 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 12/11/2012 07:22, liu ping fan ha scritto:
>> On Sat, Nov 10, 2012 at 9:54 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 09/11/2012 04:14, Liu Ping Fan ha scritto:
>>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>> ---
>>>>  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?
>
> Yes.
>
> One alternative is to remove the MRU, but add a 1-item cache to speed up
> the common case.  Then the case where you use the cache can be placed
> (later) in an RCU critical section.
>
>>> 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.
>
> Note that you would also split between unmap (which does QLIST_REMOVE)
> and free (which actually frees the block).  qemu_ram_free_from_ptr()
> would really become qemu_ram_unmap_from_ptr(), and could do part of the
> work asynchronously---which makes it free to take and release locks as
> needed.  I don't think it is problematic to delay the freeing of the
> blocks_mru item which requires BQL.
>
Right.  Then just one thing left, the big lock may be called
recursively.  Do we have some elegant method to handle  it?

Regards,
Pingfan
> Paolo

[Qemu-devel] [RFC v1 3/3] make address_space_map safe

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpu-common.h      |    8 ++++++--
 cputlb.c          |    4 ++--
 dma-helpers.c     |    4 +++-
 dma.h             |    5 ++++-
 exec.c            |   45 +++++++++++++++++++++++++++++++++++++--------
 memory.h          |    4 +++-
 target-i386/kvm.c |    4 ++--
 7 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index 69c1d7a..f257dfc 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -45,8 +45,8 @@ void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size);
 void *qemu_safe_ram_ptr(ram_addr_t addr);
 void qemu_put_ram_ptr(void *addr);
 /* This should not be used by devices.  */
-int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
-ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
+int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr, bool unmap);
+ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr, bool unmap);
 void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev);
 
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
@@ -64,6 +64,10 @@ static inline void cpu_physical_memory_write(target_phys_addr_t addr,
 {
     cpu_physical_memory_rw(addr, (void *)buf, len, 1);
 }
+void *cpu_physical_memory_map_safe(target_phys_addr_t addr,
+                              target_phys_addr_t *plen,
+                              int is_write,
+                              bool *safe);
 void *cpu_physical_memory_map(target_phys_addr_t addr,
                               target_phys_addr_t *plen,
                               int is_write);
diff --git a/cputlb.c b/cputlb.c
index 9027557..a78ba02 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -164,7 +164,7 @@ static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
     if (tlb_is_dirty_ram(tlb_entry)) {
         p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK)
             + tlb_entry->addend);
-        ram_addr = qemu_ram_addr_from_host_nofail(p);
+        ram_addr = qemu_ram_addr_from_host_nofail(p, false);
         if (!cpu_physical_memory_is_dirty(ram_addr)) {
             tlb_entry->addr_write |= TLB_NOTDIRTY;
         }
@@ -338,7 +338,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
 #endif
     }
     p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
-    return qemu_ram_addr_from_host_nofail(p);
+    return qemu_ram_addr_from_host_nofail(p, false);
 }
 
 #define MMUSUFFIX _cmmu
diff --git a/dma-helpers.c b/dma-helpers.c
index 3f09dcb..6f6e3b7 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -394,6 +394,7 @@ void *iommu_dma_memory_map(DMAContext *dma, dma_addr_t addr, dma_addr_t *len,
     int err;
     target_phys_addr_t paddr, plen;
     void *buf;
+    bool safe;
 
     if (dma->map) {
         return dma->map(dma, addr, len, dir);
@@ -414,7 +415,8 @@ void *iommu_dma_memory_map(DMAContext *dma, dma_addr_t addr, dma_addr_t *len,
         *len = plen;
     }
 
-    buf = address_space_map(dma->as, paddr, &plen, dir == DMA_DIRECTION_FROM_DEVICE);
+    buf = address_space_map(dma->as, paddr, &plen,
+                        dir == DMA_DIRECTION_FROM_DEVICE, &safe);
     *len = plen;
 
     return buf;
diff --git a/dma.h b/dma.h
index 1bd6f4a..9d67dab 100644
--- a/dma.h
+++ b/dma.h
@@ -176,11 +176,14 @@ static inline void *dma_memory_map(DMAContext *dma,
                                    dma_addr_t addr, dma_addr_t *len,
                                    DMADirection dir)
 {
+    bool safe;
+
     if (!dma_has_iommu(dma)) {
         target_phys_addr_t xlen = *len;
         void *p;
 
-        p = address_space_map(dma->as, addr, &xlen, dir == DMA_DIRECTION_FROM_DEVICE);
+        p = address_space_map(dma->as, addr, &xlen,
+                        dir == DMA_DIRECTION_FROM_DEVICE, &safe);
         *len = xlen;
         return p;
     } else {
diff --git a/exec.c b/exec.c
index e5f1c0f..e9bd695 100644
--- a/exec.c
+++ b/exec.c
@@ -2847,7 +2847,14 @@ void qemu_put_ram_ptr(void *addr)
     trace_qemu_put_ram_ptr(addr);
 }
 
-int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
+static void memory_region_unref(MemoryRegion *mr)
+{
+    if (mr->ops && mr->ops->unref) {
+        mr->ops->unref(mr);
+    }
+}
+
+int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr, bool unmap)
 {
     RAMBlock *block;
     uint8_t *host = ptr;
@@ -2866,6 +2873,9 @@ 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);
+            if (unmap) {
+                memory_region_unref(block->mr);
+            }
             return 0;
         }
     }
@@ -2876,11 +2886,11 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
 
 /* Some of the softmmu routines need to translate from a host pointer
    (typically a TLB entry) back to a ram offset.  */
-ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
+ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr, bool unmap)
 {
     ram_addr_t ram_addr;
 
-    if (qemu_ram_addr_from_host(ptr, &ram_addr)) {
+    if (qemu_ram_addr_from_host(ptr, &ram_addr, unmap)) {
         fprintf(stderr, "Bad ram pointer %p\n", ptr);
         abort();
     }
@@ -3762,7 +3772,8 @@ static void cpu_notify_map_clients(void)
 void *address_space_map(AddressSpace *as,
                         target_phys_addr_t addr,
                         target_phys_addr_t *plen,
-                        bool is_write)
+                        bool is_write,
+                        bool *safe)
 {
     target_phys_addr_t len = *plen;
     target_phys_addr_t todo = 0;
@@ -3778,7 +3789,8 @@ void *address_space_map(AddressSpace *as,
         l = (page + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        address_space_section_lookup_ref(as, page >> TARGET_PAGE_BITS, &mr_obj);
+        *safe = address_space_section_lookup_ref(as, page >> TARGET_PAGE_BITS,
+                &mr_obj);
         section = &mr_obj;
 
         if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
@@ -3805,7 +3817,10 @@ void *address_space_map(AddressSpace *as,
         len -= l;
         addr += l;
         todo += l;
-        memory_region_section_unref(&mr_obj);
+        /* len <= 0,release when unmapped */
+        if (len > 0) {
+            memory_region_section_unref(&mr_obj);
+        }
     }
     rlen = todo;
     ret = qemu_ram_ptr_length(raddr, &rlen);
@@ -3822,7 +3837,8 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
 {
     if (buffer != bounce.buffer) {
         if (is_write) {
-            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
+            /* Will release RAM refcnt */
+            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer, true);
             while (access_len) {
                 unsigned l;
                 l = TARGET_PAGE_SIZE;
@@ -3847,11 +3863,24 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
     cpu_notify_map_clients();
 }
 
+void *cpu_physical_memory_map_safe(target_phys_addr_t addr,
+                              target_phys_addr_t *plen,
+                              int is_write,
+                              bool *safe)
+{
+
+    return address_space_map(&address_space_memory, addr, plen, is_write,
+           safe);
+}
+
 void *cpu_physical_memory_map(target_phys_addr_t addr,
                               target_phys_addr_t *plen,
                               int is_write)
 {
-    return address_space_map(&address_space_memory, addr, plen, is_write);
+    bool safe;
+
+    return address_space_map(&address_space_memory, addr, plen, is_write,
+           &safe);
 }
 
 void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
diff --git a/memory.h b/memory.h
index 704d014..c9d8a04 100644
--- a/memory.h
+++ b/memory.h
@@ -863,9 +863,11 @@ void address_space_read(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
  * @addr: address within that address space
  * @plen: pointer to length of buffer; updated on return
  * @is_write: indicates the transfer direction
+ * @safe: indicates the mapped addr can be safely across big lock or not
  */
 void *address_space_map(AddressSpace *as, target_phys_addr_t addr,
-                        target_phys_addr_t *plen, bool is_write);
+                        target_phys_addr_t *plen, bool is_write,
+                        bool *safe);
 
 /* address_space_unmap: Unmaps a memory region previously mapped by address_space_map()
  *
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5b18383..40f40c4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -261,7 +261,7 @@ int kvm_arch_on_sigbus_vcpu(CPUX86State *env, int code, void *addr)
 
     if ((env->mcg_cap & MCG_SER_P) && addr
         && (code == BUS_MCEERR_AR || code == BUS_MCEERR_AO)) {
-        if (qemu_ram_addr_from_host(addr, &ram_addr) ||
+        if (qemu_ram_addr_from_host(addr, &ram_addr, false) ||
             !kvm_physical_memory_addr_from_host(env->kvm_state, addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
                     "QEMU itself instead of guest system!\n");
@@ -293,7 +293,7 @@ int kvm_arch_on_sigbus(int code, void *addr)
         target_phys_addr_t paddr;
 
         /* Hope we are lucky for AO MCE */
-        if (qemu_ram_addr_from_host(addr, &ram_addr) ||
+        if (qemu_ram_addr_from_host(addr, &ram_addr, false) ||
             !kvm_physical_memory_addr_from_host(first_cpu->kvm_state, addr,
                                                 &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
-- 
1.7.4.4

Re: [Qemu-devel] [RFC v1 3/3] make address_space_map safe

[liu ping fan]

On Wed, Feb 13, 2013 at 8:12 PM, Vasilis Liaskovitis
<vasilis.liaskovitis@profitbricks.com> wrote:
> Hi,
>
> I am looking at this old ref/unref patchset for safely removing hot-plugged
> dimms/MemoryRegions. I am not sure if the set is still actively worked on or
> relevant for qemu-master, but I had a small comment below:
>
> On Fri, Nov 09, 2012 at 11:14:30AM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  cpu-common.h      |    8 ++++++--
>>  cputlb.c          |    4 ++--
>>  dma-helpers.c     |    4 +++-
>>  dma.h             |    5 ++++-
>>  exec.c            |   45 +++++++++++++++++++++++++++++++++++++--------
>>  memory.h          |    4 +++-
>>  target-i386/kvm.c |    4 ++--
>>  7 files changed, 57 insertions(+), 17 deletions(-)
>>
> [snip]
>> diff --git a/exec.c b/exec.c
>> index e5f1c0f..e9bd695 100644
>> --- a/exec.c
>> +++ b/exec.c
> [snip]
>> @@ -3822,7 +3837,8 @@ void address_space_unmap(AddressSpace *as, void *buffer, target_phys_addr_t len,
>>  {
>>      if (buffer != bounce.buffer) {
>>          if (is_write) {
>> -            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer);
>> +            /* Will release RAM refcnt */
>> +            ram_addr_t addr1 = qemu_ram_addr_from_host_nofail(buffer, true);
>>              while (access_len) {
>>                  unsigned l;
>>                  l = TARGET_PAGE_SIZE;
>
> Since qemu_ram_addr_from_host_nofail(buffer, true) will decrease the reference
> counter for this memoryregion, I think is should be called regardless of
> read/write i.e.  outside of the "if (is_write)" clause. Otherwise references for
> reads are not decreased properly.
>
Yes, you are right. if reference hold map, it need to be released here.

Thanks,
pingfan


> thanks,
>
> - Vasilis