Date:	Mon, 31 Aug 2015 16:38:58 -0300
From:	Arnaldo Carvalho de Melo <acme@...hat.com>
To:	Wang Nan <wangnan0@...wei.com>
Cc:	mingo@...nel.org, ast@...mgrid.com, linux-kernel@...r.kernel.org,
	lizefan@...wei.com, pi3orama@....com,
	Brendan Gregg <brendan.d.gregg@...il.com>,
	Daniel Borkmann <daniel@...earbox.net>,
	David Ahern <dsahern@...il.com>, He Kuang <hekuang@...wei.com>,
	Jiri Olsa <jolsa@...nel.org>, Kaixu Xia <xiakaixu@...wei.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH 03/31] perf tools: Introduce dummy evsel

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@...wei.com>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> Cc: Alexei Starovoitov <ast@...mgrid.com>
> Cc: Brendan Gregg <brendan.d.gregg@...il.com>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: David Ahern <dsahern@...il.com>
> Cc: He Kuang <hekuang@...wei.com>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Kaixu Xia <xiakaixu@...wei.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Zefan Li <lizefan@...wei.com>
> Cc: pi3orama@....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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists