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

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

[Wang Nan]

This patch allows passing perf's own PID to '--filter' by using
'@PERFPID'. This should be useful when system-widely capturing
tracepoints events.

Before this patch, when doing something like:

 # perf record -a -e syscalls:sys_enter_write <cmd>

One could easily get result like this:

 # /tmp/perf report --stdio
 ...
 # Overhead  Command  Shared Object       Symbol
 # ........  .......  ..................  ....................
 #
     99.99%  perf     libpthread-2.18.so  [.] __write_nocancel
     0.01%   ls       libc-2.18.so        [.] write
     0.01%   sshd     libc-2.18.so        [.] write
     ...

Where most events are generated by perf itself.

A shell trick can be done to filter perf itself out:

 # cat << EOF > ./tmp
 > #!/bin/sh
 > exec perf record -e ... --filter="common_pid != \$\$" -a sleep 10
 > EOF
 # chmod a+x ./tmp
 # ./tmp

However, doing so is user unfriendly.

This patch introduces '@PERFPID' placeholder to '--filter' options. Now
user is allowed to the above work with:

  # perf record -e ... --filter="common_pid != @PERFPID' sleep 10

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/Documentation/perf-record.txt |  1 +
 tools/perf/util/parse-events.c           | 96 ++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 9b9d9d0..c2902d2 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -62,6 +62,7 @@ OPTIONS
 
 --filter=<filter>::
         Event filter.
+        String '@PERFPID' is allowed to be used to represent pid of 'perf' itself.
 
 -a::
 --all-cpus::
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index aaee24c..2d62957 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1175,6 +1175,101 @@ int parse_events_option(const struct option *opt, const char *str,
 	return ret;
 }
 
+#ifndef PAGE_SIZE
+# define PAGE_SIZE 4096
+#endif
+static int
+postproc_filter_append_token(const char *key, char *new_filter,
+			     ssize_t *pspace)
+{
+	if (strcmp(key, "PERFPID") == 0) {
+		char pid_buf[32];
+		pid_t self_pid = getpid();
+
+		snprintf(pid_buf, 32, "%d", self_pid);
+		strncat(new_filter, pid_buf, *pspace);
+		*pspace -= strlen(pid_buf);
+		if (*pspace < 0)
+			return -1;
+		return 0;
+	}
+
+	return -1;
+}
+
+static void postproc_filter(struct perf_evsel *evsel)
+{
+	char *at = NULL, *sep = NULL, *old_filter, *new_filter;
+	ssize_t space;
+
+	if (!evsel->filter)
+		return;
+
+	old_filter = evsel->filter;
+	at = strchr(old_filter, '@');
+	if (!at)
+		return;
+
+	/*
+	 * See perf_event_set_filter(). Length of a valid filter is
+	 * limited by PAGE_SIZE.
+	 */
+	new_filter = malloc(PAGE_SIZE);
+	if (!new_filter) {
+		fprintf(stderr, "No enough memory for post proc filter '%s'\n",
+			old_filter);
+		return;
+	}
+	*new_filter = '\0';
+	space = PAGE_SIZE - 1;
+
+	while (1) {
+		if (at)
+			*at = '\0';
+		strncat(new_filter, old_filter, space);
+		space -= strlen(old_filter);
+		if (space < 0)
+			goto errout;
+		if (!at)
+			break;
+		*at = '@';
+
+		sep = strchr(at + 1, ' ');
+		if (sep)
+			*sep = '\0';
+
+		if (postproc_filter_append_token(at + 1, new_filter, &space))
+			goto errout;
+
+		if (!sep)
+			break;
+		*sep = ' ';
+
+		old_filter = sep;
+		at = strchr(old_filter, '@');
+	}
+
+	free(evsel->filter);
+	/*
+	 * It is safe to use new_filter directly. However, try to
+	 * release some memory by strdup() a smaller string and free
+	 * new_filter, which takes a full page.
+	 */
+	evsel->filter = strdup(new_filter);
+	if (!evsel->filter)
+		evsel->filter = new_filter;
+	else
+		free(new_filter);
+	return;
+errout:
+	if (at)
+		*at = '@';
+	if (sep)
+		*sep = ' ';
+	fprintf(stderr, "Can't post proc filter '%s'\n", evsel->filter);
+	free(new_filter);
+}
+
 int parse_filter(const struct option *opt, const char *str,
 		 int unset __maybe_unused)
 {
@@ -1196,6 +1291,7 @@ int parse_filter(const struct option *opt, const char *str,
 		return -1;
 	}
 
+	postproc_filter(last);
 	return 0;
 }
 
-- 
1.8.3.4

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

[Arnaldo Carvalho de Melo]

Em Mon, Jul 06, 2015 at 04:17:31AM +0000, Wang Nan escreveu:
> This patch allows passing perf's own PID to '--filter' by using
> '@PERFPID'. This should be useful when system-widely capturing
> tracepoints events.

Steven, does filters have any special meaning for @?
 
> Before this patch, when doing something like:
> 
>  # perf record -a -e syscalls:sys_enter_write <cmd>
> 
> One could easily get result like this:
> 
>  # /tmp/perf report --stdio
>  ...
>  # Overhead  Command  Shared Object       Symbol
>  # ........  .......  ..................  ....................
>  #
>      99.99%  perf     libpthread-2.18.so  [.] __write_nocancel
>      0.01%   ls       libc-2.18.so        [.] write
>      0.01%   sshd     libc-2.18.so        [.] write
>      ...
> 
> Where most events are generated by perf itself.
> 
> A shell trick can be done to filter perf itself out:
> 
>  # cat << EOF > ./tmp
>  > #!/bin/sh
>  > exec perf record -e ... --filter="common_pid != \$\$" -a sleep 10
>  > EOF
>  # chmod a+x ./tmp
>  # ./tmp
> 
> However, doing so is user unfriendly.
> 
> This patch introduces '@PERFPID' placeholder to '--filter' options. Now
> user is allowed to the above work with:
> 
>   # perf record -e ... --filter="common_pid != @PERFPID' sleep 10
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/Documentation/perf-record.txt |  1 +

<SNIP>

> +++ b/tools/perf/util/parse-events.c
> @@ -1175,6 +1175,101 @@ int parse_events_option(const struct option *opt, const char *str,
>  	return ret;
>  }
>  
> +#ifndef PAGE_SIZE
> +# define PAGE_SIZE 4096
> +#endif

You can use 'page_size', its available and filled via:

   page_size = sysconf(_SC_PAGE_SIZE);

early in perf's main() routine.

> +static int
> +postproc_filter_append_token(const char *key, char *new_filter,
> +			     ssize_t *pspace)
> +{
> +	if (strcmp(key, "PERFPID") == 0) {
> +		char pid_buf[32];
> +		pid_t self_pid = getpid();
> +
> +		snprintf(pid_buf, 32, "%d", self_pid);

		snprintf(pid_buf, sizeof(pid_buf), "%d", self_pid);

> +		strncat(new_filter, pid_buf, *pspace);
> +		*pspace -= strlen(pid_buf);
> +		if (*pspace < 0)
> +			return -1;
> +		return 0;
> +	}
> +
> +	return -1;

but then, please take a look at my perf/core branch, by coincidence I
worked on having multiple filters set on a evsel in 'perf trace', where
at a minimum, the tools' pid is added to the filter for all tracepoints
used, i.e.:

     commom_pid != getpid()

is always present, to avoid a feedback loop, neverending tracing of
syscalls generated by the tracer itself.

Then, if you use --filter-pids PID-1,PID-2,PID3, it will create an
expression with that first part, for things like gnome-terminal, xorg,
etc.

Now we need to keep that in place and if the user uses -e to specify
which syscalls it wants (or wants filtered out), we need to again
concatenate with that commom_pid list, so that we call the filter ioctl
just once, else the kernel returns EEXIST.

Because I needed to append, etc, there are new functions there for
go on creating such expressions, please use them, its all in my
perf/core branch, the latest patches.

I.e. having something in the filter expression that gets transformed
into the tools' pid, I have no problem with that, just curious about
what would be the best character to signal that a substitution needs to
be performed, if it is really '@VAR', as my first selection would be
'$VAR', but then I haven't looked deeply at ftrace's filter stuff to see
if it has provision for substitution in the kernel, etc.

Andi also did this at some point, forgot why that wasn't applied at the
time :-\

For reference:

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=f47805a2af3ba83881ca52434bbbc6e9886b72fd

- Arnaldo

> +}
> +
> +static void postproc_filter(struct perf_evsel *evsel)
> +{
> +	char *at = NULL, *sep = NULL, *old_filter, *new_filter;
> +	ssize_t space;
> +
> +	if (!evsel->filter)
> +		return;
> +
> +	old_filter = evsel->filter;
> +	at = strchr(old_filter, '@');
> +	if (!at)
> +		return;
> +
> +	/*
> +	 * See perf_event_set_filter(). Length of a valid filter is
> +	 * limited by PAGE_SIZE.
> +	 */
> +	new_filter = malloc(PAGE_SIZE);
> +	if (!new_filter) {
> +		fprintf(stderr, "No enough memory for post proc filter '%s'\n",
> +			old_filter);
> +		return;
> +	}
> +	*new_filter = '\0';
> +	space = PAGE_SIZE - 1;
> +
> +	while (1) {
> +		if (at)
> +			*at = '\0';
> +		strncat(new_filter, old_filter, space);
> +		space -= strlen(old_filter);
> +		if (space < 0)
> +			goto errout;
> +		if (!at)
> +			break;
> +		*at = '@';
> +
> +		sep = strchr(at + 1, ' ');
> +		if (sep)
> +			*sep = '\0';
> +
> +		if (postproc_filter_append_token(at + 1, new_filter, &space))
> +			goto errout;
> +
> +		if (!sep)
> +			break;
> +		*sep = ' ';
> +
> +		old_filter = sep;
> +		at = strchr(old_filter, '@');
> +	}
> +
> +	free(evsel->filter);
> +	/*
> +	 * It is safe to use new_filter directly. However, try to
> +	 * release some memory by strdup() a smaller string and free
> +	 * new_filter, which takes a full page.
> +	 */
> +	evsel->filter = strdup(new_filter);
> +	if (!evsel->filter)
> +		evsel->filter = new_filter;
> +	else
> +		free(new_filter);
> +	return;
> +errout:
> +	if (at)
> +		*at = '@';
> +	if (sep)
> +		*sep = ' ';
> +	fprintf(stderr, "Can't post proc filter '%s'\n", evsel->filter);
> +	free(new_filter);
> +}
> +
>  int parse_filter(const struct option *opt, const char *str,
>  		 int unset __maybe_unused)
>  {
> @@ -1196,6 +1291,7 @@ int parse_filter(const struct option *opt, const char *str,
>  		return -1;
>  	}
>  
> +	postproc_filter(last);
>  	return 0;
>  }
>  
> -- 
> 1.8.3.4

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

[pi3orama]

发自我的 iPhone

> 在 2015年7月6日，下午9:56，Arnaldo Carvalho de Melo <acme@kernel.org> 写道：
> 
> Em Mon, Jul 06, 2015 at 04:17:31AM +0000, Wang Nan escreveu:
>> This patch allows passing perf's own PID to '--filter' by using
>> '@PERFPID'. This should be useful when system-widely capturing
>> tracepoints events.
> 
> Steven, does filters have any special meaning for @?
> 
>> Before this patch, when doing something like:
>> 
>> # perf record -a -e syscalls:sys_enter_write <cmd>
>> 
>> One could easily get result like this:
>> 
>> # /tmp/perf report --stdio
>> ...
>> # Overhead  Command  Shared Object       Symbol
>> # ........  .......  ..................  ....................
>> #
>>     99.99%  perf     libpthread-2.18.so  [.] __write_nocancel
>>     0.01%   ls       libc-2.18.so        [.] write
>>     0.01%   sshd     libc-2.18.so        [.] write
>>     ...
>> 
>> Where most events are generated by perf itself.
>> 
>> A shell trick can be done to filter perf itself out:
>> 
>> # cat << EOF > ./tmp
>>> #!/bin/sh
>>> exec perf record -e ... --filter="common_pid != \$\$" -a sleep 10
>>> EOF
>> # chmod a+x ./tmp
>> # ./tmp
>> 
>> However, doing so is user unfriendly.
>> 
>> This patch introduces '@PERFPID' placeholder to '--filter' options. Now
>> user is allowed to the above work with:
>> 
>>  # perf record -e ... --filter="common_pid != @PERFPID' sleep 10
>> 
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> ---
>> tools/perf/Documentation/perf-record.txt |  1 +
> 
> <SNIP>
> 
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1175,6 +1175,101 @@ int parse_events_option(const struct option *opt, const char *str,
>>    return ret;
>> }
>> 
>> +#ifndef PAGE_SIZE
>> +# define PAGE_SIZE 4096
>> +#endif
> 
> You can use 'page_size', its available and filled via:
> 
>   page_size = sysconf(_SC_PAGE_SIZE);
> 
> early in perf's main() routine.
> 
>> +static int
>> +postproc_filter_append_token(const char *key, char *new_filter,
>> +                 ssize_t *pspace)
>> +{
>> +    if (strcmp(key, "PERFPID") == 0) {
>> +        char pid_buf[32];
>> +        pid_t self_pid = getpid();
>> +
>> +        snprintf(pid_buf, 32, "%d", self_pid);
> 
>        snprintf(pid_buf, sizeof(pid_buf), "%d", self_pid);
> 
>> +        strncat(new_filter, pid_buf, *pspace);
>> +        *pspace -= strlen(pid_buf);
>> +        if (*pspace < 0)
>> +            return -1;
>> +        return 0;
>> +    }
>> +
>> +    return -1;
> 
> but then, please take a look at my perf/core branch, by coincidence I
> worked on having multiple filters set on a evsel in 'perf trace', where
> at a minimum, the tools' pid is added to the filter for all tracepoints
> used, i.e.:
> 
>     commom_pid != getpid()
> 
> is always present, to avoid a feedback loop, neverending tracing of
> syscalls generated by the tracer itself.
> 
> Then, if you use --filter-pids PID-1,PID-2,PID3, it will create an
> expression with that first part, for things like gnome-terminal, xorg,
> etc.
> 
> Now we need to keep that in place and if the user uses -e to specify
> which syscalls it wants (or wants filtered out), we need to again
> concatenate with that commom_pid list, so that we call the filter ioctl
> just once, else the kernel returns EEXIST.
> 
> Because I needed to append, etc, there are new functions there for
> go on creating such expressions, please use them, its all in my
> perf/core branch, the latest patches.
> 
> I.e. having something in the filter expression that gets transformed
> into the tools' pid, I have no problem with that, just curious about
> what would be the best character to signal that a substitution needs to
> be performed, if it is really '@VAR', as my first selection would be
> '$VAR',

$ has special meaning for shell. Using $ in cmdline require users use escaping or '' quoted string. Therefore I believe @ should be better. What do you think?

> but then I haven't looked deeply at ftrace's filter stuff to see
> if it has provision for substitution in the kernel, etc.
> 
> Andi also did this at some point, forgot why that wasn't applied at the
> time :-\
> 
> For reference:
> 
> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=f47805a2af3ba83881ca52434bbbc6e9886b72fd
> 

OK. I'll rebase this patch on it.

Thank you.

> - Arnaldo
> 
>> +}
>> +
>> +static void postproc_filter(struct perf_evsel *evsel)
>> +{
>> +    char *at = NULL, *sep = NULL, *old_filter, *new_filter;
>> +    ssize_t space;
>> +
>> +    if (!evsel->filter)
>> +        return;
>> +
>> +    old_filter = evsel->filter;
>> +    at = strchr(old_filter, '@');
>> +    if (!at)
>> +        return;
>> +
>> +    /*
>> +     * See perf_event_set_filter(). Length of a valid filter is
>> +     * limited by PAGE_SIZE.
>> +     */
>> +    new_filter = malloc(PAGE_SIZE);
>> +    if (!new_filter) {
>> +        fprintf(stderr, "No enough memory for post proc filter '%s'\n",
>> +            old_filter);
>> +        return;
>> +    }
>> +    *new_filter = '\0';
>> +    space = PAGE_SIZE - 1;
>> +
>> +    while (1) {
>> +        if (at)
>> +            *at = '\0';
>> +        strncat(new_filter, old_filter, space);
>> +        space -= strlen(old_filter);
>> +        if (space < 0)
>> +            goto errout;
>> +        if (!at)
>> +            break;
>> +        *at = '@';
>> +
>> +        sep = strchr(at + 1, ' ');
>> +        if (sep)
>> +            *sep = '\0';
>> +
>> +        if (postproc_filter_append_token(at + 1, new_filter, &space))
>> +            goto errout;
>> +
>> +        if (!sep)
>> +            break;
>> +        *sep = ' ';
>> +
>> +        old_filter = sep;
>> +        at = strchr(old_filter, '@');
>> +    }
>> +
>> +    free(evsel->filter);
>> +    /*
>> +     * It is safe to use new_filter directly. However, try to
>> +     * release some memory by strdup() a smaller string and free
>> +     * new_filter, which takes a full page.
>> +     */
>> +    evsel->filter = strdup(new_filter);
>> +    if (!evsel->filter)
>> +        evsel->filter = new_filter;
>> +    else
>> +        free(new_filter);
>> +    return;
>> +errout:
>> +    if (at)
>> +        *at = '@';
>> +    if (sep)
>> +        *sep = ' ';
>> +    fprintf(stderr, "Can't post proc filter '%s'\n", evsel->filter);
>> +    free(new_filter);
>> +}
>> +
>> int parse_filter(const struct option *opt, const char *str,
>>         int unset __maybe_unused)
>> {
>> @@ -1196,6 +1291,7 @@ int parse_filter(const struct option *opt, const char *str,
>>        return -1;
>>    }
>> 
>> +    postproc_filter(last);
>>    return 0;
>> }
>> 
>> -- 
>> 1.8.3.4

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

[Arnaldo Carvalho de Melo]

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> 写道：
> > I.e. having something in the filter expression that gets transformed
> > into the tools' pid, I have no problem with that, just curious about
> > what would be the best character to signal that a substitution needs to
> > be performed, if it is really '@VAR', as my first selection would be
> > '$VAR',
 
> $ has special meaning for shell. Using $ in cmdline require users use escaping or '' quoted string. Therefore I believe @ should be better. What do you think?

Yeah, that gets in the way, as it gets in the way for '!', i.e.
negating, and even tho, that is what is used in strace (and in 'perf
trace'):

  strace -e \!open,write ls

Or:

  strace -e '!open,write' ls

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 :-)

- Arnaldo

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

[Andi Kleen]

On Mon, Jul 06, 2015 at 04:17:31AM +0000, Wang Nan wrote:
> This patch allows passing perf's own PID to '--filter' by using
> '@PERFPID'. This should be useful when system-widely capturing
> tracepoints events.


I sent a similar patch some time ago.

https://lkml.org/lkml/2014/9/9/928

-Andi

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

[Steven Rostedt]

On Mon, 6 Jul 2015 10:56:50 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Mon, Jul 06, 2015 at 04:17:31AM +0000, Wang Nan escreveu:
> > This patch allows passing perf's own PID to '--filter' by using
> > '@PERFPID'. This should be useful when system-widely capturing
> > tracepoints events.
> 
> Steven, does filters have any special meaning for @?

Not as of yet.


>  
> > Before this patch, when doing something like:
> > 
> >  # perf record -a -e syscalls:sys_enter_write <cmd>
> > 
> > One could easily get result like this:
> > 
> >  # /tmp/perf report --stdio
> >  ...
> >  # Overhead  Command  Shared Object       Symbol
> >  # ........  .......  ..................  ....................
> >  #
> >      99.99%  perf     libpthread-2.18.so  [.] __write_nocancel
> >      0.01%   ls       libc-2.18.so        [.] write
> >      0.01%   sshd     libc-2.18.so        [.] write
> >      ...
> > 
> > Where most events are generated by perf itself.
> > 
> > A shell trick can be done to filter perf itself out:
> > 
> >  # cat << EOF > ./tmp
> >  > #!/bin/sh
> >  > exec perf record -e ... --filter="common_pid != \$\$" -a sleep 10
> >  > EOF
> >  # chmod a+x ./tmp
> >  # ./tmp
> > 
> > However, doing so is user unfriendly.
> > 
> > This patch introduces '@PERFPID' placeholder to '--filter' options. Now
> > user is allowed to the above work with:
> > 
> >   # perf record -e ... --filter="common_pid != @PERFPID' sleep 10

What about using '$' instead. That is more common to shell scripts of
being a variable.

-- Steve

> > 
> > Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > ---
> >  tools/perf/Documentation/perf-record.txt |  1 +
> 
> <SNIP>
> 

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

[Arnaldo Carvalho de Melo]

Em Mon, Jul 06, 2015 at 12:37:02PM -0400, Steven Rostedt escreveu:
> On Mon, 6 Jul 2015 10:56:50 -0300 Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > This patch introduces '@PERFPID' placeholder to '--filter' options. Now
> > > user is allowed to the above work with:

> > >   # perf record -e ... --filter="common_pid != @PERFPID' sleep 10
 
> What about using '$' instead. That is more common to shell scripts of
> being a variable.

Agreed.

- Arnaldo

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

[Wangnan (F)]

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> 写道：
>>> I.e. having something in the filter expression that gets transformed
>>> into the tools' pid, I have no problem with that, just curious about
>>> what would be the best character to signal that a substitution needs to
>>> be performed, if it is really '@VAR', as my first selection would be
>>> '$VAR',
>   
>> $ has special meaning for shell. Using $ in cmdline require users use escaping or '' quoted string. Therefore I believe @ should be better. What do you think?
> Yeah, that gets in the way, as it gets in the way for '!', i.e.
> negating, and even tho, that is what is used in strace (and in 'perf
> trace'):
>
>    strace -e \!open,write ls
>
> Or:
>
>    strace -e '!open,write' ls
>
> 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?

However, since you and Steven prefer '$' than '@' and '@' has its own 
meaning 'at' in 'perf probe', I'll use '$' in my next version.

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'. 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 you added these days in my next version.

Thank you.

>
> - Arnaldo

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

[Arnaldo Carvalho de Melo]

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