[PATCH v2 0/5] Fixes for setting event freq/periods

[PATCH v2 0/5] Fixes for setting event freq/periods

[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

[PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

[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

[PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[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

[PATCH v2 3/5] perf test: Ensure sample_period is set libpfm4 events

[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

[PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[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

[PATCH v2 5/5] perf test: Leader sampling shouldn't clear sample period

[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

Re: [PATCH v2 3/5] perf test: Ensure sample_period is set libpfm4 events

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

[Jiri Olsa]

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
> 

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Jiri Olsa]

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

Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Jiri Olsa]

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

Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

[Athira Rajeev]

> 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

Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

[Ian Rogers]

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

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Ian Rogers]

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
>

Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[Adrian Hunter]

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
> 

Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[Ian Rogers]

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
> >
>

Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[Adrian Hunter]

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
>>>
>>

Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[Ian Rogers]

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
> >>>
> >>
>

Re: [PATCH v2 1/5] perf record: Set PERF_RECORD_PERIOD if attr->freq is set.

[Ian Rogers]

On Wed, Jul 29, 2020 at 2:43 PM Ian Rogers <irogers@google.com> wrote:
>
> 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

Polite ping. Thanks,
Ian

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

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Ian Rogers]

On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
>
> 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

Jiri, any update? Thanks,
Ian

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

Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[Ian Rogers]

On Tue, Aug 4, 2020 at 8:50 AM Ian Rogers <irogers@google.com> wrote:
>
> 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

Ping. Do we want to turn this into multiple O(N) searches using a
helper rather than 1 as coded here?

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
> > >>>
> > >>
> >

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Jiri Olsa]

On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:
> On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
> >
> > 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
> 
> Jiri, any update? Thanks,

sorry, I rebased and ran it again and it passes for me now,
so it got fixed along the way

jirka

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Ian Rogers]

On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:
> > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > 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
> >
> > Jiri, any update? Thanks,
>
> sorry, I rebased and ran it again and it passes for me now,
> so it got fixed along the way

No worries, thanks for the update! It'd be nice to land this and the
other libpfm fixes.

Ian

> jirka
>

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Arnaldo Carvalho de Melo]

Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu:
> On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:
> > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
> > > > 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:
> > > > > [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.

> > > Jiri, any update? Thanks,

> > sorry, I rebased and ran it again and it passes for me now,
> > so it got fixed along the way

> No worries, thanks for the update! It'd be nice to land this and the
> other libpfm fixes.

I applied it and it generated this regression:

FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure

I'll look at the other patches that are pending in this regard to see
what needs to be squashed so that we don't break bisect.

- Arnaldo

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Arnaldo Carvalho de Melo]

Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu:
> > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:
> > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
> > > > > 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:
> > > > > > [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.
> 
> > > > Jiri, any update? Thanks,
> 
> > > sorry, I rebased and ran it again and it passes for me now,
> > > so it got fixed along the way
> 
> > No worries, thanks for the update! It'd be nice to land this and the
> > other libpfm fixes.
> 
> I applied it and it generated this regression:
> 
> FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
> 
> I'll look at the other patches that are pending in this regard to see
> what needs to be squashed so that we don't break bisect.

So, more context:

running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period'
expected exclude_hv=0, got 1
FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
test child finished with -1
---- end ----
Setup struct perf_event_attr: FAILED!
[root@five ~]#

Ian, can you take a look at this?

- Arnaldo

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Arnaldo Carvalho de Melo]

Em Fri, Sep 04, 2020 at 03:50:13PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu:
> > > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:
> > > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
> > > > > > 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:
> > > > > > > [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.
> > 
> > > > > Jiri, any update? Thanks,
> > 
> > > > sorry, I rebased and ran it again and it passes for me now,
> > > > so it got fixed along the way
> > 
> > > No worries, thanks for the update! It'd be nice to land this and the
> > > other libpfm fixes.
> > 
> > I applied it and it generated this regression:
> > 
> > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
> > 
> > I'll look at the other patches that are pending in this regard to see
> > what needs to be squashed so that we don't break bisect.
> 
> So, more context:
> 
> running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period'
> expected exclude_hv=0, got 1
> FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
> test child finished with -1
> ---- end ----
> Setup struct perf_event_attr: FAILED!
> [root@five ~]#
> 
> Ian, can you take a look at this?

Further tests I've performed:

    Committer testing:

    Not linking with libpfm:

      # ldd ~/bin/perf | grep libpfm
      #

    Before:

      # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001
      [ perf record: Woken up 1 times to write data ]
      [ perf record: Captured and wrote 0.052 MB perf.data (258 samples) ]
      # perf evlist -v
      cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 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
      instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
      #

    After:

      #
      # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001
      [ perf record: Woken up 1 times to write data ]
      [ perf record: Captured and wrote 0.053 MB perf.data (284 samples) ]
      # perf evlist -v
      cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 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
      instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
      #

    Linking with libpfm:

      # ldd ~/bin/perf | grep libpfm
            libpfm.so.4 => /lib64/libpfm.so.4 (0x00007f54c7d75000)
      #

      # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1
      [ perf record: Woken up 1 times to write data ]
      [ perf record: Captured and wrote 0.043 MB perf.data (141 samples) ]
      # perf evlist -v
      cycles:period=77777: size: 120, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 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
      #

    After:

      # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1
      [ perf record: Woken up 1 times to write data ]
      [ perf record: Captured and wrote 0.039 MB perf.data (19 samples) ]
      # perf evlist -v
      cycles:period=77777: size: 120, { sample_period, sample_freq }: 77777, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 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
      #

Re: [PATCH v2 4/5] perf record: Don't clear event's period if set by a term

[Adrian Hunter]

On 4/09/20 8:43 am, Ian Rogers wrote:
> On Tue, Aug 4, 2020 at 8:50 AM Ian Rogers <irogers@google.com> wrote:
>> 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
> Ping. Do we want to turn this into multiple O(N) searches using a
> helper rather than 1 as coded here?

Actually max. 30 iterations vs max. 15 iterations for the 15 current
possible config terms.

At least please don't open code the config term implementation details.

For example, make evsel__have_config_term() accept multiple terms or introduce

u64 term_mask = evsel__config_term_mask(evsel);

has_config_term(term_mask, FREQ) etc

or whatever.

>
> 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
>>>>>>

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Ian Rogers]

On Fri, Sep 4, 2020 at 11:51 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Sep 04, 2020 at 03:50:13PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu:
> > > > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:
> > > > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
> > > > > > > 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:
> > > > > > > > [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.
> > >
> > > > > > Jiri, any update? Thanks,
> > >
> > > > > sorry, I rebased and ran it again and it passes for me now,
> > > > > so it got fixed along the way
> > >
> > > > No worries, thanks for the update! It'd be nice to land this and the
> > > > other libpfm fixes.
> > >
> > > I applied it and it generated this regression:
> > >
> > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
> > >
> > > I'll look at the other patches that are pending in this regard to see
> > > what needs to be squashed so that we don't break bisect.
> >
> > So, more context:
> >
> > running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period'
> > expected exclude_hv=0, got 1
> > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
> > test child finished with -1
> > ---- end ----
> > Setup struct perf_event_attr: FAILED!
> > [root@five ~]#
> >
> > Ian, can you take a look at this?
>
> Further tests I've performed:
>
>     Committer testing:
>
>     Not linking with libpfm:
>
>       # ldd ~/bin/perf | grep libpfm
>       #
>
>     Before:
>
>       # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001
>       [ perf record: Woken up 1 times to write data ]
>       [ perf record: Captured and wrote 0.052 MB perf.data (258 samples) ]
>       # perf evlist -v
>       cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 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
>       instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
>       #
>
>     After:
>
>       #
>       # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001
>       [ perf record: Woken up 1 times to write data ]
>       [ perf record: Captured and wrote 0.053 MB perf.data (284 samples) ]
>       # perf evlist -v
>       cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 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
>       instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
>       #
>
>     Linking with libpfm:
>
>       # ldd ~/bin/perf | grep libpfm
>             libpfm.so.4 => /lib64/libpfm.so.4 (0x00007f54c7d75000)
>       #
>
>       # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1
>       [ perf record: Woken up 1 times to write data ]
>       [ perf record: Captured and wrote 0.043 MB perf.data (141 samples) ]
>       # perf evlist -v
>       cycles:period=77777: size: 120, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 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
>       #
>
>     After:
>
>       # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1
>       [ perf record: Woken up 1 times to write data ]
>       [ perf record: Captured and wrote 0.039 MB perf.data (19 samples) ]
>       # perf evlist -v
>       cycles:period=77777: size: 120, { sample_period, sample_freq }: 77777, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 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
>       #
>
Hi Arnaldo,

I've been trying to reproduce the test failure you mention and I've
not been able to. This follow up e-mail seems to show things working
as intended. Did the issue resolve itself?

Thanks,
Ian

Re: [PATCH v2 2/5] perf record: Prevent override of attr->sample_period for libpfm4 events

[Ian Rogers]

On Fri, Sep 11, 2020 at 3:34 PM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Sep 4, 2020 at 11:51 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Fri, Sep 04, 2020 at 03:50:13PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Fri, Sep 04, 2020 at 03:48:03PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Fri, Sep 04, 2020 at 09:22:10AM -0700, Ian Rogers escreveu:
> > > > > On Fri, Sep 4, 2020 at 9:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > > > On Thu, Sep 03, 2020 at 10:41:14PM -0700, Ian Rogers wrote:
> > > > > > > On Wed, Jul 29, 2020 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
> > > > > > > > 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:
> > > > > > > > > [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.
> > > >
> > > > > > > Jiri, any update? Thanks,
> > > >
> > > > > > sorry, I rebased and ran it again and it passes for me now,
> > > > > > so it got fixed along the way
> > > >
> > > > > No worries, thanks for the update! It'd be nice to land this and the
> > > > > other libpfm fixes.
> > > >
> > > > I applied it and it generated this regression:
> > > >
> > > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
> > > >
> > > > I'll look at the other patches that are pending in this regard to see
> > > > what needs to be squashed so that we don't break bisect.
> > >
> > > So, more context:
> > >
> > > running '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period'
> > > expected exclude_hv=0, got 1
> > > FAILED '/home/acme/libexec/perf-core/tests/attr/test-record-pfm-period' - match failure
> > > test child finished with -1
> > > ---- end ----
> > > Setup struct perf_event_attr: FAILED!
> > > [root@five ~]#
> > >
> > > Ian, can you take a look at this?
> >
> > Further tests I've performed:
> >
> >     Committer testing:
> >
> >     Not linking with libpfm:
> >
> >       # ldd ~/bin/perf | grep libpfm
> >       #
> >
> >     Before:
> >
> >       # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001
> >       [ perf record: Woken up 1 times to write data ]
> >       [ perf record: Captured and wrote 0.052 MB perf.data (258 samples) ]
> >       # perf evlist -v
> >       cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 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
> >       instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> >       #
> >
> >     After:
> >
> >       #
> >       # perf record -c 10000 -e cycles/period=12345/,instructions sleep 0.0001
> >       [ perf record: Woken up 1 times to write data ]
> >       [ perf record: Captured and wrote 0.053 MB perf.data (284 samples) ]
> >       # perf evlist -v
> >       cycles/period=12345/: size: 120, { sample_period, sample_freq }: 12345, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 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
> >       instructions: size: 120, config: 0x1, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME|ID, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> >       #
> >
> >     Linking with libpfm:
> >
> >       # ldd ~/bin/perf | grep libpfm
> >             libpfm.so.4 => /lib64/libpfm.so.4 (0x00007f54c7d75000)
> >       #
> >
> >       # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1
> >       [ perf record: Woken up 1 times to write data ]
> >       [ perf record: Captured and wrote 0.043 MB perf.data (141 samples) ]
> >       # perf evlist -v
> >       cycles:period=77777: size: 120, { sample_period, sample_freq }: 10000, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 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
> >       #
> >
> >     After:
> >
> >       # perf record -c 10000 --pfm-events=cycles:period=77777 sleep 1
> >       [ perf record: Woken up 1 times to write data ]
> >       [ perf record: Captured and wrote 0.039 MB perf.data (19 samples) ]
> >       # perf evlist -v
> >       cycles:period=77777: size: 120, { sample_period, sample_freq }: 77777, sample_type: IP|TID|TIME, read_format: ID, disabled: 1, inherit: 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
> >       #
> >
> Hi Arnaldo,
>
> I've been trying to reproduce the test failure you mention and I've
> not been able to. This follow up e-mail seems to show things working
> as intended. Did the issue resolve itself?

It looks like the test to ensure the period for pfm events worked:
https://lore.kernel.org/lkml/20200728124539.GB40195@kernel.org/
has been applied without the fixes in patches 1 and 2. I've resent the
patches, hopefully addressing all feedback.
https://lore.kernel.org/lkml/20200912025655.1337192-1-irogers@google.com/

Thanks,
Ian