Re: [PATCH V5] perf tools: adding support for address filters

[Masami Hiramatsu]

On Mon, 19 Sep 2016 09:15:40 +0300
Adrian Hunter  wrote:

> On 17/09/16 16:33, Masami Hiramatsu wrote:
> > On Fri, 16 Sep 2016 09:32:43 -0600
> > Mathieu Poirier  wrote:
> > 
> >> On 13 September 2016 at 17:25, Masami Hiramatsu  
> >> wrote:
> >>> On Tue, 13 Sep 2016 08:18:10 -0600
> >>> Mathieu Poirier  wrote:
> >>>
>  On 13 September 2016 at 04:01, Adrian Hunter  
>  wrote:
> > On 12/09/16 20:53, Mathieu Poirier wrote:
> >> This patch makes it possible to use the current filter
> >> framework with address filters.  That way address filters for
> >> HW tracers such as CoreSight and IntelPT can be communicated
> >> to the kernel drivers.
> >>
> >> Signed-off-by: Mathieu Poirier 
> >>
> >> ---
> >> Changes for V5:
> >>  - Modified perf_evsel__append_filter() to take a string format
> >>rather than an operation.
> >
> > Hope I'm not being a pain, but aren't there other places calling
> > perf_evsel__append_filter() that need to be changed.  Might make
> > sense as a separate patch.
> 
>  No no, you're right - I completely overlooked that.
> 
>  But shouldn't it be in the same patch?  That way a git bisect would
>  stay consistent...
> >>>
> >>> You're right. Caller and callee should be changed in atomic.
> >>>
> >>> BTW, could you add document updates how the perf command line
> >>> will be changed, and also show the result in the patch description?
> >>
> >> This patch does not change anything on the perf command line.  It
> >> simply allows current options to succeed (as they should) rather than
> >> fail.
> > 
> > Yes, and it will make perf acceptable to pass --filter to CoreSight or
> > Intel PT events, or am I misunderstand?
> > If it is correct, could you give us an example how to pass it, since
> > tools/perf/Documentation/perf-record.txt says it is only for tracepoints?
> 
> I am adding support for symbols in the address filter.  I will send the
> patches soon, but the documentation will be:
> 
> --filter=::
> Event filter. This option should follow a event selector (-e) which
>   selects either tracepoint event(s) or a hardware trace PMU
>   (e.g. Intel PT or CoreSight).
> 
>   - tracepoint filters
> 
>   In the case of tracepoints, multiple '--filter' options are combined
>   using '&&'.
> 
>   - address filters
> 
>   A hardware trace PMU advertises its ability to accept a number of
>   address filters by specifying a non-zero value in
>   /sys/bus/event_source/devices//nr_addr_filters.
> 
>   Address filters have the format:
>   
>   filter|start|stop|tracestop  [/ ] [@]
>   
>   Where:
>   - 'filter': defines a region that will be traced.
>   - 'start': defines an address at which tracing will begin.
>   - 'stop': defines an address at which tracing will stop.
>   - 'tracestop': defines a region in which tracing will stop.
> 
>is the name of the object file,  is the offset to the
>   code to trace in that file, and  is the size of the region to
>   trace. 'start' and 'stop' filters need not specify a .
> 
>   If no object file is specified then the kernel is assumed, in which case
>   the start address must be a current kernel memory address.
> 
>can also be specified by providing the name of a symbol. If the
>   symbol name is not unique, it can be disambiguated by inserting #n where
>   'n' selects the n'th symbol in address order. Alternately #0, #g or #G
>   select only a global symbol.  can also be specified by providing
>   the name of a symbol, in which case the size is calculated to the end
>   of that symbol. For 'filter' and 'tracestop' filters, if  is
>   omitted and  is a symbol, then the size is calculated to the end
>   of that symbol.
> 
>   If  is omitted and  is '*', then the start and size will
>   be calculated from the first and last symbols, i.e. to trace the whole
>   file.
> 
>   If symbol names (or "*") are provided, they must be surrounded by white
>   space.
> 
>   The filter passed to the kernel is not necessarily the same as entered.
>   To see the filter that is passed, use the -v option.
> 
>   The kernel may not be able to configure a trace region if it is not
>   within a single mapping.  MMAP events (or /proc//maps) can be
>   examined to determine if that is a possibility.
> 
>   Multiple filters can be separated with space or comma.

Perfect! :)
OK, I'll wait your patch.

Thank you,

-- 
Masami Hiramatsu 

Re: [PATCH V5] perf tools: adding support for address filters

[Masami Hiramatsu]

On Mon, 19 Sep 2016 09:15:40 +0300
Adrian Hunter  wrote:

> On 17/09/16 16:33, Masami Hiramatsu wrote:
> > On Fri, 16 Sep 2016 09:32:43 -0600
> > Mathieu Poirier  wrote:
> > 
> >> On 13 September 2016 at 17:25, Masami Hiramatsu  
> >> wrote:
> >>> On Tue, 13 Sep 2016 08:18:10 -0600
> >>> Mathieu Poirier  wrote:
> >>>
>  On 13 September 2016 at 04:01, Adrian Hunter  
>  wrote:
> > On 12/09/16 20:53, Mathieu Poirier wrote:
> >> This patch makes it possible to use the current filter
> >> framework with address filters.  That way address filters for
> >> HW tracers such as CoreSight and IntelPT can be communicated
> >> to the kernel drivers.
> >>
> >> Signed-off-by: Mathieu Poirier 
> >>
> >> ---
> >> Changes for V5:
> >>  - Modified perf_evsel__append_filter() to take a string format
> >>rather than an operation.
> >
> > Hope I'm not being a pain, but aren't there other places calling
> > perf_evsel__append_filter() that need to be changed.  Might make
> > sense as a separate patch.
> 
>  No no, you're right - I completely overlooked that.
> 
>  But shouldn't it be in the same patch?  That way a git bisect would
>  stay consistent...
> >>>
> >>> You're right. Caller and callee should be changed in atomic.
> >>>
> >>> BTW, could you add document updates how the perf command line
> >>> will be changed, and also show the result in the patch description?
> >>
> >> This patch does not change anything on the perf command line.  It
> >> simply allows current options to succeed (as they should) rather than
> >> fail.
> > 
> > Yes, and it will make perf acceptable to pass --filter to CoreSight or
> > Intel PT events, or am I misunderstand?
> > If it is correct, could you give us an example how to pass it, since
> > tools/perf/Documentation/perf-record.txt says it is only for tracepoints?
> 
> I am adding support for symbols in the address filter.  I will send the
> patches soon, but the documentation will be:
> 
> --filter=::
> Event filter. This option should follow a event selector (-e) which
>   selects either tracepoint event(s) or a hardware trace PMU
>   (e.g. Intel PT or CoreSight).
> 
>   - tracepoint filters
> 
>   In the case of tracepoints, multiple '--filter' options are combined
>   using '&&'.
> 
>   - address filters
> 
>   A hardware trace PMU advertises its ability to accept a number of
>   address filters by specifying a non-zero value in
>   /sys/bus/event_source/devices//nr_addr_filters.
> 
>   Address filters have the format:
>   
>   filter|start|stop|tracestop  [/ ] [@]
>   
>   Where:
>   - 'filter': defines a region that will be traced.
>   - 'start': defines an address at which tracing will begin.
>   - 'stop': defines an address at which tracing will stop.
>   - 'tracestop': defines a region in which tracing will stop.
> 
>is the name of the object file,  is the offset to the
>   code to trace in that file, and  is the size of the region to
>   trace. 'start' and 'stop' filters need not specify a .
> 
>   If no object file is specified then the kernel is assumed, in which case
>   the start address must be a current kernel memory address.
> 
>can also be specified by providing the name of a symbol. If the
>   symbol name is not unique, it can be disambiguated by inserting #n where
>   'n' selects the n'th symbol in address order. Alternately #0, #g or #G
>   select only a global symbol.  can also be specified by providing
>   the name of a symbol, in which case the size is calculated to the end
>   of that symbol. For 'filter' and 'tracestop' filters, if  is
>   omitted and  is a symbol, then the size is calculated to the end
>   of that symbol.
> 
>   If  is omitted and  is '*', then the start and size will
>   be calculated from the first and last symbols, i.e. to trace the whole
>   file.
> 
>   If symbol names (or "*") are provided, they must be surrounded by white
>   space.
> 
>   The filter passed to the kernel is not necessarily the same as entered.
>   To see the filter that is passed, use the -v option.
> 
>   The kernel may not be able to configure a trace region if it is not
>   within a single mapping.  MMAP events (or /proc//maps) can be
>   examined to determine if that is a possibility.
> 
>   Multiple filters can be separated with space or comma.

Perfect! :)
OK, I'll wait your patch.

Thank you,

-- 
Masami Hiramatsu 

Re: [PATCH V5] perf tools: adding support for address filters

[Adrian Hunter]

On 17/09/16 16:33, Masami Hiramatsu wrote:
> On Fri, 16 Sep 2016 09:32:43 -0600
> Mathieu Poirier  wrote:
> 
>> On 13 September 2016 at 17:25, Masami Hiramatsu  wrote:
>>> On Tue, 13 Sep 2016 08:18:10 -0600
>>> Mathieu Poirier  wrote:
>>>
 On 13 September 2016 at 04:01, Adrian Hunter  
 wrote:
> On 12/09/16 20:53, Mathieu Poirier wrote:
>> This patch makes it possible to use the current filter
>> framework with address filters.  That way address filters for
>> HW tracers such as CoreSight and IntelPT can be communicated
>> to the kernel drivers.
>>
>> Signed-off-by: Mathieu Poirier 
>>
>> ---
>> Changes for V5:
>>  - Modified perf_evsel__append_filter() to take a string format
>>rather than an operation.
>
> Hope I'm not being a pain, but aren't there other places calling
> perf_evsel__append_filter() that need to be changed.  Might make
> sense as a separate patch.

 No no, you're right - I completely overlooked that.

 But shouldn't it be in the same patch?  That way a git bisect would
 stay consistent...
>>>
>>> You're right. Caller and callee should be changed in atomic.
>>>
>>> BTW, could you add document updates how the perf command line
>>> will be changed, and also show the result in the patch description?
>>
>> This patch does not change anything on the perf command line.  It
>> simply allows current options to succeed (as they should) rather than
>> fail.
> 
> Yes, and it will make perf acceptable to pass --filter to CoreSight or
> Intel PT events, or am I misunderstand?
> If it is correct, could you give us an example how to pass it, since
> tools/perf/Documentation/perf-record.txt says it is only for tracepoints?

I am adding support for symbols in the address filter.  I will send the
patches soon, but the documentation will be:

--filter=::
Event filter. This option should follow a event selector (-e) which
selects either tracepoint event(s) or a hardware trace PMU
(e.g. Intel PT or CoreSight).

- tracepoint filters

In the case of tracepoints, multiple '--filter' options are combined
using '&&'.

- address filters

A hardware trace PMU advertises its ability to accept a number of
address filters by specifying a non-zero value in
/sys/bus/event_source/devices//nr_addr_filters.

Address filters have the format:

filter|start|stop|tracestop  [/ ] [@]

Where:
- 'filter': defines a region that will be traced.
- 'start': defines an address at which tracing will begin.
- 'stop': defines an address at which tracing will stop.
- 'tracestop': defines a region in which tracing will stop.

 is the name of the object file,  is the offset to the
code to trace in that file, and  is the size of the region to
trace. 'start' and 'stop' filters need not specify a .

If no object file is specified then the kernel is assumed, in which case
the start address must be a current kernel memory address.

 can also be specified by providing the name of a symbol. If the
symbol name is not unique, it can be disambiguated by inserting #n where
'n' selects the n'th symbol in address order. Alternately #0, #g or #G
select only a global symbol.  can also be specified by providing
the name of a symbol, in which case the size is calculated to the end
of that symbol. For 'filter' and 'tracestop' filters, if  is
omitted and  is a symbol, then the size is calculated to the end
of that symbol.

If  is omitted and  is '*', then the start and size will
be calculated from the first and last symbols, i.e. to trace the whole
file.

If symbol names (or "*") are provided, they must be surrounded by white
space.

The filter passed to the kernel is not necessarily the same as entered.
To see the filter that is passed, use the -v option.

The kernel may not be able to configure a trace region if it is not
within a single mapping.  MMAP events (or /proc//maps) can be
examined to determine if that is a possibility.

Multiple filters can be separated with space or comma.

Re: [PATCH V5] perf tools: adding support for address filters

[Adrian Hunter]

On 17/09/16 16:33, Masami Hiramatsu wrote:
> On Fri, 16 Sep 2016 09:32:43 -0600
> Mathieu Poirier  wrote:
> 
>> On 13 September 2016 at 17:25, Masami Hiramatsu  wrote:
>>> On Tue, 13 Sep 2016 08:18:10 -0600
>>> Mathieu Poirier  wrote:
>>>
 On 13 September 2016 at 04:01, Adrian Hunter  
 wrote:
> On 12/09/16 20:53, Mathieu Poirier wrote:
>> This patch makes it possible to use the current filter
>> framework with address filters.  That way address filters for
>> HW tracers such as CoreSight and IntelPT can be communicated
>> to the kernel drivers.
>>
>> Signed-off-by: Mathieu Poirier 
>>
>> ---
>> Changes for V5:
>>  - Modified perf_evsel__append_filter() to take a string format
>>rather than an operation.
>
> Hope I'm not being a pain, but aren't there other places calling
> perf_evsel__append_filter() that need to be changed.  Might make
> sense as a separate patch.

 No no, you're right - I completely overlooked that.

 But shouldn't it be in the same patch?  That way a git bisect would
 stay consistent...
>>>
>>> You're right. Caller and callee should be changed in atomic.
>>>
>>> BTW, could you add document updates how the perf command line
>>> will be changed, and also show the result in the patch description?
>>
>> This patch does not change anything on the perf command line.  It
>> simply allows current options to succeed (as they should) rather than
>> fail.
> 
> Yes, and it will make perf acceptable to pass --filter to CoreSight or
> Intel PT events, or am I misunderstand?
> If it is correct, could you give us an example how to pass it, since
> tools/perf/Documentation/perf-record.txt says it is only for tracepoints?

I am adding support for symbols in the address filter.  I will send the
patches soon, but the documentation will be:

--filter=::
Event filter. This option should follow a event selector (-e) which
selects either tracepoint event(s) or a hardware trace PMU
(e.g. Intel PT or CoreSight).

- tracepoint filters

In the case of tracepoints, multiple '--filter' options are combined
using '&&'.

- address filters

A hardware trace PMU advertises its ability to accept a number of
address filters by specifying a non-zero value in
/sys/bus/event_source/devices//nr_addr_filters.

Address filters have the format:

filter|start|stop|tracestop  [/ ] [@]

Where:
- 'filter': defines a region that will be traced.
- 'start': defines an address at which tracing will begin.
- 'stop': defines an address at which tracing will stop.
- 'tracestop': defines a region in which tracing will stop.

 is the name of the object file,  is the offset to the
code to trace in that file, and  is the size of the region to
trace. 'start' and 'stop' filters need not specify a .

If no object file is specified then the kernel is assumed, in which case
the start address must be a current kernel memory address.

 can also be specified by providing the name of a symbol. If the
symbol name is not unique, it can be disambiguated by inserting #n where
'n' selects the n'th symbol in address order. Alternately #0, #g or #G
select only a global symbol.  can also be specified by providing
the name of a symbol, in which case the size is calculated to the end
of that symbol. For 'filter' and 'tracestop' filters, if  is
omitted and  is a symbol, then the size is calculated to the end
of that symbol.

If  is omitted and  is '*', then the start and size will
be calculated from the first and last symbols, i.e. to trace the whole
file.

If symbol names (or "*") are provided, they must be surrounded by white
space.

The filter passed to the kernel is not necessarily the same as entered.
To see the filter that is passed, use the -v option.

The kernel may not be able to configure a trace region if it is not
within a single mapping.  MMAP events (or /proc//maps) can be
examined to determine if that is a possibility.

Multiple filters can be separated with space or comma.

Re: [PATCH V5] perf tools: adding support for address filters

[Masami Hiramatsu]

On Fri, 16 Sep 2016 09:32:43 -0600
Mathieu Poirier  wrote:

> On 13 September 2016 at 17:25, Masami Hiramatsu  wrote:
> > On Tue, 13 Sep 2016 08:18:10 -0600
> > Mathieu Poirier  wrote:
> >
> >> On 13 September 2016 at 04:01, Adrian Hunter  
> >> wrote:
> >> > On 12/09/16 20:53, Mathieu Poirier wrote:
> >> >> This patch makes it possible to use the current filter
> >> >> framework with address filters.  That way address filters for
> >> >> HW tracers such as CoreSight and IntelPT can be communicated
> >> >> to the kernel drivers.
> >> >>
> >> >> Signed-off-by: Mathieu Poirier 
> >> >>
> >> >> ---
> >> >> Changes for V5:
> >> >>  - Modified perf_evsel__append_filter() to take a string format
> >> >>rather than an operation.
> >> >
> >> > Hope I'm not being a pain, but aren't there other places calling
> >> > perf_evsel__append_filter() that need to be changed.  Might make
> >> > sense as a separate patch.
> >>
> >> No no, you're right - I completely overlooked that.
> >>
> >> But shouldn't it be in the same patch?  That way a git bisect would
> >> stay consistent...
> >
> > You're right. Caller and callee should be changed in atomic.
> >
> > BTW, could you add document updates how the perf command line
> > will be changed, and also show the result in the patch description?
> 
> This patch does not change anything on the perf command line.  It
> simply allows current options to succeed (as they should) rather than
> fail.

Yes, and it will make perf acceptable to pass --filter to CoreSight or
Intel PT events, or am I misunderstand?
If it is correct, could you give us an example how to pass it, since
tools/perf/Documentation/perf-record.txt says it is only for tracepoints?

Thank you,


> 
> Thanks,
> Mathieu
> 
> >
> > Thank you,
> >
> >>
> >> >
> >> >>
> >> >> Changes for V4:
> >> >>  - Added support for address filters over more than one
> >> >>nibble.
> >> >>  - Removed Jiri's ack, this version is too different from
> >> >>what was reviewed.
> >> >>
> >> >> Changes for V3:
> >> >>  - Added Jiri's ack.
> >> >>  - Rebased to v4.8-rc5.
> >> >>
> >> >> Changes for V2:
> >> >>  - Rebased to v4.8-rc4.
> >> >>  - Revisited error path.
> >> >>
> >> >>
> >> >>  tools/perf/util/evsel.c|  4 ++--
> >> >>  tools/perf/util/evsel.h|  2 +-
> >> >>  tools/perf/util/parse-events.c | 40 
> >> >> +++-
> >> >>  3 files changed, 38 insertions(+), 8 deletions(-)
> >> >>
> >> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> >> index d40f852d2de2..a9bb277f221f 100644
> >> >> --- a/tools/perf/util/evsel.c
> >> >> +++ b/tools/perf/util/evsel.c
> >> >> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
> >> >> *evsel, const char *filter)
> >> >>  }
> >> >>
> >> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> >> -   const char *op, const char *filter)
> >> >> +   const char *fmt, const char *filter)
> >> >>  {
> >> >>   char *new_filter;
> >> >>
> >> >>   if (evsel->filter == NULL)
> >> >>   return perf_evsel__set_filter(evsel, filter);
> >> >>
> >> >> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, 
> >> >> filter) > 0) {
> >> >> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
> >> >>   free(evsel->filter);
> >> >>   evsel->filter = new_filter;
> >> >>   return 0;
> >> >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> >> >> index 8ceb7ebb51f5..50595c8c7207 100644
> >> >> --- a/tools/perf/util/evsel.h
> >> >> +++ b/tools/perf/util/evsel.h
> >> >> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel 
> >> >> *evsel,
> >> >>
> >> >>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char 
> >> >> *filter);
> >> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> >> -   const char *op, const char *filter);
> >> >> +   const char *fmt, const char *filter);
> >> >>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
> >> >> nthreads,
> >> >>const char *filter);
> >> >>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
> >> >> diff --git a/tools/perf/util/parse-events.c 
> >> >> b/tools/perf/util/parse-events.c
> >> >> index 2eb8b1ed4cc8..8e683979ccd8 100644
> >> >> --- a/tools/perf/util/parse-events.c
> >> >> +++ b/tools/perf/util/parse-events.c
> >> >> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
> >> >> *evlist,
> >> >>  static int set_filter(struct perf_evsel *evsel, const void *arg)
> >> >>  {
> >> >>   const char *str = arg;
> >> >> + bool found = false;
> >> >> + int nr_addr_filters = 0;
> >> >> + struct perf_pmu *pmu = NULL;
> >> >>
> >> >> - if 

Re: [PATCH V5] perf tools: adding support for address filters

[Masami Hiramatsu]

On Fri, 16 Sep 2016 09:32:43 -0600
Mathieu Poirier  wrote:

> On 13 September 2016 at 17:25, Masami Hiramatsu  wrote:
> > On Tue, 13 Sep 2016 08:18:10 -0600
> > Mathieu Poirier  wrote:
> >
> >> On 13 September 2016 at 04:01, Adrian Hunter  
> >> wrote:
> >> > On 12/09/16 20:53, Mathieu Poirier wrote:
> >> >> This patch makes it possible to use the current filter
> >> >> framework with address filters.  That way address filters for
> >> >> HW tracers such as CoreSight and IntelPT can be communicated
> >> >> to the kernel drivers.
> >> >>
> >> >> Signed-off-by: Mathieu Poirier 
> >> >>
> >> >> ---
> >> >> Changes for V5:
> >> >>  - Modified perf_evsel__append_filter() to take a string format
> >> >>rather than an operation.
> >> >
> >> > Hope I'm not being a pain, but aren't there other places calling
> >> > perf_evsel__append_filter() that need to be changed.  Might make
> >> > sense as a separate patch.
> >>
> >> No no, you're right - I completely overlooked that.
> >>
> >> But shouldn't it be in the same patch?  That way a git bisect would
> >> stay consistent...
> >
> > You're right. Caller and callee should be changed in atomic.
> >
> > BTW, could you add document updates how the perf command line
> > will be changed, and also show the result in the patch description?
> 
> This patch does not change anything on the perf command line.  It
> simply allows current options to succeed (as they should) rather than
> fail.

Yes, and it will make perf acceptable to pass --filter to CoreSight or
Intel PT events, or am I misunderstand?
If it is correct, could you give us an example how to pass it, since
tools/perf/Documentation/perf-record.txt says it is only for tracepoints?

Thank you,


> 
> Thanks,
> Mathieu
> 
> >
> > Thank you,
> >
> >>
> >> >
> >> >>
> >> >> Changes for V4:
> >> >>  - Added support for address filters over more than one
> >> >>nibble.
> >> >>  - Removed Jiri's ack, this version is too different from
> >> >>what was reviewed.
> >> >>
> >> >> Changes for V3:
> >> >>  - Added Jiri's ack.
> >> >>  - Rebased to v4.8-rc5.
> >> >>
> >> >> Changes for V2:
> >> >>  - Rebased to v4.8-rc4.
> >> >>  - Revisited error path.
> >> >>
> >> >>
> >> >>  tools/perf/util/evsel.c|  4 ++--
> >> >>  tools/perf/util/evsel.h|  2 +-
> >> >>  tools/perf/util/parse-events.c | 40 
> >> >> +++-
> >> >>  3 files changed, 38 insertions(+), 8 deletions(-)
> >> >>
> >> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> >> index d40f852d2de2..a9bb277f221f 100644
> >> >> --- a/tools/perf/util/evsel.c
> >> >> +++ b/tools/perf/util/evsel.c
> >> >> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
> >> >> *evsel, const char *filter)
> >> >>  }
> >> >>
> >> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> >> -   const char *op, const char *filter)
> >> >> +   const char *fmt, const char *filter)
> >> >>  {
> >> >>   char *new_filter;
> >> >>
> >> >>   if (evsel->filter == NULL)
> >> >>   return perf_evsel__set_filter(evsel, filter);
> >> >>
> >> >> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, 
> >> >> filter) > 0) {
> >> >> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
> >> >>   free(evsel->filter);
> >> >>   evsel->filter = new_filter;
> >> >>   return 0;
> >> >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> >> >> index 8ceb7ebb51f5..50595c8c7207 100644
> >> >> --- a/tools/perf/util/evsel.h
> >> >> +++ b/tools/perf/util/evsel.h
> >> >> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel 
> >> >> *evsel,
> >> >>
> >> >>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char 
> >> >> *filter);
> >> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> >> -   const char *op, const char *filter);
> >> >> +   const char *fmt, const char *filter);
> >> >>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
> >> >> nthreads,
> >> >>const char *filter);
> >> >>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
> >> >> diff --git a/tools/perf/util/parse-events.c 
> >> >> b/tools/perf/util/parse-events.c
> >> >> index 2eb8b1ed4cc8..8e683979ccd8 100644
> >> >> --- a/tools/perf/util/parse-events.c
> >> >> +++ b/tools/perf/util/parse-events.c
> >> >> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
> >> >> *evlist,
> >> >>  static int set_filter(struct perf_evsel *evsel, const void *arg)
> >> >>  {
> >> >>   const char *str = arg;
> >> >> + bool found = false;
> >> >> + int nr_addr_filters = 0;
> >> >> + struct perf_pmu *pmu = NULL;
> >> >>
> >> >> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> >> >> - fprintf(stderr,
> >> >> - 

Re: [PATCH V5] perf tools: adding support for address filters

[Mathieu Poirier]

On 13 September 2016 at 17:25, Masami Hiramatsu  wrote:
> On Tue, 13 Sep 2016 08:18:10 -0600
> Mathieu Poirier  wrote:
>
>> On 13 September 2016 at 04:01, Adrian Hunter  wrote:
>> > On 12/09/16 20:53, Mathieu Poirier wrote:
>> >> This patch makes it possible to use the current filter
>> >> framework with address filters.  That way address filters for
>> >> HW tracers such as CoreSight and IntelPT can be communicated
>> >> to the kernel drivers.
>> >>
>> >> Signed-off-by: Mathieu Poirier 
>> >>
>> >> ---
>> >> Changes for V5:
>> >>  - Modified perf_evsel__append_filter() to take a string format
>> >>rather than an operation.
>> >
>> > Hope I'm not being a pain, but aren't there other places calling
>> > perf_evsel__append_filter() that need to be changed.  Might make
>> > sense as a separate patch.
>>
>> No no, you're right - I completely overlooked that.
>>
>> But shouldn't it be in the same patch?  That way a git bisect would
>> stay consistent...
>
> You're right. Caller and callee should be changed in atomic.
>
> BTW, could you add document updates how the perf command line
> will be changed, and also show the result in the patch description?

This patch does not change anything on the perf command line.  It
simply allows current options to succeed (as they should) rather than
fail.

Thanks,
Mathieu

>
> Thank you,
>
>>
>> >
>> >>
>> >> Changes for V4:
>> >>  - Added support for address filters over more than one
>> >>nibble.
>> >>  - Removed Jiri's ack, this version is too different from
>> >>what was reviewed.
>> >>
>> >> Changes for V3:
>> >>  - Added Jiri's ack.
>> >>  - Rebased to v4.8-rc5.
>> >>
>> >> Changes for V2:
>> >>  - Rebased to v4.8-rc4.
>> >>  - Revisited error path.
>> >>
>> >>
>> >>  tools/perf/util/evsel.c|  4 ++--
>> >>  tools/perf/util/evsel.h|  2 +-
>> >>  tools/perf/util/parse-events.c | 40 
>> >> +++-
>> >>  3 files changed, 38 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> >> index d40f852d2de2..a9bb277f221f 100644
>> >> --- a/tools/perf/util/evsel.c
>> >> +++ b/tools/perf/util/evsel.c
>> >> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
>> >> *evsel, const char *filter)
>> >>  }
>> >>
>> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> >> -   const char *op, const char *filter)
>> >> +   const char *fmt, const char *filter)
>> >>  {
>> >>   char *new_filter;
>> >>
>> >>   if (evsel->filter == NULL)
>> >>   return perf_evsel__set_filter(evsel, filter);
>> >>
>> >> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) 
>> >> > 0) {
>> >> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>> >>   free(evsel->filter);
>> >>   evsel->filter = new_filter;
>> >>   return 0;
>> >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> >> index 8ceb7ebb51f5..50595c8c7207 100644
>> >> --- a/tools/perf/util/evsel.h
>> >> +++ b/tools/perf/util/evsel.h
>> >> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel 
>> >> *evsel,
>> >>
>> >>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> >> -   const char *op, const char *filter);
>> >> +   const char *fmt, const char *filter);
>> >>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
>> >> nthreads,
>> >>const char *filter);
>> >>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
>> >> diff --git a/tools/perf/util/parse-events.c 
>> >> b/tools/perf/util/parse-events.c
>> >> index 2eb8b1ed4cc8..8e683979ccd8 100644
>> >> --- a/tools/perf/util/parse-events.c
>> >> +++ b/tools/perf/util/parse-events.c
>> >> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
>> >> *evlist,
>> >>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>> >>  {
>> >>   const char *str = arg;
>> >> + bool found = false;
>> >> + int nr_addr_filters = 0;
>> >> + struct perf_pmu *pmu = NULL;
>> >>
>> >> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
>> >> - fprintf(stderr,
>> >> - "--filter option should follow a -e tracepoint 
>> >> option\n");
>> >> - return -1;
>> >> + if (evsel == NULL)
>> >> + goto err;
>> >> +
>> >> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
>> >> + if (perf_evsel__append_filter(evsel,
>> >> +   "(%s) && (%s)", str) < 0) {
>> >> + fprintf(stderr,
>> >> + "not enough memory to hold filter 
>> >> 

Re: [PATCH V5] perf tools: adding support for address filters

[Mathieu Poirier]

On 13 September 2016 at 17:25, Masami Hiramatsu  wrote:
> On Tue, 13 Sep 2016 08:18:10 -0600
> Mathieu Poirier  wrote:
>
>> On 13 September 2016 at 04:01, Adrian Hunter  wrote:
>> > On 12/09/16 20:53, Mathieu Poirier wrote:
>> >> This patch makes it possible to use the current filter
>> >> framework with address filters.  That way address filters for
>> >> HW tracers such as CoreSight and IntelPT can be communicated
>> >> to the kernel drivers.
>> >>
>> >> Signed-off-by: Mathieu Poirier 
>> >>
>> >> ---
>> >> Changes for V5:
>> >>  - Modified perf_evsel__append_filter() to take a string format
>> >>rather than an operation.
>> >
>> > Hope I'm not being a pain, but aren't there other places calling
>> > perf_evsel__append_filter() that need to be changed.  Might make
>> > sense as a separate patch.
>>
>> No no, you're right - I completely overlooked that.
>>
>> But shouldn't it be in the same patch?  That way a git bisect would
>> stay consistent...
>
> You're right. Caller and callee should be changed in atomic.
>
> BTW, could you add document updates how the perf command line
> will be changed, and also show the result in the patch description?

This patch does not change anything on the perf command line.  It
simply allows current options to succeed (as they should) rather than
fail.

Thanks,
Mathieu

>
> Thank you,
>
>>
>> >
>> >>
>> >> Changes for V4:
>> >>  - Added support for address filters over more than one
>> >>nibble.
>> >>  - Removed Jiri's ack, this version is too different from
>> >>what was reviewed.
>> >>
>> >> Changes for V3:
>> >>  - Added Jiri's ack.
>> >>  - Rebased to v4.8-rc5.
>> >>
>> >> Changes for V2:
>> >>  - Rebased to v4.8-rc4.
>> >>  - Revisited error path.
>> >>
>> >>
>> >>  tools/perf/util/evsel.c|  4 ++--
>> >>  tools/perf/util/evsel.h|  2 +-
>> >>  tools/perf/util/parse-events.c | 40 
>> >> +++-
>> >>  3 files changed, 38 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> >> index d40f852d2de2..a9bb277f221f 100644
>> >> --- a/tools/perf/util/evsel.c
>> >> +++ b/tools/perf/util/evsel.c
>> >> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
>> >> *evsel, const char *filter)
>> >>  }
>> >>
>> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> >> -   const char *op, const char *filter)
>> >> +   const char *fmt, const char *filter)
>> >>  {
>> >>   char *new_filter;
>> >>
>> >>   if (evsel->filter == NULL)
>> >>   return perf_evsel__set_filter(evsel, filter);
>> >>
>> >> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) 
>> >> > 0) {
>> >> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>> >>   free(evsel->filter);
>> >>   evsel->filter = new_filter;
>> >>   return 0;
>> >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> >> index 8ceb7ebb51f5..50595c8c7207 100644
>> >> --- a/tools/perf/util/evsel.h
>> >> +++ b/tools/perf/util/evsel.h
>> >> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel 
>> >> *evsel,
>> >>
>> >>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> >> -   const char *op, const char *filter);
>> >> +   const char *fmt, const char *filter);
>> >>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
>> >> nthreads,
>> >>const char *filter);
>> >>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
>> >> diff --git a/tools/perf/util/parse-events.c 
>> >> b/tools/perf/util/parse-events.c
>> >> index 2eb8b1ed4cc8..8e683979ccd8 100644
>> >> --- a/tools/perf/util/parse-events.c
>> >> +++ b/tools/perf/util/parse-events.c
>> >> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
>> >> *evlist,
>> >>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>> >>  {
>> >>   const char *str = arg;
>> >> + bool found = false;
>> >> + int nr_addr_filters = 0;
>> >> + struct perf_pmu *pmu = NULL;
>> >>
>> >> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
>> >> - fprintf(stderr,
>> >> - "--filter option should follow a -e tracepoint 
>> >> option\n");
>> >> - return -1;
>> >> + if (evsel == NULL)
>> >> + goto err;
>> >> +
>> >> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
>> >> + if (perf_evsel__append_filter(evsel,
>> >> +   "(%s) && (%s)", str) < 0) {
>> >> + fprintf(stderr,
>> >> + "not enough memory to hold filter 
>> >> string\n");
>> >> + return -1;
>> >> + }
>> >> +
>> >> + return 

Re: [PATCH V5] perf tools: adding support for address filters

[Adrian Hunter]

On 13/09/16 17:18, Mathieu Poirier wrote:
> On 13 September 2016 at 04:01, Adrian Hunter  wrote:
>> On 12/09/16 20:53, Mathieu Poirier wrote:
>>> This patch makes it possible to use the current filter
>>> framework with address filters.  That way address filters for
>>> HW tracers such as CoreSight and IntelPT can be communicated
>>> to the kernel drivers.
>>>
>>> Signed-off-by: Mathieu Poirier 
>>>
>>> ---
>>> Changes for V5:
>>>  - Modified perf_evsel__append_filter() to take a string format
>>>rather than an operation.
>>
>> Hope I'm not being a pain, but aren't there other places calling
>> perf_evsel__append_filter() that need to be changed.  Might make
>> sense as a separate patch.
> 
> No no, you're right - I completely overlooked that.
> 
> But shouldn't it be in the same patch?  That way a git bisect would
> stay consistent...

I meant changing perf_evsel__append_filter() and callers in a separate
preparatory patch.  Perhaps add and use:

int perf_evsel__append_tp_filter(struct perf_evsel *evsel, const char *filter)
{
return perf_evsel__append_filter(evsel, "(%s) && (%s)", filter);
}

> 
>>
>>>
>>> Changes for V4:
>>>  - Added support for address filters over more than one
>>>nibble.
>>>  - Removed Jiri's ack, this version is too different from
>>>what was reviewed.
>>>
>>> Changes for V3:
>>>  - Added Jiri's ack.
>>>  - Rebased to v4.8-rc5.
>>>
>>> Changes for V2:
>>>  - Rebased to v4.8-rc4.
>>>  - Revisited error path.
>>>
>>>
>>>  tools/perf/util/evsel.c|  4 ++--
>>>  tools/perf/util/evsel.h|  2 +-
>>>  tools/perf/util/parse-events.c | 40 
>>> +++-
>>>  3 files changed, 38 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>> index d40f852d2de2..a9bb277f221f 100644
>>> --- a/tools/perf/util/evsel.c
>>> +++ b/tools/perf/util/evsel.c
>>> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
>>> *evsel, const char *filter)
>>>  }
>>>
>>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>>> -   const char *op, const char *filter)
>>> +   const char *fmt, const char *filter)
>>>  {
>>>   char *new_filter;
>>>
>>>   if (evsel->filter == NULL)
>>>   return perf_evsel__set_filter(evsel, filter);
>>>
>>> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) > 
>>> 0) {
>>> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>>>   free(evsel->filter);
>>>   evsel->filter = new_filter;
>>>   return 0;
>>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>>> index 8ceb7ebb51f5..50595c8c7207 100644
>>> --- a/tools/perf/util/evsel.h
>>> +++ b/tools/perf/util/evsel.h
>>> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel *evsel,
>>>
>>>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>>> -   const char *op, const char *filter);
>>> +   const char *fmt, const char *filter);
>>>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
>>> nthreads,
>>>const char *filter);
>>>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>> index 2eb8b1ed4cc8..8e683979ccd8 100644
>>> --- a/tools/perf/util/parse-events.c
>>> +++ b/tools/perf/util/parse-events.c
>>> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
>>> *evlist,
>>>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>>>  {
>>>   const char *str = arg;
>>> + bool found = false;
>>> + int nr_addr_filters = 0;
>>> + struct perf_pmu *pmu = NULL;
>>>
>>> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
>>> - fprintf(stderr,
>>> - "--filter option should follow a -e tracepoint 
>>> option\n");
>>> - return -1;
>>> + if (evsel == NULL)
>>> + goto err;
>>> +
>>> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
>>> + if (perf_evsel__append_filter(evsel,
>>> +   "(%s) && (%s)", str) < 0) {
>>> + fprintf(stderr,
>>> + "not enough memory to hold filter string\n");
>>> + return -1;
>>> + }
>>> +
>>> + return 0;
>>>   }
>>>
>>> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
>>> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
>>> + if (pmu->type == evsel->attr.type) {
>>> + found = true;
>>> + break;
>>> + }
>>> +
>>> + if (found)
>>> + perf_pmu__scan_file(pmu, 

Re: [PATCH V5] perf tools: adding support for address filters

[Adrian Hunter]

On 13/09/16 17:18, Mathieu Poirier wrote:
> On 13 September 2016 at 04:01, Adrian Hunter  wrote:
>> On 12/09/16 20:53, Mathieu Poirier wrote:
>>> This patch makes it possible to use the current filter
>>> framework with address filters.  That way address filters for
>>> HW tracers such as CoreSight and IntelPT can be communicated
>>> to the kernel drivers.
>>>
>>> Signed-off-by: Mathieu Poirier 
>>>
>>> ---
>>> Changes for V5:
>>>  - Modified perf_evsel__append_filter() to take a string format
>>>rather than an operation.
>>
>> Hope I'm not being a pain, but aren't there other places calling
>> perf_evsel__append_filter() that need to be changed.  Might make
>> sense as a separate patch.
> 
> No no, you're right - I completely overlooked that.
> 
> But shouldn't it be in the same patch?  That way a git bisect would
> stay consistent...

I meant changing perf_evsel__append_filter() and callers in a separate
preparatory patch.  Perhaps add and use:

int perf_evsel__append_tp_filter(struct perf_evsel *evsel, const char *filter)
{
return perf_evsel__append_filter(evsel, "(%s) && (%s)", filter);
}

> 
>>
>>>
>>> Changes for V4:
>>>  - Added support for address filters over more than one
>>>nibble.
>>>  - Removed Jiri's ack, this version is too different from
>>>what was reviewed.
>>>
>>> Changes for V3:
>>>  - Added Jiri's ack.
>>>  - Rebased to v4.8-rc5.
>>>
>>> Changes for V2:
>>>  - Rebased to v4.8-rc4.
>>>  - Revisited error path.
>>>
>>>
>>>  tools/perf/util/evsel.c|  4 ++--
>>>  tools/perf/util/evsel.h|  2 +-
>>>  tools/perf/util/parse-events.c | 40 
>>> +++-
>>>  3 files changed, 38 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>> index d40f852d2de2..a9bb277f221f 100644
>>> --- a/tools/perf/util/evsel.c
>>> +++ b/tools/perf/util/evsel.c
>>> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
>>> *evsel, const char *filter)
>>>  }
>>>
>>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>>> -   const char *op, const char *filter)
>>> +   const char *fmt, const char *filter)
>>>  {
>>>   char *new_filter;
>>>
>>>   if (evsel->filter == NULL)
>>>   return perf_evsel__set_filter(evsel, filter);
>>>
>>> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) > 
>>> 0) {
>>> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>>>   free(evsel->filter);
>>>   evsel->filter = new_filter;
>>>   return 0;
>>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>>> index 8ceb7ebb51f5..50595c8c7207 100644
>>> --- a/tools/perf/util/evsel.h
>>> +++ b/tools/perf/util/evsel.h
>>> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel *evsel,
>>>
>>>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>>> -   const char *op, const char *filter);
>>> +   const char *fmt, const char *filter);
>>>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
>>> nthreads,
>>>const char *filter);
>>>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>> index 2eb8b1ed4cc8..8e683979ccd8 100644
>>> --- a/tools/perf/util/parse-events.c
>>> +++ b/tools/perf/util/parse-events.c
>>> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
>>> *evlist,
>>>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>>>  {
>>>   const char *str = arg;
>>> + bool found = false;
>>> + int nr_addr_filters = 0;
>>> + struct perf_pmu *pmu = NULL;
>>>
>>> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
>>> - fprintf(stderr,
>>> - "--filter option should follow a -e tracepoint 
>>> option\n");
>>> - return -1;
>>> + if (evsel == NULL)
>>> + goto err;
>>> +
>>> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
>>> + if (perf_evsel__append_filter(evsel,
>>> +   "(%s) && (%s)", str) < 0) {
>>> + fprintf(stderr,
>>> + "not enough memory to hold filter string\n");
>>> + return -1;
>>> + }
>>> +
>>> + return 0;
>>>   }
>>>
>>> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
>>> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
>>> + if (pmu->type == evsel->attr.type) {
>>> + found = true;
>>> + break;
>>> + }
>>> +
>>> + if (found)
>>> + perf_pmu__scan_file(pmu, "nr_addr_filters",
>>> + "%d", 

Re: [PATCH V5] perf tools: adding support for address filters

[Masami Hiramatsu]

On Tue, 13 Sep 2016 08:18:10 -0600
Mathieu Poirier  wrote:

> On 13 September 2016 at 04:01, Adrian Hunter  wrote:
> > On 12/09/16 20:53, Mathieu Poirier wrote:
> >> This patch makes it possible to use the current filter
> >> framework with address filters.  That way address filters for
> >> HW tracers such as CoreSight and IntelPT can be communicated
> >> to the kernel drivers.
> >>
> >> Signed-off-by: Mathieu Poirier 
> >>
> >> ---
> >> Changes for V5:
> >>  - Modified perf_evsel__append_filter() to take a string format
> >>rather than an operation.
> >
> > Hope I'm not being a pain, but aren't there other places calling
> > perf_evsel__append_filter() that need to be changed.  Might make
> > sense as a separate patch.
> 
> No no, you're right - I completely overlooked that.
> 
> But shouldn't it be in the same patch?  That way a git bisect would
> stay consistent...

You're right. Caller and callee should be changed in atomic.

BTW, could you add document updates how the perf command line
will be changed, and also show the result in the patch description?

Thank you,

> 
> >
> >>
> >> Changes for V4:
> >>  - Added support for address filters over more than one
> >>nibble.
> >>  - Removed Jiri's ack, this version is too different from
> >>what was reviewed.
> >>
> >> Changes for V3:
> >>  - Added Jiri's ack.
> >>  - Rebased to v4.8-rc5.
> >>
> >> Changes for V2:
> >>  - Rebased to v4.8-rc4.
> >>  - Revisited error path.
> >>
> >>
> >>  tools/perf/util/evsel.c|  4 ++--
> >>  tools/perf/util/evsel.h|  2 +-
> >>  tools/perf/util/parse-events.c | 40 
> >> +++-
> >>  3 files changed, 38 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> index d40f852d2de2..a9bb277f221f 100644
> >> --- a/tools/perf/util/evsel.c
> >> +++ b/tools/perf/util/evsel.c
> >> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
> >> *evsel, const char *filter)
> >>  }
> >>
> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> -   const char *op, const char *filter)
> >> +   const char *fmt, const char *filter)
> >>  {
> >>   char *new_filter;
> >>
> >>   if (evsel->filter == NULL)
> >>   return perf_evsel__set_filter(evsel, filter);
> >>
> >> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) 
> >> > 0) {
> >> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
> >>   free(evsel->filter);
> >>   evsel->filter = new_filter;
> >>   return 0;
> >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> >> index 8ceb7ebb51f5..50595c8c7207 100644
> >> --- a/tools/perf/util/evsel.h
> >> +++ b/tools/perf/util/evsel.h
> >> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel 
> >> *evsel,
> >>
> >>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> -   const char *op, const char *filter);
> >> +   const char *fmt, const char *filter);
> >>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
> >> nthreads,
> >>const char *filter);
> >>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
> >> diff --git a/tools/perf/util/parse-events.c 
> >> b/tools/perf/util/parse-events.c
> >> index 2eb8b1ed4cc8..8e683979ccd8 100644
> >> --- a/tools/perf/util/parse-events.c
> >> +++ b/tools/perf/util/parse-events.c
> >> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
> >> *evlist,
> >>  static int set_filter(struct perf_evsel *evsel, const void *arg)
> >>  {
> >>   const char *str = arg;
> >> + bool found = false;
> >> + int nr_addr_filters = 0;
> >> + struct perf_pmu *pmu = NULL;
> >>
> >> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> >> - fprintf(stderr,
> >> - "--filter option should follow a -e tracepoint 
> >> option\n");
> >> - return -1;
> >> + if (evsel == NULL)
> >> + goto err;
> >> +
> >> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
> >> + if (perf_evsel__append_filter(evsel,
> >> +   "(%s) && (%s)", str) < 0) {
> >> + fprintf(stderr,
> >> + "not enough memory to hold filter string\n");
> >> + return -1;
> >> + }
> >> +
> >> + return 0;
> >>   }
> >>
> >> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
> >> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
> >> + if (pmu->type == evsel->attr.type) {
> >> + found = true;
> >> + 

Re: [PATCH V5] perf tools: adding support for address filters

[Masami Hiramatsu]

On Tue, 13 Sep 2016 08:18:10 -0600
Mathieu Poirier  wrote:

> On 13 September 2016 at 04:01, Adrian Hunter  wrote:
> > On 12/09/16 20:53, Mathieu Poirier wrote:
> >> This patch makes it possible to use the current filter
> >> framework with address filters.  That way address filters for
> >> HW tracers such as CoreSight and IntelPT can be communicated
> >> to the kernel drivers.
> >>
> >> Signed-off-by: Mathieu Poirier 
> >>
> >> ---
> >> Changes for V5:
> >>  - Modified perf_evsel__append_filter() to take a string format
> >>rather than an operation.
> >
> > Hope I'm not being a pain, but aren't there other places calling
> > perf_evsel__append_filter() that need to be changed.  Might make
> > sense as a separate patch.
> 
> No no, you're right - I completely overlooked that.
> 
> But shouldn't it be in the same patch?  That way a git bisect would
> stay consistent...

You're right. Caller and callee should be changed in atomic.

BTW, could you add document updates how the perf command line
will be changed, and also show the result in the patch description?

Thank you,

> 
> >
> >>
> >> Changes for V4:
> >>  - Added support for address filters over more than one
> >>nibble.
> >>  - Removed Jiri's ack, this version is too different from
> >>what was reviewed.
> >>
> >> Changes for V3:
> >>  - Added Jiri's ack.
> >>  - Rebased to v4.8-rc5.
> >>
> >> Changes for V2:
> >>  - Rebased to v4.8-rc4.
> >>  - Revisited error path.
> >>
> >>
> >>  tools/perf/util/evsel.c|  4 ++--
> >>  tools/perf/util/evsel.h|  2 +-
> >>  tools/perf/util/parse-events.c | 40 
> >> +++-
> >>  3 files changed, 38 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> index d40f852d2de2..a9bb277f221f 100644
> >> --- a/tools/perf/util/evsel.c
> >> +++ b/tools/perf/util/evsel.c
> >> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel 
> >> *evsel, const char *filter)
> >>  }
> >>
> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> -   const char *op, const char *filter)
> >> +   const char *fmt, const char *filter)
> >>  {
> >>   char *new_filter;
> >>
> >>   if (evsel->filter == NULL)
> >>   return perf_evsel__set_filter(evsel, filter);
> >>
> >> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) 
> >> > 0) {
> >> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
> >>   free(evsel->filter);
> >>   evsel->filter = new_filter;
> >>   return 0;
> >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> >> index 8ceb7ebb51f5..50595c8c7207 100644
> >> --- a/tools/perf/util/evsel.h
> >> +++ b/tools/perf/util/evsel.h
> >> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel 
> >> *evsel,
> >>
> >>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
> >>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> >> -   const char *op, const char *filter);
> >> +   const char *fmt, const char *filter);
> >>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
> >> nthreads,
> >>const char *filter);
> >>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
> >> diff --git a/tools/perf/util/parse-events.c 
> >> b/tools/perf/util/parse-events.c
> >> index 2eb8b1ed4cc8..8e683979ccd8 100644
> >> --- a/tools/perf/util/parse-events.c
> >> +++ b/tools/perf/util/parse-events.c
> >> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
> >> *evlist,
> >>  static int set_filter(struct perf_evsel *evsel, const void *arg)
> >>  {
> >>   const char *str = arg;
> >> + bool found = false;
> >> + int nr_addr_filters = 0;
> >> + struct perf_pmu *pmu = NULL;
> >>
> >> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> >> - fprintf(stderr,
> >> - "--filter option should follow a -e tracepoint 
> >> option\n");
> >> - return -1;
> >> + if (evsel == NULL)
> >> + goto err;
> >> +
> >> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
> >> + if (perf_evsel__append_filter(evsel,
> >> +   "(%s) && (%s)", str) < 0) {
> >> + fprintf(stderr,
> >> + "not enough memory to hold filter string\n");
> >> + return -1;
> >> + }
> >> +
> >> + return 0;
> >>   }
> >>
> >> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
> >> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
> >> + if (pmu->type == evsel->attr.type) {
> >> + found = true;
> >> + break;
> >> + }
> >> +
> >> + if (found)
> >> + 

Re: [PATCH V5] perf tools: adding support for address filters

[Mathieu Poirier]

On 13 September 2016 at 04:01, Adrian Hunter  wrote:
> On 12/09/16 20:53, Mathieu Poirier wrote:
>> This patch makes it possible to use the current filter
>> framework with address filters.  That way address filters for
>> HW tracers such as CoreSight and IntelPT can be communicated
>> to the kernel drivers.
>>
>> Signed-off-by: Mathieu Poirier 
>>
>> ---
>> Changes for V5:
>>  - Modified perf_evsel__append_filter() to take a string format
>>rather than an operation.
>
> Hope I'm not being a pain, but aren't there other places calling
> perf_evsel__append_filter() that need to be changed.  Might make
> sense as a separate patch.

No no, you're right - I completely overlooked that.

But shouldn't it be in the same patch?  That way a git bisect would
stay consistent...

>
>>
>> Changes for V4:
>>  - Added support for address filters over more than one
>>nibble.
>>  - Removed Jiri's ack, this version is too different from
>>what was reviewed.
>>
>> Changes for V3:
>>  - Added Jiri's ack.
>>  - Rebased to v4.8-rc5.
>>
>> Changes for V2:
>>  - Rebased to v4.8-rc4.
>>  - Revisited error path.
>>
>>
>>  tools/perf/util/evsel.c|  4 ++--
>>  tools/perf/util/evsel.h|  2 +-
>>  tools/perf/util/parse-events.c | 40 +++-
>>  3 files changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index d40f852d2de2..a9bb277f221f 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel *evsel, 
>> const char *filter)
>>  }
>>
>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> -   const char *op, const char *filter)
>> +   const char *fmt, const char *filter)
>>  {
>>   char *new_filter;
>>
>>   if (evsel->filter == NULL)
>>   return perf_evsel__set_filter(evsel, filter);
>>
>> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) > 
>> 0) {
>> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>>   free(evsel->filter);
>>   evsel->filter = new_filter;
>>   return 0;
>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> index 8ceb7ebb51f5..50595c8c7207 100644
>> --- a/tools/perf/util/evsel.h
>> +++ b/tools/perf/util/evsel.h
>> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel *evsel,
>>
>>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> -   const char *op, const char *filter);
>> +   const char *fmt, const char *filter);
>>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
>> nthreads,
>>const char *filter);
>>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 2eb8b1ed4cc8..8e683979ccd8 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
>> *evlist,
>>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>>  {
>>   const char *str = arg;
>> + bool found = false;
>> + int nr_addr_filters = 0;
>> + struct perf_pmu *pmu = NULL;
>>
>> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
>> - fprintf(stderr,
>> - "--filter option should follow a -e tracepoint 
>> option\n");
>> - return -1;
>> + if (evsel == NULL)
>> + goto err;
>> +
>> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
>> + if (perf_evsel__append_filter(evsel,
>> +   "(%s) && (%s)", str) < 0) {
>> + fprintf(stderr,
>> + "not enough memory to hold filter string\n");
>> + return -1;
>> + }
>> +
>> + return 0;
>>   }
>>
>> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
>> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
>> + if (pmu->type == evsel->attr.type) {
>> + found = true;
>> + break;
>> + }
>> +
>> + if (found)
>> + perf_pmu__scan_file(pmu, "nr_addr_filters",
>> + "%d", _addr_filters);
>> +
>> + if (!nr_addr_filters)
>> + goto err;
>> +
>> + if (perf_evsel__append_filter(evsel, "%s,%s", str) < 0) {
>>   fprintf(stderr,
>>   "not enough memory to hold filter string\n");
>>   return -1;
>>   }
>>
>>   return 0;
>> +
>> +err:
>> + fprintf(stderr,
>> + "--filter option should 

Re: [PATCH V5] perf tools: adding support for address filters

[Mathieu Poirier]

On 13 September 2016 at 04:01, Adrian Hunter  wrote:
> On 12/09/16 20:53, Mathieu Poirier wrote:
>> This patch makes it possible to use the current filter
>> framework with address filters.  That way address filters for
>> HW tracers such as CoreSight and IntelPT can be communicated
>> to the kernel drivers.
>>
>> Signed-off-by: Mathieu Poirier 
>>
>> ---
>> Changes for V5:
>>  - Modified perf_evsel__append_filter() to take a string format
>>rather than an operation.
>
> Hope I'm not being a pain, but aren't there other places calling
> perf_evsel__append_filter() that need to be changed.  Might make
> sense as a separate patch.

No no, you're right - I completely overlooked that.

But shouldn't it be in the same patch?  That way a git bisect would
stay consistent...

>
>>
>> Changes for V4:
>>  - Added support for address filters over more than one
>>nibble.
>>  - Removed Jiri's ack, this version is too different from
>>what was reviewed.
>>
>> Changes for V3:
>>  - Added Jiri's ack.
>>  - Rebased to v4.8-rc5.
>>
>> Changes for V2:
>>  - Rebased to v4.8-rc4.
>>  - Revisited error path.
>>
>>
>>  tools/perf/util/evsel.c|  4 ++--
>>  tools/perf/util/evsel.h|  2 +-
>>  tools/perf/util/parse-events.c | 40 +++-
>>  3 files changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index d40f852d2de2..a9bb277f221f 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel *evsel, 
>> const char *filter)
>>  }
>>
>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> -   const char *op, const char *filter)
>> +   const char *fmt, const char *filter)
>>  {
>>   char *new_filter;
>>
>>   if (evsel->filter == NULL)
>>   return perf_evsel__set_filter(evsel, filter);
>>
>> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) > 
>> 0) {
>> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>>   free(evsel->filter);
>>   evsel->filter = new_filter;
>>   return 0;
>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> index 8ceb7ebb51f5..50595c8c7207 100644
>> --- a/tools/perf/util/evsel.h
>> +++ b/tools/perf/util/evsel.h
>> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel *evsel,
>>
>>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>>  int perf_evsel__append_filter(struct perf_evsel *evsel,
>> -   const char *op, const char *filter);
>> +   const char *fmt, const char *filter);
>>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
>> nthreads,
>>const char *filter);
>>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 2eb8b1ed4cc8..8e683979ccd8 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist 
>> *evlist,
>>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>>  {
>>   const char *str = arg;
>> + bool found = false;
>> + int nr_addr_filters = 0;
>> + struct perf_pmu *pmu = NULL;
>>
>> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
>> - fprintf(stderr,
>> - "--filter option should follow a -e tracepoint 
>> option\n");
>> - return -1;
>> + if (evsel == NULL)
>> + goto err;
>> +
>> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
>> + if (perf_evsel__append_filter(evsel,
>> +   "(%s) && (%s)", str) < 0) {
>> + fprintf(stderr,
>> + "not enough memory to hold filter string\n");
>> + return -1;
>> + }
>> +
>> + return 0;
>>   }
>>
>> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
>> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
>> + if (pmu->type == evsel->attr.type) {
>> + found = true;
>> + break;
>> + }
>> +
>> + if (found)
>> + perf_pmu__scan_file(pmu, "nr_addr_filters",
>> + "%d", _addr_filters);
>> +
>> + if (!nr_addr_filters)
>> + goto err;
>> +
>> + if (perf_evsel__append_filter(evsel, "%s,%s", str) < 0) {
>>   fprintf(stderr,
>>   "not enough memory to hold filter string\n");
>>   return -1;
>>   }
>>
>>   return 0;
>> +
>> +err:
>> + fprintf(stderr,
>> + "--filter option should follow a -e tracepoint or HW tracer 
>> option\n");
>> +

Re: [PATCH V5] perf tools: adding support for address filters

[Adrian Hunter]

On 12/09/16 20:53, Mathieu Poirier wrote:
> This patch makes it possible to use the current filter
> framework with address filters.  That way address filters for
> HW tracers such as CoreSight and IntelPT can be communicated
> to the kernel drivers.
> 
> Signed-off-by: Mathieu Poirier 
> 
> ---
> Changes for V5:
>  - Modified perf_evsel__append_filter() to take a string format
>rather than an operation.

Hope I'm not being a pain, but aren't there other places calling
perf_evsel__append_filter() that need to be changed.  Might make
sense as a separate patch.

> 
> Changes for V4:
>  - Added support for address filters over more than one
>nibble.
>  - Removed Jiri's ack, this version is too different from
>what was reviewed.
> 
> Changes for V3:
>  - Added Jiri's ack.
>  - Rebased to v4.8-rc5.
> 
> Changes for V2:
>  - Rebased to v4.8-rc4.
>  - Revisited error path.
> 
> 
>  tools/perf/util/evsel.c|  4 ++--
>  tools/perf/util/evsel.h|  2 +-
>  tools/perf/util/parse-events.c | 40 +++-
>  3 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d40f852d2de2..a9bb277f221f 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel *evsel, 
> const char *filter)
>  }
>  
>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> -   const char *op, const char *filter)
> +   const char *fmt, const char *filter)
>  {
>   char *new_filter;
>  
>   if (evsel->filter == NULL)
>   return perf_evsel__set_filter(evsel, filter);
>  
> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) > 
> 0) {
> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>   free(evsel->filter);
>   evsel->filter = new_filter;
>   return 0;
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 8ceb7ebb51f5..50595c8c7207 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel *evsel,
>  
>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> -   const char *op, const char *filter);
> +   const char *fmt, const char *filter);
>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
> nthreads,
>const char *filter);
>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 2eb8b1ed4cc8..8e683979ccd8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist,
>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>  {
>   const char *str = arg;
> + bool found = false;
> + int nr_addr_filters = 0;
> + struct perf_pmu *pmu = NULL;
>  
> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> - fprintf(stderr,
> - "--filter option should follow a -e tracepoint 
> option\n");
> - return -1;
> + if (evsel == NULL)
> + goto err;
> +
> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
> + if (perf_evsel__append_filter(evsel,
> +   "(%s) && (%s)", str) < 0) {
> + fprintf(stderr,
> + "not enough memory to hold filter string\n");
> + return -1;
> + }
> +
> + return 0;
>   }
>  
> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
> + if (pmu->type == evsel->attr.type) {
> + found = true;
> + break;
> + }
> +
> + if (found)
> + perf_pmu__scan_file(pmu, "nr_addr_filters",
> + "%d", _addr_filters);
> +
> + if (!nr_addr_filters)
> + goto err;
> +
> + if (perf_evsel__append_filter(evsel, "%s,%s", str) < 0) {
>   fprintf(stderr,
>   "not enough memory to hold filter string\n");
>   return -1;
>   }
>  
>   return 0;
> +
> +err:
> + fprintf(stderr,
> + "--filter option should follow a -e tracepoint or HW tracer 
> option\n");
> +
> + return -1;
>  }
>  
>  int parse_filter(const struct option *opt, const char *str,
> 

Re: [PATCH V5] perf tools: adding support for address filters

[Adrian Hunter]

On 12/09/16 20:53, Mathieu Poirier wrote:
> This patch makes it possible to use the current filter
> framework with address filters.  That way address filters for
> HW tracers such as CoreSight and IntelPT can be communicated
> to the kernel drivers.
> 
> Signed-off-by: Mathieu Poirier 
> 
> ---
> Changes for V5:
>  - Modified perf_evsel__append_filter() to take a string format
>rather than an operation.

Hope I'm not being a pain, but aren't there other places calling
perf_evsel__append_filter() that need to be changed.  Might make
sense as a separate patch.

> 
> Changes for V4:
>  - Added support for address filters over more than one
>nibble.
>  - Removed Jiri's ack, this version is too different from
>what was reviewed.
> 
> Changes for V3:
>  - Added Jiri's ack.
>  - Rebased to v4.8-rc5.
> 
> Changes for V2:
>  - Rebased to v4.8-rc4.
>  - Revisited error path.
> 
> 
>  tools/perf/util/evsel.c|  4 ++--
>  tools/perf/util/evsel.h|  2 +-
>  tools/perf/util/parse-events.c | 40 +++-
>  3 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d40f852d2de2..a9bb277f221f 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1047,14 +1047,14 @@ int perf_evsel__set_filter(struct perf_evsel *evsel, 
> const char *filter)
>  }
>  
>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> -   const char *op, const char *filter)
> +   const char *fmt, const char *filter)
>  {
>   char *new_filter;
>  
>   if (evsel->filter == NULL)
>   return perf_evsel__set_filter(evsel, filter);
>  
> - if (asprintf(_filter,"(%s) %s (%s)", evsel->filter, op, filter) > 
> 0) {
> + if (asprintf(_filter, fmt, evsel->filter, filter) > 0) {
>   free(evsel->filter);
>   evsel->filter = new_filter;
>   return 0;
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 8ceb7ebb51f5..50595c8c7207 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -236,7 +236,7 @@ void perf_evsel__set_sample_id(struct perf_evsel *evsel,
>  
>  int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
>  int perf_evsel__append_filter(struct perf_evsel *evsel,
> -   const char *op, const char *filter);
> +   const char *fmt, const char *filter);
>  int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int 
> nthreads,
>const char *filter);
>  int perf_evsel__apply_drv_configs(struct perf_evsel *evsel,
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 2eb8b1ed4cc8..8e683979ccd8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1760,20 +1760,50 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist,
>  static int set_filter(struct perf_evsel *evsel, const void *arg)
>  {
>   const char *str = arg;
> + bool found = false;
> + int nr_addr_filters = 0;
> + struct perf_pmu *pmu = NULL;
>  
> - if (evsel == NULL || evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> - fprintf(stderr,
> - "--filter option should follow a -e tracepoint 
> option\n");
> - return -1;
> + if (evsel == NULL)
> + goto err;
> +
> + if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
> + if (perf_evsel__append_filter(evsel,
> +   "(%s) && (%s)", str) < 0) {
> + fprintf(stderr,
> + "not enough memory to hold filter string\n");
> + return -1;
> + }
> +
> + return 0;
>   }
>  
> - if (perf_evsel__append_filter(evsel, "&&", str) < 0) {
> + while ((pmu = perf_pmu__scan(pmu)) != NULL)
> + if (pmu->type == evsel->attr.type) {
> + found = true;
> + break;
> + }
> +
> + if (found)
> + perf_pmu__scan_file(pmu, "nr_addr_filters",
> + "%d", _addr_filters);
> +
> + if (!nr_addr_filters)
> + goto err;
> +
> + if (perf_evsel__append_filter(evsel, "%s,%s", str) < 0) {
>   fprintf(stderr,
>   "not enough memory to hold filter string\n");
>   return -1;
>   }
>  
>   return 0;
> +
> +err:
> + fprintf(stderr,
> + "--filter option should follow a -e tracepoint or HW tracer 
> option\n");
> +
> + return -1;
>  }
>  
>  int parse_filter(const struct option *opt, const char *str,
>