[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, &ltt_traces.head, list) {
>> +     cds_list_for_each_entry_rcu(trace, &ltt_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, &ltt_traces.head, list) {
>> +     cds_list_for_each_entry(trace, &ltt_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, &ltt_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, &ltt_traces.head, list) {
>> +     cds_list_for_each_entry_safe(trace, trace_tmp, &ltt_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, &ltt_traces.head, list)
>> +     cds_list_for_each_entry(trace, &ltt_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, &ltt_traces.setup_head, list)
>> -             if (!strncmp(trace->trace_name, trace_name, NAME_MAX))
>> +     cds_list_for_each_entry(trace, &ltt_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, &ltt_traces.setup_head);
>> +     new_trace->state = TRACE_SETUP;
>> +
>> +     cds_list_add(&new_trace->list, &ltt_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, &ltt_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, &ltt_traces.head, list) {
>> +     cds_list_for_each_entry_rcu(trace, &ltt_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