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

Groups > linux.kernel > #1215684 > unrolled thread

[PATCH 03/31] perf tools: Introduce dummy evsel

Started byWang Nan <wangnan0@huawei.com>
First post2015-08-29 06:30 +0200
Last post2015-09-06 08:10 +0200
Articles 6 — 4 participants

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

  [PATCH 03/31] perf tools: Introduce dummy evsel Wang Nan <wangnan0@huawei.com> - 2015-08-29 06:30 +0200
    Re: [PATCH 03/31] perf tools: Introduce dummy evsel Arnaldo Carvalho de Melo <acme@redhat.com> - 2015-08-31 21:40 +0200
    Re: [PATCH 03/31] perf tools: Introduce dummy evsel Namhyung Kim <namhyung@kernel.org> - 2015-09-03 02:30 +0200
      Re: [PATCH 03/31] perf tools: Introduce dummy evsel pi3orama <pi3orama@163.com> - 2015-09-03 02:50 +0200
    [PATCH] perf tools: Allow BPF placeholder dummy events to collect --filter options Wang Nan <wangnan0@huawei.com> - 2015-09-06 08:00 +0200
      [PATCH] perf tools: Sync setting of real bpf events with placeholder Wang Nan <wangnan0@huawei.com> - 2015-09-06 08:10 +0200

#1215684 — [PATCH 03/31] perf tools: Introduce dummy evsel

FromWang Nan <wangnan0@huawei.com>
Date2015-08-29 06:30 +0200
Subject[PATCH 03/31] perf tools: Introduce dummy evsel
Message-ID<q2FWa-7Rj-3@gated-at.bofh.it>
This patch allows linking dummy evsel onto evlist as a placeholder. It
is for following patch which allows passing BPF object using '--event
object.o'.

Doesn't link other event selectors, if passing a BPF object file to
'--event', nothing is linked onto evlist. Instead, events described in
BPF object file are probed and linked in a delayed manner because we
want do all probing work together. Therefore, evsel for events in BPF
object would be linked at the end of evlist. Which causes a small
problem that, if passing '--filter' setting after object file, the
filter option won't be correctly applied to those events.

This patch links dummy onto evlist, so following --filter can be
collected by the dummy evsel. For this reason dummy evsels are set to
PERF_TYPE_TRACEPOINT.

Due to the possibility of existance of dummy evsel,
perf_evlist__purge_dummy() must be called right after parse_options().
This patch adds it to record, top, trace and stat builtin commands.
Further patch moves it down after real BPF events are processed with.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David Ahern <dsahern@gmail.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kaixu Xia <xiakaixu@huawei.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
Link: http://lkml.kernel.org/r/1440742821-44548-4-git-send-email-wangnan0@huawei.com
---
 tools/perf/builtin-record.c    |  2 ++
 tools/perf/builtin-stat.c      |  1 +
 tools/perf/builtin-top.c       |  1 +
 tools/perf/builtin-trace.c     |  1 +
 tools/perf/util/evlist.c       | 19 +++++++++++++++++++
 tools/perf/util/evlist.h       |  1 +
 tools/perf/util/evsel.c        | 32 ++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.h        |  6 ++++++
 tools/perf/util/parse-events.c | 25 +++++++++++++++++++++----
 9 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index a660022..81829de 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1112,6 +1112,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	argc = parse_options(argc, argv, record_options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
+	perf_evlist__purge_dummy(rec->evlist);
+
 	if (!argc && target__none(&rec->opts.target))
 		usage_with_options(record_usage, record_options);
 
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 7aa039b..99b62f1 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1208,6 +1208,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	argc = parse_options(argc, argv, options, stat_usage,
 		PARSE_OPT_STOP_AT_NON_OPTION);
+	perf_evlist__purge_dummy(evsel_list);
 
 	interval = stat_config.interval;
 
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 8c465c8..246203b 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1198,6 +1198,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	perf_config(perf_top_config, &top);
 
 	argc = parse_options(argc, argv, options, top_usage, 0);
+	perf_evlist__purge_dummy(top.evlist);
 	if (argc)
 		usage_with_options(top_usage, options);
 
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 4e3abba..57712b9 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -3099,6 +3099,7 @@ int cmd_trace(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	argc = parse_options_subcommand(argc, argv, trace_options, trace_subcommands,
 				 trace_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+	perf_evlist__purge_dummy(trace.evlist);
 
 	if (trace.trace_pgfaults) {
 		trace.opts.sample_address = true;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 8d00039..8a4e64d 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1696,3 +1696,22 @@ void perf_evlist__set_tracking_event(struct perf_evlist *evlist,
 
 	tracking_evsel->tracking = true;
 }
+
+void perf_evlist__purge_dummy(struct perf_evlist *evlist)
+{
+	struct perf_evsel *pos, *n;
+
+	/*
+	 * Remove all dummy events.
+	 * During linking, we don't touch anything except link
+	 * it into evlist. As a result, we don't
+	 * need to adjust evlist->nr_entries during removal.
+	 */
+
+	evlist__for_each_safe(evlist, n, pos) {
+		if (perf_evsel__is_dummy(pos)) {
+			list_del_init(&pos->node);
+			perf_evsel__delete(pos);
+		}
+	}
+}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index b39a619..7f15727 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -181,6 +181,7 @@ bool perf_evlist__valid_read_format(struct perf_evlist *evlist);
 void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
 				   struct list_head *list,
 				   int nr_entries);
+void perf_evlist__purge_dummy(struct perf_evlist *evlist);
 
 static inline struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
 {
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index bac25f4..01267f4 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -213,6 +213,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
 	evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
 	perf_evsel__calc_id_pos(evsel);
 	evsel->cmdline_group_boundary = false;
+	evsel->is_dummy = false;
 }
 
 struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
@@ -225,6 +226,37 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
 	return evsel;
 }
 
+struct perf_evsel *perf_evsel__new_dummy(const char *name)
+{
+	struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
+
+	if (!evsel)
+		return NULL;
+
+	/*
+	 * Don't need call perf_evsel__init() for dummy evsel.
+	 * Keep it simple.
+	 */
+	evsel->name = strdup(name);
+	if (!evsel->name)
+		goto out_free;
+
+	INIT_LIST_HEAD(&evsel->node);
+	INIT_LIST_HEAD(&evsel->config_terms);
+
+	evsel->cmdline_group_boundary = false;
+	/*
+	 * Set dummy evsel as TRACEPOINT event so it can collect filter
+	 * options.
+	 */
+	evsel->attr.type = PERF_TYPE_TRACEPOINT;
+	evsel->is_dummy = true;
+	return evsel;
+out_free:
+	free(evsel);
+	return NULL;
+}
+
 struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int idx)
 {
 	struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 298e6bb..0b8e47d 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -118,6 +118,7 @@ struct perf_evsel {
 	struct perf_evsel	*leader;
 	char			*group_name;
 	bool			cmdline_group_boundary;
+	bool			is_dummy;
 	struct list_head	config_terms;
 };
 
@@ -153,6 +154,11 @@ int perf_evsel__object_config(size_t object_size,
 			      void (*fini)(struct perf_evsel *evsel));
 
 struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx);
+struct perf_evsel *perf_evsel__new_dummy(const char *name);
+static inline bool perf_evsel__is_dummy(struct perf_evsel *evsel)
+{
+	return evsel->is_dummy;
+}
 
 static inline struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr)
 {
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 14cd7e3..71d91fb 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1141,7 +1141,7 @@ int parse_events(struct perf_evlist *evlist, const char *str,
 	perf_pmu__parse_cleanup();
 	if (!ret) {
 		int entries = data.idx - evlist->nr_entries;
-		struct perf_evsel *last;
+		struct perf_evsel *last = NULL;
 
 		if (!list_empty(&data.list)) {
 			last = list_entry(data.list.prev,
@@ -1149,8 +1149,25 @@ int parse_events(struct perf_evlist *evlist, const char *str,
 			last->cmdline_group_boundary = true;
 		}
 
-		perf_evlist__splice_list_tail(evlist, &data.list, entries);
-		evlist->nr_groups += data.nr_groups;
+		if (last && perf_evsel__is_dummy(last)) {
+			if (!list_is_singular(&data.list)) {
+				parse_events_evlist_error(&data, 0,
+					"Dummy evsel error: not on a singular list");
+				return -1;
+			}
+			/*
+			 * We are introducing a dummy event. Don't touch
+			 * anything, just link it.
+			 *
+			 * Don't use perf_evlist__splice_list_tail() since
+			 * it alerts evlist->nr_entries, which affect header
+			 * of resulting perf.data.
+			 */
+			list_splice_tail(&data.list, &evlist->entries);
+		} else {
+			perf_evlist__splice_list_tail(evlist, &data.list, entries);
+			evlist->nr_groups += data.nr_groups;
+		}
 
 		return 0;
 	}
@@ -1256,7 +1273,7 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist,
 	struct perf_evsel *last = NULL;
 	int err;
 
-	if (evlist->nr_entries > 0)
+	if (!list_empty(&evlist->entries))
 		last = perf_evlist__last(evlist);
 
 	do {
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1216399

FromArnaldo Carvalho de Melo <acme@redhat.com>
Date2015-08-31 21:40 +0200
Message-ID<q3D5V-pQ-11@gated-at.bofh.it>
In reply to#1215684
Em Sat, Aug 29, 2015 at 04:21:37AM +0000, Wang Nan escreveu:
> This patch allows linking dummy evsel onto evlist as a placeholder. It
> is for following patch which allows passing BPF object using '--event
> object.o'.

Summary: this patch ended up adding too many subtle clever tricks to
achieve what it needs to accomplish, please try to clearly describe the
problem, then describe how you implemented it.
 
> Doesn't link other event selectors, if passing a BPF object file to
> '--event', nothing is linked onto evlist.

-ENOPARSE, can you rewrite the above sentence?

You mean you will segregate the events that related to eBPF to process
them in a separate step? Like, for instance, putting them in a separate
evlist or perhaps flipping a bit like: evsel->process_me_later = true
and then avoid those and process them at some later stage?

> Instead, events described in BPF object file are probed and linked in
> a delayed manner because we want do all probing work together.
> Therefore, evsel for events in BPF object would be linked at the end
> of evlist. Which causes a small problem that, if passing '--filter'
> setting after object file, the filter option won't be correctly
> applied to those events.
 
> This patch links dummy onto evlist, so following --filter can be
> collected by the dummy evsel. For this reason dummy evsels are set to
> PERF_TYPE_TRACEPOINT.

Looks like a roundabout way of applying the --filter to the eBPF, but I
really need to read the patch then... See more below.
 
> Due to the possibility of existance of dummy evsel,
> perf_evlist__purge_dummy() must be called right after parse_options().
> This patch adds it to record, top, trace and stat builtin commands.
> Further patch moves it down after real BPF events are processed with.
 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: He Kuang <hekuang@huawei.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kaixu Xia <xiakaixu@huawei.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> Link: http://lkml.kernel.org/r/1440742821-44548-4-git-send-email-wangnan0@huawei.com
> ---
>  tools/perf/builtin-record.c    |  2 ++
>  tools/perf/builtin-stat.c      |  1 +
>  tools/perf/builtin-top.c       |  1 +
>  tools/perf/builtin-trace.c     |  1 +
>  tools/perf/util/evlist.c       | 19 +++++++++++++++++++
>  tools/perf/util/evlist.h       |  1 +
>  tools/perf/util/evsel.c        | 32 ++++++++++++++++++++++++++++++++
>  tools/perf/util/evsel.h        |  6 ++++++
>  tools/perf/util/parse-events.c | 25 +++++++++++++++++++++----
>  9 files changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index a660022..81829de 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1112,6 +1112,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	argc = parse_options(argc, argv, record_options, record_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
> +	perf_evlist__purge_dummy(rec->evlist);
> +
>  	if (!argc && target__none(&rec->opts.target))
>  		usage_with_options(record_usage, record_options);
>  
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 7aa039b..99b62f1 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1208,6 +1208,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	argc = parse_options(argc, argv, options, stat_usage,
>  		PARSE_OPT_STOP_AT_NON_OPTION);
> +	perf_evlist__purge_dummy(evsel_list);
>  
>  	interval = stat_config.interval;
>  
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 8c465c8..246203b 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1198,6 +1198,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
>  	perf_config(perf_top_config, &top);
>  
>  	argc = parse_options(argc, argv, options, top_usage, 0);
> +	perf_evlist__purge_dummy(top.evlist);
>  	if (argc)
>  		usage_with_options(top_usage, options);
>  
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 4e3abba..57712b9 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3099,6 +3099,7 @@ int cmd_trace(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	argc = parse_options_subcommand(argc, argv, trace_options, trace_subcommands,
>  				 trace_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> +	perf_evlist__purge_dummy(trace.evlist);
>  
>  	if (trace.trace_pgfaults) {
>  		trace.opts.sample_address = true;
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 8d00039..8a4e64d 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1696,3 +1696,22 @@ void perf_evlist__set_tracking_event(struct perf_evlist *evlist,
>  
>  	tracking_evsel->tracking = true;
>  }
> +
> +void perf_evlist__purge_dummy(struct perf_evlist *evlist)

If it remove more than one event, then it should be named accordingly,
either:

  perf_evlist__purge_dummies() or perf_evlist__purge_dummy_events().

I would prefer the former.

But we already have a "dummy" event:

[root@zoo linux]# perf stat -e dummy -a sleep 10s

 Performance counter stats for 'system wide':

                 0      dummy                                                       

      10.003173114 seconds time elapsed

[root@zoo linux]#

It has some specific purpose, but then now, with your patch, we need to
figure out which dummy is which, so I think this needs rethinking.


> +{
> +	struct perf_evsel *pos, *n;
> +
> +	/*
> +	 * Remove all dummy events.
> +	 * During linking, we don't touch anything except link
> +	 * it into evlist. As a result, we don't
> +	 * need to adjust evlist->nr_entries during removal.
> +	 */
> +
> +	evlist__for_each_safe(evlist, n, pos) {
> +		if (perf_evsel__is_dummy(pos)) {
> +			list_del_init(&pos->node);
> +			perf_evsel__delete(pos);
> +		}
> +	}
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index b39a619..7f15727 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -181,6 +181,7 @@ bool perf_evlist__valid_read_format(struct perf_evlist *evlist);
>  void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
>  				   struct list_head *list,
>  				   int nr_entries);
> +void perf_evlist__purge_dummy(struct perf_evlist *evlist);
>  
>  static inline struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
>  {
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index bac25f4..01267f4 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -213,6 +213,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
>  	evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
>  	perf_evsel__calc_id_pos(evsel);
>  	evsel->cmdline_group_boundary = false;
> +	evsel->is_dummy = false;
>  }
>  
>  struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
> @@ -225,6 +226,37 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
>  	return evsel;
>  }
>  
> +struct perf_evsel *perf_evsel__new_dummy(const char *name)
> +{
> +	struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
> +
> +	if (!evsel)
> +		return NULL;
> +
> +	/*
> +	 * Don't need call perf_evsel__init() for dummy evsel.
> +	 * Keep it simple.
> +	 */
> +	evsel->name = strdup(name);
> +	if (!evsel->name)
> +		goto out_free;
> +
> +	INIT_LIST_HEAD(&evsel->node);
> +	INIT_LIST_HEAD(&evsel->config_terms);
> +
> +	evsel->cmdline_group_boundary = false;
> +	/*
> +	 * Set dummy evsel as TRACEPOINT event so it can collect filter
> +	 * options.
> +	 */
> +	evsel->attr.type = PERF_TYPE_TRACEPOINT;
> +	evsel->is_dummy = true;
> +	return evsel;
> +out_free:
> +	free(evsel);
> +	return NULL;
> +}
> +
>  struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int idx)
>  {
>  	struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 298e6bb..0b8e47d 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -118,6 +118,7 @@ struct perf_evsel {
>  	struct perf_evsel	*leader;
>  	char			*group_name;
>  	bool			cmdline_group_boundary;
> +	bool			is_dummy;
>  	struct list_head	config_terms;
>  };
>  
> @@ -153,6 +154,11 @@ int perf_evsel__object_config(size_t object_size,
>  			      void (*fini)(struct perf_evsel *evsel));
>  
>  struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx);
> +struct perf_evsel *perf_evsel__new_dummy(const char *name);
> +static inline bool perf_evsel__is_dummy(struct perf_evsel *evsel)
> +{
> +	return evsel->is_dummy;
> +}
>  
>  static inline struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr)
>  {
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 14cd7e3..71d91fb 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1141,7 +1141,7 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>  	perf_pmu__parse_cleanup();
>  	if (!ret) {
>  		int entries = data.idx - evlist->nr_entries;
> -		struct perf_evsel *last;
> +		struct perf_evsel *last = NULL;
>  
>  		if (!list_empty(&data.list)) {
>  			last = list_entry(data.list.prev,
> @@ -1149,8 +1149,25 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>  			last->cmdline_group_boundary = true;
>  		}
>  
> -		perf_evlist__splice_list_tail(evlist, &data.list, entries);
> -		evlist->nr_groups += data.nr_groups;
> +		if (last && perf_evsel__is_dummy(last)) {
> +			if (!list_is_singular(&data.list)) {
> +				parse_events_evlist_error(&data, 0,
> +					"Dummy evsel error: not on a singular list");
> +				return -1;
> +			}
> +			/*
> +			 * We are introducing a dummy event. Don't touch
> +			 * anything, just link it.

What is the advantage of "just linking it"? What will we achieve by
that, you told what you want to avoid, i.e. "alerting"
evlist->nr_entries, but why is that important and what is the part you
want to reuse?

> +			 * Don't use perf_evlist__splice_list_tail() since
> +			 * it alerts evlist->nr_entries, which affect header
> +			 * of resulting perf.data.
> +			 */
> +			list_splice_tail(&data.list, &evlist->entries);
> +		} else {
> +			perf_evlist__splice_list_tail(evlist, &data.list, entries);
> +			evlist->nr_groups += data.nr_groups;
> +		}
>  
>  		return 0;
>  	}
> @@ -1256,7 +1273,7 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist,
>  	struct perf_evsel *last = NULL;
>  	int err;
>  
> -	if (evlist->nr_entries > 0)
> +	if (!list_empty(&evlist->entries))

So here is part of that clever trick, i.e. evlist->nr_entries, that so
far we could rely on being the number of evsels in evlist->entries,
can't be trusted for that, argh :-\

>  		last = perf_evlist__last(evlist);
>  
>  	do {
> -- 
> 2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1217962

FromNamhyung Kim <namhyung@kernel.org>
Date2015-09-03 02:30 +0200
Message-ID<q4qzE-4aY-9@gated-at.bofh.it>
In reply to#1215684
Hi,

On Sat, Aug 29, 2015 at 04:21:37AM +0000, Wang Nan wrote:
> This patch allows linking dummy evsel onto evlist as a placeholder. It
> is for following patch which allows passing BPF object using '--event
> object.o'.
> 
> Doesn't link other event selectors, if passing a BPF object file to
> '--event', nothing is linked onto evlist. Instead, events described in
> BPF object file are probed and linked in a delayed manner because we
> want do all probing work together. Therefore, evsel for events in BPF
> object would be linked at the end of evlist. Which causes a small
> problem that, if passing '--filter' setting after object file, the
> filter option won't be correctly applied to those events.
> 
> This patch links dummy onto evlist, so following --filter can be
> collected by the dummy evsel. For this reason dummy evsels are set to
> PERF_TYPE_TRACEPOINT.

I understand the need of the dummy event.  But we already have dummy
event so it's confusing to have similar event IMHO.  So what about
using existing dummy event instead?  You can save a link to a bpf
object in the dummy evsel (to check it later) and change to allow
setting filter on dummy events IMHO.

> 
> Due to the possibility of existance of dummy evsel,
> perf_evlist__purge_dummy() must be called right after parse_options().
> This patch adds it to record, top, trace and stat builtin commands.
> Further patch moves it down after real BPF events are processed with.

IMHO it'd be better to do this kind of job in a single place -
e.g. perf_evlist__config() ? - so that other commands get benefits
from it easily.

Thanks,
Namhyung


> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: He Kuang <hekuang@huawei.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kaixu Xia <xiakaixu@huawei.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> Link: http://lkml.kernel.org/r/1440742821-44548-4-git-send-email-wangnan0@huawei.com
> ---
>  tools/perf/builtin-record.c    |  2 ++
>  tools/perf/builtin-stat.c      |  1 +
>  tools/perf/builtin-top.c       |  1 +
>  tools/perf/builtin-trace.c     |  1 +
>  tools/perf/util/evlist.c       | 19 +++++++++++++++++++
>  tools/perf/util/evlist.h       |  1 +
>  tools/perf/util/evsel.c        | 32 ++++++++++++++++++++++++++++++++
>  tools/perf/util/evsel.h        |  6 ++++++
>  tools/perf/util/parse-events.c | 25 +++++++++++++++++++++----
>  9 files changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index a660022..81829de 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1112,6 +1112,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	argc = parse_options(argc, argv, record_options, record_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
> +	perf_evlist__purge_dummy(rec->evlist);
> +
>  	if (!argc && target__none(&rec->opts.target))
>  		usage_with_options(record_usage, record_options);
>  
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 7aa039b..99b62f1 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1208,6 +1208,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	argc = parse_options(argc, argv, options, stat_usage,
>  		PARSE_OPT_STOP_AT_NON_OPTION);
> +	perf_evlist__purge_dummy(evsel_list);
>  
>  	interval = stat_config.interval;
>  
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 8c465c8..246203b 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1198,6 +1198,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
>  	perf_config(perf_top_config, &top);
>  
>  	argc = parse_options(argc, argv, options, top_usage, 0);
> +	perf_evlist__purge_dummy(top.evlist);
>  	if (argc)
>  		usage_with_options(top_usage, options);
>  
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 4e3abba..57712b9 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3099,6 +3099,7 @@ int cmd_trace(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	argc = parse_options_subcommand(argc, argv, trace_options, trace_subcommands,
>  				 trace_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> +	perf_evlist__purge_dummy(trace.evlist);
>  
>  	if (trace.trace_pgfaults) {
>  		trace.opts.sample_address = true;
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 8d00039..8a4e64d 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1696,3 +1696,22 @@ void perf_evlist__set_tracking_event(struct perf_evlist *evlist,
>  
>  	tracking_evsel->tracking = true;
>  }
> +
> +void perf_evlist__purge_dummy(struct perf_evlist *evlist)
> +{
> +	struct perf_evsel *pos, *n;
> +
> +	/*
> +	 * Remove all dummy events.
> +	 * During linking, we don't touch anything except link
> +	 * it into evlist. As a result, we don't
> +	 * need to adjust evlist->nr_entries during removal.
> +	 */
> +
> +	evlist__for_each_safe(evlist, n, pos) {
> +		if (perf_evsel__is_dummy(pos)) {
> +			list_del_init(&pos->node);
> +			perf_evsel__delete(pos);
> +		}
> +	}
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index b39a619..7f15727 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -181,6 +181,7 @@ bool perf_evlist__valid_read_format(struct perf_evlist *evlist);
>  void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
>  				   struct list_head *list,
>  				   int nr_entries);
> +void perf_evlist__purge_dummy(struct perf_evlist *evlist);
>  
>  static inline struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
>  {
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index bac25f4..01267f4 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -213,6 +213,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
>  	evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
>  	perf_evsel__calc_id_pos(evsel);
>  	evsel->cmdline_group_boundary = false;
> +	evsel->is_dummy = false;
>  }
>  
>  struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
> @@ -225,6 +226,37 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
>  	return evsel;
>  }
>  
> +struct perf_evsel *perf_evsel__new_dummy(const char *name)
> +{
> +	struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
> +
> +	if (!evsel)
> +		return NULL;
> +
> +	/*
> +	 * Don't need call perf_evsel__init() for dummy evsel.
> +	 * Keep it simple.
> +	 */
> +	evsel->name = strdup(name);
> +	if (!evsel->name)
> +		goto out_free;
> +
> +	INIT_LIST_HEAD(&evsel->node);
> +	INIT_LIST_HEAD(&evsel->config_terms);
> +
> +	evsel->cmdline_group_boundary = false;
> +	/*
> +	 * Set dummy evsel as TRACEPOINT event so it can collect filter
> +	 * options.
> +	 */
> +	evsel->attr.type = PERF_TYPE_TRACEPOINT;
> +	evsel->is_dummy = true;
> +	return evsel;
> +out_free:
> +	free(evsel);
> +	return NULL;
> +}
> +
>  struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int idx)
>  {
>  	struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 298e6bb..0b8e47d 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -118,6 +118,7 @@ struct perf_evsel {
>  	struct perf_evsel	*leader;
>  	char			*group_name;
>  	bool			cmdline_group_boundary;
> +	bool			is_dummy;
>  	struct list_head	config_terms;
>  };
>  
> @@ -153,6 +154,11 @@ int perf_evsel__object_config(size_t object_size,
>  			      void (*fini)(struct perf_evsel *evsel));
>  
>  struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx);
> +struct perf_evsel *perf_evsel__new_dummy(const char *name);
> +static inline bool perf_evsel__is_dummy(struct perf_evsel *evsel)
> +{
> +	return evsel->is_dummy;
> +}
>  
>  static inline struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr)
>  {
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 14cd7e3..71d91fb 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1141,7 +1141,7 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>  	perf_pmu__parse_cleanup();
>  	if (!ret) {
>  		int entries = data.idx - evlist->nr_entries;
> -		struct perf_evsel *last;
> +		struct perf_evsel *last = NULL;
>  
>  		if (!list_empty(&data.list)) {
>  			last = list_entry(data.list.prev,
> @@ -1149,8 +1149,25 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>  			last->cmdline_group_boundary = true;
>  		}
>  
> -		perf_evlist__splice_list_tail(evlist, &data.list, entries);
> -		evlist->nr_groups += data.nr_groups;
> +		if (last && perf_evsel__is_dummy(last)) {
> +			if (!list_is_singular(&data.list)) {
> +				parse_events_evlist_error(&data, 0,
> +					"Dummy evsel error: not on a singular list");
> +				return -1;
> +			}
> +			/*
> +			 * We are introducing a dummy event. Don't touch
> +			 * anything, just link it.
> +			 *
> +			 * Don't use perf_evlist__splice_list_tail() since
> +			 * it alerts evlist->nr_entries, which affect header
> +			 * of resulting perf.data.
> +			 */
> +			list_splice_tail(&data.list, &evlist->entries);
> +		} else {
> +			perf_evlist__splice_list_tail(evlist, &data.list, entries);
> +			evlist->nr_groups += data.nr_groups;
> +		}
>  
>  		return 0;
>  	}
> @@ -1256,7 +1273,7 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist,
>  	struct perf_evsel *last = NULL;
>  	int err;
>  
> -	if (evlist->nr_entries > 0)
> +	if (!list_empty(&evlist->entries))
>  		last = perf_evlist__last(evlist);
>  
>  	do {
> -- 
> 2.1.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1217971

Frompi3orama <pi3orama@163.com>
Date2015-09-03 02:50 +0200
Message-ID<q4qSZ-4x8-3@gated-at.bofh.it>
In reply to#1217962

发自我的 iPhone

> 在 2015年9月3日，上午8:11，Namhyung Kim <namhyung@kernel.org> 写道：
> 
> Hi,
> 
>> On Sat, Aug 29, 2015 at 04:21:37AM +0000, Wang Nan wrote:
>> This patch allows linking dummy evsel onto evlist as a placeholder. It
>> is for following patch which allows passing BPF object using '--event
>> object.o'.
>> 
>> Doesn't link other event selectors, if passing a BPF object file to
>> '--event', nothing is linked onto evlist. Instead, events described in
>> BPF object file are probed and linked in a delayed manner because we
>> want do all probing work together. Therefore, evsel for events in BPF
>> object would be linked at the end of evlist. Which causes a small
>> problem that, if passing '--filter' setting after object file, the
>> filter option won't be correctly applied to those events.
>> 
>> This patch links dummy onto evlist, so following --filter can be
>> collected by the dummy evsel. For this reason dummy evsels are set to
>> PERF_TYPE_TRACEPOINT.
> 
> I understand the need of the dummy event.  But we already have dummy
> event so it's confusing to have similar event IMHO.  So what about
> using existing dummy event instead?  You can save a link to a bpf
> object in the dummy evsel (to check it later) and change to allow
> setting filter on dummy events IMHO.
> 

Yes, in my working-in-progress implement I use existing dummy event. Connect it
to the object by setting its name to the name of object.

Thank you.

>> 
>> Due to the possibility of existance of dummy evsel,
>> perf_evlist__purge_dummy() must be called right after parse_options().
>> This patch adds it to record, top, trace and stat builtin commands.
>> Further patch moves it down after real BPF events are processed with.
> 
> IMHO it'd be better to do this kind of job in a single place -
> e.g. perf_evlist__config() ? - so that other commands get benefits
> from it easily.
> 
> Thanks,
> Namhyung
> 
> 
>> 
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David Ahern <dsahern@gmail.com>
>> Cc: He Kuang <hekuang@huawei.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Kaixu Xia <xiakaixu@huawei.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Cc: Zefan Li <lizefan@huawei.com>
>> Cc: pi3orama@163.com
>> Link: http://lkml.kernel.org/r/1440742821-44548-4-git-send-email-wangnan0@huawei.com
>> ---
>> tools/perf/builtin-record.c    |  2 ++
>> tools/perf/builtin-stat.c      |  1 +
>> tools/perf/builtin-top.c       |  1 +
>> tools/perf/builtin-trace.c     |  1 +
>> tools/perf/util/evlist.c       | 19 +++++++++++++++++++
>> tools/perf/util/evlist.h       |  1 +
>> tools/perf/util/evsel.c        | 32 ++++++++++++++++++++++++++++++++
>> tools/perf/util/evsel.h        |  6 ++++++
>> tools/perf/util/parse-events.c | 25 +++++++++++++++++++++----
>> 9 files changed, 84 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index a660022..81829de 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -1112,6 +1112,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>> 
>>    argc = parse_options(argc, argv, record_options, record_usage,
>>                PARSE_OPT_STOP_AT_NON_OPTION);
>> +    perf_evlist__purge_dummy(rec->evlist);
>> +
>>    if (!argc && target__none(&rec->opts.target))
>>        usage_with_options(record_usage, record_options);
>> 
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 7aa039b..99b62f1 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -1208,6 +1208,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>> 
>>    argc = parse_options(argc, argv, options, stat_usage,
>>        PARSE_OPT_STOP_AT_NON_OPTION);
>> +    perf_evlist__purge_dummy(evsel_list);
>> 
>>    interval = stat_config.interval;
>> 
>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>> index 8c465c8..246203b 100644
>> --- a/tools/perf/builtin-top.c
>> +++ b/tools/perf/builtin-top.c
>> @@ -1198,6 +1198,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
>>    perf_config(perf_top_config, &top);
>> 
>>    argc = parse_options(argc, argv, options, top_usage, 0);
>> +    perf_evlist__purge_dummy(top.evlist);
>>    if (argc)
>>        usage_with_options(top_usage, options);
>> 
>> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
>> index 4e3abba..57712b9 100644
>> --- a/tools/perf/builtin-trace.c
>> +++ b/tools/perf/builtin-trace.c
>> @@ -3099,6 +3099,7 @@ int cmd_trace(int argc, const char **argv, const char *prefix __maybe_unused)
>> 
>>    argc = parse_options_subcommand(argc, argv, trace_options, trace_subcommands,
>>                 trace_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>> +    perf_evlist__purge_dummy(trace.evlist);
>> 
>>    if (trace.trace_pgfaults) {
>>        trace.opts.sample_address = true;
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 8d00039..8a4e64d 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -1696,3 +1696,22 @@ void perf_evlist__set_tracking_event(struct perf_evlist *evlist,
>> 
>>    tracking_evsel->tracking = true;
>> }
>> +
>> +void perf_evlist__purge_dummy(struct perf_evlist *evlist)
>> +{
>> +    struct perf_evsel *pos, *n;
>> +
>> +    /*
>> +     * Remove all dummy events.
>> +     * During linking, we don't touch anything except link
>> +     * it into evlist. As a result, we don't
>> +     * need to adjust evlist->nr_entries during removal.
>> +     */
>> +
>> +    evlist__for_each_safe(evlist, n, pos) {
>> +        if (perf_evsel__is_dummy(pos)) {
>> +            list_del_init(&pos->node);
>> +            perf_evsel__delete(pos);
>> +        }
>> +    }
>> +}
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index b39a619..7f15727 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -181,6 +181,7 @@ bool perf_evlist__valid_read_format(struct perf_evlist *evlist);
>> void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
>>                   struct list_head *list,
>>                   int nr_entries);
>> +void perf_evlist__purge_dummy(struct perf_evlist *evlist);
>> 
>> static inline struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
>> {
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index bac25f4..01267f4 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -213,6 +213,7 @@ void perf_evsel__init(struct perf_evsel *evsel,
>>    evsel->sample_size = __perf_evsel__sample_size(attr->sample_type);
>>    perf_evsel__calc_id_pos(evsel);
>>    evsel->cmdline_group_boundary = false;
>> +    evsel->is_dummy = false;
>> }
>> 
>> struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
>> @@ -225,6 +226,37 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
>>    return evsel;
>> }
>> 
>> +struct perf_evsel *perf_evsel__new_dummy(const char *name)
>> +{
>> +    struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
>> +
>> +    if (!evsel)
>> +        return NULL;
>> +
>> +    /*
>> +     * Don't need call perf_evsel__init() for dummy evsel.
>> +     * Keep it simple.
>> +     */
>> +    evsel->name = strdup(name);
>> +    if (!evsel->name)
>> +        goto out_free;
>> +
>> +    INIT_LIST_HEAD(&evsel->node);
>> +    INIT_LIST_HEAD(&evsel->config_terms);
>> +
>> +    evsel->cmdline_group_boundary = false;
>> +    /*
>> +     * Set dummy evsel as TRACEPOINT event so it can collect filter
>> +     * options.
>> +     */
>> +    evsel->attr.type = PERF_TYPE_TRACEPOINT;
>> +    evsel->is_dummy = true;
>> +    return evsel;
>> +out_free:
>> +    free(evsel);
>> +    return NULL;
>> +}
>> +
>> struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int idx)
>> {
>>    struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> index 298e6bb..0b8e47d 100644
>> --- a/tools/perf/util/evsel.h
>> +++ b/tools/perf/util/evsel.h
>> @@ -118,6 +118,7 @@ struct perf_evsel {
>>    struct perf_evsel    *leader;
>>    char            *group_name;
>>    bool            cmdline_group_boundary;
>> +    bool            is_dummy;
>>    struct list_head    config_terms;
>> };
>> 
>> @@ -153,6 +154,11 @@ int perf_evsel__object_config(size_t object_size,
>>                  void (*fini)(struct perf_evsel *evsel));
>> 
>> struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx);
>> +struct perf_evsel *perf_evsel__new_dummy(const char *name);
>> +static inline bool perf_evsel__is_dummy(struct perf_evsel *evsel)
>> +{
>> +    return evsel->is_dummy;
>> +}
>> 
>> static inline struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr)
>> {
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 14cd7e3..71d91fb 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1141,7 +1141,7 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>>    perf_pmu__parse_cleanup();
>>    if (!ret) {
>>        int entries = data.idx - evlist->nr_entries;
>> -        struct perf_evsel *last;
>> +        struct perf_evsel *last = NULL;
>> 
>>        if (!list_empty(&data.list)) {
>>            last = list_entry(data.list.prev,
>> @@ -1149,8 +1149,25 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>>            last->cmdline_group_boundary = true;
>>        }
>> 
>> -        perf_evlist__splice_list_tail(evlist, &data.list, entries);
>> -        evlist->nr_groups += data.nr_groups;
>> +        if (last && perf_evsel__is_dummy(last)) {
>> +            if (!list_is_singular(&data.list)) {
>> +                parse_events_evlist_error(&data, 0,
>> +                    "Dummy evsel error: not on a singular list");
>> +                return -1;
>> +            }
>> +            /*
>> +             * We are introducing a dummy event. Don't touch
>> +             * anything, just link it.
>> +             *
>> +             * Don't use perf_evlist__splice_list_tail() since
>> +             * it alerts evlist->nr_entries, which affect header
>> +             * of resulting perf.data.
>> +             */
>> +            list_splice_tail(&data.list, &evlist->entries);
>> +        } else {
>> +            perf_evlist__splice_list_tail(evlist, &data.list, entries);
>> +            evlist->nr_groups += data.nr_groups;
>> +        }
>> 
>>        return 0;
>>    }
>> @@ -1256,7 +1273,7 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist,
>>    struct perf_evsel *last = NULL;
>>    int err;
>> 
>> -    if (evlist->nr_entries > 0)
>> +    if (!list_empty(&evlist->entries))
>>        last = perf_evlist__last(evlist);
>> 
>>    do {
>> -- 
>> 2.1.0
>> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1219709 — [PATCH] perf tools: Allow BPF placeholder dummy events to collect --filter options

FromWang Nan <wangnan0@huawei.com>
Date2015-09-06 08:00 +0200
Subject[PATCH] perf tools: Allow BPF placeholder dummy events to collect --filter options
Message-ID<q5B9E-6Jt-17@gated-at.bofh.it>
In reply to#1215684
This patch improves collection and setting of filters, allows --filter
be set to BPF placeholder events (which is a software dummy event).

perf_evsel__is_dummy(), perf_evsel__is_bpf_placeholder() and
perf_evsel__can_filter() are introduced for this.

Test result:

 # perf record --event dummy --exclude-perf
 --exclude-perf option should follow a -e tracepoint option

 # perf record --event dummy:u --exclude-perf ls
 --exclude-perf option should follow a -e tracepoint option

 # perf record --event test.o --exclude-perf ls
 Added new event:
   perf_bpf_probe:func_vfs_write (on vfs_write)
 ...

 # perf record --event dummy.o --exclude-perf ls
 Added new event:
   perf_bpf_probe:func_write (on sys_write)
 ...

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David Ahern <dsahern@gmail.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kaixu Xia <xiakaixu@huawei.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c       |  7 +++++++
 tools/perf/util/evsel.c        | 32 ++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.h        | 23 +++++++++++++++++++++++
 tools/perf/util/parse-events.c |  4 ++--
 4 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 93db4c1..29212dc 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1223,6 +1223,13 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e
 			continue;
 
 		/*
+		 * Filters are allowed to be set to dummy event for BPF object
+		 * placeholder. Don't really apply them.
+		 */
+		if (perf_evsel__is_dummy(evsel))
+			continue;
+
+		/*
 		 * filters only work for tracepoint event, which doesn't have cpu limit.
 		 * So evlist and evsel should always be same.
 		 */
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 73cf9fc..e307ea2 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2344,3 +2344,35 @@ int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target,
 			 err, strerror_r(err, sbuf, sizeof(sbuf)),
 			 perf_evsel__name(evsel));
 }
+
+bool perf_evsel__is_bpf_placeholder(struct perf_evsel *evsel)
+{
+	if (!perf_evsel__is_dummy(evsel))
+		return false;
+	if (!evsel->name)
+		return false;
+	/*
+	 * If evsel->name doesn't starts with 'dummy', it must be a BPF
+	 * place holder.
+	 */
+	if (strncmp(evsel->name, perf_evsel__sw_names[PERF_COUNT_SW_DUMMY],
+			strlen(perf_evsel__sw_names[PERF_COUNT_SW_DUMMY])))
+		return true;
+	/*
+	 * Very rare case: evsel->name is 'dummy_crazy.bpf'.
+	 *
+	 * Let's check name suffix. A bpf file should ends with one of:
+	 * '.o', '.c' or '.bpf'.
+	 */
+#define SUFFIX_CMP(s)\
+	strcmp(evsel->name + strlen(evsel->name) - (sizeof(s) - 1), s)
+
+	if (SUFFIX_CMP(".o") == 0)
+		return true;
+	if (SUFFIX_CMP(".c") == 0)
+		return true;
+	if (SUFFIX_CMP(".bpf") == 0)
+		return true;
+	return false;
+#undef SUFFIX_CMP
+}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index fd22f83..864fd3f 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -372,11 +372,34 @@ bool perf_evsel__fallback(struct perf_evsel *evsel, int err,
 int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target,
 			      int err, char *msg, size_t size);
 
+bool perf_evsel__is_bpf_placeholder(struct perf_evsel *evsel);
+
 static inline int perf_evsel__group_idx(struct perf_evsel *evsel)
 {
 	return evsel->idx - evsel->leader->idx;
 }
 
+static inline bool perf_evsel__is_dummy(struct perf_evsel *evsel)
+{
+	if (!evsel)
+		return false;
+	if (evsel->attr.type != PERF_TYPE_SOFTWARE)
+		return false;
+	if (evsel->attr.config != PERF_COUNT_SW_DUMMY)
+		return false;
+	return true;
+}
+
+static inline int perf_evsel__can_filter(struct perf_evsel *evsel)
+{
+	if (!evsel)
+		return false;
+	if (evsel->attr.type == PERF_TYPE_TRACEPOINT)
+		return true;
+
+	return perf_evsel__is_bpf_placeholder(evsel);
+}
+
 #define for_each_group_member(_evsel, _leader) 					\
 for ((_evsel) = list_entry((_leader)->node.next, struct perf_evsel, node); 	\
      (_evsel) && (_evsel)->leader == (_leader);					\
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index b560f5f..d961e90 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1346,7 +1346,7 @@ static int set_filter(struct perf_evsel *evsel, const void *arg)
 {
 	const char *str = arg;
 
-	if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
+	if (!perf_evsel__can_filter(evsel)) {
 		fprintf(stderr,
 			"--filter option should follow a -e tracepoint option\n");
 		return -1;
@@ -1375,7 +1375,7 @@ static int add_exclude_perf_filter(struct perf_evsel *evsel,
 {
 	char new_filter[64];
 
-	if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
+	if (!perf_evsel__can_filter(evsel)) {
 		fprintf(stderr,
 			"--exclude-perf option should follow a -e tracepoint option\n");
 		return -1;
-- 
1.8.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1219710 — [PATCH] perf tools: Sync setting of real bpf events with placeholder

FromWang Nan <wangnan0@huawei.com>
Date2015-09-06 08:10 +0200
Subject[PATCH] perf tools: Sync setting of real bpf events with placeholder
Message-ID<q5Bjj-79Y-3@gated-at.bofh.it>
In reply to#1219709
In this patch, when adding real events described in BPF objects, sync
filter and tracking settings with previous dummy placeholder. After
this patch, command like:

 # perf record --test-filter.o --exclude-perf ls

work as we expect.

After all settings are synced, we remove those placeholder from evlist
so they won't appear in the final perf.data.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David Ahern <dsahern@gmail.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kaixu Xia <xiakaixu@huawei.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/bpf-loader.c |  8 ++++-
 tools/perf/util/bpf-loader.h |  1 +
 tools/perf/util/evlist.c     | 75 +++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 2880dbf..3400538 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -325,6 +325,12 @@ int bpf__foreach_tev(bpf_prog_iter_callback_t func, void *arg)
 	int err;
 
 	bpf_object__for_each_safe(obj, tmp) {
+		const char *obj_name;
+
+		obj_name = bpf_object__get_name(obj);
+		if (!obj_name)
+			obj_name = "[unknown].o";
+
 		bpf_object__for_each_program(prog, obj) {
 			struct probe_trace_event *tev;
 			struct perf_probe_event *pev;
@@ -348,7 +354,7 @@ int bpf__foreach_tev(bpf_prog_iter_callback_t func, void *arg)
 					return fd;
 				}
 
-				err = (*func)(tev, fd, arg);
+				err = (*func)(tev, obj_name, fd, arg);
 				if (err) {
 					pr_debug("bpf: call back failed, stop iterate\n");
 					return err;
diff --git a/tools/perf/util/bpf-loader.h b/tools/perf/util/bpf-loader.h
index 34656f8..5bac423 100644
--- a/tools/perf/util/bpf-loader.h
+++ b/tools/perf/util/bpf-loader.h
@@ -13,6 +13,7 @@
 #define PERF_BPF_PROBE_GROUP "perf_bpf_probe"
 
 typedef int (*bpf_prog_iter_callback_t)(struct probe_trace_event *tev,
+					const char *obj_name,
 					int fd, void *arg);
 
 #ifdef HAVE_LIBBPF_SUPPORT
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 29212dc..7e36563 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -197,7 +197,54 @@ error:
 	return -ENOMEM;
 }
 
-static int add_bpf_event(struct probe_trace_event *tev, int fd,
+static void sync_with_bpf_placeholder(struct perf_evlist *evlist,
+				      const char *obj_name,
+				      struct list_head *list)
+{
+	struct perf_evsel *dummy_evsel, *pos;
+
+	const char *filter;
+	bool tracking_set = false;
+	bool found = false;
+
+	evlist__for_each(evlist, dummy_evsel) {
+		if (!perf_evsel__is_bpf_placeholder(dummy_evsel))
+			continue;
+
+		if (strcmp(dummy_evsel->name, obj_name) == 0) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		pr_debug("Failed to find dummy event of '%s'\n",
+			 obj_name);
+		return;
+	}
+
+	filter = dummy_evsel->filter;
+
+	list_for_each_entry(pos, list, node) {
+		if (filter && perf_evsel__set_filter(pos, filter)) {
+			pr_debug("Failed to set filter '%s' to evsel %s\n",
+				 filter, pos->name);
+		}
+
+		/* Sync tracking */
+		if (dummy_evsel->tracking && !tracking_set)
+			pos->tracking = tracking_set = true;
+
+		/*
+		 * If someday we allow to add config terms or modifiers
+		 * to placeholder, we should sync them with real events
+		 * here. Currently only tracking needs to be considered.
+		 */
+	}
+}
+
+static int add_bpf_event(struct probe_trace_event *tev,
+			 const char *obj_name, int fd,
 			 void *arg)
 {
 	struct perf_evlist *evlist = arg;
@@ -205,8 +252,8 @@ static int add_bpf_event(struct probe_trace_event *tev, int fd,
 	struct list_head list;
 	int err, idx, entries;
 
-	pr_debug("add bpf event %s:%s and attach bpf program %d\n",
-			tev->group, tev->event, fd);
+	pr_debug("add bpf event %s:%s and attach bpf program %d (from %s)\n",
+			tev->group, tev->event, fd, obj_name);
 	INIT_LIST_HEAD(&list);
 	idx = evlist->nr_entries;
 
@@ -228,13 +275,33 @@ static int add_bpf_event(struct probe_trace_event *tev, int fd,
 	list_for_each_entry(pos, &list, node)
 		pos->bpf_fd = fd;
 	entries = idx - evlist->nr_entries;
+
+	sync_with_bpf_placeholder(evlist, obj_name, &list);
+	/*
+	 * Currectly we don't need to link those new events at the
+	 * same place where dummy node reside because order of
+	 * events in cmdline won't be used after
+	 * 'perf_evlist__add_bpf'.
+	 */
 	perf_evlist__splice_list_tail(evlist, &list, entries);
 	return 0;
 }
 
 int perf_evlist__add_bpf(struct perf_evlist *evlist)
 {
-	return bpf__foreach_tev(add_bpf_event, evlist);
+	struct perf_evsel *pos, *n;
+	int err;
+
+	err = bpf__foreach_tev(add_bpf_event, evlist);
+
+	evlist__for_each_safe(evlist, n, pos) {
+		if (perf_evsel__is_bpf_placeholder(pos)) {
+			list_del_init(&pos->node);
+			perf_evsel__delete(pos);
+			evlist->nr_entries--;
+		}
+	}
+	return err;
 }
 
 static int perf_evlist__add_attrs(struct perf_evlist *evlist,
-- 
1.8.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web