Re: [Qemu-devel] [PATCH v2 2/4] exec: Atomic access to map_client_list

[Paolo Bonzini]

On 13/03/2015 02:38, Fam Zheng wrote:
> Change map_client_list to QSLIST which supports atomic operations.
> 
> There are two access points to map_client_list. One is
> cpu_register_map_client, the other is cpu_notify_map_clients called
> after releasing the global bounce buffer in address_space_unmap. Each
> is now converted to a single atomic operation.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  exec.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 8d4e134..93ccd5a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2489,11 +2489,11 @@ static BounceBuffer bounce;
>  typedef struct MapClient {
>      void *opaque;
>      void (*callback)(void *opaque);
> -    QLIST_ENTRY(MapClient) link;
> +    QSLIST_ENTRY(MapClient) link;
>  } MapClient;
>  
> -static QLIST_HEAD(map_client_list, MapClient) map_client_list
> -    = QLIST_HEAD_INITIALIZER(map_client_list);
> +static QSLIST_HEAD(map_client_list, MapClient) map_client_list
> +    = QSLIST_HEAD_INITIALIZER(map_client_list);
>  
>  void *cpu_register_map_client(void *opaque, void (*callback)(void *opaque))
>  {
> @@ -2501,26 +2501,23 @@ void *cpu_register_map_client(void *opaque, void 
> (*callback)(void *opaque))
>  
>      client->opaque = opaque;
>      client->callback = callback;
> -    QLIST_INSERT_HEAD(&map_client_list, client, link);
> +    QSLIST_INSERT_HEAD_ATOMIC(&map_client_list, client, link);
>      return client;
>  }
>  
> -static void cpu_unregister_map_client(void *_client)
> -{
> -    MapClient *client = (MapClient *)_client;
> -
> -    QLIST_REMOVE(client, link);
> -    g_free(client);
> -}
> -
>  static void cpu_notify_map_clients(void)
>  {
>      MapClient *client;
> +    QSLIST_HEAD(, MapClient) mclist;
>  
> -    while (!QLIST_EMPTY(&map_client_list)) {
> -        client = QLIST_FIRST(&map_client_list);
> +    /* Only the bounce buffer owner thread could possibly call this, but we
> +     * should also coordinate with cpu_register_map_client. */
> +    QSLIST_MOVE_ATOMIC(&mclist, &map_client_list);
> +    while (!QSLIST_EMPTY(&mclist)) {
> +        client = QSLIST_FIRST(&mclist);
>          client->callback(client->opaque);
> -        cpu_unregister_map_client(client);
> +        QSLIST_REMOVE_HEAD(&mclist, link);
> +        g_free(client);
>      }
>  }
>  
> 

Looks good.

Paolo