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

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

[Tzvetomir Stoyanov]

This patch is a PoC about transforming libtraceevent
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.

[v2 - added local thread data per tep context]

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
 tools/lib/traceevent/event-parse-local.h  | 14 ++++--
 tools/lib/traceevent/event-parse-thread.c | 57 +++++++++++++++++++++++
 tools/lib/traceevent/event-parse.c        | 23 +++++----
 3 files changed, 81 insertions(+), 13 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..6fa0f05b29c3 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,7 @@ 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);
 
+struct tep_thread_data *tep_get_thread_local(struct tep_handle *tep);
+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..39350dc82c5d
--- /dev/null
+++ b/tools/lib/traceevent/event-parse-thread.c
@@ -0,0 +1,57 @@
+// 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;
+
+
+struct tep_thread_data *tep_get_thread_local(struct tep_handle *tep)
+{
+	struct tep_thread_data *local = tep_thread_local;
+
+	while(local) {
+		if (local->tep == tep)
+			return local;
+		local = local->next;
+	}
+
+	local = calloc(1, sizeof(struct tep_thread_data));
+	if(local) {
+		local->tep = tep;
+		local->next = tep_thread_local;
+		tep_thread_local = local;
+	}
+	return local;
+}
+
+
+void tep_destroy_thread_local(struct tep_handle *tep)
+{
+	struct tep_thread_data **local, *del;
+
+	if(!tep_thread_local)
+		return;
+	local = &tep_thread_local;
+
+	while(*local) {
+		if ((*local)->tep == tep) {
+			tep_thread_local = (*local)->next;
+			free(*local);
+			return;
+		}
+		if ((*local)->next && (*local)->next->tep == tep) {
+			del = (*local)->next;
+			(*local)->next = (*local)->next->next;
+			free(del);
+			return;
+		}
+		local = &((*local)->next);
+	}
+}
+
diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index a5048c1b9bec..25f4ceab9a58 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -3485,10 +3485,11 @@ struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
 	struct tep_event **eventptr;
 	struct tep_event key;
 	struct tep_event *pkey = &key;
-
+	struct tep_thread_data *local = tep_get_thread_local(pevent);
 	/* Check cache first */
-	if (pevent->last_event && pevent->last_event->id == id)
-		return pevent->last_event;
+
+	if (local && local->last_event && local->last_event->id == id)
+		return local->last_event;
 
 	key.id = id;
 
@@ -3496,7 +3497,8 @@ struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
 			   sizeof(*pevent->events), events_id_cmp);
 
 	if (eventptr) {
-		pevent->last_event = *eventptr;
+		if (local)
+			local->last_event = *eventptr;
 		return *eventptr;
 	}
 
@@ -3516,13 +3518,14 @@ struct tep_event *
 tep_find_event_by_name(struct tep_handle *pevent,
 		       const char *sys, const char *name)
 {
+	struct tep_thread_data *local = tep_get_thread_local(pevent);
 	struct tep_event *event = NULL;
 	int i;
 
-	if (pevent->last_event &&
-	    strcmp(pevent->last_event->name, name) == 0 &&
-	    (!sys || strcmp(pevent->last_event->system, sys) == 0))
-		return pevent->last_event;
+	if (local && local->last_event &&
+	    strcmp(local->last_event->name, name) == 0 &&
+	    (!sys || strcmp(local->last_event->system, sys) == 0))
+		return local->last_event;
 
 	for (i = 0; i < pevent->nr_events; i++) {
 		event = pevent->events[i];
@@ -3535,8 +3538,8 @@ tep_find_event_by_name(struct tep_handle *pevent,
 	}
 	if (i == pevent->nr_events)
 		event = NULL;
-
-	pevent->last_event = event;
+	if (local)
+		local->last_event = event;
 	return event;
 }
 
-- 
2.19.1

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

[Steven Rostedt]

On Mon, 26 Nov 2018 13:22:59 +0000
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:

> This patch is a PoC about transforming libtraceevent

FYI, when posting a proof of concept, you can add it to the subject:

 [POC][PATCH v2] tools/lib/traceevent: make libtraceevent thread safe

This lets people know that it's not something to be applied.

> 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.
> 
> [v2 - added local thread data per tep context]
> 
> Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> ---
>  tools/lib/traceevent/event-parse-local.h  | 14 ++++--
>  tools/lib/traceevent/event-parse-thread.c | 57 +++++++++++++++++++++++
>  tools/lib/traceevent/event-parse.c        | 23 +++++----
>  3 files changed, 81 insertions(+), 13 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..6fa0f05b29c3 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,7 @@ 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);
>  
> +struct tep_thread_data *tep_get_thread_local(struct tep_handle *tep);
> +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..39350dc82c5d
> --- /dev/null
> +++ b/tools/lib/traceevent/event-parse-thread.c
> @@ -0,0 +1,57 @@
> +// 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;
> +
> +
> +struct tep_thread_data *tep_get_thread_local(struct tep_handle *tep)
> +{
> +	struct tep_thread_data *local = tep_thread_local;
> +
> +	while(local) {

I wouldn't do make it a list. I would either cache the last one, and if
the tep doesn't match, then return NULL (not a match).

> +		if (local->tep == tep)
> +			return local;
> +		local = local->next;
> +	}
> +
> +	local = calloc(1, sizeof(struct tep_thread_data));

Let's create a new function to create it:

tep_alloc_thread_local(tep, event);

> +	if(local) {
> +		local->tep = tep;
> +		local->next = tep_thread_local;
> +		tep_thread_local = local;
> +	}
> +	return local;
> +}
> +
> +
> +void tep_destroy_thread_local(struct tep_handle *tep)
> +{
> +	struct tep_thread_data **local, *del;
> +
> +	if(!tep_thread_local)
> +		return;
> +	local = &tep_thread_local;
> +
> +	while(*local) {
> +		if ((*local)->tep == tep) {
> +			tep_thread_local = (*local)->next;
> +			free(*local);
> +			return;
> +		}
> +		if ((*local)->next && (*local)->next->tep == tep) {
> +			del = (*local)->next;
> +			(*local)->next = (*local)->next->next;
> +			free(del);
> +			return;
> +		}
> +		local = &((*local)->next);
> +	}
> +}
> +
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index a5048c1b9bec..25f4ceab9a58 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -3485,10 +3485,11 @@ struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
>  	struct tep_event **eventptr;
>  	struct tep_event key;
>  	struct tep_event *pkey = &key;
> -
> +	struct tep_thread_data *local = tep_get_thread_local(pevent);

Let's make this:

	local = tep_get_thread_local(pevent, id);

	if (local)
		return local->last_event;


>  	/* Check cache first */
> -	if (pevent->last_event && pevent->last_event->id == id)
> -		return pevent->last_event;
> +
> +	if (local && local->last_event && local->last_event->id == id)
> +		return local->last_event;
>  
>  	key.id = id;
>  
> @@ -3496,7 +3497,8 @@ struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
>  			   sizeof(*pevent->events), events_id_cmp);
>  
>  	if (eventptr) {
> -		pevent->last_event = *eventptr;

		tep_alloc_thread_local(tep, *eventptr);

And have that function do the assignment if it got allocated.

> +		if (local)
> +			local->last_event = *eventptr;
>  		return *eventptr;
>  	}
>  
> @@ -3516,13 +3518,14 @@ struct tep_event *
>  tep_find_event_by_name(struct tep_handle *pevent,
>  		       const char *sys, const char *name)
>  {
> +	struct tep_thread_data *local = tep_get_thread_local(pevent);

Make it: local = tep_get_thread_local_name(pevent, name);

>  	struct tep_event *event = NULL;
>  	int i;
>  

	if (local)
		return local->last_event;

-- Steve

> -	if (pevent->last_event &&
> -	    strcmp(pevent->last_event->name, name) == 0 &&
> -	    (!sys || strcmp(pevent->last_event->system, sys) == 0))
> -		return pevent->last_event;
> +	if (local && local->last_event &&
> +	    strcmp(local->last_event->name, name) == 0 &&
> +	    (!sys || strcmp(local->last_event->system, sys) == 0))
> +		return local->last_event;
>  
>  	for (i = 0; i < pevent->nr_events; i++) {
>  		event = pevent->events[i];
> @@ -3535,8 +3538,8 @@ tep_find_event_by_name(struct tep_handle *pevent,
>  	}
>  	if (i == pevent->nr_events)
>  		event = NULL;
> -
> -	pevent->last_event = event;
> +	if (local)
> +		local->last_event = event;
>  	return event;
>  }
>  

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

[Steven Rostedt]

On Wed, 28 Nov 2018 10:59:18 +0000
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:

> Hi Steven

Hi Tzvetomir,

Can you make sure that your replies have HTML turned off. As it doesn't
seem to do inlined replies well (my responses don't have a ">" in front
of them thus it's hard to know which is your reply or my old one).



> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > index a5048c1b9bec..25f4ceab9a58 100644
> > --- a/tools/lib/traceevent/event-parse.c
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -3485,10 +3485,11 @@ struct tep_event *tep_find_event(struct tep_handle *pevent, int id)
> >       struct tep_event **eventptr;
> >       struct tep_event key;
> >       struct tep_event *pkey = &key;
> > -
> > +     struct tep_thread_data *local = tep_get_thread_local(pevent);  
> 
> Let's make this:
> 
>         local = tep_get_thread_local(pevent, id);
> 
>         if (local)
>                 return local->last_event;
> 
> 

> I got the idea, but have one comment about naming of the new functions.
> My initial idea was tep_get_thread_local() to access all tread specific data, not only
> last_event cache. That's why I named it this way, and make it to not depend on an event.
> As these are internal APIs, we are not going to expose them to the users, we can assume
> that the callers are aware of  "struct tep_thread_data" and can access it directly.
> As I understand,  your idea is to hide "struct tep_thread_data", and to implement APIs to
> access only specific tread local data, as "last_event cache" at the first stage. When we add
> more into "struct tep_thread_data", new APIs will be implemented. I'm ok with this
> approach, but in this case we should name the new APIs to follow the name of thread local item,
> something like this:

>     tep_find_event_by_id_cache(struct tep_handle *, int);
>     tep_find_event_by_name_cache(struct tep_handle *, const char *);
>     tep_set_serach_event_cache(struct tep_handle *, struct tep_event *);
> 

No, I don't think the cached should be visible to the user. That's too
much implementation details. But we can have several thread caches. We
probably want to drop the "tep_" prefix too, as these functions
shouldn't be exposed.

We could have:

  get_thread_local_event_by_id(tep, id);
  get_thread_local_event_by_name(tep, name);

and such.

The point is, the cache is suppose to be a fast access where code might
be accessing the same event over and over. A loop searching for a cache
item defeats that purpose. The last access either matches or it
doesn't. If it is bouncing between multiple tep handlers, then it will
flush the cache of the previous one.

-- Steve