[ltt-dev] [UST PATCH] libust: Replace the the two trace lists with a single list
Nils Carlson
nils.carlson at ericsson.com
Thu Mar 17 10:47:43 EDT 2011
Mathieu Desnoyers wrote:
> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>
>> Get rid of the two trace lists and introduce a state into each
>> trace. This simplifies things a bit as two lists don't have
>> to be searched to know if a trace exists. Also, if traces pass
>> through more states in the future new states will be easier to
>> add.
>>
>
> Nope, we don't want to iterate on "setup" traces on the tracing fast
> path. You seem to have forgotten to put a check in the tracing fast path
> to exclude these setup traces.
>
Nope, consider that the activate flag is checked for.
> There will be some changes coming there in the UST rework, so I really
> don't think it is worth it to merge these two lists. The whole concept
> of "setup trace" will go away: a trace session that will be created but
> not active will be a "template" session that is not in any of the trace
> lists (it will stay local to the file descriptor responsible for its
> creation until it is activated).
>
Ok.
> You might want to have a look at the lttng snapshot tree to see in which
> direction things are heading before doing these changes (I'm fine with
> counter-arguing the patch by email too, it's just that I am trying to
> find ways to use your time efficiently).
>
>
I'll starting working on session daemon integration then.
/Nils
> Thanks,
>
> Mathieu
>
>
>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>> ---
>> libust/serialize.c | 3 +-
>> libust/tracectl.c | 33 +++++++-----------
>> libust/tracer.c | 81 ++++++++++++++++++++++++---------------------
>> libust/tracer.h | 14 +++++++-
>> libust/tracercore.c | 3 +-
>> libust/tracercore.h | 3 +-
>> libust/type-serializer.c | 2 +-
>> 7 files changed, 73 insertions(+), 66 deletions(-)
>>
>> diff --git a/libust/serialize.c b/libust/serialize.c
>> index 8aa3f4b..f1620d1 100644
>> --- a/libust/serialize.c
>> +++ b/libust/serialize.c
>> @@ -685,7 +685,8 @@ notrace void ltt_vtrace(const struct marker *mdata, void *probe_data,
>> va_end(args_copy);
>>
>> /* Iterate on each trace */
>> - cds_list_for_each_entry_rcu(trace, <t_traces.head, list) {
>> + cds_list_for_each_entry_rcu(trace, <t_traces.trace_list, list) {
>> +
>> /*
>> * Expect the filter to filter out events. If we get here,
>> * we went through tracepoint activation as a first step.
>> diff --git a/libust/tracectl.c b/libust/tracectl.c
>> index 33c7280..628b19a 100644
>> --- a/libust/tracectl.c
>> +++ b/libust/tracectl.c
>> @@ -214,7 +214,7 @@ static void inform_consumer_daemon(const char *trace_name)
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>> if (trace == NULL) {
>> WARN("inform_consumer_daemon: could not find trace \"%s\"; it is probably already destroyed", trace_name);
>> goto unlock_traces;
>> @@ -266,7 +266,7 @@ static int get_buffer_shmid_pipe_fd(const char *trace_name, const char *ch_name,
>> DBG("get_buffer_shmid_pipe_fd");
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>> ltt_unlock_traces();
>>
>> if (trace == NULL) {
>> @@ -298,7 +298,7 @@ static int get_subbuf_num_size(const char *trace_name, const char *ch_name,
>> DBG("get_subbuf_size");
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace(trace_name);
>> ltt_unlock_traces();
>>
>> if (!trace) {
>> @@ -348,9 +348,9 @@ static int set_subbuf_size(const char *trace_name, const char *ch_name,
>> }
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (trace == NULL) {
>> - ERR("cannot find trace!");
>> + ERR("cannot find trace %s in state setup!", trace_name);
>> retval = -ENODATA;
>> goto unlock_traces;
>> }
>> @@ -386,7 +386,7 @@ static int set_subbuf_num(const char *trace_name, const char *ch_name,
>> }
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (trace == NULL) {
>> ERR("cannot find trace!");
>> retval = -ENODATA;
>> @@ -421,7 +421,7 @@ static int get_subbuffer(const char *trace_name, const char *ch_name,
>> *consumed_old = 0;
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>>
>> if (!trace) {
>> DBG("Cannot find trace. It was likely destroyed by the user.");
>> @@ -462,7 +462,7 @@ static int notify_buffer_mapped(const char *trace_name,
>> DBG("get_buffer_fd");
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>>
>> if (!trace) {
>> retval = -ENODATA;
>> @@ -504,7 +504,7 @@ static int put_subbuffer(const char *trace_name, const char *ch_name,
>> DBG("put_subbuf");
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>>
>> if (!trace) {
>> retval = -ENODATA;
>> @@ -557,7 +557,7 @@ static int force_subbuf_switch(const char *trace_name)
>> int i, j, retval = 0;
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>> if (!trace) {
>> retval = -ENODATA;
>> DBG("Cannot find trace. It was likely destroyed by the user.");
>> @@ -1488,7 +1488,7 @@ static int trace_recording(void)
>>
>> ltt_lock_traces();
>>
>> - cds_list_for_each_entry(trace, <t_traces.head, list) {
>> + cds_list_for_each_entry(trace, <t_traces.trace_list, list) {
>> if (trace->active) {
>> retval = 1;
>> break;
>> @@ -1601,14 +1601,6 @@ static void ust_fork(void)
>> /* Get the pid of the new process */
>> processpid = getpid();
>>
>> - /*
>> - * FIXME: This could be prettier, we loop over the list twice and
>> - * following good locking practice should lock around the loop
>> - */
>> - cds_list_for_each_entry_safe(trace, trace_tmp, <t_traces.head, list) {
>> - ltt_trace_stop(trace->trace_name);
>> - }
>> -
>> /* Delete all active connections, but leave them in the epoll set */
>> cds_list_for_each_entry_safe(sock, sock_tmp, &ust_socks, list) {
>> ustcomm_del_sock(sock, 1);
>> @@ -1618,7 +1610,8 @@ static void ust_fork(void)
>> * FIXME: This could be prettier, we loop over the list twice and
>> * following good locking practice should lock around the loop
>> */
>> - cds_list_for_each_entry_safe(trace, trace_tmp, <t_traces.head, list) {
>> + cds_list_for_each_entry_safe(trace, trace_tmp, <t_traces.trace_list, list) {
>> + ltt_trace_stop(trace->trace_name);
>> ltt_trace_destroy(trace->trace_name, 1);
>> }
>>
>> diff --git a/libust/tracer.c b/libust/tracer.c
>> index 3b4fae4..4cefc02 100644
>> --- a/libust/tracer.c
>> +++ b/libust/tracer.c
>> @@ -139,33 +139,37 @@ static void trace_async_wakeup(struct ust_trace *trace)
>> }
>>
>> /**
>> - * _ltt_trace_find - find a trace by given name.
>> + * find_trace - find a trace by given name.
>> * trace_name: trace name
>> *
>> * Returns a pointer to the trace structure, NULL if not found.
>> + * Locking: Must be called with the traces mutex held.
>> */
>> -struct ust_trace *_ltt_trace_find(const char *trace_name)
>> +struct ust_trace *find_trace(const char *trace_name)
>> {
>> struct ust_trace *trace;
>>
>> - cds_list_for_each_entry(trace, <t_traces.head, list)
>> + cds_list_for_each_entry(trace, <t_traces.trace_list, list)
>> if (!strncmp(trace->trace_name, trace_name, NAME_MAX))
>> return trace;
>>
>> return NULL;
>> }
>> -
>> -/* _ltt_trace_find_setup :
>> - * find a trace in setup list by given name.
>> +/**
>> + * find_trace_in_state - find a trace by given name and state
>> + * trace_name: trace name
>> + * trace_state: trace state
>> *
>> * Returns a pointer to the trace structure, NULL if not found.
>> + * Locking: Must be called with the traces mutex held.
>> */
>> -struct ust_trace *_ltt_trace_find_setup(const char *trace_name)
>> +struct ust_trace *find_trace_with_state(const char *trace_name, int trace_state)
>> {
>> struct ust_trace *trace;
>>
>> - cds_list_for_each_entry(trace, <t_traces.setup_head, list)
>> - if (!strncmp(trace->trace_name, trace_name, NAME_MAX))
>> + cds_list_for_each_entry(trace, <t_traces.trace_list, list)
>> + if (trace_state == trace->state &&
>> + !strncmp(trace->trace_name, trace_name, NAME_MAX))
>> return trace;
>>
>> return NULL;
>> @@ -215,13 +219,7 @@ int _ltt_trace_setup(const char *trace_name)
>> unsigned int chan;
>> enum ltt_channels chantype;
>>
>> - if (_ltt_trace_find_setup(trace_name)) {
>> - ERR("Trace name %s already used", trace_name);
>> - err = -EEXIST;
>> - goto traces_error;
>> - }
>> -
>> - if (_ltt_trace_find(trace_name)) {
>> + if (find_trace(trace_name)) {
>> ERR("Trace name %s already used", trace_name);
>> err = -EEXIST;
>> goto traces_error;
>> @@ -266,7 +264,9 @@ int _ltt_trace_setup(const char *trace_name)
>> chan_infos[chantype].def_subbufcount;
>> }
>>
>> - cds_list_add(&new_trace->list, <t_traces.setup_head);
>> + new_trace->state = TRACE_SETUP;
>> +
>> + cds_list_add(&new_trace->list, <t_traces.trace_list);
>> return 0;
>>
>> trace_free:
>> @@ -300,7 +300,7 @@ int ltt_trace_set_type(const char *trace_name, const char *trace_type)
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (!trace) {
>> ERR("Trace not found %s", trace_name);
>> err = -ENOENT;
>> @@ -338,9 +338,10 @@ int ltt_trace_set_channel_subbufsize(const char *trace_name,
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (!trace) {
>> - ERR("Trace not found %s", trace_name);
>> + ERR("Trace %s has not been setup or does not exist",
>> + trace_name);
>> err = -ENOENT;
>> goto traces_error;
>> }
>> @@ -367,9 +368,10 @@ int ltt_trace_set_channel_subbufcount(const char *trace_name,
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (!trace) {
>> - ERR("Trace not found %s", trace_name);
>> + ERR("Trace %s has not been setup or does not exist",
>> + trace_name);
>> err = -ENOENT;
>> goto traces_error;
>> }
>> @@ -396,9 +398,10 @@ int ltt_trace_set_channel_enable(const char *trace_name,
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (!trace) {
>> - ERR("Trace not found %s", trace_name);
>> + ERR("Trace %s has not been setup or does not exist",
>> + trace_name);
>> err = -ENOENT;
>> goto traces_error;
>> }
>> @@ -436,9 +439,10 @@ int ltt_trace_set_channel_overwrite(const char *trace_name,
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (!trace) {
>> - ERR("Trace not found %s", trace_name);
>> + ERR("Trace %s has not been setup or does not exist",
>> + trace_name);
>> err = -ENOENT;
>> goto traces_error;
>> }
>> @@ -479,15 +483,17 @@ int ltt_trace_alloc(const char *trace_name)
>>
>> ltt_lock_traces();
>>
>> - if (_ltt_trace_find(trace_name)) { /* Trace already allocated */
>> - err = 1;
>> + trace = find_trace(trace_name);
>> + if (!trace) {
>> + ERR("Trace %s does not exist",
>> + trace_name);
>> + err = -ENOENT;
>> goto traces_error;
>> }
>>
>> - trace = _ltt_trace_find_setup(trace_name);
>> - if (!trace) {
>> - ERR("Trace not found %s", trace_name);
>> - err = -ENOENT;
>> + if (trace->state == TRACE_ALLOCATED) {
>> + /* Trace was already allocated, return 1 */
>> + err = 1;
>> goto traces_error;
>> }
>>
>> @@ -528,8 +534,7 @@ int ltt_trace_alloc(const char *trace_name)
>> }
>> }
>>
>> - cds_list_del(&trace->list);
>> - cds_list_add_rcu(&trace->list, <t_traces.head);
>> + trace->state = TRACE_ALLOCATED;
>>
>> ltt_unlock_traces();
>>
>> @@ -610,7 +615,7 @@ int ltt_trace_destroy(const char *trace_name, int drop)
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>> if (trace) {
>> err = _ltt_trace_destroy(trace);
>> if (err)
>> @@ -623,7 +628,7 @@ int ltt_trace_destroy(const char *trace_name, int drop)
>> return 0;
>> }
>>
>> - trace = _ltt_trace_find_setup(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_SETUP);
>> if (trace) {
>> _ltt_trace_free(trace);
>> ltt_unlock_traces();
>> @@ -664,7 +669,7 @@ int ltt_trace_start(const char *trace_name)
>>
>> ltt_lock_traces();
>>
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>> err = _ltt_trace_start(trace);
>> if (err)
>> goto no_trace;
>> @@ -716,7 +721,7 @@ int ltt_trace_stop(const char *trace_name)
>> struct ust_trace *trace;
>>
>> ltt_lock_traces();
>> - trace = _ltt_trace_find(trace_name);
>> + trace = find_trace_with_state(trace_name, TRACE_ALLOCATED);
>> err = _ltt_trace_stop(trace);
>> ltt_unlock_traces();
>> return err;
>> diff --git a/libust/tracer.h b/libust/tracer.h
>> index 2f489f6..16b1e1a 100644
>> --- a/libust/tracer.h
>> +++ b/libust/tracer.h
>> @@ -167,6 +167,12 @@ enum trace_mode { LTT_TRACE_NORMAL, LTT_TRACE_FLIGHT, LTT_TRACE_HYBRID };
>> #define CHANNEL_FLAG_ENABLE (1U<<0)
>> #define CHANNEL_FLAG_OVERWRITE (1U<<1)
>>
>> +enum trace_states {
>> + TRACE_CREATED,
>> + TRACE_SETUP,
>> + TRACE_ALLOCATED,
>> +};
>> +
>> /* Per-trace information - each trace/flight recorder represented by one */
>> struct ust_trace {
>> /* First 32 bytes cache-hot cacheline */
>> @@ -179,6 +185,8 @@ struct ust_trace {
>> u32 freq_scale;
>> u64 start_freq;
>> u64 start_tsc;
>> + /* End of cache-hot section */
>> + int state;
>> unsigned long long start_monotonic;
>> struct timeval start_time;
>> struct ltt_channel_setting *settings;
>> @@ -409,7 +417,7 @@ union ltt_control_args {
>>
>> extern int _ltt_trace_setup(const char *trace_name);
>> extern int ltt_trace_setup(const char *trace_name);
>> -extern struct ust_trace *_ltt_trace_find_setup(const char *trace_name);
>> +
>> extern int ltt_trace_set_type(const char *trace_name, const char *trace_type);
>> extern int ltt_trace_set_channel_subbufsize(const char *trace_name,
>> const char *channel_name, unsigned int size);
>> @@ -450,6 +458,8 @@ extern void ltt_dump_marker_state(struct ust_trace *trace);
>> extern void ltt_lock_traces(void);
>> extern void ltt_unlock_traces(void);
>>
>> -extern struct ust_trace *_ltt_trace_find(const char *trace_name);
>> +extern struct ust_trace *find_trace(const char *trace_name);
>> +
>> +extern struct ust_trace *find_trace_with_state(const char *trace_name, int state);
>>
>> #endif /* _LTT_TRACER_H */
>> diff --git a/libust/tracercore.c b/libust/tracercore.c
>> index 1e418b6..1532c88 100644
>> --- a/libust/tracercore.c
>> +++ b/libust/tracercore.c
>> @@ -22,8 +22,7 @@
>>
>> /* Traces structures */
>> struct ltt_traces ltt_traces = {
>> - .setup_head = CDS_LIST_HEAD_INIT(ltt_traces.setup_head),
>> - .head = CDS_LIST_HEAD_INIT(ltt_traces.head),
>> + .trace_list = CDS_LIST_HEAD_INIT(ltt_traces.trace_list),
>> };
>>
>> /* Traces list writer locking */
>> diff --git a/libust/tracercore.h b/libust/tracercore.h
>> index 9673cca..c5c81fd 100644
>> --- a/libust/tracercore.h
>> +++ b/libust/tracercore.h
>> @@ -32,8 +32,7 @@
>> * list.
>> */
>> struct ltt_traces {
>> - struct cds_list_head setup_head; /* Pre-allocated traces list */
>> - struct cds_list_head head; /* Allocated Traces list */
>> + struct cds_list_head trace_list; /* Allocated Traces list */
>> unsigned int num_active_traces; /* Number of active traces */
>> } ____cacheline_aligned;
>>
>> diff --git a/libust/type-serializer.c b/libust/type-serializer.c
>> index dcaea1e..352f9be 100644
>> --- a/libust/type-serializer.c
>> +++ b/libust/type-serializer.c
>> @@ -60,7 +60,7 @@ void _ltt_specialized_trace(const struct marker *mdata, void *probe_data,
>> * Iterate on each trace, typically small number of active traces,
>> * list iteration with prefetch is usually slower.
>> */
>> - cds_list_for_each_entry_rcu(trace, <t_traces.head, list) {
>> + cds_list_for_each_entry_rcu(trace, <t_traces.trace_list, list) {
>> if (unlikely(!trace->active))
>> continue;
>> //ust// if (unlikely(!ltt_run_filter(trace, eID)))
>> --
>> 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
>
More information about the lttng-dev
mailing list