[lttng-dev] [lttng-tools PATCH] Quick fix for Bug #7

[Mathieu Desnoyers]

* David Goulet (dgoulet at efficios.com) wrote:
> Temporary fix the bug #7 issue for the 2.0 stable release. Refer to
> bugs.lttng.org for more information on the issues.
> 
> The real stable fix is planned for 2.1+ stable release.
> 
> Signed-off-by: David Goulet <dgoulet at efficios.com>
> ---
>  src/bin/lttng-sessiond/ust-app.c |   36 +++++++++++++++++++++++++++++++++++-
>  src/common/hashtable/hashtable.c |    2 --
>  src/common/hashtable/hashtable.h |    2 ++
>  3 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c
> index 2b66b92..e82d38a 100644
> --- a/src/bin/lttng-sessiond/ust-app.c
> +++ b/src/bin/lttng-sessiond/ust-app.c
> @@ -29,6 +29,9 @@
>  
>  #include <common/common.h>
>  
> +/* For add_unique_ust_app quick fix function for 2.0 stable release */
> +#include <common/hashtable/hashtable.h>
> +
>  #include "ust-app.h"
>  #include "ust-consumer.h"
>  #include "ust-ctl.h"
> @@ -1257,6 +1260,31 @@ error:
>  }
>  
>  /*
> + * TEMP function call to add uniquely a registered apps to a hash table.
> + *
> + * If a duplicate node is found when adding apps, we delete the old one and
> + * close the socket file descriptor as well. Must be inside a read side lock.
> + *
> + * Please refer to BUG #7 on bugs.lttng.org to understand the purpose and
> + * lifespan of this function.
> + */
> +static void add_unique_ust_app(struct lttng_ht *ht,
> +		struct lttng_ht_node_ulong *node)
> +{
> +	struct cds_lfht_node *node_ptr;
> +	struct lttng_ht_node_ulong *old_node;
> +
> +	node_ptr = cds_lfht_add_unique(ht->ht,
> +			ht->hash_fct((void *) node->key, HASH_SEED), ht->match_fct,
> +			(void *) node->key, &node->node);
> +	if (node_ptr != &node->node) {
> +		/* Double registration with same PID */
> +		old_node = caa_container_of(node_ptr, struct lttng_ht_node_ulong, node);
> +		call_rcu(&old_node->head, delete_ust_app_rcu);

ust_app_unregister():

        /* Get the node reference for a call_rcu */
        lttng_ht_lookup(ust_app_ht, (void *)((unsigned long) lta->key.pid), &iter);
        node = lttng_ht_iter_get_node_ulong(&iter);
        if (node == NULL) {
                ERR("Unable to find app sock %d by pid %d", sock, lta->key.pid);
                goto error;
        }

Should be allowed to not find the per pid entry without printing an
error. This will happen on the socket fd close that will follow this
removal.

> +	}

Missing: lttng_ht_del() call before the call_rcu for delete_ust_app_rcu.

Missing: retry loop! After we cleared room for the new node we want to
add, we need to retry the add_unique.

Thanks,

Mathieu

> +}
> +
> +/*
>   * Using pid and uid (of the app), allocate a new ust_app struct and
>   * add it to the global traceable app list.
>   *
> @@ -1307,7 +1335,13 @@ int ust_app_register(struct ust_register_msg *msg, int sock)
>  
>  	rcu_read_lock();
>  	lttng_ht_add_unique_ulong(ust_app_sock_key_map, &lta->key.node);
> -	lttng_ht_add_unique_ulong(ust_app_ht, &lta->node);
> +	/*
> +	 * Changed for now. Refer to BUG #7 called "Double PID registering and
> +	 * unregistering race"
> +	 *
> +	 * lttng_ht_add_unique_ulong(ust_app_ht, &lta->node);
> +	 */
> +	add_unique_ust_app(ust_app_ht, &lta->node);
>  	rcu_read_unlock();
>  
>  	DBG("App registered with pid:%d ppid:%d uid:%d gid:%d sock:%d name:%s"
> diff --git a/src/common/hashtable/hashtable.c b/src/common/hashtable/hashtable.c
> index 2080107..83d4dfd 100644
> --- a/src/common/hashtable/hashtable.c
> +++ b/src/common/hashtable/hashtable.c
> @@ -27,8 +27,6 @@
>  #include "hashtable.h"
>  #include "utils.h"
>  
> -#define HASH_SEED            0x42UL		/* The answer to life */
> -
>  static unsigned long min_hash_alloc_size = 1;
>  static unsigned long max_hash_buckets_size = (1UL << 20);
>  
> diff --git a/src/common/hashtable/hashtable.h b/src/common/hashtable/hashtable.h
> index 1c2889d..0f0339c 100644
> --- a/src/common/hashtable/hashtable.h
> +++ b/src/common/hashtable/hashtable.h
> @@ -23,6 +23,8 @@
>  #include "rculfhash.h"
>  #include "rculfhash-internal.h"
>  
> +#define HASH_SEED            0x42UL		/* The answer to life */
> +
>  typedef unsigned long (*hash_fct)(void *_key, unsigned long seed);
>  typedef cds_lfht_match_fct hash_match_fct;
>  
> -- 
> 1.7.9
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com