BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/5] Fixes for setting event freq/periods
@ 2020-07-28  8:57 Ian Rogers
  2020-07-28  8:57 ` [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set Ian Rogers
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Ian Rogers @ 2020-07-28  8:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf
  Cc: Stephane Eranian, Ian Rogers

Some fixes that address issues for regular and pfm4 events with 2
additional perf_event_attr tests. Various authors, David Sharp isn't
currently at Google.

v2. corrects the commit message following Athira Rajeev's suggestion.

David Sharp (1):
  perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

Ian Rogers (3):
  perf test: Ensure sample_period is set libpfm4 events
  perf record: Don't clear event's period if set by a term
  perf test: Leader sampling shouldn't clear sample period

Stephane Eranian (1):
  perf record: Prevent override of attr->sample_period for libpfm4
    events

 tools/perf/tests/attr/README                 |  2 ++
 tools/perf/tests/attr/test-record-group2     | 29 ++++++++++++++++++++
 tools/perf/tests/attr/test-record-pfm-period |  9 ++++++
 tools/perf/util/evsel.c                      | 10 +++++--
 tools/perf/util/record.c                     | 28 +++++++++++++------
 5 files changed, 67 insertions(+), 11 deletions(-)
 create mode 100644 tools/perf/tests/attr/test-record-group2
 create mode 100644 tools/perf/tests/attr/test-record-pfm-period

-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.
  2020-07-28  8:57 [PATCH v2 0/5] Fixes for setting event freq/periods Ian Rogers
@ 2020-07-28  8:57 ` Ian Rogers
  2020-07-28 15:43   ` Jiri Olsa
  2020-07-29 18:52   ` Arnaldo Carvalho de Melo
  2020-07-28  8:57 ` [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events Ian Rogers
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Ian Rogers @ 2020-07-28  8:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf
  Cc: Stephane Eranian, David Sharp, Ian Rogers

From: David Sharp <dhsharp@google.com>

evsel__config() would only set PERF_RECORD_PERIOD if it set attr->freq
from perf record options. When it is set by libpfm events, it would not
get set. This changes evsel__config to see if attr->freq is set outside of
whether or not it changes attr->freq itself.

Signed-off-by: David Sharp <dhsharp@google.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ef802f6d40c1..811f538f7d77 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -979,13 +979,18 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
 	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
 				     opts->user_interval != ULLONG_MAX)) {
 		if (opts->freq) {
-			evsel__set_sample_bit(evsel, PERIOD);
 			attr->freq		= 1;
 			attr->sample_freq	= opts->freq;
 		} else {
 			attr->sample_period = opts->default_interval;
 		}
 	}
+	/*
+	 * If attr->freq was set (here or earlier), ask for period
+	 * to be sampled.
+	 */
+	if (attr->freq)
+		evsel__set_sample_bit(evsel, PERIOD);
 
 	if (opts->no_samples)
 		attr->sample_freq = 0;
-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events
  2020-07-28  8:57 [PATCH v2 0/5] Fixes for setting event freq/periods Ian Rogers
  2020-07-28  8:57 ` [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set Ian Rogers
@ 2020-07-28  8:57 ` Ian Rogers
  2020-07-28 15:59   ` Jiri Olsa
  2020-07-29 18:54   ` Arnaldo Carvalho de Melo
  2020-07-28  8:57 ` [PATCH v2 3/5] perf test: Ensure sample_period is set " Ian Rogers
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Ian Rogers @ 2020-07-28  8:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf
  Cc: Stephane Eranian, Ian Rogers

From: Stephane Eranian <eranian@google.com>

Before:
$ perf record -c 10000 --pfm-events=cycles:period=77777

Would yield a cycles event with period=10000, instead of 77777.

This was due to an ordering issue between libpfm4 parsing
the event string and perf record initializing the event.

This patch fixes the problem by preventing override for
events with attr->sample_period != 0 by the time
perf_evsel__config() is invoked. This seems to have been the
intent of the author.

Signed-off-by: Stephane Eranian <eranian@google.com>
Reviewed-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 811f538f7d77..8afc24e2ec52 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -976,8 +976,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
 	 * 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 ||
-				     opts->user_interval != ULLONG_MAX)) {
+	if (!attr->sample_period) {
 		if (opts->freq) {
 			attr->freq		= 1;
 			attr->sample_freq	= opts->freq;
-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 3/5] perf test: Ensure sample_period is set libpfm4 events
  2020-07-28  8:57 [PATCH v2 0/5] Fixes for setting event freq/periods Ian Rogers
  2020-07-28  8:57 ` [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set Ian Rogers
  2020-07-28  8:57 ` [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events Ian Rogers
@ 2020-07-28  8:57 ` Ian Rogers
  2020-07-28 12:45   ` Arnaldo Carvalho de Melo
  2020-07-28  8:57 ` [PATCH v2 4/5] perf record: Don't clear event's period if set by a term Ian Rogers
  2020-07-28  8:57 ` [PATCH v2 5/5] perf test: Leader sampling shouldn't clear sample period Ian Rogers
  4 siblings, 1 reply; 21+ messages in thread
From: Ian Rogers @ 2020-07-28  8:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf
  Cc: Stephane Eranian, Ian Rogers

Test that a command line option doesn't override the period set on a
libpfm4 event.
Without libpfm4 test passes as unsupported.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/attr/README                 | 1 +
 tools/perf/tests/attr/test-record-pfm-period | 9 +++++++++
 2 files changed, 10 insertions(+)
 create mode 100644 tools/perf/tests/attr/test-record-pfm-period

diff --git a/tools/perf/tests/attr/README b/tools/perf/tests/attr/README
index 430024f618f1..6cd408108595 100644
--- a/tools/perf/tests/attr/README
+++ b/tools/perf/tests/attr/README
@@ -53,6 +53,7 @@ Following tests are defined (with perf commands):
   perf record -i kill                           (test-record-no-inherit)
   perf record -n kill                           (test-record-no-samples)
   perf record -c 100 -P kill                    (test-record-period)
+  perf record -c 1 --pfm-events=cycles:period=2 (test-record-pfm-period)
   perf record -R kill                           (test-record-raw)
   perf stat -e cycles kill                      (test-stat-basic)
   perf stat kill                                (test-stat-default)
diff --git a/tools/perf/tests/attr/test-record-pfm-period b/tools/perf/tests/attr/test-record-pfm-period
new file mode 100644
index 000000000000..368f5b814094
--- /dev/null
+++ b/tools/perf/tests/attr/test-record-pfm-period
@@ -0,0 +1,9 @@
+[config]
+command = record
+args    = --no-bpf-event -c 10000 --pfm-events=cycles:period=77777 kill >/dev/null 2>&1
+ret     = 1
+
+[event:base-record]
+sample_period=77777
+sample_type=7
+freq=0
-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 4/5] perf record: Don't clear event's period if set by a term
  2020-07-28  8:57 [PATCH v2 0/5] Fixes for setting event freq/periods Ian Rogers
                   ` (2 preceding siblings ...)
  2020-07-28  8:57 ` [PATCH v2 3/5] perf test: Ensure sample_period is set " Ian Rogers
@ 2020-07-28  8:57 ` Ian Rogers
  2020-07-29 18:58   ` Arnaldo Carvalho de Melo
  2020-08-04 10:08   ` Adrian Hunter
  2020-07-28  8:57 ` [PATCH v2 5/5] perf test: Leader sampling shouldn't clear sample period Ian Rogers
  4 siblings, 2 replies; 21+ messages in thread
From: Ian Rogers @ 2020-07-28  8:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf
  Cc: Stephane Eranian, Ian Rogers

If events in a group explicitly set a frequency or period with leader
sampling, don't disable the samples on those events.

Prior to 5.8:
perf record -e '{cycles/period=12345000/,instructions/period=6789000/}:S'
would clear the attributes then apply the config terms. In commit
5f34278867b7 leader sampling configuration was moved to after applying the
config terms, in the example, making the instructions' event have its period
cleared.
This change makes it so that sampling is only disabled if configuration
terms aren't present.

Fixes: 5f34278867b7 ("perf evlist: Move leader-sampling configuration")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/record.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
index a4cc11592f6b..01d1c6c613f7 100644
--- a/tools/perf/util/record.c
+++ b/tools/perf/util/record.c
@@ -2,6 +2,7 @@
 #include "debug.h"
 #include "evlist.h"
 #include "evsel.h"
+#include "evsel_config.h"
 #include "parse-events.h"
 #include <errno.h>
 #include <limits.h>
@@ -38,6 +39,9 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
 	struct perf_event_attr *attr = &evsel->core.attr;
 	struct evsel *leader = evsel->leader;
 	struct evsel *read_sampler;
+	struct evsel_config_term *term;
+	struct list_head *config_terms = &evsel->config_terms;
+	int term_types, freq_mask;
 
 	if (!leader->sample_read)
 		return;
@@ -47,16 +51,24 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
 	if (evsel == read_sampler)
 		return;
 
+	/* Determine the evsel's config term types. */
+	term_types = 0;
+	list_for_each_entry(term, config_terms, list) {
+		term_types |= 1 << term->type;
+	}
 	/*
-	 * Disable sampling for all group members other than the leader in
-	 * case the leader 'leads' the sampling, except when the leader is an
-	 * AUX area event, in which case the 2nd event in the group is the one
-	 * that 'leads' the sampling.
+	 * Disable sampling for all group members except those with explicit
+	 * config terms or the leader. In the case of an AUX area event, the 2nd
+	 * event in the group is the one that 'leads' the sampling.
 	 */
-	attr->freq           = 0;
-	attr->sample_freq    = 0;
-	attr->sample_period  = 0;
-	attr->write_backward = 0;
+	freq_mask = (1 << EVSEL__CONFIG_TERM_FREQ) | (1 << EVSEL__CONFIG_TERM_PERIOD);
+	if ((term_types & freq_mask) == 0) {
+		attr->freq           = 0;
+		attr->sample_freq    = 0;
+		attr->sample_period  = 0;
+	}
+	if ((term_types & (1 << EVSEL__CONFIG_TERM_OVERWRITE)) == 0)
+		attr->write_backward = 0;
 
 	/*
 	 * We don't get a sample for slave events, we make them when delivering
-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 5/5] perf test: Leader sampling shouldn't clear sample period
  2020-07-28  8:57 [PATCH v2 0/5] Fixes for setting event freq/periods Ian Rogers
                   ` (3 preceding siblings ...)
  2020-07-28  8:57 ` [PATCH v2 4/5] perf record: Don't clear event's period if set by a term Ian Rogers
@ 2020-07-28  8:57 ` Ian Rogers
  4 siblings, 0 replies; 21+ messages in thread
From: Ian Rogers @ 2020-07-28  8:57 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf
  Cc: Stephane Eranian, Ian Rogers

Add test that a sibling with leader sampling doesn't have its period
cleared.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/attr/README             |  1 +
 tools/perf/tests/attr/test-record-group2 | 29 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 tools/perf/tests/attr/test-record-group2

diff --git a/tools/perf/tests/attr/README b/tools/perf/tests/attr/README
index 6cd408108595..a36f49fb4dbe 100644
--- a/tools/perf/tests/attr/README
+++ b/tools/perf/tests/attr/README
@@ -49,6 +49,7 @@ Following tests are defined (with perf commands):
   perf record --call-graph fp kill              (test-record-graph-fp)
   perf record --group -e cycles,instructions kill (test-record-group)
   perf record -e '{cycles,instructions}' kill   (test-record-group1)
+  perf record -e '{cycles/period=1/,instructions/period=2/}:S' kill (test-record-group2)
   perf record -D kill                           (test-record-no-delay)
   perf record -i kill                           (test-record-no-inherit)
   perf record -n kill                           (test-record-no-samples)
diff --git a/tools/perf/tests/attr/test-record-group2 b/tools/perf/tests/attr/test-record-group2
new file mode 100644
index 000000000000..6b9f8d182ce1
--- /dev/null
+++ b/tools/perf/tests/attr/test-record-group2
@@ -0,0 +1,29 @@
+[config]
+command = record
+args    = --no-bpf-event -e '{cycles/period=1234000/,instructions/period=6789000/}:S' kill >/dev/null 2>&1
+ret     = 1
+
+[event-1:base-record]
+fd=1
+group_fd=-1
+config=0|1
+sample_period=1234000
+sample_type=87
+read_format=12
+inherit=0
+freq=0
+
+[event-2:base-record]
+fd=2
+group_fd=1
+config=0|1
+sample_period=6789000
+sample_type=87
+read_format=12
+disabled=0
+inherit=0
+mmap=0
+comm=0
+freq=0
+enable_on_exec=0
+task=0
-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/5] perf test: Ensure sample_period is set libpfm4 events
  2020-07-28  8:57 ` [PATCH v2 3/5] perf test: Ensure sample_period is set " Ian Rogers
@ 2020-07-28 12:45   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-07-28 12:45 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Adrian Hunter, Andi Kleen,
	Athira Rajeev, linux-kernel, netdev, bpf, Stephane Eranian

Em Tue, Jul 28, 2020 at 01:57:32AM -0700, Ian Rogers escreveu:
> Test that a command line option doesn't override the period set on a
> libpfm4 event.
> Without libpfm4 test passes as unsupported.

Thanks, applied.

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.
  2020-07-28  8:57 ` [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set Ian Rogers
@ 2020-07-28 15:43   ` Jiri Olsa
  2020-07-28 16:03     ` Arnaldo Carvalho de Melo
  2020-07-29 18:52   ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2020-07-28 15:43 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf, Stephane Eranian, David Sharp

On Tue, Jul 28, 2020 at 01:57:30AM -0700, Ian Rogers wrote:
> From: David Sharp <dhsharp@google.com>
> 
> evsel__config() would only set PERF_RECORD_PERIOD if it set attr->freq
> from perf record options. When it is set by libpfm events, it would not
> get set. This changes evsel__config to see if attr->freq is set outside of
> whether or not it changes attr->freq itself.
> 
> Signed-off-by: David Sharp <dhsharp@google.com>
> Signed-off-by: Ian Rogers <irogers@google.com>

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

thanks,
jirka

> ---
>  tools/perf/util/evsel.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ef802f6d40c1..811f538f7d77 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -979,13 +979,18 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
>  	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
>  				     opts->user_interval != ULLONG_MAX)) {
>  		if (opts->freq) {
> -			evsel__set_sample_bit(evsel, PERIOD);
>  			attr->freq		= 1;
>  			attr->sample_freq	= opts->freq;
>  		} else {
>  			attr->sample_period = opts->default_interval;
>  		}
>  	}
> +	/*
> +	 * If attr->freq was set (here or earlier), ask for period
> +	 * to be sampled.
> +	 */
> +	if (attr->freq)
> +		evsel__set_sample_bit(evsel, PERIOD);
>  
>  	if (opts->no_samples)
>  		attr->sample_freq = 0;
> -- 
> 2.28.0.163.g6104cc2f0b6-goog
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events
  2020-07-28  8:57 ` [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events Ian Rogers
@ 2020-07-28 15:59   ` Jiri Olsa
  2020-07-28 16:09     ` Jiri Olsa
  2020-07-29 18:54   ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2020-07-28 15:59 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf, Stephane Eranian

On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote:
> From: Stephane Eranian <eranian@google.com>
> 
> Before:
> $ perf record -c 10000 --pfm-events=cycles:period=77777
> 
> Would yield a cycles event with period=10000, instead of 77777.
> 
> This was due to an ordering issue between libpfm4 parsing
> the event string and perf record initializing the event.
> 
> This patch fixes the problem by preventing override for
> events with attr->sample_period != 0 by the time
> perf_evsel__config() is invoked. This seems to have been the
> intent of the author.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 811f538f7d77..8afc24e2ec52 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -976,8 +976,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
>  	 * 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 ||
> -				     opts->user_interval != ULLONG_MAX)) {
> +	if (!attr->sample_period) {

I was wondering why this wouldn't break record/top
but we take care of the via record_opts__config

as long as 'perf test attr' works it looks ok to me

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

thanks,
jirka


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.
  2020-07-28 15:43   ` Jiri Olsa
@ 2020-07-28 16:03     ` Arnaldo Carvalho de Melo
  2020-07-29 15:11       ` Athira Rajeev
  0 siblings, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-07-28 16:03 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, John Fastabend, KP Singh, Adrian Hunter,
	Andi Kleen, Athira Rajeev, linux-kernel, netdev, bpf,
	Stephane Eranian, David Sharp

Em Tue, Jul 28, 2020 at 05:43:47PM +0200, Jiri Olsa escreveu:
> On Tue, Jul 28, 2020 at 01:57:30AM -0700, Ian Rogers wrote:
> > From: David Sharp <dhsharp@google.com>
> > 
> > evsel__config() would only set PERF_RECORD_PERIOD if it set attr->freq
> > from perf record options. When it is set by libpfm events, it would not
> > get set. This changes evsel__config to see if attr->freq is set outside of
> > whether or not it changes attr->freq itself.
> > 
> > Signed-off-by: David Sharp <dhsharp@google.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

So, somebody else complained that its not PERF_RECORD_PERIOD (there is
no such thing) that is being set, its PERF_SAMPLE_PERIOD.

Since you acked it I merged it now, with that correction,

- Arnaldo
 
> thanks,
> jirka
> 
> > ---
> >  tools/perf/util/evsel.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index ef802f6d40c1..811f538f7d77 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -979,13 +979,18 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
> >  	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
> >  				     opts->user_interval != ULLONG_MAX)) {
> >  		if (opts->freq) {
> > -			evsel__set_sample_bit(evsel, PERIOD);
> >  			attr->freq		= 1;
> >  			attr->sample_freq	= opts->freq;
> >  		} else {
> >  			attr->sample_period = opts->default_interval;
> >  		}
> >  	}
> > +	/*
> > +	 * If attr->freq was set (here or earlier), ask for period
> > +	 * to be sampled.
> > +	 */
> > +	if (attr->freq)
> > +		evsel__set_sample_bit(evsel, PERIOD);
> >  
> >  	if (opts->no_samples)
> >  		attr->sample_freq = 0;
> > -- 
> > 2.28.0.163.g6104cc2f0b6-goog
> > 
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events
  2020-07-28 15:59   ` Jiri Olsa
@ 2020-07-28 16:09     ` Jiri Olsa
  2020-07-29 23:24       ` Ian Rogers
  0 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2020-07-28 16:09 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
	bpf, Stephane Eranian

On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote:
> On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote:
> > From: Stephane Eranian <eranian@google.com>
> > 
> > Before:
> > $ perf record -c 10000 --pfm-events=cycles:period=77777
> > 
> > Would yield a cycles event with period=10000, instead of 77777.
> > 
> > This was due to an ordering issue between libpfm4 parsing
> > the event string and perf record initializing the event.
> > 
> > This patch fixes the problem by preventing override for
> > events with attr->sample_period != 0 by the time
> > perf_evsel__config() is invoked. This seems to have been the
> > intent of the author.
> > 
> > Signed-off-by: Stephane Eranian <eranian@google.com>
> > Reviewed-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/evsel.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 811f538f7d77..8afc24e2ec52 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -976,8 +976,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
> >  	 * 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 ||
> > -				     opts->user_interval != ULLONG_MAX)) {
> > +	if (!attr->sample_period) {
> 
> I was wondering why this wouldn't break record/top
> but we take care of the via record_opts__config
> 
> as long as 'perf test attr' works it looks ok to me

hum ;-)

[jolsa@krava perf]$ sudo ./perf test 17 -v
17: Setup struct perf_event_attr                          :
...
running './tests/attr/test-record-C0'
expected sample_period=4000, got 3000
FAILED './tests/attr/test-record-C0' - match failure

jirka

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.
  2020-07-28 16:03     ` Arnaldo Carvalho de Melo
@ 2020-07-29 15:11       ` Athira Rajeev
  0 siblings, 0 replies; 21+ messages in thread
From: Athira Rajeev @ 2020-07-29 15:11 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, John Fastabend, KP Singh, Adrian Hunter,
	Andi Kleen, linux-kernel, netdev, bpf, Stephane Eranian,
	David Sharp



> On 28-Jul-2020, at 9:33 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> Em Tue, Jul 28, 2020 at 05:43:47PM +0200, Jiri Olsa escreveu:
>> On Tue, Jul 28, 2020 at 01:57:30AM -0700, Ian Rogers wrote:
>>> From: David Sharp <dhsharp@google.com>
>>> 
>>> evsel__config() would only set PERF_RECORD_PERIOD if it set attr->freq
>>> from perf record options. When it is set by libpfm events, it would not
>>> get set. This changes evsel__config to see if attr->freq is set outside of
>>> whether or not it changes attr->freq itself.
>>> 
>>> Signed-off-by: David Sharp <dhsharp@google.com>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>> 
>> Acked-by: Jiri Olsa <jolsa@redhat.com>
> 
> So, somebody else complained that its not PERF_RECORD_PERIOD (there is
> no such thing) that is being set, its PERF_SAMPLE_PERIOD.

Hi Arnaldo

Thanks for adding in that correction.

Athira
> 
> Since you acked it I merged it now, with that correction,
> 
> - Arnaldo
> 
>> thanks,
>> jirka
>> 
>>> ---
>>> tools/perf/util/evsel.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>> index ef802f6d40c1..811f538f7d77 100644
>>> --- a/tools/perf/util/evsel.c
>>> +++ b/tools/perf/util/evsel.c
>>> @@ -979,13 +979,18 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
>>> 	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
>>> 				     opts->user_interval != ULLONG_MAX)) {
>>> 		if (opts->freq) {
>>> -			evsel__set_sample_bit(evsel, PERIOD);
>>> 			attr->freq		= 1;
>>> 			attr->sample_freq	= opts->freq;
>>> 		} else {
>>> 			attr->sample_period = opts->default_interval;
>>> 		}
>>> 	}
>>> +	/*
>>> +	 * If attr->freq was set (here or earlier), ask for period
>>> +	 * to be sampled.
>>> +	 */
>>> +	if (attr->freq)
>>> +		evsel__set_sample_bit(evsel, PERIOD);
>>> 
>>> 	if (opts->no_samples)
>>> 		attr->sample_freq = 0;
>>> -- 
>>> 2.28.0.163.g6104cc2f0b6-goog
>>> 
>> 
> 
> -- 
> 
> - Arnaldo


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.
  2020-07-28  8:57 ` [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set Ian Rogers
  2020-07-28 15:43   ` Jiri Olsa
@ 2020-07-29 18:52   ` Arnaldo Carvalho de Melo
  2020-07-29 21:43     ` Ian Rogers
  1 sibling, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-07-29 18:52 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Adrian Hunter, Andi Kleen,
	Athira Rajeev, linux-kernel, netdev, bpf, Stephane Eranian,
	David Sharp

Em Tue, Jul 28, 2020 at 01:57:30AM -0700, Ian Rogers escreveu:
> From: David Sharp <dhsharp@google.com>
> 
> evsel__config() would only set PERF_RECORD_PERIOD if it set attr->freq

There is no such thing as 'PERF_RECORD_PERIOD', its PERF_SAMPLE_PERIOD,
also...

> from perf record options. When it is set by libpfm events, it would not
> get set. This changes evsel__config to see if attr->freq is set outside of
> whether or not it changes attr->freq itself.
> 
> Signed-off-by: David Sharp <dhsharp@google.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ef802f6d40c1..811f538f7d77 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -979,13 +979,18 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
>  	if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
>  				     opts->user_interval != ULLONG_MAX)) {
>  		if (opts->freq) {
> -			evsel__set_sample_bit(evsel, PERIOD);
>  			attr->freq		= 1;
>  			attr->sample_freq	= opts->freq;
>  		} else {
>  			attr->sample_period = opts->default_interval;
>  		}
>  	}
> +	/*
> +	 * If attr->freq was set (here or earlier), ask for period
> +	 * to be sampled.
> +	 */
> +	if (attr->freq)
> +		evsel__set_sample_bit(evsel, PERIOD);

Why can't the libpfm code set opts?

With this patch we will end up calling evsel__set_sample_bit(evsel,
PERIOD) twice, which isn't a problem but looks strange.

- Arnaldo

>  
>  	if (opts->no_samples)
>  		attr->sample_freq = 0;
> -- 
> 2.28.0.163.g6104cc2f0b6-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events
  2020-07-28  8:57 ` [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events Ian Rogers
  2020-07-28 15:59   ` Jiri Olsa
@ 2020-07-29 18:54   ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-07-29 18:54 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Adrian Hunter, Andi Kleen,
	Athira Rajeev, linux-kernel, netdev, bpf, Stephane Eranian

Em Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers escreveu:
> From: Stephane Eranian <eranian@google.com>
> 
> Before:
> $ perf record -c 10000 --pfm-events=cycles:period=77777
> 
> Would yield a cycles event with period=10000, instead of 77777.

I tried the equivalent without libpfm and it works:

  $ perf record -c 10000 -e cycles/period=20000/ sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.024 MB perf.data (23 samples) ]
  $ perf evlist -v
  cycles/period=20000/u: size: 120, { sample_period, sample_freq }: 20000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 1, exclude_kernel: 1, exclude_hv: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1
  $
 
> This was due to an ordering issue between libpfm4 parsing
> the event string and perf record initializing the event.
 
> This patch fixes the problem by preventing override for
> events with attr->sample_period != 0 by the time
> perf_evsel__config() is invoked. This seems to have been the
> intent of the author.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> Reviewed-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 811f538f7d77..8afc24e2ec52 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -976,8 +976,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
>  	 * 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 ||
> -				     opts->user_interval != ULLONG_MAX)) {
> +	if (!attr->sample_period) {
>  		if (opts->freq) {
>  			attr->freq		= 1;
>  			attr->sample_freq	= opts->freq;
> -- 
> 2.28.0.163.g6104cc2f0b6-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term
  2020-07-28  8:57 ` [PATCH v2 4/5] perf record: Don't clear event's period if set by a term Ian Rogers
@ 2020-07-29 18:58   ` Arnaldo Carvalho de Melo
  2020-08-04 10:08   ` Adrian Hunter
  1 sibling, 0 replies; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-07-29 18:58 UTC (permalink / raw)
  To: Adrian Hunter, Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Andi Kleen, Athira Rajeev,
	linux-kernel, netdev, bpf, Stephane Eranian

Em Tue, Jul 28, 2020 at 01:57:33AM -0700, Ian Rogers escreveu:
> If events in a group explicitly set a frequency or period with leader
> sampling, don't disable the samples on those events.
> 
> Prior to 5.8:
> perf record -e '{cycles/period=12345000/,instructions/period=6789000/}:S'
> would clear the attributes then apply the config terms. In commit
> 5f34278867b7 leader sampling configuration was moved to after applying the
> config terms, in the example, making the instructions' event have its period
> cleared.
> This change makes it so that sampling is only disabled if configuration
> terms aren't present.

Adrian, can you take a look at this one?

- Arnaldo
 
> Fixes: 5f34278867b7 ("perf evlist: Move leader-sampling configuration")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/record.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> index a4cc11592f6b..01d1c6c613f7 100644
> --- a/tools/perf/util/record.c
> +++ b/tools/perf/util/record.c
> @@ -2,6 +2,7 @@
>  #include "debug.h"
>  #include "evlist.h"
>  #include "evsel.h"
> +#include "evsel_config.h"
>  #include "parse-events.h"
>  #include <errno.h>
>  #include <limits.h>
> @@ -38,6 +39,9 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
>  	struct perf_event_attr *attr = &evsel->core.attr;
>  	struct evsel *leader = evsel->leader;
>  	struct evsel *read_sampler;
> +	struct evsel_config_term *term;
> +	struct list_head *config_terms = &evsel->config_terms;
> +	int term_types, freq_mask;
>  
>  	if (!leader->sample_read)
>  		return;
> @@ -47,16 +51,24 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
>  	if (evsel == read_sampler)
>  		return;
>  
> +	/* Determine the evsel's config term types. */
> +	term_types = 0;
> +	list_for_each_entry(term, config_terms, list) {
> +		term_types |= 1 << term->type;
> +	}
>  	/*
> -	 * Disable sampling for all group members other than the leader in
> -	 * case the leader 'leads' the sampling, except when the leader is an
> -	 * AUX area event, in which case the 2nd event in the group is the one
> -	 * that 'leads' the sampling.
> +	 * Disable sampling for all group members except those with explicit
> +	 * config terms or the leader. In the case of an AUX area event, the 2nd
> +	 * event in the group is the one that 'leads' the sampling.
>  	 */
> -	attr->freq           = 0;
> -	attr->sample_freq    = 0;
> -	attr->sample_period  = 0;
> -	attr->write_backward = 0;
> +	freq_mask = (1 << EVSEL__CONFIG_TERM_FREQ) | (1 << EVSEL__CONFIG_TERM_PERIOD);
> +	if ((term_types & freq_mask) == 0) {
> +		attr->freq           = 0;
> +		attr->sample_freq    = 0;
> +		attr->sample_period  = 0;
> +	}
> +	if ((term_types & (1 << EVSEL__CONFIG_TERM_OVERWRITE)) == 0)
> +		attr->write_backward = 0;
>  
>  	/*
>  	 * We don't get a sample for slave events, we make them when delivering
> -- 
> 2.28.0.163.g6104cc2f0b6-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.
  2020-07-29 18:52   ` Arnaldo Carvalho de Melo
@ 2020-07-29 21:43     ` Ian Rogers
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Rogers @ 2020-07-29 21:43 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Adrian Hunter, Andi Kleen,
	Athira Rajeev, LKML, Networking, bpf, Stephane Eranian,
	David Sharp

On Wed, Jul 29, 2020 at 11:52 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Jul 28, 2020 at 01:57:30AM -0700, Ian Rogers escreveu:
> > From: David Sharp <dhsharp@google.com>
> >
> > evsel__config() would only set PERF_RECORD_PERIOD if it set attr->freq
>
> There is no such thing as 'PERF_RECORD_PERIOD', its PERF_SAMPLE_PERIOD,
> also...
>
> > from perf record options. When it is set by libpfm events, it would not
> > get set. This changes evsel__config to see if attr->freq is set outside of
> > whether or not it changes attr->freq itself.
> >
> > Signed-off-by: David Sharp <dhsharp@google.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/evsel.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index ef802f6d40c1..811f538f7d77 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -979,13 +979,18 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
> >       if (!attr->sample_period || (opts->user_freq != UINT_MAX ||
> >                                    opts->user_interval != ULLONG_MAX)) {
> >               if (opts->freq) {
> > -                     evsel__set_sample_bit(evsel, PERIOD);
> >                       attr->freq              = 1;
> >                       attr->sample_freq       = opts->freq;
> >               } else {
> >                       attr->sample_period = opts->default_interval;
> >               }
> >       }
> > +     /*
> > +      * If attr->freq was set (here or earlier), ask for period
> > +      * to be sampled.
> > +      */
> > +     if (attr->freq)
> > +             evsel__set_sample_bit(evsel, PERIOD);
>
> Why can't the libpfm code set opts?
>
> With this patch we will end up calling evsel__set_sample_bit(evsel,
> PERIOD) twice, which isn't a problem but looks strange.

Thanks Arnaldo! The case I was looking at was something like:
perf record --pfm-events cycles:freq=1000

For regular events this would be:
perf record -e cycles/freq=1000/

With libpfm4 events the perf_event_attr is set up (a public API in
linux/perf_event.h) and then parse_events__add_event is used (an
internal API) to make the evsel and this added to the evlist
(parse_libpfm_events_option). This is similar to the parse_events
function except rather than set up a perf_event_attr the regular
parsing sets up config terms that are then applied to evsel and attr
later in evsel__config, via evsel__apply_config_terms.

I think we can  update this change so that in pfm.c after
parse_events__add_event we do:
if (attr.freq)
  evsel__set_sample_bit(evsel, PERIOD);

This code could also be part of parse_events__add_event. I think the
intent in placing this code here was that it is close to the similar
evsel__apply_config_terms and setting of sample bits in the evsel. The
logic here is already dependent on reading the attr->sample_period.

I'm not sure I follow the double setting case - I think that is only
possible with a config term or with period_set (-P).

Thanks,
Ian


> - Arnaldo
>
> >
> >       if (opts->no_samples)
> >               attr->sample_freq = 0;
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
>
> --
>
> - Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events
  2020-07-28 16:09     ` Jiri Olsa
@ 2020-07-29 23:24       ` Ian Rogers
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Rogers @ 2020-07-29 23:24 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Adrian Hunter, Andi Kleen, Athira Rajeev, LKML, Networking, bpf,
	Stephane Eranian

On Tue, Jul 28, 2020 at 9:10 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jul 28, 2020 at 05:59:46PM +0200, Jiri Olsa wrote:
> > On Tue, Jul 28, 2020 at 01:57:31AM -0700, Ian Rogers wrote:
> > > From: Stephane Eranian <eranian@google.com>
> > >
> > > Before:
> > > $ perf record -c 10000 --pfm-events=cycles:period=77777
> > >
> > > Would yield a cycles event with period=10000, instead of 77777.
> > >
> > > This was due to an ordering issue between libpfm4 parsing
> > > the event string and perf record initializing the event.
> > >
> > > This patch fixes the problem by preventing override for
> > > events with attr->sample_period != 0 by the time
> > > perf_evsel__config() is invoked. This seems to have been the
> > > intent of the author.
> > >
> > > Signed-off-by: Stephane Eranian <eranian@google.com>
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/evsel.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index 811f538f7d77..8afc24e2ec52 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -976,8 +976,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
> > >      * 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 ||
> > > -                                opts->user_interval != ULLONG_MAX)) {
> > > +   if (!attr->sample_period) {
> >
> > I was wondering why this wouldn't break record/top
> > but we take care of the via record_opts__config
> >
> > as long as 'perf test attr' works it looks ok to me
>
> hum ;-)
>
> [jolsa@krava perf]$ sudo ./perf test 17 -v
> 17: Setup struct perf_event_attr                          :
> ...
> running './tests/attr/test-record-C0'
> expected sample_period=4000, got 3000
> FAILED './tests/attr/test-record-C0' - match failure

I'm not able to reproduce this. Do you have a build configuration or
something else to look at? The test doesn't seem obviously connected
with this patch.

Thanks,
Ian

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term
  2020-07-28  8:57 ` [PATCH v2 4/5] perf record: Don't clear event's period if set by a term Ian Rogers
  2020-07-29 18:58   ` Arnaldo Carvalho de Melo
@ 2020-08-04 10:08   ` Adrian Hunter
  2020-08-04 13:33     ` Ian Rogers
  1 sibling, 1 reply; 21+ messages in thread
From: Adrian Hunter @ 2020-08-04 10:08 UTC (permalink / raw)
  To: Ian Rogers, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Andi Kleen, Athira Rajeev,
	linux-kernel, netdev, bpf
  Cc: Stephane Eranian

On 28/07/20 11:57 am, Ian Rogers wrote:
> If events in a group explicitly set a frequency or period with leader
> sampling, don't disable the samples on those events.
> 
> Prior to 5.8:
> perf record -e '{cycles/period=12345000/,instructions/period=6789000/}:S'

Might be worth explaining this use-case some more.
Perhaps add it to the leader sampling documentation for perf-list.

> would clear the attributes then apply the config terms. In commit
> 5f34278867b7 leader sampling configuration was moved to after applying the
> config terms, in the example, making the instructions' event have its period
> cleared.
> This change makes it so that sampling is only disabled if configuration
> terms aren't present.
> 
> Fixes: 5f34278867b7 ("perf evlist: Move leader-sampling configuration")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/record.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> index a4cc11592f6b..01d1c6c613f7 100644
> --- a/tools/perf/util/record.c
> +++ b/tools/perf/util/record.c
> @@ -2,6 +2,7 @@
>  #include "debug.h"
>  #include "evlist.h"
>  #include "evsel.h"
> +#include "evsel_config.h"
>  #include "parse-events.h"
>  #include <errno.h>
>  #include <limits.h>
> @@ -38,6 +39,9 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
>  	struct perf_event_attr *attr = &evsel->core.attr;
>  	struct evsel *leader = evsel->leader;
>  	struct evsel *read_sampler;
> +	struct evsel_config_term *term;
> +	struct list_head *config_terms = &evsel->config_terms;
> +	int term_types, freq_mask;
>  
>  	if (!leader->sample_read)
>  		return;
> @@ -47,16 +51,24 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
>  	if (evsel == read_sampler)
>  		return;
>  
> +	/* Determine the evsel's config term types. */
> +	term_types = 0;
> +	list_for_each_entry(term, config_terms, list) {
> +		term_types |= 1 << term->type;
> +	}
>  	/*
> -	 * Disable sampling for all group members other than the leader in
> -	 * case the leader 'leads' the sampling, except when the leader is an
> -	 * AUX area event, in which case the 2nd event in the group is the one
> -	 * that 'leads' the sampling.
> +	 * Disable sampling for all group members except those with explicit
> +	 * config terms or the leader. In the case of an AUX area event, the 2nd
> +	 * event in the group is the one that 'leads' the sampling.
>  	 */
> -	attr->freq           = 0;
> -	attr->sample_freq    = 0;
> -	attr->sample_period  = 0;
> -	attr->write_backward = 0;
> +	freq_mask = (1 << EVSEL__CONFIG_TERM_FREQ) | (1 << EVSEL__CONFIG_TERM_PERIOD);
> +	if ((term_types & freq_mask) == 0) {

It would be nicer to have a helper e.g.

	if (!evsel__have_config_term(evsel, FREQ) &&
	    !evsel__have_config_term(evsel, PERIOD)) {

> +		attr->freq           = 0;
> +		attr->sample_freq    = 0;
> +		attr->sample_period  = 0;

If we are not sampling, then maybe we should also put here:

		attr->write_backward = 0;

> +	}

Then, if we are sampling this evsel shouldn't the backward setting
match the leader? e.g.

	if (attr->sample_freq)
		attr->write_backward = leader->core.attr.write_backward;


> +	if ((term_types & (1 << EVSEL__CONFIG_TERM_OVERWRITE)) == 0)
> +		attr->write_backward = 0;
>  
>  	/*
>  	 * We don't get a sample for slave events, we make them when delivering
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term
  2020-08-04 10:08   ` Adrian Hunter
@ 2020-08-04 13:33     ` Ian Rogers
  2020-08-04 14:48       ` Adrian Hunter
  0 siblings, 1 reply; 21+ messages in thread
From: Ian Rogers @ 2020-08-04 13:33 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Andi Kleen, Athira Rajeev, LKML, Networking, bpf,
	Stephane Eranian

On Tue, Aug 4, 2020 at 3:08 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 28/07/20 11:57 am, Ian Rogers wrote:
> > If events in a group explicitly set a frequency or period with leader
> > sampling, don't disable the samples on those events.
> >
> > Prior to 5.8:
> > perf record -e '{cycles/period=12345000/,instructions/period=6789000/}:S'
>
> Might be worth explaining this use-case some more.
> Perhaps add it to the leader sampling documentation for perf-list.
>
> > would clear the attributes then apply the config terms. In commit
> > 5f34278867b7 leader sampling configuration was moved to after applying the
> > config terms, in the example, making the instructions' event have its period
> > cleared.
> > This change makes it so that sampling is only disabled if configuration
> > terms aren't present.
> >
> > Fixes: 5f34278867b7 ("perf evlist: Move leader-sampling configuration")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/record.c | 28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> > index a4cc11592f6b..01d1c6c613f7 100644
> > --- a/tools/perf/util/record.c
> > +++ b/tools/perf/util/record.c
> > @@ -2,6 +2,7 @@
> >  #include "debug.h"
> >  #include "evlist.h"
> >  #include "evsel.h"
> > +#include "evsel_config.h"
> >  #include "parse-events.h"
> >  #include <errno.h>
> >  #include <limits.h>
> > @@ -38,6 +39,9 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
> >       struct perf_event_attr *attr = &evsel->core.attr;
> >       struct evsel *leader = evsel->leader;
> >       struct evsel *read_sampler;
> > +     struct evsel_config_term *term;
> > +     struct list_head *config_terms = &evsel->config_terms;
> > +     int term_types, freq_mask;
> >
> >       if (!leader->sample_read)
> >               return;
> > @@ -47,16 +51,24 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
> >       if (evsel == read_sampler)
> >               return;
> >
> > +     /* Determine the evsel's config term types. */
> > +     term_types = 0;
> > +     list_for_each_entry(term, config_terms, list) {
> > +             term_types |= 1 << term->type;
> > +     }
> >       /*
> > -      * Disable sampling for all group members other than the leader in
> > -      * case the leader 'leads' the sampling, except when the leader is an
> > -      * AUX area event, in which case the 2nd event in the group is the one
> > -      * that 'leads' the sampling.
> > +      * Disable sampling for all group members except those with explicit
> > +      * config terms or the leader. In the case of an AUX area event, the 2nd
> > +      * event in the group is the one that 'leads' the sampling.
> >        */
> > -     attr->freq           = 0;
> > -     attr->sample_freq    = 0;
> > -     attr->sample_period  = 0;
> > -     attr->write_backward = 0;
> > +     freq_mask = (1 << EVSEL__CONFIG_TERM_FREQ) | (1 << EVSEL__CONFIG_TERM_PERIOD);
> > +     if ((term_types & freq_mask) == 0) {
>
> It would be nicer to have a helper e.g.
>
>         if (!evsel__have_config_term(evsel, FREQ) &&
>             !evsel__have_config_term(evsel, PERIOD)) {

Sure. The point of doing it this way was to avoid repeatedly iterating
over the config term list.

> > +             attr->freq           = 0;
> > +             attr->sample_freq    = 0;
> > +             attr->sample_period  = 0;
>
> If we are not sampling, then maybe we should also put here:
>
>                 attr->write_backward = 0;
>
> > +     }
>
> Then, if we are sampling this evsel shouldn't the backward setting
> match the leader? e.g.
>
>         if (attr->sample_freq)
>                 attr->write_backward = leader->core.attr.write_backward;

Perhaps that should be a follow up change? This change is trying to
make the behavior match the previous behavior.

Thanks,
Ian

> > +     if ((term_types & (1 << EVSEL__CONFIG_TERM_OVERWRITE)) == 0)
> > +             attr->write_backward = 0;
> >
> >       /*
> >        * We don't get a sample for slave events, we make them when delivering
> >
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term
  2020-08-04 13:33     ` Ian Rogers
@ 2020-08-04 14:48       ` Adrian Hunter
  2020-08-04 15:50         ` Ian Rogers
  0 siblings, 1 reply; 21+ messages in thread
From: Adrian Hunter @ 2020-08-04 14:48 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Andi Kleen, Athira Rajeev, LKML, Networking, bpf,
	Stephane Eranian

On 4/08/20 4:33 pm, Ian Rogers wrote:
> On Tue, Aug 4, 2020 at 3:08 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 28/07/20 11:57 am, Ian Rogers wrote:
>>> If events in a group explicitly set a frequency or period with leader
>>> sampling, don't disable the samples on those events.
>>>
>>> Prior to 5.8:
>>> perf record -e '{cycles/period=12345000/,instructions/period=6789000/}:S'
>>
>> Might be worth explaining this use-case some more.
>> Perhaps add it to the leader sampling documentation for perf-list.
>>
>>> would clear the attributes then apply the config terms. In commit
>>> 5f34278867b7 leader sampling configuration was moved to after applying the
>>> config terms, in the example, making the instructions' event have its period
>>> cleared.
>>> This change makes it so that sampling is only disabled if configuration
>>> terms aren't present.
>>>
>>> Fixes: 5f34278867b7 ("perf evlist: Move leader-sampling configuration")
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>  tools/perf/util/record.c | 28 ++++++++++++++++++++--------
>>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
>>> index a4cc11592f6b..01d1c6c613f7 100644
>>> --- a/tools/perf/util/record.c
>>> +++ b/tools/perf/util/record.c
>>> @@ -2,6 +2,7 @@
>>>  #include "debug.h"
>>>  #include "evlist.h"
>>>  #include "evsel.h"
>>> +#include "evsel_config.h"
>>>  #include "parse-events.h"
>>>  #include <errno.h>
>>>  #include <limits.h>
>>> @@ -38,6 +39,9 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
>>>       struct perf_event_attr *attr = &evsel->core.attr;
>>>       struct evsel *leader = evsel->leader;
>>>       struct evsel *read_sampler;
>>> +     struct evsel_config_term *term;
>>> +     struct list_head *config_terms = &evsel->config_terms;
>>> +     int term_types, freq_mask;
>>>
>>>       if (!leader->sample_read)
>>>               return;
>>> @@ -47,16 +51,24 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
>>>       if (evsel == read_sampler)
>>>               return;
>>>
>>> +     /* Determine the evsel's config term types. */
>>> +     term_types = 0;
>>> +     list_for_each_entry(term, config_terms, list) {
>>> +             term_types |= 1 << term->type;
>>> +     }
>>>       /*
>>> -      * Disable sampling for all group members other than the leader in
>>> -      * case the leader 'leads' the sampling, except when the leader is an
>>> -      * AUX area event, in which case the 2nd event in the group is the one
>>> -      * that 'leads' the sampling.
>>> +      * Disable sampling for all group members except those with explicit
>>> +      * config terms or the leader. In the case of an AUX area event, the 2nd
>>> +      * event in the group is the one that 'leads' the sampling.
>>>        */
>>> -     attr->freq           = 0;
>>> -     attr->sample_freq    = 0;
>>> -     attr->sample_period  = 0;
>>> -     attr->write_backward = 0;
>>> +     freq_mask = (1 << EVSEL__CONFIG_TERM_FREQ) | (1 << EVSEL__CONFIG_TERM_PERIOD);
>>> +     if ((term_types & freq_mask) == 0) {
>>
>> It would be nicer to have a helper e.g.
>>
>>         if (!evsel__have_config_term(evsel, FREQ) &&
>>             !evsel__have_config_term(evsel, PERIOD)) {
> 
> Sure. The point of doing it this way was to avoid repeatedly iterating
> over the config term list.

But perhaps it is premature optimization

> 
>>> +             attr->freq           = 0;
>>> +             attr->sample_freq    = 0;
>>> +             attr->sample_period  = 0;
>>
>> If we are not sampling, then maybe we should also put here:
>>
>>                 attr->write_backward = 0;
>>
>>> +     }
>>
>> Then, if we are sampling this evsel shouldn't the backward setting
>> match the leader? e.g.
>>
>>         if (attr->sample_freq)
>>                 attr->write_backward = leader->core.attr.write_backward;
> 
> Perhaps that should be a follow up change? This change is trying to
> make the behavior match the previous behavior.

Sure

> 
> Thanks,
> Ian
> 
>>> +     if ((term_types & (1 << EVSEL__CONFIG_TERM_OVERWRITE)) == 0)
>>> +             attr->write_backward = 0;
>>>
>>>       /*
>>>        * We don't get a sample for slave events, we make them when delivering
>>>
>>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term
  2020-08-04 14:48       ` Adrian Hunter
@ 2020-08-04 15:50         ` Ian Rogers
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Rogers @ 2020-08-04 15:50 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Andi Kleen, Athira Rajeev, LKML, Networking, bpf,
	Stephane Eranian

On Tue, Aug 4, 2020 at 7:49 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 4/08/20 4:33 pm, Ian Rogers wrote:
> > On Tue, Aug 4, 2020 at 3:08 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 28/07/20 11:57 am, Ian Rogers wrote:
> >>> If events in a group explicitly set a frequency or period with leader
> >>> sampling, don't disable the samples on those events.
> >>>
> >>> Prior to 5.8:
> >>> perf record -e '{cycles/period=12345000/,instructions/period=6789000/}:S'
> >>
> >> Might be worth explaining this use-case some more.
> >> Perhaps add it to the leader sampling documentation for perf-list.
> >>
> >>> would clear the attributes then apply the config terms. In commit
> >>> 5f34278867b7 leader sampling configuration was moved to after applying the
> >>> config terms, in the example, making the instructions' event have its period
> >>> cleared.
> >>> This change makes it so that sampling is only disabled if configuration
> >>> terms aren't present.
> >>>
> >>> Fixes: 5f34278867b7 ("perf evlist: Move leader-sampling configuration")
> >>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>> ---
> >>>  tools/perf/util/record.c | 28 ++++++++++++++++++++--------
> >>>  1 file changed, 20 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> >>> index a4cc11592f6b..01d1c6c613f7 100644
> >>> --- a/tools/perf/util/record.c
> >>> +++ b/tools/perf/util/record.c
> >>> @@ -2,6 +2,7 @@
> >>>  #include "debug.h"
> >>>  #include "evlist.h"
> >>>  #include "evsel.h"
> >>> +#include "evsel_config.h"
> >>>  #include "parse-events.h"
> >>>  #include <errno.h>
> >>>  #include <limits.h>
> >>> @@ -38,6 +39,9 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
> >>>       struct perf_event_attr *attr = &evsel->core.attr;
> >>>       struct evsel *leader = evsel->leader;
> >>>       struct evsel *read_sampler;
> >>> +     struct evsel_config_term *term;
> >>> +     struct list_head *config_terms = &evsel->config_terms;
> >>> +     int term_types, freq_mask;
> >>>
> >>>       if (!leader->sample_read)
> >>>               return;
> >>> @@ -47,16 +51,24 @@ static void evsel__config_leader_sampling(struct evsel *evsel, struct evlist *ev
> >>>       if (evsel == read_sampler)
> >>>               return;
> >>>
> >>> +     /* Determine the evsel's config term types. */
> >>> +     term_types = 0;
> >>> +     list_for_each_entry(term, config_terms, list) {
> >>> +             term_types |= 1 << term->type;
> >>> +     }
> >>>       /*
> >>> -      * Disable sampling for all group members other than the leader in
> >>> -      * case the leader 'leads' the sampling, except when the leader is an
> >>> -      * AUX area event, in which case the 2nd event in the group is the one
> >>> -      * that 'leads' the sampling.
> >>> +      * Disable sampling for all group members except those with explicit
> >>> +      * config terms or the leader. In the case of an AUX area event, the 2nd
> >>> +      * event in the group is the one that 'leads' the sampling.
> >>>        */
> >>> -     attr->freq           = 0;
> >>> -     attr->sample_freq    = 0;
> >>> -     attr->sample_period  = 0;
> >>> -     attr->write_backward = 0;
> >>> +     freq_mask = (1 << EVSEL__CONFIG_TERM_FREQ) | (1 << EVSEL__CONFIG_TERM_PERIOD);
> >>> +     if ((term_types & freq_mask) == 0) {
> >>
> >> It would be nicer to have a helper e.g.
> >>
> >>         if (!evsel__have_config_term(evsel, FREQ) &&
> >>             !evsel__have_config_term(evsel, PERIOD)) {
> >
> > Sure. The point of doing it this way was to avoid repeatedly iterating
> > over the config term list.
>
> But perhaps it is premature optimization

The alternative is more loc. I think we can bike shed on this but it's
not really changing the substance of the change. I'm keen to try to be
efficient where we can as we see issues at scale.

Thanks,
Ian

> >
> >>> +             attr->freq           = 0;
> >>> +             attr->sample_freq    = 0;
> >>> +             attr->sample_period  = 0;
> >>
> >> If we are not sampling, then maybe we should also put here:
> >>
> >>                 attr->write_backward = 0;
> >>
> >>> +     }
> >>
> >> Then, if we are sampling this evsel shouldn't the backward setting
> >> match the leader? e.g.
> >>
> >>         if (attr->sample_freq)
> >>                 attr->write_backward = leader->core.attr.write_backward;
> >
> > Perhaps that should be a follow up change? This change is trying to
> > make the behavior match the previous behavior.
>
> Sure
>
> >
> > Thanks,
> > Ian
> >
> >>> +     if ((term_types & (1 << EVSEL__CONFIG_TERM_OVERWRITE)) == 0)
> >>> +             attr->write_backward = 0;
> >>>
> >>>       /*
> >>>        * We don't get a sample for slave events, we make them when delivering
> >>>
> >>
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28  8:57 [PATCH v2 0/5] Fixes for setting event freq/periods Ian Rogers
2020-07-28  8:57 ` [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set Ian Rogers
2020-07-28 15:43   ` Jiri Olsa
2020-07-28 16:03     ` Arnaldo Carvalho de Melo
2020-07-29 15:11       ` Athira Rajeev
2020-07-29 18:52   ` Arnaldo Carvalho de Melo
2020-07-29 21:43     ` Ian Rogers
2020-07-28  8:57 ` [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events Ian Rogers
2020-07-28 15:59   ` Jiri Olsa
2020-07-28 16:09     ` Jiri Olsa
2020-07-29 23:24       ` Ian Rogers
2020-07-29 18:54   ` Arnaldo Carvalho de Melo
2020-07-28  8:57 ` [PATCH v2 3/5] perf test: Ensure sample_period is set " Ian Rogers
2020-07-28 12:45   ` Arnaldo Carvalho de Melo
2020-07-28  8:57 ` [PATCH v2 4/5] perf record: Don't clear event's period if set by a term Ian Rogers
2020-07-29 18:58   ` Arnaldo Carvalho de Melo
2020-08-04 10:08   ` Adrian Hunter
2020-08-04 13:33     ` Ian Rogers
2020-08-04 14:48       ` Adrian Hunter
2020-08-04 15:50         ` Ian Rogers
2020-07-28  8:57 ` [PATCH v2 5/5] perf test: Leader sampling shouldn't clear sample period Ian Rogers

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git