Re: [PATCH] perf record: Allow passing perf's own pid to '--filter'

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Jul 07, 2015 at 10:43:15AM +0800, Wangnan (F) escreveu:
> On 2015/7/6 23:40, Arnaldo Carvalho de Melo wrote:
> >Em Mon, Jul 06, 2015 at 11:00:10PM +0800, pi3orama escreveu:
> >>发自我的 iPhone
> >>>在 2015年7月6日，下午9:56，Arnaldo Carvalho de Melo <acme@kernel.org> 写道：
> >But apart from that, it would be good if expressions used in 'perf
> >probe' and here could have as much as possible the same semantics for
> >those markers, i.e. 'perf probe' already uses @ for some stuff, probably
> >the meaning is for "at", i.e. something at some place.

> >'$' strongly associated with variables, so I don't think it would be a
> >big problem to enclose expressions where variables (we may end having
> >others, no?)  in '', i.e.

> >   perf record -e sched:*switch --filter 'common_pid != $PERF_PID' -a

> >Doesn't look so ugly or cumbersome :-)
 
> But what about user want to use real shell variables also?
 
>  perf record -e raw_syscalls:* "common_pid !="'$PERF_PID'" && common_pid !=
> $X_PID"
 
> Or
 
>  perf record -e raw_syscalls:* "common_pid !=\$PERF_PID && common_pid !=
> $X_PID"
 
> right?

Yes, it depends to what entity you want to ask for the variable to be
expanded into its value, if for the shell, that gets to interpret the
line first, do no escaping, if for perf, that is next in line, do the
escaping, the shell will remove the escaping character, perf will see
act on it.

For both, '$', unescaped means: this is a variable, please replace it
with its current value.
 
> However, since you and Steven prefer '$' than '@' and '@' has its own
> meaning 'at' in 'perf probe', I'll use '$' in my next version.

Thanks!
 
> I looked your new code. You added perf_evsel__append_filter() to enable us
> append a filter expression in '(%s) <op> (%s)' manner, and also
> perf_evlist__set_filter_pid() to add 'common_pid != %d' expression. They are
> nice scaffolds if we'd like to add a new cmdline option '--filter-pids' and
> '--filter-perf'.

For --filter-perf I think we would need to either use int and do:

	int pids[] = { getpid(), }, err = -1;
	char *filter = asprintf_expr_in_ints("common_pid", 1, pids);

	if (!filter)
		goto out_err;

	err = perf_evlist__append_filter(evlist, "&&", filter))
	free(filter);

	if (err)
		goto out_error;

To be done after we parse the command line, where we may have set some
filters already to a subset of the specified --events.

BTW above is an ad-hoc implementation of perf_evlist__append_filter_pid/pids(),
in this case with just one entry, for the tracer itself.

> However, I think we should let users who use --filter take
> full control of their filters, instead of providing many helpers which can
> do similar things to confuse them. So I decide not to use those functions

Well, we don't have to add many helpers, just the ones that makes sense
and may reduce existing code duplication among builtin- tools or that
could be useful in other builtins, like these appends, that I added for
'trace', but expected to be useful for 'record', as we are seeing here.

It will be useful in 'top' too.

> you added these days in my next version.

Well, if they make sense, use it, if not, do as you need to do :-)

After looking at the end result we may see more patterns emerging and do
something to improve/reduce redundancies.

- Arnaldo