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

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ericsson.com) wrote:
> 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...

As long as you comment, around each data structure and around each lock,
which lock protect which structure, yes. This is not the case at the
moment, but we should really do that.

I think there are two ways to reach a good level of lock traceability:
fine-grained locking is one, but good documentation should also get us
there.

By the way, fine-grained locks maybe make it easier to link which lock
go with which data structure, but then you run into the even more
complex problem of figuring out the proper locking order for _each_ lock
that is being taken together. And this can reach very ... interesting
levels of complexity. ;) This is why documenting around each lock what
locks it nests into, and which locks are nesting into the lock will help
us making sure we never run into deadlocks.

Thanks!

Mathieu

>
> /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
>>>
>>>     
>>
>>   
>

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