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