[ovs-dev] [threads v2 03/13] netlink-socket: Make thread-safe.

[Ansis Atteka]

On Fri, Jul 12, 2013 at 2:54 PM, Ben Pfaff <blp at nicira.com> wrote:

> The uses of vlog in this module are not thread-safe, because vlog itself
> is not yet thread-safe.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/netlink-socket.c |   24 +++++++++++++++++++-----
>  lib/netlink-socket.h |    6 ++++++
>  2 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
> index 8e08841..da32284 100644
> --- a/lib/netlink-socket.c
> +++ b/lib/netlink-socket.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -29,6 +29,7 @@
>  #include "netlink.h"
>  #include "netlink-protocol.h"
>  #include "ofpbuf.h"
> +#include "ovs-thread.h"
>  #include "poll-loop.h"
>  #include "socket-util.h"
>  #include "util.h"
> @@ -85,13 +86,14 @@ static void nl_pool_release(struct nl_sock *);
>  int
>  nl_sock_create(int protocol, struct nl_sock **sockp)
>  {
> +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>      struct nl_sock *sock;
>      struct sockaddr_nl local, remote;
>      socklen_t local_size;
>      int rcvbuf;
>      int retval = 0;
>
> -    if (!max_iovs) {
> +    if (ovsthread_once_start(&once)) {
>

I could be wrong, but isn't max_iovs shared among all threads and hence
should be protected too? I guess the code in this "if" statement could
potentially be executed simultaneously by multiple threads until
ovsthread_once_done() is called? is that right?


Other than that looks good to me.

         int save_errno = errno;
>          errno = 0;
>
> @@ -106,6 +108,7 @@ nl_sock_create(int protocol, struct nl_sock **sockp)
>          }
>
>          errno = save_errno;
> +        ovsthread_once_done(&once);
>      }
>
>      *sockp = NULL;
> @@ -1010,17 +1013,25 @@ struct nl_pool {
>  };
>
>  static struct nl_pool pools[MAX_LINKS];
> +static pthread_mutex_t pool_mutex = PTHREAD_ADAPTIVE_MUTEX_INITIALIZER;
>
>  static int
>  nl_pool_alloc(int protocol, struct nl_sock **sockp)
>  {
> +    struct nl_sock *sock = NULL;
>      struct nl_pool *pool;
>
>      ovs_assert(protocol >= 0 && protocol < ARRAY_SIZE(pools));
>
> +    xpthread_mutex_lock(&pool_mutex);
>      pool = &pools[protocol];
>      if (pool->n > 0) {
> -        *sockp = pool->socks[--pool->n];
> +        sock = pool->socks[--pool->n];
> +    }
> +    xpthread_mutex_unlock(&pool_mutex);
> +
> +    if (sock) {
> +        *sockp = sock;
>          return 0;
>      } else {
>          return nl_sock_create(protocol, sockp);
> @@ -1033,11 +1044,14 @@ nl_pool_release(struct nl_sock *sock)
>      if (sock) {
>          struct nl_pool *pool = &pools[sock->protocol];
>
> +        xpthread_mutex_lock(&pool_mutex);
>          if (pool->n < ARRAY_SIZE(pool->socks)) {
>              pool->socks[pool->n++] = sock;
> -        } else {
> -            nl_sock_destroy(sock);
> +            sock = NULL;
>          }
> +        xpthread_mutex_unlock(&pool_mutex);
> +
> +        nl_sock_destroy(sock);
>      }
>  }
>
> diff --git a/lib/netlink-socket.h b/lib/netlink-socket.h
> index c77050e..986b574 100644
> --- a/lib/netlink-socket.h
> +++ b/lib/netlink-socket.h
> @@ -30,6 +30,12 @@
>   * are Linux-specific.  For Netlink protocol definitions, see
>   * netlink-protocol.h.  For helper functions for working with Netlink
> messages,
>   * see netlink.h.
> + *
> + *
> + * Thread-safety
> + * =============
> + *
> + * Only a single thread may use a given nl_sock or nl_dump at one time.
>   */
>
>  #include <stdbool.h>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130717/7c2083a6/attachment-0003.html>