[PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type

[PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type

[Jiri Olsa]

We use PERF_SAMPLE_PERIOD sample type only for frequency
setup -F (default) option. The -c does not need store period,
because it's always the same.

In -c case the report code uses '1' as  period. Fixing
it to perf_event_attr::sample_period.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/evsel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 55407c5..c6f8ce9 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1220,7 +1220,7 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 	memset(data, 0, sizeof(*data));
 	data->cpu = data->pid = data->tid = -1;
 	data->stream_id = data->id = data->time = -1ULL;
-	data->period = 1;
+	data->period = evsel->attr.sample_period;
 	data->weight = 0;
 
 	if (event->header.type != PERF_RECORD_SAMPLE) {
-- 
1.8.3.1

[PATCH 2/3] perf tools: Add call-graph option support into .perfconfig

[Jiri Olsa]

Adding call-graph option support into .perfconfig file,
so it's now possible use call-graph option like:

  [top]
        call-graph = fp

  [record]
        call-graph = dwarf,8192

Above options ONLY setup the unwind method. To enable
perf record/top to actually use it the command line
option -g/-G must be specified.

The --call-graph option overloads .perfconfig setup.

Assuming above configuration:

  $ perf record -g ls
  - enables dwarf unwind with user stack size dump 8192 bytes

  $ perf top -G
  - enables frame pointer unwind

  $ perf record --call-graph=fp ls
  - enables frame pointer unwind

  $ perf top --call-graph=dwarf,4096 ls
  - enables dwarf unwind with user stack size dump 4096 bytes

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-record.c | 16 ++++++++++++++++
 tools/perf/builtin-top.c    | 12 ++++++++++++
 tools/perf/perf.h           |  1 +
 tools/perf/util/evsel.c     |  2 +-
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 3c394bf..3f7ec8a 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -755,6 +755,8 @@ int record_parse_callchain_opt(const struct option *opt,
 	struct record_opts *opts = opt->value;
 	int ret;
 
+	opts->call_graph_enabled = !unset;
+
 	/* --no-call-graph */
 	if (unset) {
 		opts->call_graph = CALLCHAIN_NONE;
@@ -775,6 +777,8 @@ int record_callchain_opt(const struct option *opt,
 {
 	struct record_opts *opts = opt->value;
 
+	opts->call_graph_enabled = !unset;
+
 	if (opts->call_graph == CALLCHAIN_NONE)
 		opts->call_graph = CALLCHAIN_FP;
 
@@ -782,6 +786,16 @@ int record_callchain_opt(const struct option *opt,
 	return 0;
 }
 
+static int perf_record_config(const char *var, const char *value, void *cb)
+{
+	struct record *rec = cb;
+
+	if (!strcmp(var, "record.call-graph"))
+		return record_parse_callchain(value, &rec->opts);
+
+	return perf_default_config(var, value, cb);
+}
+
 static const char * const record_usage[] = {
 	"perf record [<options>] [<command>]",
 	"perf record [<options>] -- <command> [<options>]",
@@ -913,6 +927,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (rec->evlist == NULL)
 		return -ENOMEM;
 
+	perf_config(perf_record_config, rec);
+
 	argc = parse_options(argc, argv, record_options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
 	if (!argc && target__none(&rec->opts.target))
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 76cd510..ed99ec4 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -991,6 +991,16 @@ parse_callchain_opt(const struct option *opt, const char *arg, int unset)
 	return record_parse_callchain_opt(opt, arg, unset);
 }
 
+static int perf_top_config(const char *var, const char *value, void *cb)
+{
+	struct perf_top *top = cb;
+
+	if (!strcmp(var, "top.call-graph"))
+		return record_parse_callchain(value, &top->record_opts);
+
+	return perf_default_config(var, value, cb);
+}
+
 static int
 parse_percent_limit(const struct option *opt, const char *arg,
 		    int unset __maybe_unused)
@@ -1115,6 +1125,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (top.evlist == NULL)
 		return -ENOMEM;
 
+	perf_config(perf_top_config, &top);
+
 	argc = parse_options(argc, argv, options, top_usage, 0);
 	if (argc)
 		usage_with_options(top_usage, options);
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 7daa806..6dabc15 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -257,6 +257,7 @@ enum perf_call_graph_mode {
 struct record_opts {
 	struct target target;
 	int	     call_graph;
+	bool         call_graph_enabled;
 	bool	     group;
 	bool	     inherit_stat;
 	bool	     no_buffering;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index c6f8ce9..8201abe 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -595,7 +595,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 		attr->mmap_data = track;
 	}
 
-	if (opts->call_graph) {
+	if (opts->call_graph_enabled) {
 		perf_evsel__set_sample_bit(evsel, CALLCHAIN);
 
 		if (opts->call_graph == CALLCHAIN_DWARF) {
-- 
1.8.3.1

[tip:perf/core] perf tools: Add call-graph option support into .perfconfig

[tip-bot for Jiri Olsa]

Commit-ID:  eb853e80324fa87faf7ae7e1a763ad643f908f2d
Gitweb:     http://git.kernel.org/tip/eb853e80324fa87faf7ae7e1a763ad643f908f2d
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 3 Feb 2014 12:44:42 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 18 Feb 2014 09:34:47 -0300

perf tools: Add call-graph option support into .perfconfig

Adding call-graph option support into .perfconfig file, so it's now
possible use call-graph option like:

  [top]
        call-graph = fp

  [record]
        call-graph = dwarf,8192

Above options ONLY setup the unwind method. To enable perf record/top to
actually use it the command line option -g/-G must be specified.

The --call-graph option overloads .perfconfig setup.

Assuming above configuration:

  $ perf record -g ls
  - enables dwarf unwind with user stack size dump 8192 bytes

  $ perf top -G
  - enables frame pointer unwind

  $ perf record --call-graph=fp ls
  - enables frame pointer unwind

  $ perf top --call-graph=dwarf,4096 ls
  - enables dwarf unwind with user stack size dump 4096 bytes

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1391427883-13443-2-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 16 ++++++++++++++++
 tools/perf/builtin-top.c    | 12 ++++++++++++
 tools/perf/perf.h           |  1 +
 tools/perf/util/evsel.c     |  2 +-
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index af47531..be9e8bc 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -749,6 +749,8 @@ int record_parse_callchain_opt(const struct option *opt,
 	struct record_opts *opts = opt->value;
 	int ret;
 
+	opts->call_graph_enabled = !unset;
+
 	/* --no-call-graph */
 	if (unset) {
 		opts->call_graph = CALLCHAIN_NONE;
@@ -769,6 +771,8 @@ int record_callchain_opt(const struct option *opt,
 {
 	struct record_opts *opts = opt->value;
 
+	opts->call_graph_enabled = !unset;
+
 	if (opts->call_graph == CALLCHAIN_NONE)
 		opts->call_graph = CALLCHAIN_FP;
 
@@ -776,6 +780,16 @@ int record_callchain_opt(const struct option *opt,
 	return 0;
 }
 
+static int perf_record_config(const char *var, const char *value, void *cb)
+{
+	struct record *rec = cb;
+
+	if (!strcmp(var, "record.call-graph"))
+		return record_parse_callchain(value, &rec->opts);
+
+	return perf_default_config(var, value, cb);
+}
+
 static const char * const record_usage[] = {
 	"perf record [<options>] [<command>]",
 	"perf record [<options>] -- <command> [<options>]",
@@ -907,6 +921,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (rec->evlist == NULL)
 		return -ENOMEM;
 
+	perf_config(perf_record_config, rec);
+
 	argc = parse_options(argc, argv, record_options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
 	if (!argc && target__none(&rec->opts.target))
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 76cd510..ed99ec4 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -991,6 +991,16 @@ parse_callchain_opt(const struct option *opt, const char *arg, int unset)
 	return record_parse_callchain_opt(opt, arg, unset);
 }
 
+static int perf_top_config(const char *var, const char *value, void *cb)
+{
+	struct perf_top *top = cb;
+
+	if (!strcmp(var, "top.call-graph"))
+		return record_parse_callchain(value, &top->record_opts);
+
+	return perf_default_config(var, value, cb);
+}
+
 static int
 parse_percent_limit(const struct option *opt, const char *arg,
 		    int unset __maybe_unused)
@@ -1115,6 +1125,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (top.evlist == NULL)
 		return -ENOMEM;
 
+	perf_config(perf_top_config, &top);
+
 	argc = parse_options(argc, argv, options, top_usage, 0);
 	if (argc)
 		usage_with_options(top_usage, options);
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index e84fa26..2078f33 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -257,6 +257,7 @@ enum perf_call_graph_mode {
 struct record_opts {
 	struct target target;
 	int	     call_graph;
+	bool         call_graph_enabled;
 	bool	     group;
 	bool	     inherit_stat;
 	bool	     no_buffering;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index c6f8ce9..8201abe 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -595,7 +595,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 		attr->mmap_data = track;
 	}
 
-	if (opts->call_graph) {
+	if (opts->call_graph_enabled) {
 		perf_evsel__set_sample_bit(evsel, CALLCHAIN);
 
 		if (opts->call_graph == CALLCHAIN_DWARF) {

[PATCH 3/3] perf tools: Add readable output for callchain debug

[Jiri Olsa]

Adding people readable output for callchain debug,
to get following '-v' output:

  $ perf record -v -g ls
  callchain: type DWARF
  callchain: stack dump size 4096
  ...

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-record.c | 4 +++-
 tools/perf/perf.h           | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 3f7ec8a..679758f 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -741,7 +741,9 @@ int record_parse_callchain(const char *arg, struct record_opts *opts)
 
 static void callchain_debug(struct record_opts *opts)
 {
-	pr_debug("callchain: type %d\n", opts->call_graph);
+	static const char *str[CALLCHAIN_MAX] = { "NONE", "FP", "DWARF" };
+
+	pr_debug("callchain: type %s\n", str[opts->call_graph]);
 
 	if (opts->call_graph == CALLCHAIN_DWARF)
 		pr_debug("callchain: stack dump size %d\n",
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 6dabc15..c30e1ae 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -251,7 +251,8 @@ void pthread__unblock_sigwinch(void);
 enum perf_call_graph_mode {
 	CALLCHAIN_NONE,
 	CALLCHAIN_FP,
-	CALLCHAIN_DWARF
+	CALLCHAIN_DWARF,
+	CALLCHAIN_MAX
 };
 
 struct record_opts {
-- 
1.8.3.1

Re: [PATCH 3/3] perf tools: Add readable output for callchain debug

[Arnaldo Carvalho de Melo]

Em Mon, Feb 03, 2014 at 12:44:43PM +0100, Jiri Olsa escreveu:
> Adding people readable output for callchain debug,
> to get following '-v' output:
> 
>   $ perf record -v -g ls
>   callchain: type DWARF
>   callchain: stack dump size 4096
>   ...

Applied, but then I tried:

[acme@zoo linux]$ perf evlist -v
cycles: sample_freq=4000, size: 96, sample_type:
IP|TID|TIME|CALLCHAIN|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1,
freq: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
[acme@zoo linux]$ 


I.e. using the tool that provides info about 'perf.data' files, and
there I couldn't figure it out which kind of callchains was used, can
you fix it there too?

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/builtin-record.c | 4 +++-
>  tools/perf/perf.h           | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 3f7ec8a..679758f 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -741,7 +741,9 @@ int record_parse_callchain(const char *arg, struct record_opts *opts)
>  
>  static void callchain_debug(struct record_opts *opts)
>  {
> -	pr_debug("callchain: type %d\n", opts->call_graph);
> +	static const char *str[CALLCHAIN_MAX] = { "NONE", "FP", "DWARF" };
> +
> +	pr_debug("callchain: type %s\n", str[opts->call_graph]);
>  
>  	if (opts->call_graph == CALLCHAIN_DWARF)
>  		pr_debug("callchain: stack dump size %d\n",
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index 6dabc15..c30e1ae 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -251,7 +251,8 @@ void pthread__unblock_sigwinch(void);
>  enum perf_call_graph_mode {
>  	CALLCHAIN_NONE,
>  	CALLCHAIN_FP,
> -	CALLCHAIN_DWARF
> +	CALLCHAIN_DWARF,
> +	CALLCHAIN_MAX
>  };
>  
>  struct record_opts {
> -- 
> 1.8.3.1

Re: [PATCH 3/3] perf tools: Add readable output for callchain debug

[Jiri Olsa]

On Mon, Feb 03, 2014 at 01:09:15PM -0200, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 03, 2014 at 12:44:43PM +0100, Jiri Olsa escreveu:
> > Adding people readable output for callchain debug,
> > to get following '-v' output:
> > 
> >   $ perf record -v -g ls
> >   callchain: type DWARF
> >   callchain: stack dump size 4096
> >   ...
> 
> Applied, but then I tried:
> 
> [acme@zoo linux]$ perf evlist -v
> cycles: sample_freq=4000, size: 96, sample_type:
> IP|TID|TIME|CALLCHAIN|PERIOD, disabled: 1, inherit: 1, mmap: 1, comm: 1,
> freq: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> [acme@zoo linux]$ 
> 
> 
> I.e. using the tool that provides info about 'perf.data' files, and
> there I couldn't figure it out which kind of callchains was used, can
> you fix it there too?

todo updated ;-)

jirka

[tip:perf/core] perf record: Add readable output for callchain debug

[tip-bot for Jiri Olsa]

Commit-ID:  a601fdff1af20ea0208e918f5e97a247a3c37a40
Gitweb:     http://git.kernel.org/tip/a601fdff1af20ea0208e918f5e97a247a3c37a40
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 3 Feb 2014 12:44:43 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 18 Feb 2014 09:34:47 -0300

perf record: Add readable output for callchain debug

Adding people readable output for callchain debug, to get following '-v'
output:

  $ perf record -v -g ls
  callchain: type DWARF
  callchain: stack dump size 4096
  ...

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1391427883-13443-3-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 4 +++-
 tools/perf/perf.h           | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index be9e8bc..7b8f0e6 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -735,7 +735,9 @@ int record_parse_callchain(const char *arg, struct record_opts *opts)
 
 static void callchain_debug(struct record_opts *opts)
 {
-	pr_debug("callchain: type %d\n", opts->call_graph);
+	static const char *str[CALLCHAIN_MAX] = { "NONE", "FP", "DWARF" };
+
+	pr_debug("callchain: type %s\n", str[opts->call_graph]);
 
 	if (opts->call_graph == CALLCHAIN_DWARF)
 		pr_debug("callchain: stack dump size %d\n",
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 2078f33..6898ad0 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -251,7 +251,8 @@ void pthread__unblock_sigwinch(void);
 enum perf_call_graph_mode {
 	CALLCHAIN_NONE,
 	CALLCHAIN_FP,
-	CALLCHAIN_DWARF
+	CALLCHAIN_DWARF,
+	CALLCHAIN_MAX
 };
 
 struct record_opts {

Re: [PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type

[Namhyung Kim]

Hi Jiri,

On Mon,  3 Feb 2014 12:44:41 +0100, Jiri Olsa wrote:
> We use PERF_SAMPLE_PERIOD sample type only for frequency
> setup -F (default) option. The -c does not need store period,
> because it's always the same.
>
> In -c case the report code uses '1' as  period. Fixing
> it to perf_event_attr::sample_period.

All 3 patches look good.  But I found something strange.  When we
setup/config evsel attrs following code is used:

 util/evsel.c::perf_evsel__config()

	/*
	 * We default some events to a 1 default interval. But keep
	 * it a weak assumption overridable by the user.
	 */
	if (!attr->sample_period || (opts->user_freq != UINT_MAX &&
				     opts->user_interval != ULLONG_MAX)) {
		if (opts->freq) {
			perf_evsel__set_sample_bit(evsel, PERIOD);
			attr->freq		= 1;
			attr->sample_freq	= opts->freq;
		} else {
			attr->sample_period = opts->default_interval;
		}
	}


But shouldn't it be "||" instead of "&&" for checking
user_freq/interval?  The opts->freq was setup by code below

  util/record.c::record_opts__config_freq()

	bool user_freq = opts->user_freq != UINT_MAX;
	unsigned int max_rate;

	if (opts->user_interval != ULLONG_MAX)
		opts->default_interval = opts->user_interval;
	if (user_freq)
		opts->freq = opts->user_freq;

	/*
	 * User specified count overrides default frequency.
	 */
	if (opts->default_interval)
		opts->freq = 0;
	else if (opts->freq) {
		opts->default_interval = opts->freq;
	} else {
		pr_err("frequency and count are zero, aborting\n");
		return -1;
	}


Thanks,
Namhyung

Re: [PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type

[Jiri Olsa]

On Wed, Feb 05, 2014 at 10:27:30AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Mon,  3 Feb 2014 12:44:41 +0100, Jiri Olsa wrote:
> > We use PERF_SAMPLE_PERIOD sample type only for frequency
> > setup -F (default) option. The -c does not need store period,
> > because it's always the same.
> >
> > In -c case the report code uses '1' as  period. Fixing
> > it to perf_event_attr::sample_period.
> 
> All 3 patches look good.  But I found something strange.  When we
> setup/config evsel attrs following code is used:
> 
>  util/evsel.c::perf_evsel__config()
> 
> 	/*
> 	 * We default some events to a 1 default interval. But keep
> 	 * it a weak assumption overridable by the user.
> 	 */
> 	if (!attr->sample_period || (opts->user_freq != UINT_MAX &&
> 				     opts->user_interval != ULLONG_MAX)) {
> 		if (opts->freq) {
> 			perf_evsel__set_sample_bit(evsel, PERIOD);
> 			attr->freq		= 1;
> 			attr->sample_freq	= opts->freq;
> 		} else {
> 			attr->sample_period = opts->default_interval;
> 		}
> 	}

yes, I think thats right.. we should use || instead of &&

It will allow to change period for event types with predefined
attr->sample_period like tracepoints.

However, I tried with tracepoints and even with this fix
and following command line:

  # perf record -e syscalls:sys_enter_read -c 2 ls

you'll still get samples with period 1. The reason is in
kernel code:

static void perf_swevent_event(struct perf_event *event, u64 nr,
                               struct perf_sample_data *data,
                               struct pt_regs *regs)
{
...
        if ((event->attr.sample_type & PERF_SAMPLE_PERIOD) && !event->attr.freq) {
                data->period = nr;
                return perf_swevent_overflow(event, 1, data, regs);

bacause above condition is true for tracepoints.

It looks like a bug, but I'm not sure how handy it'd be
set period other than 1 for tracepoints thought.. ;)

Maybe it's not that big issue in comparison of screwing
up other software events processing.

I haven't checked other software events.

jirka

Re: [PATCH 1/3] perf tools: Put proper period for for samples without PERIOD sample_type

[Namhyung Kim]

Hi Jiri,

On Wed, 5 Feb 2014 15:33:29 +0100, Jiri Olsa wrote:
> On Wed, Feb 05, 2014 at 10:27:30AM +0900, Namhyung Kim wrote:
>> Hi Jiri,
>> 
>> On Mon,  3 Feb 2014 12:44:41 +0100, Jiri Olsa wrote:
>> > We use PERF_SAMPLE_PERIOD sample type only for frequency
>> > setup -F (default) option. The -c does not need store period,
>> > because it's always the same.
>> >
>> > In -c case the report code uses '1' as  period. Fixing
>> > it to perf_event_attr::sample_period.
>> 
>> All 3 patches look good.  But I found something strange.  When we
>> setup/config evsel attrs following code is used:
>> 
>>  util/evsel.c::perf_evsel__config()
>> 
>> 	/*
>> 	 * We default some events to a 1 default interval. But keep
>> 	 * it a weak assumption overridable by the user.
>> 	 */
>> 	if (!attr->sample_period || (opts->user_freq != UINT_MAX &&
>> 				     opts->user_interval != ULLONG_MAX)) {
>> 		if (opts->freq) {
>> 			perf_evsel__set_sample_bit(evsel, PERIOD);
>> 			attr->freq		= 1;
>> 			attr->sample_freq	= opts->freq;
>> 		} else {
>> 			attr->sample_period = opts->default_interval;
>> 		}
>> 	}
>
> yes, I think thats right.. we should use || instead of &&
>
> It will allow to change period for event types with predefined
> attr->sample_period like tracepoints.

Right.  As I read the code, it works "if (!attr->sample_period)" case only.

>
> However, I tried with tracepoints and even with this fix
> and following command line:
>
>   # perf record -e syscalls:sys_enter_read -c 2 ls
>
> you'll still get samples with period 1. The reason is in
> kernel code:
>
> static void perf_swevent_event(struct perf_event *event, u64 nr,
>                                struct perf_sample_data *data,
>                                struct pt_regs *regs)
> {
> ...
>         if ((event->attr.sample_type & PERF_SAMPLE_PERIOD) && !event->attr.freq) {
>                 data->period = nr;
>                 return perf_swevent_overflow(event, 1, data, regs);
>
> bacause above condition is true for tracepoints.
>
> It looks like a bug, but I'm not sure how handy it'd be
> set period other than 1 for tracepoints thought.. ;)

Agreed.  But at least we should support whatever user wants IMHO..

>
> Maybe it's not that big issue in comparison of screwing
> up other software events processing.

:)

Thanks,
Namhyung

[tip:perf/core] perf tools: Put proper period for for samples without PERIOD sample_type

[tip-bot for Jiri Olsa]

Commit-ID:  bc5290869d0a7f7abbde76ac95a7f7b6f5d7bb7b
Gitweb:     http://git.kernel.org/tip/bc5290869d0a7f7abbde76ac95a7f7b6f5d7bb7b
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 3 Feb 2014 12:44:41 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 18 Feb 2014 09:34:46 -0300

perf tools: Put proper period for for samples without PERIOD sample_type

We use PERF_SAMPLE_PERIOD sample type only for frequency
setup -F (default) option. The -c does not need store period,
because it's always the same.

In -c case the report code uses '1' as  period. Fixing
it to perf_event_attr::sample_period.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1391427883-13443-1-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 55407c5..c6f8ce9 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1220,7 +1220,7 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 	memset(data, 0, sizeof(*data));
 	data->cpu = data->pid = data->tid = -1;
 	data->stream_id = data->id = data->time = -1ULL;
-	data->period = 1;
+	data->period = evsel->attr.sample_period;
 	data->weight = 0;
 
 	if (event->header.type != PERF_RECORD_SAMPLE) {