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

[David Goulet]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 12-02-09 03:54 PM, Mathieu Desnoyers wrote:
> * 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.

Oh add_unique fails you are right! Should we put a replace instead thus removing
the lttng_ht_del.. ?

David

> 
> 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
>>
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPNDMmAAoJEELoaioR9I02UZgH+wQA74aT2l4a7xuRb7rpTcXz
15yCjY0m0foSenzo+bMKX4hAK37igvqQc60G6nKC7uDup80yXfXoGrpL7Olm18+I
3JqFGls5QugN83aBJJCWJl+WNb2/xRj7uBqoS9Md2iFSt693bpacwippLejtjFN2
XFvn/XVf0wqF14Xy7nas86NYtQylYIUXPSqQBcfumfdFxpuzsAwtNAm6TKHMexRq
KnOi8GWcGKa0EpTfoFj1C/ukRLwZuNAnpfG9QEeBxOnJ0nhkusO/vXNawo+t9CJ9
kf2jnk66UWo14kYeDjYJYZCcDL0o6FhS2Uqo10VNqUkzV9rUmVkvm8MQ6pWGydI=
=Z75H
-----END PGP SIGNATURE-----