* [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
@ 2022-01-14 21:21 German Gomez
2022-01-17 9:59 ` James Clark
0 siblings, 1 reply; 8+ messages in thread
From: German Gomez @ 2022-01-14 21:21 UTC (permalink / raw)
To: linux-kernel, linux-perf-users, acme
Cc: German Gomez, Chase Conklin, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Ian Rogers,
Arnaldo Carvalho de Melo, Stephane Eranian, netdev, bpf
A previous commit preventing attr->sample_period values from being
overridden in pfm events changed a related behaviour in arm_spe.
Before this patch:
perf record -c 10000 -e arm_spe_0// -- sleep 1
Would not yield an SPE event with period=10000, because the arm-spe code
initializes sample_period to a non-0 value, so the "-c 10000" is ignored.
This patch restores the previous behaviour for non-libpfm4 events.
Reported-by: Chase Conklin <chase.conklin@arm.com>
Fixes: ae5dcc8abe31 (“perf record: Prevent override of attr->sample_period for libpfm4 events”)
Signed-off-by: German Gomez <german.gomez@arm.com>
---
tools/perf/util/evsel.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a59fb2ecb84e..86ab038f020f 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1065,6 +1065,17 @@ void __weak arch_evsel__fixup_new_cycles(struct perf_event_attr *attr __maybe_un
{
}
+static void evsel__set_default_freq_period(struct record_opts *opts,
+ struct perf_event_attr *attr)
+{
+ if (opts->freq) {
+ attr->freq = 1;
+ attr->sample_freq = opts->freq;
+ } else {
+ attr->sample_period = opts->default_interval;
+ }
+}
+
/*
* The enable_on_exec/disabled value strategy:
*
@@ -1131,14 +1142,12 @@ 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) {
- if (opts->freq) {
- attr->freq = 1;
- attr->sample_freq = opts->freq;
- } else {
- attr->sample_period = opts->default_interval;
- }
- }
+ if ((evsel->is_libpfm_event && !attr->sample_period) ||
+ (!evsel->is_libpfm_event && (!attr->sample_period ||
+ opts->user_freq != UINT_MAX ||
+ opts->user_interval != ULLONG_MAX)))
+ evsel__set_default_freq_period(opts, attr);
+
/*
* If attr->freq was set (here or earlier), ask for period
* to be sampled.
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
2022-01-14 21:21 [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events German Gomez
@ 2022-01-17 9:59 ` James Clark
2022-01-17 10:27 ` German Gomez
0 siblings, 1 reply; 8+ messages in thread
From: James Clark @ 2022-01-17 9:59 UTC (permalink / raw)
To: German Gomez, linux-kernel, linux-perf-users
Cc: Chase Conklin, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Ian Rogers, Arnaldo Carvalho de Melo,
Stephane Eranian, netdev, bpf, acme
On 14/01/2022 21:21, German Gomez wrote:
> A previous commit preventing attr->sample_period values from being
> overridden in pfm events changed a related behaviour in arm_spe.
>
> Before this patch:
> perf record -c 10000 -e arm_spe_0// -- sleep 1
>
> Would not yield an SPE event with period=10000, because the arm-spe code
Just to clarify, this seems like it should say "Would yield", not "Would not yield",
as in it was previously working?
> initializes sample_period to a non-0 value, so the "-c 10000" is ignored.
>
> This patch restores the previous behaviour for non-libpfm4 events.
>
> Reported-by: Chase Conklin <chase.conklin@arm.com>
> Fixes: ae5dcc8abe31 (“perf record: Prevent override of attr->sample_period for libpfm4 events”)
> Signed-off-by: German Gomez <german.gomez@arm.com>
> ---
> tools/perf/util/evsel.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a59fb2ecb84e..86ab038f020f 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1065,6 +1065,17 @@ void __weak arch_evsel__fixup_new_cycles(struct perf_event_attr *attr __maybe_un
> {
> }
>
> +static void evsel__set_default_freq_period(struct record_opts *opts,
> + struct perf_event_attr *attr)
> +{
> + if (opts->freq) {
> + attr->freq = 1;
> + attr->sample_freq = opts->freq;
> + } else {
> + attr->sample_period = opts->default_interval;
> + }
> +}
> +
> /*
> * The enable_on_exec/disabled value strategy:
> *
> @@ -1131,14 +1142,12 @@ 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) {
> - if (opts->freq) {
> - attr->freq = 1;
> - attr->sample_freq = opts->freq;
> - } else {
> - attr->sample_period = opts->default_interval;
> - }
> - }
> + if ((evsel->is_libpfm_event && !attr->sample_period) ||
> + (!evsel->is_libpfm_event && (!attr->sample_period ||
> + opts->user_freq != UINT_MAX ||
> + opts->user_interval != ULLONG_MAX)))
> + evsel__set_default_freq_period(opts, attr);
> +
> /*
> * If attr->freq was set (here or earlier), ask for period
> * to be sampled.
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
2022-01-17 9:59 ` James Clark
@ 2022-01-17 10:27 ` German Gomez
2022-01-17 16:28 ` Ian Rogers
0 siblings, 1 reply; 8+ messages in thread
From: German Gomez @ 2022-01-17 10:27 UTC (permalink / raw)
To: James Clark, linux-kernel, linux-perf-users
Cc: Chase Conklin, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Ian Rogers, Arnaldo Carvalho de Melo,
Stephane Eranian, netdev, bpf, acme
Hi James,
On 17/01/2022 09:59, James Clark wrote:
>
> On 14/01/2022 21:21, German Gomez wrote:
>> A previous commit preventing attr->sample_period values from being
>> overridden in pfm events changed a related behaviour in arm_spe.
>>
>> Before this patch:
>> perf record -c 10000 -e arm_spe_0// -- sleep 1
>>
>> Would not yield an SPE event with period=10000, because the arm-spe code
> Just to clarify, this seems like it should say "Would yield", not "Would not yield",
> as in it was previously working?
"this patch" refers to the patch I'm sending, not the one it's fixing.
I might have to rewrite this to make it more clear. How about:
===
A previous patch preventing "attr->sample_period" values from being
overridden in pfm events changed a related behaviour in arm-spe.
Before said patch:
perf record -c 10000 -e arm_spe_0// -- sleep 1
Would yield an SPE event with period=10000. After the patch, the period
in "-c 10000" was being ignored because the arm-spe code initializes
sample_period to a non-zero value.
This patch restores the previous behaviour for non-libpfm4 events.
===
Thanks for the review,
German
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
2022-01-17 10:27 ` German Gomez
@ 2022-01-17 16:28 ` Ian Rogers
2022-01-17 21:32 ` German Gomez
2022-01-22 20:17 ` Arnaldo Carvalho de Melo
0 siblings, 2 replies; 8+ messages in thread
From: Ian Rogers @ 2022-01-17 16:28 UTC (permalink / raw)
To: German Gomez
Cc: James Clark, linux-kernel, linux-perf-users, Chase Conklin,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Arnaldo Carvalho de Melo, Stephane Eranian, netdev,
bpf, acme
On Mon, Jan 17, 2022 at 2:27 AM German Gomez <german.gomez@arm.com> wrote:
>
> Hi James,
>
> On 17/01/2022 09:59, James Clark wrote:
> >
> > On 14/01/2022 21:21, German Gomez wrote:
> >> A previous commit preventing attr->sample_period values from being
> >> overridden in pfm events changed a related behaviour in arm_spe.
> >>
> >> Before this patch:
> >> perf record -c 10000 -e arm_spe_0// -- sleep 1
> >>
> >> Would not yield an SPE event with period=10000, because the arm-spe code
> > Just to clarify, this seems like it should say "Would yield", not "Would not yield",
> > as in it was previously working?
>
> "this patch" refers to the patch I'm sending, not the one it's fixing.
> I might have to rewrite this to make it more clear. How about:
>
> ===
> A previous patch preventing "attr->sample_period" values from being
> overridden in pfm events changed a related behaviour in arm-spe.
>
> Before said patch:
> perf record -c 10000 -e arm_spe_0// -- sleep 1
>
> Would yield an SPE event with period=10000. After the patch, the period
> in "-c 10000" was being ignored because the arm-spe code initializes
> sample_period to a non-zero value.
>
> This patch restores the previous behaviour for non-libpfm4 events.
> ===
Thanks for fixing this, I can add an acked-by for the v2 patch. Could
we add a test for this to avoid future regressions? There are similar
tests for frequency like:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr/test-record-freq
based on the attr.py test:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr.py
The test specifies a base type of event attribute and then what is
modified by the test. It takes a little to get your head around but
having a test for this would be a welcome addition.
Thanks!
Ian
> Thanks for the review,
> German
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
2022-01-17 16:28 ` Ian Rogers
@ 2022-01-17 21:32 ` German Gomez
2022-01-18 12:32 ` Arnaldo Carvalho de Melo
2022-01-22 20:17 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 8+ messages in thread
From: German Gomez @ 2022-01-17 21:32 UTC (permalink / raw)
To: Ian Rogers
Cc: James Clark, linux-kernel, linux-perf-users, Chase Conklin,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Arnaldo Carvalho de Melo, Stephane Eranian, netdev,
bpf, acme
Hi Ian,
On 17/01/2022 16:28, Ian Rogers wrote:
> [...]
> Thanks for fixing this, I can add an acked-by for the v2 patch. Could
> we add a test for this to avoid future regressions? There are similar
> tests for frequency like:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr/test-record-freq
> based on the attr.py test:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr.py
> The test specifies a base type of event attribute and then what is
> modified by the test. It takes a little to get your head around but
> having a test for this would be a welcome addition.
I agree I should have included a test for this fix. I'll look into this for the v2.
Other events such as "-p 10000 -e cycles//" worked fine. Only the ones with aux area tracing (arm_spe, cs_etm, intel_pt) were ignoring the global config flags.
Thank you for the pointers, and the review,
German
>
> Thanks!
> Ian
>
>> Thanks for the review,
>> German
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
2022-01-17 21:32 ` German Gomez
@ 2022-01-18 12:32 ` Arnaldo Carvalho de Melo
2022-01-22 20:15 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-01-18 12:32 UTC (permalink / raw)
To: German Gomez
Cc: Ian Rogers, James Clark, linux-kernel, linux-perf-users,
Chase Conklin, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Arnaldo Carvalho de Melo,
Stephane Eranian, netdev, bpf
Em Mon, Jan 17, 2022 at 09:32:55PM +0000, German Gomez escreveu:
> Hi Ian,
>
> On 17/01/2022 16:28, Ian Rogers wrote:
> > [...]
> > Thanks for fixing this, I can add an acked-by for the v2 patch. Could
> > we add a test for this to avoid future regressions? There are similar
> > tests for frequency like:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr/test-record-freq
> > based on the attr.py test:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr.py
> > The test specifies a base type of event attribute and then what is
> > modified by the test. It takes a little to get your head around but
> > having a test for this would be a welcome addition.
>
> I agree I should have included a test for this fix. I'll look into this for the v2.
A test is always good to have, we need more, yeah.
But since this is a fix and what is needed for v2 is just to improve the
wording, please don't let the test to prevent you from sending the
updated fix.
Then you can go on and work on the test.
I say this because the merge window may close before the test gets ready
and its better for us to have fixes merged as soon as possible so that
we have more time to figure out if it has unintended consequences as it
gets in place for longer.
> Other events such as "-p 10000 -e cycles//" worked fine. Only the ones with aux area tracing (arm_spe, cs_etm, intel_pt) were ignoring the global config flags.
>
> Thank you for the pointers, and the review,
> German
>
> >
> > Thanks!
> > Ian
> >
> >> Thanks for the review,
> >> German
--
- Arnaldo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
2022-01-18 12:32 ` Arnaldo Carvalho de Melo
@ 2022-01-22 20:15 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-01-22 20:15 UTC (permalink / raw)
To: German Gomez
Cc: Ian Rogers, James Clark, linux-kernel, linux-perf-users,
Chase Conklin, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Arnaldo Carvalho de Melo,
Stephane Eranian, netdev, bpf
Em Tue, Jan 18, 2022 at 09:32:30AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 17, 2022 at 09:32:55PM +0000, German Gomez escreveu:
> > Hi Ian,
> >
> > On 17/01/2022 16:28, Ian Rogers wrote:
> > > [...]
> > > Thanks for fixing this, I can add an acked-by for the v2 patch. Could
> > > we add a test for this to avoid future regressions? There are similar
> > > tests for frequency like:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr/test-record-freq
> > > based on the attr.py test:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr.py
> > > The test specifies a base type of event attribute and then what is
> > > modified by the test. It takes a little to get your head around but
> > > having a test for this would be a welcome addition.
> >
> > I agree I should have included a test for this fix. I'll look into this for the v2.
>
> A test is always good to have, we need more, yeah.
>
> But since this is a fix and what is needed for v2 is just to improve the
> wording, please don't let the test to prevent you from sending the
> updated fix.
>
> Then you can go on and work on the test.
>
> I say this because the merge window may close before the test gets ready
> and its better for us to have fixes merged as soon as possible so that
> we have more time to figure out if it has unintended consequences as it
> gets in place for longer.
So, any news about this?
- ARnaldo
> > Other events such as "-p 10000 -e cycles//" worked fine. Only the ones with aux area tracing (arm_spe, cs_etm, intel_pt) were ignoring the global config flags.
> >
> > Thank you for the pointers, and the review,
> > German
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events
2022-01-17 16:28 ` Ian Rogers
2022-01-17 21:32 ` German Gomez
@ 2022-01-22 20:17 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-01-22 20:17 UTC (permalink / raw)
To: Ian Rogers
Cc: German Gomez, James Clark, linux-kernel, linux-perf-users,
Chase Conklin, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Arnaldo Carvalho de Melo,
Stephane Eranian, netdev, bpf
Em Mon, Jan 17, 2022 at 08:28:07AM -0800, Ian Rogers escreveu:
> On Mon, Jan 17, 2022 at 2:27 AM German Gomez <german.gomez@arm.com> wrote:
> >
> > Hi James,
> >
> > On 17/01/2022 09:59, James Clark wrote:
> > >
> > > On 14/01/2022 21:21, German Gomez wrote:
> > >> A previous commit preventing attr->sample_period values from being
> > >> overridden in pfm events changed a related behaviour in arm_spe.
> > >>
> > >> Before this patch:
> > >> perf record -c 10000 -e arm_spe_0// -- sleep 1
> > >>
> > >> Would not yield an SPE event with period=10000, because the arm-spe code
> > > Just to clarify, this seems like it should say "Would yield", not "Would not yield",
> > > as in it was previously working?
> >
> > "this patch" refers to the patch I'm sending, not the one it's fixing.
> > I might have to rewrite this to make it more clear. How about:
> >
> > ===
> > A previous patch preventing "attr->sample_period" values from being
> > overridden in pfm events changed a related behaviour in arm-spe.
> >
> > Before said patch:
> > perf record -c 10000 -e arm_spe_0// -- sleep 1
> >
> > Would yield an SPE event with period=10000. After the patch, the period
> > in "-c 10000" was being ignored because the arm-spe code initializes
> > sample_period to a non-zero value.
> >
> > This patch restores the previous behaviour for non-libpfm4 events.
> > ===
>
> Thanks for fixing this, I can add an acked-by for the v2 patch. Could
Ian,
He posted a v2, can I add your Acked-by?
- Arnaldo
> we add a test for this to avoid future regressions? There are similar
> tests for frequency like:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr/test-record-freq
> based on the attr.py test:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/attr.py
> The test specifies a base type of event attribute and then what is
> modified by the test. It takes a little to get your head around but
> having a test for this would be a welcome addition.
>
> Thanks!
> Ian
>
> > Thanks for the review,
> > German
--
- Arnaldo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-22 20:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-14 21:21 [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events German Gomez
2022-01-17 9:59 ` James Clark
2022-01-17 10:27 ` German Gomez
2022-01-17 16:28 ` Ian Rogers
2022-01-17 21:32 ` German Gomez
2022-01-18 12:32 ` Arnaldo Carvalho de Melo
2022-01-22 20:15 ` Arnaldo Carvalho de Melo
2022-01-22 20:17 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.