* Nils Carlson ([email protected]) 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. 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). 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). Thanks, Mathieu > > Signed-off-by: Nils Carlson <[email protected]> > --- > 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 > [email protected] > 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 _______________________________________________ ltt-dev mailing list [email protected] http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
