Subject: Re: [PATCH] perf record/arm-spe: Override attr->sample_period for non-libpfm4 events

From: Arnaldo Carvalho de Melo <[email-masked]>
Date: Tue, 18 Jan 2022 09:32:30 -0300

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