[PATCH] perf record: Fix to honor user freq/interval properly

[PATCH] perf record: Fix to honor user freq/interval properly

[Namhyung Kim]

When configuring event perf checked a wrong condition that user
specified both of freq (-F) and period (-c) or the event has no
default value.  This worked because most of events don't have default
value and only tracepoint events have default of 1 (and it's not
desirable to change it for those events).

However, Andi's downloadable event patch changes the situation so it
cannot change the value for those events.  Fix it by allowing override
the default value if user gives one of the options.

  $ perf record -a -e uops_retired.all -F 4000 sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.325 MB perf.data (~14185 samples) ]

  $ perf evlist -F
  cpu/uops_retired.all/: sample_freq=4000

Cc: Andi Kleen <andi@firstfloor.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evsel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 5c28d82b76c4..5ff811c67adc 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -589,10 +589,10 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 	}
 
 	/*
-	 * We default some events to a 1 default interval. But keep
+	 * We default some events to have a default interval. But keep
 	 * it a weak assumption overridable by the user.
 	 */
-	if (!attr->sample_period || (opts->user_freq != UINT_MAX &&
+	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
 				     opts->user_interval != ULLONG_MAX)) {
 		if (opts->freq) {
 			perf_evsel__set_sample_bit(evsel, PERIOD);
-- 
2.0.0

Re: [PATCH] perf record: Fix to honor user freq/interval properly

[Jiri Olsa]

On Mon, Jun 09, 2014 at 02:43:37PM +0900, Namhyung Kim wrote:
> When configuring event perf checked a wrong condition that user
> specified both of freq (-F) and period (-c) or the event has no
> default value.  This worked because most of events don't have default
> value and only tracepoint events have default of 1 (and it's not
> desirable to change it for those events).
> 
> However, Andi's downloadable event patch changes the situation so it
> cannot change the value for those events.  Fix it by allowing override
> the default value if user gives one of the options.
> 
>   $ perf record -a -e uops_retired.all -F 4000 sleep 1
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.325 MB perf.data (~14185 samples) ]
> 
>   $ perf evlist -F
>   cpu/uops_retired.all/: sample_freq=4000
> 
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/evsel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 5c28d82b76c4..5ff811c67adc 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -589,10 +589,10 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
>  	}
>  
>  	/*
> -	 * We default some events to a 1 default interval. But keep
> +	 * We default some events to have a default interval. But keep
>  	 * it a weak assumption overridable by the user.
>  	 */
> -	if (!attr->sample_period || (opts->user_freq != UINT_MAX &&
> +	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
>  				     opts->user_interval != ULLONG_MAX)) {

seems ok to me, maybe we could fixup brackets in that condition so
it does not seems like there's some hidden message ;-)

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

Re: [PATCH] perf record: Fix to honor user freq/interval properly

[Andi Kleen]

On Mon, Jun 09, 2014 at 02:43:37PM +0900, Namhyung Kim wrote:
> When configuring event perf checked a wrong condition that user
> specified both of freq (-F) and period (-c) or the event has no
> default value.  This worked because most of events don't have default
> value and only tracepoint events have default of 1 (and it's not
> desirable to change it for those events).
> 
> However, Andi's downloadable event patch changes the situation so it
> cannot change the value for those events.  Fix it by allowing override
> the default value if user gives one of the options.

Thanks. Looks good. I'll drop my version.

-Andi

[tip:perf/core] perf record: Fix to honor user freq/ interval properly

[tip-bot for Namhyung Kim]

Commit-ID:  17314e2385c6627fcab4b8f97bd6668bb63495c0
Gitweb:     http://git.kernel.org/tip/17314e2385c6627fcab4b8f97bd6668bb63495c0
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 9 Jun 2014 14:43:37 +0900
Committer:  Jiri Olsa <jolsa@kernel.org>
CommitDate: Thu, 12 Jun 2014 16:53:18 +0200

perf record: Fix to honor user freq/interval properly

When configuring event perf checked a wrong condition that user
specified both of freq (-F) and period (-c) or the event has no
default value.  This worked because most of events don't have default
value and only tracepoint events have default of 1 (and it's not
desirable to change it for those events).

However, Andi's downloadable event patch changes the situation so it
cannot change the value for those events.  Fix it by allowing override
the default value if user gives one of the options.

  $ perf record -a -e uops_retired.all -F 4000 sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.325 MB perf.data (~14185 samples) ]

  $ perf evlist -F
  cpu/uops_retired.all/: sample_freq=4000

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Link: http://lkml.kernel.org/r/1402292617-26278-1-git-send-email-namhyung@kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evsel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 21154da..8606175 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -589,10 +589,10 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 	}
 
 	/*
-	 * We default some events to a 1 default interval. But keep
+	 * We default some events to have a default interval. But keep
 	 * it a weak assumption overridable by the user.
 	 */
-	if (!attr->sample_period || (opts->user_freq != UINT_MAX &&
+	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
 				     opts->user_interval != ULLONG_MAX)) {
 		if (opts->freq) {
 			perf_evsel__set_sample_bit(evsel, PERIOD);