Hi Mathieu,
Mathieu Desnoyers wrote:
* Nils Carlson ([email protected]) wrote:
Make the locking around the transport list more intelligent, give
it its own mutex.
Signed-off-by: Nils Carlson <[email protected]>
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(<t_transport_mutex);
cds_list_add_tail(&transport->node, <t_transport_list);
- ltt_unlock_traces();
+ pthread_mutex_unlock(<t_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(<t_transport_mutex);
cds_list_del(&transport->node);
- ltt_unlock_traces();
+ pthread_mutex_unlock(<t_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(<t_transport_mutex);
cds_list_for_each_entry(tran_iter, <t_transport_list, node) {
if (!strcmp(tran_iter->name, trace_type)) {
transport = tran_iter;
break;
}
}
+ pthread_mutex_unlock(<t_transport_mutex);
+
if (!transport) {
ERR("Transport %s is not present", trace_type);
err = -EINVAL;
--
1.7.1
_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
