Re: [PATCH 2/4] perf tools: allow user to specify hardware breakpoint bp_len

[Jiri Olsa <jolsa@redhat.com>]

On Thu, May 29, 2014 at 05:26:51PM +0200, Frederic Weisbecker wrote:
> From: Jacob Shin <jacob.w.shin@gmail.com>
> 
> Currently bp_len is given a default value of 4. Allow user to override it:
> 
>   $ perf stat -e mem:0x1000/8
>                             ^
>                             bp_len
> 
> If no value is given, it will default to 4 as it did before.

Namhyung,
both perf tols patches from this patchset mess up with hists
tests..  I havent found any connection yet.. any idea? ;-)

[root@krava perf]# ./perf test 
...
19: Test software clock events have valid period values    : Ok
20: Test converting perf time to TSC                       : Ok
21: Test object code reading                               : Ok
22: Test sample parsing                                    : Ok
23: Test using a dummy software event to keep tracking     : Ok
24: Test parsing with no sample_id_all bit set             : Ok
25: Test dwarf unwind                                      : Ok
26: Test filtering hist entries                            : FAILED!
27: Test mmap thread lookup                                : Ok
28: Test thread mg sharing                                 : Ok
29: Test output sorting of hist entries                    : FAILED!
30: Test cumulation of child hist entries                  : FAILED!

thanks,
jirka

> 
> Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: xiakaixu <xiakaixu@huawei.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  tools/perf/Documentation/perf-record.txt |  7 +++++--
>  tools/perf/util/parse-events.c           | 21 +++++++++++----------
>  tools/perf/util/parse-events.h           |  2 +-
>  tools/perf/util/parse-events.l           |  1 +
>  tools/perf/util/parse-events.y           | 26 ++++++++++++++++++++++++--
>  5 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index c71b0f3..5cac20d 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -33,12 +33,15 @@ OPTIONS
>          - a raw PMU event (eventsel+umask) in the form of rNNN where NNN is a
>  	  hexadecimal event descriptor.
>  
> -        - a hardware breakpoint event in the form of '\mem:addr[:access]'
> +        - a hardware breakpoint event in the form of '\mem:addr[/len][:access]'
>            where addr is the address in memory you want to break in.
>            Access is the memory access type (read, write, execute) it can
> -          be passed as follows: '\mem:addr[:[r][w][x]]'.
> +          be passed as follows: '\mem:addr[:[r][w][x]]'. len is the range,
> +          number of bytes from specified addr, which the breakpoint will cover.
>            If you want to profile read-write accesses in 0x1000, just set
>            'mem:0x1000:rw'.
> +          If you want to profile write accesses in [0x1000~1008), just set
> +          'mem:0x1000/8:w'.
>  
>  --filter=<filter>::
>          Event filter.
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 1e15df1..8dd9287 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -516,7 +516,7 @@ do {					\
>  }
>  
>  int parse_events_add_breakpoint(struct list_head *list, int *idx,
> -				void *ptr, char *type)
> +				void *ptr, char *type, u64 len)
>  {
>  	struct perf_event_attr attr;
>  
> @@ -526,14 +526,15 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
>  	if (parse_breakpoint_type(type, &attr))
>  		return -EINVAL;
>  
> -	/*
> -	 * We should find a nice way to override the access length
> -	 * Provide some defaults for now
> -	 */
> -	if (attr.bp_type == HW_BREAKPOINT_X)
> -		attr.bp_len = sizeof(long);
> -	else
> -		attr.bp_len = HW_BREAKPOINT_LEN_4;
> +	/* Provide some defaults if len is not specified */
> +	if (!len) {
> +		if (attr.bp_type == HW_BREAKPOINT_X)
> +			len = sizeof(long);
> +		else
> +			len = HW_BREAKPOINT_LEN_4;
> +	}
> +
> +	attr.bp_len = len;
>  
>  	attr.type = PERF_TYPE_BREAKPOINT;
>  	attr.sample_period = 1;
> @@ -1262,7 +1263,7 @@ void print_events(const char *event_glob, bool name_only)
>  		printf("\n");
>  
>  		printf("  %-50s [%s]\n",
> -		       "mem:<addr>[:access]",
> +		       "mem:<addr>[/len][:access]",
>  			event_type_descriptors[PERF_TYPE_BREAKPOINT]);
>  		printf("\n");
>  	}
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index df094b4..00a5eb4 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -92,7 +92,7 @@ int parse_events_add_numeric(struct list_head *list, int *idx,
>  int parse_events_add_cache(struct list_head *list, int *idx,
>  			   char *type, char *op_result1, char *op_result2);
>  int parse_events_add_breakpoint(struct list_head *list, int *idx,
> -				void *ptr, char *type);
> +				void *ptr, char *type, u64 len);
>  int parse_events_add_pmu(struct list_head *list, int *idx,
>  			 char *pmu , struct list_head *head_config);
>  void parse_events__set_leader(char *name, struct list_head *list);
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 3432995..199e18a 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -141,6 +141,7 @@ branch_type		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE
>  <mem>{
>  {modifier_bp}		{ return str(yyscanner, PE_MODIFIER_BP); }
>  :			{ return ':'; }
> +"/"			{ return '/'; }
>  {num_dec}		{ return value(yyscanner, 10); }
>  {num_hex}		{ return value(yyscanner, 16); }
>  	/*
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 0bc87ba..456f4de 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -276,6 +276,28 @@ PE_NAME_CACHE_TYPE
>  }
>  
>  event_legacy_mem:
> +PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
> +{
> +	struct parse_events_evlist *data = _data;
> +	struct list_head *list;
> +
> +	ALLOC_LIST(list);
> +	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> +					     (void *) $2, $6, $4));
> +	$$ = list;
> +}
> +|
> +PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc
> +{
> +	struct parse_events_evlist *data = _data;
> +	struct list_head *list;
> +
> +	ALLOC_LIST(list);
> +	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> +					     (void *) $2, NULL, $4));
> +	$$ = list;
> +}
> +|
>  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
>  {
>  	struct parse_events_evlist *data = _data;
> @@ -283,7 +305,7 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
>  
>  	ALLOC_LIST(list);
>  	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> -					     (void *) $2, $4));
> +					     (void *) $2, $4, 0));
>  	$$ = list;
>  }
>  |
> @@ -294,7 +316,7 @@ PE_PREFIX_MEM PE_VALUE sep_dc
>  
>  	ALLOC_LIST(list);
>  	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> -					     (void *) $2, NULL));
> +					     (void *) $2, NULL, 0));
>  	$$ = list;
>  }
>  
> -- 
> 1.8.3.1
>