[PATCH 0/2] convert traceevent into a thread safe library

[PATCH 0/2] convert traceevent into a thread safe library

[Tzvetomir Stoyanov]

This short patch series begins tranformation of libtraceevent into a
thread safe library. It implements per thread local storage for 
tep->last_event cache, and converts input_buf, input_buf_ptr
and input_buf_siz internal variables to be thread specific.

Tzvetomir Stoyanov (2):
  tools/lib/traceevent: make libtraceevent thread safe
  tools/lib/traceevent: make few libtraceevent internal variables to be
    per thread

 tools/lib/traceevent/Build                |  1 +
 tools/lib/traceevent/event-parse-local.h  | 15 ++++--
 tools/lib/traceevent/event-parse-thread.c | 63 +++++++++++++++++++++++
 tools/lib/traceevent/event-parse.c        | 45 ++++++++--------
 4 files changed, 100 insertions(+), 24 deletions(-)
 create mode 100644 tools/lib/traceevent/event-parse-thread.c

-- 
2.19.1

[PATCH 1/2] tools/lib/traceevent: make libtraceevent thread safe

[Tzvetomir Stoyanov]

This patch begins tranformation of libtraceevent into a
thread safe library. It implements per thread local storage
of tep hadler thread sensitive data and internal
APIs to access it. It covers tep->last_event cache,
more library's thread sensitive data will be added with
additional patches.

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
---
 tools/lib/traceevent/Build                |  1 +
 tools/lib/traceevent/event-parse-local.h  | 15 ++++--
 tools/lib/traceevent/event-parse-thread.c | 63 +++++++++++++++++++++++
 tools/lib/traceevent/event-parse.c        | 39 +++++++-------
 4 files changed, 97 insertions(+), 21 deletions(-)
 create mode 100644 tools/lib/traceevent/event-parse-thread.c

diff --git a/tools/lib/traceevent/Build b/tools/lib/traceevent/Build
index c10a937cc85a..a6a1edd25827 100644
--- a/tools/lib/traceevent/Build
+++ b/tools/lib/traceevent/Build
@@ -5,6 +5,7 @@ libtraceevent-y += parse-filter.o
 libtraceevent-y += parse-utils.o
 libtraceevent-y += kbuffer-parse.o
 libtraceevent-y += event-parse-api.o
+libtraceevent-y += event-parse-thread.o
 
 plugin_jbd2-y         += plugin_jbd2.o
 plugin_hrtimer-y      += plugin_hrtimer.o
diff --git a/tools/lib/traceevent/event-parse-local.h b/tools/lib/traceevent/event-parse-local.h
index 35833ee32d6c..8b8feb6c67d2 100644
--- a/tools/lib/traceevent/event-parse-local.h
+++ b/tools/lib/traceevent/event-parse-local.h
@@ -14,6 +14,12 @@ struct func_list;
 struct event_handler;
 struct func_resolver;
 
+/* cache */
+struct tep_thread_data {
+	struct tep_handle *tep;
+	struct tep_event *last_event;
+};
+
 struct tep_handle {
 	int ref_count;
 
@@ -83,9 +89,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 +99,10 @@ 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 *get_thread_local_event_by_id(struct tep_handle *tep, int id);
+struct tep_event *
+get_thread_local_event_by_name(struct tep_handle *tep, const char *name, const char *sys);
+void set_thread_local_event_cache(struct tep_handle *tep, struct tep_event *event);
+
 #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..f84635030845
--- /dev/null
+++ b/tools/lib/traceevent/event-parse-thread.c
@@ -0,0 +1,63 @@
+// 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;
+
+/**
+ * get_thread_local_event_by_id - Find event with given id in the cache
+ * @tep: a handle to the tep context
+ * @id: event's id
+ *
+ * Searches in the cache for event with given id
+ */
+struct tep_event *get_thread_local_event_by_id(struct tep_handle *tep, int id)
+{
+
+	if (tep_thread_local.last_event && tep_thread_local.tep == tep &&
+	    tep_thread_local.last_event->id == id)
+		return tep_thread_local.last_event;
+
+	return NULL;
+}
+
+/**
+ * get_thread_local_event_by_name - Find event with given name and sys in the cache
+ * @tep: a handle to the tep context
+ * @name: event's name
+ * @sys: event's system
+ *
+ * Searches in the cache for event with given name and system
+ */
+struct tep_event *
+get_thread_local_event_by_name(struct tep_handle *tep, const char *name, const char *sys)
+{
+	if (tep_thread_local.last_event &&
+	    tep_thread_local.tep == tep &&
+	    strcmp(tep_thread_local.last_event->name, name) == 0 &&
+	    (!sys || strcmp(tep_thread_local.last_event->system, sys) == 0))
+		return tep_thread_local.last_event;
+
+	return NULL;
+}
+
+/**
+ * set_thread_local_event_cache - set last used event in the cache
+ * @tep: a handle to the tep context
+ * @event: pointer to the last used event
+ *
+ * Sets last used event in the cache, to speed up the next searches
+ */
+void set_thread_local_event_cache(struct tep_handle *tep, struct tep_event *event)
+{
+	tep_thread_local.tep = tep;
+	tep_thread_local.last_event = event;
+}
+
+
diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 70f04b02c8c5..8f19dcdd06ec 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -3514,28 +3514,29 @@ static int events_id_cmp(const void *a, const void *b);
 
 /**
  * tep_find_event - find an event by given id
- * @pevent: a handle to the pevent
+ * @tep: a handle to the tep context
  * @id: the id of the event
  *
  * Returns an event that has a given @id.
  */
-struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
+struct tep_event *tep_find_event(struct tep_handle *tep, int id)
 {
 	struct tep_event **eventptr;
 	struct tep_event key;
 	struct tep_event *pkey = &key;
+	struct tep_event *event_cache = get_thread_local_event_by_id(tep, id);
 
 	/* Check cache first */
-	if (pevent->last_event && pevent->last_event->id == id)
-		return pevent->last_event;
+	if (event_cache)
+		return event_cache;
 
 	key.id = id;
 
-	eventptr = bsearch(&pkey, pevent->events, pevent->nr_events,
-			   sizeof(*pevent->events), events_id_cmp);
+	eventptr = bsearch(&pkey, tep->events, tep->nr_events,
+			   sizeof(*tep->events), events_id_cmp);
 
 	if (eventptr) {
-		pevent->last_event = *eventptr;
+		set_thread_local_event_cache(tep, *eventptr);
 		return *eventptr;
 	}
 
@@ -3544,7 +3545,7 @@ struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
 
 /**
  * tep_find_event_by_name - find an event by given name
- * @pevent: a handle to the pevent
+ * @tep: a handle to the tep context
  * @sys: the system name to search for
  * @name: the name of the event to search for
  *
@@ -3552,19 +3553,19 @@ struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
  * @sys. If @sys is NULL the first event with @name is returned.
  */
 struct tep_event *
-tep_find_event_by_name(struct tep_handle *pevent,
+tep_find_event_by_name(struct tep_handle *tep,
 		       const char *sys, const char *name)
 {
-	struct tep_event *event = NULL;
 	int i;
+	struct tep_event *event = NULL;
+	struct tep_event *event_cache = get_thread_local_event_by_name(tep, name, sys);
 
-	if (pevent->last_event &&
-	    strcmp(pevent->last_event->name, name) == 0 &&
-	    (!sys || strcmp(pevent->last_event->system, sys) == 0))
-		return pevent->last_event;
+	/* Check cache first */
+	if (event_cache)
+		return event_cache;
 
-	for (i = 0; i < pevent->nr_events; i++) {
-		event = pevent->events[i];
+	for (i = 0; i < tep->nr_events; i++) {
+		event = tep->events[i];
 		if (strcmp(event->name, name) == 0) {
 			if (!sys)
 				break;
@@ -3572,10 +3573,12 @@ tep_find_event_by_name(struct tep_handle *pevent,
 				break;
 		}
 	}
-	if (i == pevent->nr_events)
+
+	if (i == tep->nr_events)
 		event = NULL;
+	if (event)
+		set_thread_local_event_cache(tep, event);
 
-	pevent->last_event = event;
 	return event;
 }
 
-- 
2.19.1

[PATCH 2/2] tools/lib/traceevent: make few libtraceevent internal variables to be per thread

[Tzvetomir Stoyanov]

This patch continues the effort to transform libtraceevent
into a thread safe library. It converts input_buf, input_buf_ptr
and input_buf_siz internal variables to be thread specific. This
buffer is not related to a specific tep context. It is used
internally by the library during the parsing of various strings.

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
 tools/lib/traceevent/event-parse.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 8f19dcdd06ec..f6c926467fa3 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -42,9 +42,9 @@
 #include "event-utils.h"
 #include "trace-seq.h"
 
-static const char *input_buf;
-static unsigned long long input_buf_ptr;
-static unsigned long long input_buf_siz;
+static __thread const char *input_buf;
+static __thread unsigned long long input_buf_ptr;
+static __thread unsigned long long input_buf_siz;
 
 static int is_flag_field;
 static int is_symbolic_field;
-- 
2.19.1

Re: [PATCH 0/2] convert traceevent into a thread safe library

[Steven Rostedt]

On Fri, 30 Nov 2018 13:13:06 +0000
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:

> This short patch series begins tranformation of libtraceevent into a
> thread safe library. It implements per thread local storage for 
> tep->last_event cache, and converts input_buf, input_buf_ptr
> and input_buf_siz internal variables to be thread specific.
> 

I've been thinking about this some. And we should change our
development model. We should be pushing these changes into the
trace-cmd tree first.

The reason I wanted to send to Linux tools/lib/traceevent was because
that would open it up to more people in the review stage. But
unfortunately, it leaves us vulnerable to having bugs and unexpected
consequences of these changes.

I think the renames and man pages were fine to go to Linux first. But
now that we are starting to change the way things work, that should go
into trace-cmd first, so that we can have a bit more thorough testing.

Let's get all the patches that have been sent upstream (including my
last patchset I sent to Arnaldo) backported into trace-cmd, and start
doing updates to the trace-cmd repo first. This way we can make sure
the code is tested before we send it to Arnaldo, because I believe they
think we are testing these patches before we send them, and honestly,
we are not.

Sound good?

Note, when writing the patches to trace-cmd, lets continue with the
naming convention "tools/lib/traceevent" just as if we were to be
sending them to Linux.

-- Steve


> Tzvetomir Stoyanov (2):
>   tools/lib/traceevent: make libtraceevent thread safe
>   tools/lib/traceevent: make few libtraceevent internal variables to be
>     per thread
> 
>  tools/lib/traceevent/Build                |  1 +
>  tools/lib/traceevent/event-parse-local.h  | 15 ++++--
>  tools/lib/traceevent/event-parse-thread.c | 63 +++++++++++++++++++++++
>  tools/lib/traceevent/event-parse.c        | 45 ++++++++--------
>  4 files changed, 100 insertions(+), 24 deletions(-)
>  create mode 100644 tools/lib/traceevent/event-parse-thread.c
> 

[PATCH 2/2] tools/lib/traceevent: make few libtraceevent internal variables to be per thread

[Tzvetomir Stoyanov]

This patch continues the effort to transform libtraceevent
into a thread safe library. It converts input_buf, input_buf_ptr
and input_buf_siz internal variables to be thread specific. This
buffer is not related to a specific tep context. It is used
internally by the library during the parsing of various strings.

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
 lib/traceevent/event-parse.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/traceevent/event-parse.c b/lib/traceevent/event-parse.c
index cacdcc4..1cab1a5 100644
--- a/lib/traceevent/event-parse.c
+++ b/lib/traceevent/event-parse.c
@@ -28,9 +28,9 @@
 #include "event-utils.h"
 #include "trace-seq.h"
 
-static const char *input_buf;
-static unsigned long long input_buf_ptr;
-static unsigned long long input_buf_siz;
+static __thread const char *input_buf;
+static __thread unsigned long long input_buf_ptr;
+static __thread unsigned long long input_buf_siz;
 
 static int is_flag_field;
 static int is_symbolic_field;
-- 
2.19.2