Re: [PATCH v3] tools/lib/traceevent: make libtraceevent thread safe

[Tzvetomir Stoyanov <tstoyanov@vmware.com>]

Hi Steven
On Wed, Nov 28, 2018 at 4:27 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> On Wed, 28 Nov 2018 13:31:31 +0000
> Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:
>
> > This patch is a PoC about transforming libtraceevent
>
> Hi Tzvetomir,
>
> You may want to talk with Yordan about how to send patches with a
> subject like:
>
> [POC][PATCH v3] tools/lib/traceevent: make libtraceevent thread safe
>
> As the subject should state that this is a proof of concept patch.
>
> > into a thread safe library. It implements per thread local
> > storage and internal APIs to access it. It covers only
> > tep->last_event cache, but easily can be extended with all
> > library's thread sensitive data.
> >
> > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> > ---
> > v2: Added local thread data per tep context
> > v3: Implemented APIs to access specific tread local data,
> >     instead of the whole struct tep_thread_data.
> > ---
> >  tools/lib/traceevent/event-parse-local.h  |  18 ++-
> >  tools/lib/traceevent/event-parse-thread.c | 131 ++++++++++++++++++++++
> >  tools/lib/traceevent/event-parse.c        |  21 ++--
> >  3 files changed, 158 insertions(+), 12 deletions(-)
> >  create mode 100644 tools/lib/traceevent/event-parse-thread.c
> >
> > diff --git a/tools/lib/traceevent/event-parse-local.h b/tools/lib/traceevent/event-parse-local.h
> > index 9a092dd4a86d..aa9418fdefd0 100644
> > --- a/tools/lib/traceevent/event-parse-local.h
> > +++ b/tools/lib/traceevent/event-parse-local.h
> > @@ -14,6 +14,14 @@ struct func_list;
> >  struct event_handler;
> >  struct func_resolver;
> >
> > +/* cache */
> > +struct tep_thread_data {
> > +     struct tep_handle *tep;
> > +     struct tep_thread_data *next;
> > +
> > +     struct tep_event *last_event;
> > +};
> > +
> >  struct tep_handle {
> >       int ref_count;
> >
> > @@ -83,9 +91,6 @@ struct tep_handle {
> >       struct event_handler *handlers;
> >       struct tep_function_handler *func_handlers;
> >
> > -     /* cache */
> > -     struct tep_event *last_event;
> > -
> >       char *trace_clock;
> >  };
> >
> > @@ -96,4 +101,11 @@ unsigned short tep_data2host2(struct tep_handle *pevent, unsigned short data);
> >  unsigned int tep_data2host4(struct tep_handle *pevent, unsigned int data);
> >  unsigned long long tep_data2host8(struct tep_handle *pevent, unsigned long long data);
> >
> > +/* tep cache per thread */
> > +struct tep_event *tep_find_event_by_id_cache(struct tep_handle *tep, int id);
> > +struct tep_event *
> > +tep_find_event_by_name_cache(struct tep_handle *tep, const char *name, const char *sys);
> > +void tep_set_serach_event_cache(struct tep_handle *tep, struct tep_event *event);
> > +void tep_destroy_thread_local(struct tep_handle *tep);
> > +
> >  #endif /* _PARSE_EVENTS_INT_H */
> > diff --git a/tools/lib/traceevent/event-parse-thread.c b/tools/lib/traceevent/event-parse-thread.c
> > new file mode 100644
> > index 000000000000..84707c24ac6b
> > --- /dev/null
> > +++ b/tools/lib/traceevent/event-parse-thread.c
> > @@ -0,0 +1,131 @@
> > +// SPDX-License-Identifier: LGPL-2.1
> > +/*
> > + * Copyright (C) 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
> > + *
> > + */
> > +
> > +#include "event-parse.h"
> > +#include "event-parse-local.h"
> > +#include "event-utils.h"
> > +
> > +static __thread struct tep_thread_data *tep_thread_local;
> > +
> > +static struct tep_thread_data *tep_alloc_thread_local(struct tep_handle *tep)
> > +{
> > +     struct tep_thread_data *local;
> > +
> > +     local = calloc(1, sizeof(struct tep_thread_data));
> > +     if (local) {
> > +             local->tep = tep;
> > +             local->next = tep_thread_local;
>
> I really don't want a loop. We should have one thread cache. Heck, then
> we don't even need to allocate it.
>

The reason for dynamic allocation is that we should support the case
where we have
multiple threads, working with multiple tep handlers. We need each
thread to has local
data for each tep handler, so we should use list (or other dynamic
structure) of all
tep handlers per thread. The global variable
static __thread struct tep_thread_data *tep_thread_local;
has to store locals for multiple tep handlers, in context of current thread.

> -- Steve
>
>
> > +             tep_thread_local = local;
> > +     }
> > +     return local;
> > +}
> > +
>
>


-- 

Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center