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 <[email protected]>
Fixes: ae5dcc8abe31 (“perf record: Prevent override of attr->sample_period for libpfm4 events”)
Signed-off-by: German Gomez <[email protected]>
---
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
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 <[email protected]>
> Fixes: ae5dcc8abe31 (“perf record: Prevent override of attr->sample_period for libpfm4 events”)
> Signed-off-by: German Gomez <[email protected]>
> ---
> 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.
>
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
On Mon, Jan 17, 2022 at 2:27 AM German Gomez <[email protected]> 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
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
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
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
Em Mon, Jan 17, 2022 at 08:28:07AM -0800, Ian Rogers escreveu:
> On Mon, Jan 17, 2022 at 2:27 AM German Gomez <[email protected]> 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