Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582438 > unrolled thread

[RFC] perf/sdt: Directly record SDT event with 'perf record'

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC] perf/sdt: Directly record SDT event with 'perf record' Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2017-02-16 11:20 +0100
    Re: [RFC] perf/sdt: Directly record SDT event with 'perf record' Ingo Molnar <mingo@kernel.org> - 2017-02-20 08:10 +0100
      Re: [RFC] perf/sdt: Directly record SDT event with 'perf record' Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2017-02-20 09:30 +0100
        Re: [RFC] perf/sdt: Directly record SDT event with 'perf record' Ingo Molnar <mingo@kernel.org> - 2017-02-20 10:00 +0100
          Re: [RFC] perf/sdt: Directly record SDT event with 'perf record' Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2017-02-20 14:20 +0100
            Re: [RFC] perf/sdt: Directly record SDT event with 'perf record' Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-20 15:20 +0100
              Re: [RFC] perf/sdt: Directly record SDT event with 'perf record' Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> - 2017-02-23 09:20 +0100
                Re: [RFC] perf/sdt: Directly record SDT event with 'perf record' Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-23 13:50 +0100

#1582438 — [RFC] perf/sdt: Directly record SDT event with 'perf record'

All events from 'perf list', except SDT events, can be directly recorded
with 'perf record'. But, the flow is little different for SDT events.
Probe point for SDT event needs to be created using 'perf probe' before
recording it using 'perf record'.

As suggested by Ingo[1], it's better to make this process simple by
creating probe points automatically with 'perf record' for SDT events.

This patch disables 'perf probe' on SDT events to simplify usage. It
enables recording SDT event only with 'perf record'.

This removes all those 'multiple events with same name' issues by not
allowing manual probe creation to user. When there are multiple events
with same name, 'perf record' will record all of them (in line with
other tools supporting SDT (systemtap)).

I know 'perf probe' for SDT events has already became interface and
people are using it. But, doing this change will make user interface very
easy and also it will make tool behaviour consistent. Also, it won't
require any changes in uprobe_events structure (suggested by Masami[2]).

After patch:

  $ perf list
      ...
      sdt_libpthread:mutex_entry                         [SDT event]
      ...

  $ perf probe -x /usr/lib64/libpthread-2.24.so %sdt_libpthread:mutex_entry
    SDT events don't need to be put in place using 'perf probe' anymore.
    You can directly record on SDT events using 'perf record'
      Error: Command Parse Error.

  $ perf record -a -e %sdt_libpthread:mutex_entry
    Warning : Recording on 2 occurences of sdt_libpthread:mutex_entry
    ^C[ perf record: Woken up 1 times to write data ]
    [ perf record: Captured and wrote 0.190 MB perf.data (33 samples) ]

  $ perf evlist
    sdt_libpthread:mutex_entry
    sdt_libpthread:mutex_entry_1


Note:
  - This is no way a proper patch. Sending this RFC to get thoughts on the
    idea.
  - This is Hemant's patch[3] rebased to acme/perf/core, with few changes
    to disable 'perf probe' on SDT event.


[1] https://lkml.org/lkml/2017/2/7/59
[2] https://lkml.org/lkml/2016/4/30/50
[3] https://lkml.org/lkml/2016/5/3/810




Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/builtin-record.c    | 21 +++++++++
 tools/perf/perf.h              |  1 +
 tools/perf/util/parse-events.c | 53 ++++++++++++++++++++++-
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/probe-event.c  | 43 ++++++++++++++++++-
 tools/perf/util/probe-event.h  |  4 ++
 tools/perf/util/probe-file.c   | 97 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/probe-file.h   |  8 ++++
 8 files changed, 225 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2ddf189..37722f0 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -39,6 +39,7 @@
 #include "util/trigger.h"
 #include "util/perf-hooks.h"
 #include "asm/bug.h"
+#include "util/probe-file.h"
 
 #include <unistd.h>
 #include <sched.h>
@@ -73,6 +74,7 @@ struct record {
 	bool			timestamp_filename;
 	struct switch_output	switch_output;
 	unsigned long long	samples;
+	struct list_head	sdt_event_list;
 };
 
 static volatile int auxtrace_record__snapshot_started;
@@ -1502,6 +1504,23 @@ static struct record record = {
 	},
 };
 
+void sdt_event_list__add(struct list_head *sdt_event_list)
+{
+	if (list_empty(sdt_event_list))
+		return;
+	list_splice(sdt_event_list, &record.sdt_event_list);
+}
+
+bool is_cmd_record(void)
+{
+	return (record.evlist != NULL);
+}
+
+static void sdt_event_list__remove(struct list_head *sdt_event_list)
+{
+	return remove_sdt_event_list(sdt_event_list);
+}
+
 const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
 	"\n\t\t\t\tDefault: fp";
 
@@ -1670,6 +1689,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (rec->evlist == NULL)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&rec->sdt_event_list);
 	err = perf_config(perf_record_config, rec);
 	if (err)
 		return err;
@@ -1836,6 +1856,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	perf_evlist__delete(rec->evlist);
 	symbol__exit();
 	auxtrace_record__free(rec->itr);
+	sdt_event_list__remove(&rec->sdt_event_list);
 	return err;
 }
 
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 1c27d94..9d8e5fe 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -76,4 +76,5 @@ struct record_opts {
 struct option;
 extern const char * const *record_usage;
 extern struct option *record_options;
+bool is_cmd_record(void);
 #endif
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 07be076..3400a5a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1726,13 +1726,64 @@ static void parse_events_print_error(struct parse_events_error *err,
 
 #undef MAX_WIDTH
 
+static int parse_sdt_event(const char *str, struct list_head **sdt_list)
+{
+	char *ptr = NULL;
+	int ret;
+	struct list_head *sdt_evlist;
+
+	ptr = strdup(str);
+	if (ptr == NULL)
+		return -ENOMEM;
+
+	sdt_evlist = zalloc(sizeof(*sdt_evlist));
+	if (!sdt_evlist) {
+		free(ptr);
+		pr_err("Error in sdt_evlist memory allocation\n");
+		return -ENOMEM;
+	}
+	INIT_LIST_HEAD(sdt_evlist);
+
+	/*
+	 * If there is an error in this call, no need to free
+	 * up sdt_evlist, its already free'ed up in the previous
+	 * call. Free up 'ptr' though.
+	 */
+	ret = add_sdt_event(ptr, sdt_evlist);
+
+	free(ptr);
+	if (!ret)
+		*sdt_list = sdt_evlist;
+
+	return ret;
+}
+
 int parse_events_option(const struct option *opt, const char *str,
 			int unset __maybe_unused)
 {
 	struct perf_evlist *evlist = *(struct perf_evlist **)opt->value;
 	struct parse_events_error err = { .idx = 0, };
-	int ret = parse_events(evlist, str, &err);
+	int ret = 0;
+	struct list_head *sdt_list = NULL;
+	struct sdt_event_list *event;
+
+	if (*str == '%' && is_cmd_record()) {
+		ret = parse_sdt_event(str, &sdt_list);
+		if (!ret) {
+			list_for_each_entry(event, sdt_list, list) {
+				ret = parse_events(evlist, event->event_info,
+						   &err);
+				if (ret < 0)
+					goto error;
+			}
+			/* Add it to the record struct */
+			sdt_event_list__add(sdt_list);
+		}
+	} else {
+		ret = parse_events(evlist, str, &err);
+	}
 
+error:
 	if (ret)
 		parse_events_print_error(&err, str);
 
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index da246a3..7dbeb7a 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -194,4 +194,5 @@ int is_valid_tracepoint(const char *event_string);
 int valid_event_mount(const char *eventfs);
 char *parse_events_formats_error_string(char *additional_terms);
 
+void sdt_event_list__add(struct list_head *sdt_event_list);
 #endif /* __PERF_PARSE_EVENTS_H */
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 2c1bca2..5bd5568 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1293,7 +1293,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
 	return err;
 }
 
-static int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev)
+int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev)
 {
 	char *ptr;
 
@@ -1347,7 +1347,8 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
 	if (arg[0] == '%' ||
 	    (!strncmp(arg, "sdt_", 4) &&
 	     !!strchr(arg, ':') && !strchr(arg, '='))) {
-		pev->sdt = true;
+		pr_err("SDT events don't need to be put in place using 'perf probe' anymore.\nYou can directly record on SDT events using 'perf record'\n");
+		return -EINVAL;
 		if (arg[0] == '%')
 			arg++;
 	}
@@ -3173,6 +3174,12 @@ static int find_cached_events_all(struct perf_probe_event *pev,
 	return ret;
 }
 
+int find_sdt_events_from_cache(struct perf_probe_event *pev,
+			       struct probe_trace_event **tevs)
+{
+	return find_cached_events_all(pev, tevs);
+}
+
 static int find_probe_trace_events_from_cache(struct perf_probe_event *pev,
 					      struct probe_trace_event **tevs)
 {
@@ -3483,3 +3490,35 @@ int copy_to_probe_trace_arg(struct probe_trace_arg *tvar,
 		tvar->name = NULL;
 	return 0;
 }
+
+/*
+ * Record session for SDT events has ended. Delete the SDT events
+ * from uprobe_events file that were created initially.
+ */
+void remove_sdt_event_list(struct list_head *sdt_events)
+{
+	struct sdt_event_list *event;
+	struct strfilter *filter = NULL;
+	const char *err = NULL;
+	int ret = 0;
+
+	if (list_empty(sdt_events))
+		return;
+
+	list_for_each_entry(event, sdt_events, list) {
+		if (!filter) {
+			filter = strfilter__new(event->event_info, &err);
+			if (!filter)
+				goto free_list;
+		} else {
+			ret = strfilter__or(filter, event->event_info, &err);
+		}
+	}
+
+	ret = del_perf_probe_events(filter);
+	if (ret)
+		pr_err("Error in deleting the SDT list\n");
+
+free_list:
+	free_sdt_list(sdt_events);
+}
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index 5d4e940..5ec648e 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -182,4 +182,8 @@ struct map *get_target_map(const char *target, bool user);
 void arch__post_process_probe_trace_events(struct perf_probe_event *pev,
 					   int ntevs);
 
+int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev);
+
+int find_sdt_events_from_cache(struct perf_probe_event *pev,
+			       struct probe_trace_event **tevs);
 #endif /*_PROBE_EVENT_H */
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 436b647..2a6ad02 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -27,8 +27,10 @@
 #include "probe-event.h"
 #include "probe-file.h"
 #include "session.h"
+#include "probe-finder.h"
 
 #define MAX_CMDLEN 256
+#define MAX_EVENT_LENGTH 512
 
 static void print_open_warning(int err, bool uprobe)
 {
@@ -933,3 +935,98 @@ bool probe_type_is_available(enum probe_type type)
 
 	return ret;
 }
+
+void free_sdt_list(struct list_head *sdt_events)
+{
+	struct sdt_event_list *tmp, *ptr;
+
+	if (list_empty(sdt_events))
+		return;
+	list_for_each_entry_safe(tmp, ptr, sdt_events, list) {
+		list_del(&tmp->list);
+		free(tmp->event_info);
+		free(tmp);
+	}
+}
+
+/*
+ * Find the SDT event from the cache and if found add it/them
+ * to the uprobe_events file
+ */
+int add_sdt_event(char *event, struct list_head *sdt_events)
+{
+	struct perf_probe_event *pev;
+	int ret, i;
+	char *str = event + 1;
+	struct sdt_event_list *tmp;
+
+	pev = zalloc(sizeof(*pev));
+	if (!pev)
+		return -ENOMEM;
+
+	pev->sdt = true;
+	pev->uprobes = true;
+
+	/*
+	 * Parse str to find the group name and event name of
+	 * the sdt event.
+	 */
+	ret = parse_perf_probe_event_name(&str, pev);
+	if (ret) {
+		pr_err("Error in parsing sdt event %s\n", str);
+		free(pev);
+		return ret;
+	}
+
+	probe_conf.max_probes = MAX_PROBES;
+	probe_conf.force_add = 1;
+
+	/*
+	 * Find the sdt event from the cache, only cached SDT
+	 * events can be directly recorded.
+	 */
+	pev->ntevs = find_sdt_events_from_cache(pev, &pev->tevs);
+	if (pev->ntevs) {
+		if (pev->ntevs > 1) {
+			pr_warning("Warning : Recording on %d occurences of %s:%s\n",
+				   pev->ntevs, pev->group, pev->event);
+		}
+		ret = apply_perf_probe_events(pev, 1);
+		if (ret) {
+			pr_err("Error in adding SDT event : %s\n", event);
+			goto free_pev;
+		}
+	} else {
+		pr_err(" %s:%s not found in the cache\n", pev->group,
+			pev->event);
+		ret = -EINVAL;
+		goto free_pev;
+	}
+
+	/* Add the event name to "sdt_events" list */
+	for (i = 0; i < pev->ntevs; i++) {
+		tmp = zalloc(sizeof(*tmp));
+		if (!tmp) {
+			ret = -ENOMEM;
+			goto free_pev;
+		}
+
+		INIT_LIST_HEAD(&tmp->list);
+		tmp->event_info = zalloc(MAX_EVENT_LENGTH * sizeof(char));
+		if (!tmp->event_info) {
+			free_sdt_list(sdt_events);
+			ret = -ENOMEM;
+			goto free_pev;
+		}
+		snprintf(tmp->event_info, strlen(pev->tevs[i].group) +
+			 strlen(pev->tevs[i].event) + 2, "%s:%s",
+			 pev->tevs[i].group, pev->tevs[i].event);
+		list_add(&tmp->list, sdt_events);
+	}
+
+	ret = 0;
+
+free_pev:
+	cleanup_perf_probe_events(pev, 1);
+	return ret;
+}
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index eba44c3..3b39681 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -19,6 +19,11 @@ struct probe_cache {
 	struct list_head entries;
 };
 
+struct sdt_event_list {
+	char *event_info;
+	struct list_head list;
+};
+
 enum probe_type {
 	PROBE_TYPE_U = 0,
 	PROBE_TYPE_S,
@@ -64,6 +69,9 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
 					const char *group, const char *event);
 int probe_cache__show_all_caches(struct strfilter *filter);
 bool probe_type_is_available(enum probe_type type);
+int add_sdt_event(char *event, struct list_head *sdt_event_list);
+void remove_sdt_event_list(struct list_head *sdt_event_list);
+void free_sdt_list(struct list_head *sdt_events);
 #else	/* ! HAVE_LIBELF_SUPPORT */
 static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
 {
-- 
2.9.3

[toc] | [next] | [standalone]

#1584369

* Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> All events from 'perf list', except SDT events, can be directly recorded
> with 'perf record'. But, the flow is little different for SDT events.
> Probe point for SDT event needs to be created using 'perf probe' before
> recording it using 'perf record'.
> 
> As suggested by Ingo[1], it's better to make this process simple by
> creating probe points automatically with 'perf record' for SDT events.
> 
> This patch disables 'perf probe' on SDT events to simplify usage. It
> enables recording SDT event only with 'perf record'.
> 
> This removes all those 'multiple events with same name' issues by not
> allowing manual probe creation to user. When there are multiple events
> with same name, 'perf record' will record all of them (in line with
> other tools supporting SDT (systemtap)).
> 
> I know 'perf probe' for SDT events has already became interface and
> people are using it. But, doing this change will make user interface very
> easy and also it will make tool behaviour consistent. Also, it won't
> require any changes in uprobe_events structure (suggested by Masami[2]).

So I like the automatism you implemented for 'perf record', but why not keep the 
'perf probe' flow as well, if people got used to it?

It's not like computer software is bad at sorting apart and handling the two cases 
properly, right?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1584401

On Monday 20 February 2017 12:38 PM, Ingo Molnar wrote:
> * Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> All events from 'perf list', except SDT events, can be directly recorded
>> with 'perf record'. But, the flow is little different for SDT events.
>> Probe point for SDT event needs to be created using 'perf probe' before
>> recording it using 'perf record'.
>>
>> As suggested by Ingo[1], it's better to make this process simple by
>> creating probe points automatically with 'perf record' for SDT events.
>>
>> This patch disables 'perf probe' on SDT events to simplify usage. It
>> enables recording SDT event only with 'perf record'.
>>
>> This removes all those 'multiple events with same name' issues by not
>> allowing manual probe creation to user. When there are multiple events
>> with same name, 'perf record' will record all of them (in line with
>> other tools supporting SDT (systemtap)).
>>
>> I know 'perf probe' for SDT events has already became interface and
>> people are using it. But, doing this change will make user interface very
>> easy and also it will make tool behaviour consistent. Also, it won't
>> require any changes in uprobe_events structure (suggested by Masami[2]).
> So I like the automatism you implemented for 'perf record', but why not keep the 
> 'perf probe' flow as well, if people got used to it?
>
> It's not like computer software is bad at sorting apart and handling the two cases 
> properly, right?

Thanks Ingo for the reply,

Yes, initially I thought about allowing both, 'perf probe' and
'perf record' for SDT event. But there are few complications with
it, esp. when multiple SDT events with same name exists. For ex,

  $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider
      Provider: libpthread
      Name: mutex_entry
      Location: 0x0000000000009ddb, ...
    --
      Provider: libpthread
      Name: mutex_entry
      Location: 0x000000000000bcbb, ...

At the time of record, perf has to check first if there is any
matching entry exists in uprobe_events with that name. If found,
record it, if not, go look into probe cache. If events exists with
same name in probe cache, record all of them. Like,

If probe point _is not_ created,
  $ perf record -a -e sdt_libpthread:mutex_entry
    /** Record both sdt_libpthread:mutex_entry **/

If probe point _is_ created manually, record that particular event,
  $ perf probe -x /usr/lib64/libpthread-2.24.so sdt_libpthread:mutex_entry
    Added new events:
      sdt_libpthread:mutex_entry   (on %mutex_entry in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_entry_1 (on %mutex_entry in /usr/lib64/libpthread-2.24.so)

  $ perf record -a -e sdt_libpthread:mutex_entry
    /** Record only first sdt_libpthread:mutex_entry **/

Here, same command gives different behaviour for different scenarios.

Now consider a scenario when probe point exists for any one event:

  $ perf probe -d sdt_libpthread:mutex_entry_1
  $ perf probe --list
    sdt_libpthread:mutex_entry (on pthread_mutex_lock+11 in /usr/lib64/libpthread-2.24.so)

And user tries to record it by,
  $ perf record -a -e sdt_libpthread:*

What should be the behavior of the tool? Should it record only one
'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it
should record all the SDT events from libpthread? We can choose either
of two but both the cases are ambiguous.

Not allowing 'perf probe' for SDT event will solve all such issues.
Also it will make user interface simple and consistent. Other current
tooling (systemtap, for instance) also do not allow probing individual
markers when there are multiple markers with the same name.

-Ravi

[toc] | [prev] | [next] | [standalone]

#1584419

* Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Yes, initially I thought about allowing both, 'perf probe' and
> 'perf record' for SDT event. But there are few complications with
> it, esp. when multiple SDT events with same name exists. For ex,
> 
>   $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider
>       Provider: libpthread
>       Name: mutex_entry
>       Location: 0x0000000000009ddb, ...
>     --
>       Provider: libpthread
>       Name: mutex_entry
>       Location: 0x000000000000bcbb, ...
> 
> At the time of record, perf has to check first if there is any
> matching entry exists in uprobe_events with that name. If found,
> record it, if not, go look into probe cache. If events exists with
> same name in probe cache, record all of them. Like,
> 
> If probe point _is not_ created,
>   $ perf record -a -e sdt_libpthread:mutex_entry
>     /** Record both sdt_libpthread:mutex_entry **/
> 
> If probe point _is_ created manually, record that particular event,
>   $ perf probe -x /usr/lib64/libpthread-2.24.so sdt_libpthread:mutex_entry
>     Added new events:
>       sdt_libpthread:mutex_entry   (on %mutex_entry in /usr/lib64/libpthread-2.24.so)
>       sdt_libpthread:mutex_entry_1 (on %mutex_entry in /usr/lib64/libpthread-2.24.so)
> 
>   $ perf record -a -e sdt_libpthread:mutex_entry
>     /** Record only first sdt_libpthread:mutex_entry **/
> 
> Here, same command gives different behaviour for different scenarios.
> 
> Now consider a scenario when probe point exists for any one event:
> 
>   $ perf probe -d sdt_libpthread:mutex_entry_1
>   $ perf probe --list
>     sdt_libpthread:mutex_entry (on pthread_mutex_lock+11 in /usr/lib64/libpthread-2.24.so)
> 
> And user tries to record it by,
>   $ perf record -a -e sdt_libpthread:*
> 
> What should be the behavior of the tool? Should it record only one
> 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it
> should record all the SDT events from libpthread? We can choose either
> of two but both the cases are ambiguous.

They are not ambiguous really if coded right: just pick one of the outcomes and 
maybe print a warning to inform the user that something weird is going on because 
not all markers are enabled?

As a user I'd expect 'perf record' to enable all markers and print a warning that 
the markers were in a partial state. This would result in consistent behaviour.

Does it make sense to only enable some of the markers that alias on the same name? 
If not then maybe disallow that in perf probe - or change perf probe to do the 
same thing as perf record.

I.e. this is IMHO an artificial problem that users should not be exposed to and 
which can be solved by tooling.

In particular if it's possible to enable only a part of the markers then perf 
record not continuing would be a failure mode: if for example a previous perf 
record session segfaulted (or ran out of RAM or was killed in the wrong moment or 
whatever) then it would not be possible to (easily) clean up the mess.

> Not allowing 'perf probe' for SDT event will solve all such issues.
> Also it will make user interface simple and consistent. Other current
> tooling (systemtap, for instance) also do not allow probing individual
> markers when there are multiple markers with the same name.

In any case if others agree with your change in UI flow too then it's fine by me, 
but please make it robust, i.e. if perf record sees partially enabled probes it 
should still continue.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1584609

Thanks Ingo,

On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote:
> * Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> What should be the behavior of the tool? Should it record only one
>> 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it
>> should record all the SDT events from libpthread? We can choose either
>> of two but both the cases are ambiguous.
> They are not ambiguous really if coded right: just pick one of the outcomes and 
> maybe print a warning to inform the user that something weird is going on because 
> not all markers are enabled?
>
> As a user I'd expect 'perf record' to enable all markers and print a warning that 
> the markers were in a partial state. This would result in consistent behaviour.

Yes, makes sense.

> Does it make sense to only enable some of the markers that alias on the same name? 
> If not then maybe disallow that in perf probe - or change perf probe to do the 
> same thing as perf record.

'perf probe' is doing that correctly. It fetches all events with given name from
probe-cache and creates entries for them in uprobe_events.

The problem is the 2-step process of adding probes and then recording,
allowing users to select individual markers to record on.

>
> I.e. this is IMHO an artificial problem that users should not be exposed to and 
> which can be solved by tooling.
>
> In particular if it's possible to enable only a part of the markers then perf 
> record not continuing would be a failure mode: if for example a previous perf 
> record session segfaulted (or ran out of RAM or was killed in the wrong moment or 
> whatever) then it would not be possible to (easily) clean up the mess.

Agreed. We need to make this more robust.

>
>> Not allowing 'perf probe' for SDT event will solve all such issues.
>> Also it will make user interface simple and consistent. Other current
>> tooling (systemtap, for instance) also do not allow probing individual
>> markers when there are multiple markers with the same name.
> In any case if others agree with your change in UI flow too then it's fine by me, 
> but please make it robust, i.e. if perf record sees partially enabled probes it 
> should still continue.

@Masami, can you please provide your thoughts as well.

Thanks,
Ravi

[toc] | [prev] | [next] | [standalone]

#1584640

Em Mon, Feb 20, 2017 at 04:31:50PM +0530, Ravi Bangoria escreveu:
> Thanks Ingo,
> 
> On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote:
> > * Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> >
> >> What should be the behavior of the tool? Should it record only one
> >> 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it
> >> should record all the SDT events from libpthread? We can choose either
> >> of two but both the cases are ambiguous.
> > They are not ambiguous really if coded right: just pick one of the outcomes and 
> > maybe print a warning to inform the user that something weird is going on because 
> > not all markers are enabled?
> >
> > As a user I'd expect 'perf record' to enable all markers and print a warning that 
> > the markers were in a partial state. This would result in consistent behaviour.
> 
> Yes, makes sense.
> 
> > Does it make sense to only enable some of the markers that alias on the same name? 
> > If not then maybe disallow that in perf probe - or change perf probe to do the 
> > same thing as perf record.
> 
> 'perf probe' is doing that correctly. It fetches all events with given name from
> probe-cache and creates entries for them in uprobe_events.
> 
> The problem is the 2-step process of adding probes and then recording,
> allowing users to select individual markers to record on.

So, the more streamlined one works for most people, i.e. just use perf
record, no need to perf probe anything. But, for people who "know what
they are doing", perf probe can be used first to control exactly which
SDT probes one wants in place, and then those will be used.

We need to make sure that when processing the file there is information
that says which probes were in place and enabled in the record session,
tho. Is that possible?
 
> > I.e. this is IMHO an artificial problem that users should not be exposed to and 
> > which can be solved by tooling.
> >
> > In particular if it's possible to enable only a part of the markers then perf 
> > record not continuing would be a failure mode: if for example a previous perf 
> > record session segfaulted (or ran out of RAM or was killed in the wrong moment or 
> > whatever) then it would not be possible to (easily) clean up the mess.
 
> Agreed. We need to make this more robust.

Right, disambiguating a 'probes left by a session that did auto-probing'
from a 'hey, those probes are there intentionally, just use those' is
important.
 
> >> Not allowing 'perf probe' for SDT event will solve all such issues.
> >> Also it will make user interface simple and consistent. Other current
> >> tooling (systemtap, for instance) also do not allow probing individual
> >> markers when there are multiple markers with the same name.

> > In any case if others agree with your change in UI flow too then it's fine by me, 
> > but please make it robust, i.e. if perf record sees partially enabled probes it 
> > should still continue.
 
> @Masami, can you please provide your thoughts as well.
 
Yeah, if technically possible to allow both variants, we should leave
it up to users to decide what is best?

I.e. most people will do auto-probing, not using 'perf probe' at all,
documentation should state the pitfalls in doing so.

So, after writing the above, perhaps we should warn the user that
pre-existing probes are being used, as this will be the odd case?

The normal flow will be just using perf record with SDT probes, that
will auto-probe them and remove on exit, or better drop a reference to
them, as simultaneous use also needs to be covered?

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1586715

Thanks Arnaldo,

I'm working on this but it's taking bit longer time. Will post out a patch within
few days.

Ravi

On Monday 20 February 2017 07:41 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 20, 2017 at 04:31:50PM +0530, Ravi Bangoria escreveu:
>> Thanks Ingo,
>>
>> On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote:
>>> * Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>>
>>>> What should be the behavior of the tool? Should it record only one
>>>> 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it
>>>> should record all the SDT events from libpthread? We can choose either
>>>> of two but both the cases are ambiguous.
>>> They are not ambiguous really if coded right: just pick one of the outcomes and 
>>> maybe print a warning to inform the user that something weird is going on because 
>>> not all markers are enabled?
>>>
>>> As a user I'd expect 'perf record' to enable all markers and print a warning that 
>>> the markers were in a partial state. This would result in consistent behaviour.
>> Yes, makes sense.
>>
>>> Does it make sense to only enable some of the markers that alias on the same name? 
>>> If not then maybe disallow that in perf probe - or change perf probe to do the 
>>> same thing as perf record.
>> 'perf probe' is doing that correctly. It fetches all events with given name from
>> probe-cache and creates entries for them in uprobe_events.
>>
>> The problem is the 2-step process of adding probes and then recording,
>> allowing users to select individual markers to record on.
> So, the more streamlined one works for most people, i.e. just use perf
> record, no need to perf probe anything. But, for people who "know what
> they are doing", perf probe can be used first to control exactly which
> SDT probes one wants in place, and then those will be used.
>
> We need to make sure that when processing the file there is information
> that says which probes were in place and enabled in the record session,
> tho. Is that possible?
>
>>> I.e. this is IMHO an artificial problem that users should not be exposed to and 
>>> which can be solved by tooling.
>>>
>>> In particular if it's possible to enable only a part of the markers then perf 
>>> record not continuing would be a failure mode: if for example a previous perf 
>>> record session segfaulted (or ran out of RAM or was killed in the wrong moment or 
>>> whatever) then it would not be possible to (easily) clean up the mess.
>> Agreed. We need to make this more robust.
> Right, disambiguating a 'probes left by a session that did auto-probing'
> from a 'hey, those probes are there intentionally, just use those' is
> important.
>
>>>> Not allowing 'perf probe' for SDT event will solve all such issues.
>>>> Also it will make user interface simple and consistent. Other current
>>>> tooling (systemtap, for instance) also do not allow probing individual
>>>> markers when there are multiple markers with the same name.
>>> In any case if others agree with your change in UI flow too then it's fine by me, 
>>> but please make it robust, i.e. if perf record sees partially enabled probes it 
>>> should still continue.
>> @Masami, can you please provide your thoughts as well.
> Yeah, if technically possible to allow both variants, we should leave
> it up to users to decide what is best?
>
> I.e. most people will do auto-probing, not using 'perf probe' at all,
> documentation should state the pitfalls in doing so.
>
> So, after writing the above, perhaps we should warn the user that
> pre-existing probes are being used, as this will be the odd case?
>
> The normal flow will be just using perf record with SDT probes, that
> will auto-probe them and remove on exit, or better drop a reference to
> them, as simultaneous use also needs to be covered?
>
> - Arnaldo
>

[toc] | [prev] | [next] | [standalone]

#1586858

Em Thu, Feb 23, 2017 at 01:43:38PM +0530, Ravi Bangoria escreveu:
> Thanks Arnaldo,
> 
> I'm working on this but it's taking bit longer time. Will post out a patch within
> few days.

Take your time and thanks for giving consideration to my observations,

Regards,

- Arnaldo
 
> Ravi
> 
> On Monday 20 February 2017 07:41 PM, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Feb 20, 2017 at 04:31:50PM +0530, Ravi Bangoria escreveu:
> >> Thanks Ingo,
> >>
> >> On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote:
> >>> * Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> >>>
> >>>> What should be the behavior of the tool? Should it record only one
> >>>> 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it
> >>>> should record all the SDT events from libpthread? We can choose either
> >>>> of two but both the cases are ambiguous.
> >>> They are not ambiguous really if coded right: just pick one of the outcomes and 
> >>> maybe print a warning to inform the user that something weird is going on because 
> >>> not all markers are enabled?
> >>>
> >>> As a user I'd expect 'perf record' to enable all markers and print a warning that 
> >>> the markers were in a partial state. This would result in consistent behaviour.
> >> Yes, makes sense.
> >>
> >>> Does it make sense to only enable some of the markers that alias on the same name? 
> >>> If not then maybe disallow that in perf probe - or change perf probe to do the 
> >>> same thing as perf record.
> >> 'perf probe' is doing that correctly. It fetches all events with given name from
> >> probe-cache and creates entries for them in uprobe_events.
> >>
> >> The problem is the 2-step process of adding probes and then recording,
> >> allowing users to select individual markers to record on.
> > So, the more streamlined one works for most people, i.e. just use perf
> > record, no need to perf probe anything. But, for people who "know what
> > they are doing", perf probe can be used first to control exactly which
> > SDT probes one wants in place, and then those will be used.
> >
> > We need to make sure that when processing the file there is information
> > that says which probes were in place and enabled in the record session,
> > tho. Is that possible?
> >
> >>> I.e. this is IMHO an artificial problem that users should not be exposed to and 
> >>> which can be solved by tooling.
> >>>
> >>> In particular if it's possible to enable only a part of the markers then perf 
> >>> record not continuing would be a failure mode: if for example a previous perf 
> >>> record session segfaulted (or ran out of RAM or was killed in the wrong moment or 
> >>> whatever) then it would not be possible to (easily) clean up the mess.
> >> Agreed. We need to make this more robust.
> > Right, disambiguating a 'probes left by a session that did auto-probing'
> > from a 'hey, those probes are there intentionally, just use those' is
> > important.
> >
> >>>> Not allowing 'perf probe' for SDT event will solve all such issues.
> >>>> Also it will make user interface simple and consistent. Other current
> >>>> tooling (systemtap, for instance) also do not allow probing individual
> >>>> markers when there are multiple markers with the same name.
> >>> In any case if others agree with your change in UI flow too then it's fine by me, 
> >>> but please make it robust, i.e. if perf record sees partially enabled probes it 
> >>> should still continue.
> >> @Masami, can you please provide your thoughts as well.
> > Yeah, if technically possible to allow both variants, we should leave
> > it up to users to decide what is best?
> >
> > I.e. most people will do auto-probing, not using 'perf probe' at all,
> > documentation should state the pitfalls in doing so.
> >
> > So, after writing the above, perhaps we should warn the user that
> > pre-existing probes are being used, as this will be the odd case?
> >
> > The normal flow will be just using perf record with SDT probes, that
> > will auto-probe them and remove on exit, or better drop a reference to
> > them, as simultaneous use also needs to be covered?
> >
> > - Arnaldo
> >

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web