[ltt-dev] [UST PATCH] libust: New transport mutex

[Nils Carlson]

Hi Mathieu,

Mathieu Desnoyers wrote:
> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>   
>> Make the locking around the transport list more intelligent, give
>> it its own mutex.
>>
>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>     
>
> Hi Nils,
>
> When adding this kind of new mutex, can you:
>
> a) explain why it is needed (or what the advantage is)
> b) comment, beside the mutex, what the nesting of the mutex with
>    respect to other is.
>   
Tricky tricky.

a) it isn't. Not since we introduced the even bigger hammer 
"listener_thread_data_mutex", at that point all other mutexes became 
basically unnecessary as we lock around everything.
b) can fix. but read on..
> In the past, I went for a very fine-grained locking scheme in the LTTng
> control internals, only to find out that it complexified the code
> needlessly and added race conditions here and there. At that point, I
> went back to the big hammer (lock_traces()).
>
>   
Yeah, the big hammer already got bigger... Hm, if we decide to simplify 
the locking then I can remove all the mutexes except the big hammer, 
this would make my life a lot easier.

> As long as adding more mutexes does not change anything in terms of
> end-user scalability experience, I don't see any reason to add more.
>
>   
This one came into being because its good practice to lock around list 
handling, and to give a mutex to something being protected, sharing 
mutexes across data structures (as was the case here) is dangerous 
because forces you to figure out why these two datastructures need to 
share locking.

But back to the big hammer approach; can I then eliminate all the 
unnecessary locking and we leave only the "listener_thread_data_mutex" 
in place? This would make refactoring so much easier...

/Nils

> Thanks,
>
> Mathieu
>
>   
>> ---
>>  libust/tracer.c |   13 ++++++++-----
>>  1 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/libust/tracer.c b/libust/tracer.c
>> index 100dec0..ceb5b74 100644
>> --- a/libust/tracer.c
>> +++ b/libust/tracer.c
>> @@ -182,7 +182,7 @@ static enum ltt_channels get_channel_type_from_name(const char *name)
>>  //ust// }
>>  
>>  static CDS_LIST_HEAD(ltt_transport_list);
>> -
>> +static DEFINE_MUTEX(ltt_transport_mutex);
>>  /**
>>   * ltt_transport_register - LTT transport registration
>>   * @transport: transport structure
>> @@ -204,9 +204,9 @@ void ltt_transport_register(struct ltt_transport *transport)
>>  	 */
>>  //ust//	vmalloc_sync_all();
>>  
>> -	ltt_lock_traces();
>> +	pthread_mutex_lock(&ltt_transport_mutex);
>>  	cds_list_add_tail(&transport->node, &ltt_transport_list);
>> -	ltt_unlock_traces();
>> +	pthread_mutex_unlock(&ltt_transport_mutex);
>>  }
>>  
>>  /**
>> @@ -215,9 +215,9 @@ void ltt_transport_register(struct ltt_transport *transport)
>>   */
>>  void ltt_transport_unregister(struct ltt_transport *transport)
>>  {
>> -	ltt_lock_traces();
>> +	pthread_mutex_lock(&ltt_transport_mutex);
>>  	cds_list_del(&transport->node);
>> -	ltt_unlock_traces();
>> +	pthread_mutex_unlock(&ltt_transport_mutex);
>>  }
>>  
>>  static inline int is_channel_overwrite(enum ltt_channels chan,
>> @@ -458,12 +458,15 @@ int ltt_trace_set_type(const char *trace_name, const char *trace_type)
>>  		goto traces_error;
>>  	}
>>  
>> +	pthread_mutex_lock(&ltt_transport_mutex);
>>  	cds_list_for_each_entry(tran_iter, &ltt_transport_list, node) {
>>  		if (!strcmp(tran_iter->name, trace_type)) {
>>  			transport = tran_iter;
>>  			break;
>>  		}
>>  	}
>> +	pthread_mutex_unlock(&ltt_transport_mutex);
>> +
>>  	if (!transport) {
>>  		ERR("Transport %s is not present", trace_type);
>>  		err = -EINVAL;
>> -- 
>> 1.7.1
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>>     
>
>