[ltt-dev] [UST PATCH] libust: Replace the the two trace lists with a single list
Mathieu Desnoyers
compudj at krystal.dyndns.org
Thu Mar 17 08:20:12 EDT 2011
* 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.
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 <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