Patchwork [RFC,v1,1/3] bouce buffer has fine grain lock

login
register
mail settings
Submitter pingfan liu
Date Nov. 9, 2012, 3:14 a.m.
Message ID <1352430870-10129-2-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/197919/
State New
Headers show

Comments

pingfan liu - Nov. 9, 2012, 3:14 a.m.
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(-)
Paolo Bonzini - Nov. 10, 2012, 1:49 a.m.
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();
>  }
>  
>
pingfan liu - Nov. 12, 2012, 6:23 a.m.
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();
>>  }
>>
>>
>
Paolo Bonzini - Nov. 12, 2012, 8:53 a.m.
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

Patch

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