| Message ID | 20200728085734.609930-2-irogers@google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Fixes for setting event freq/periods | expand |
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 >
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 > > >
> 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
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 >
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
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
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;