Re: [PATCH] perf test stat_all_pmu.sh: Correctly check 'perf stat' result

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Mon, Feb 24, 2025 at 05:34:15PM +0800, Qiao Zhao wrote:
> On Mon, Feb 24, 2025 at 2:59 PM Qiao Zhao <qzhao@redhat.com> wrote:
> >
> > On Sun, Jan 19, 2025 at 2:15 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > On Thu, Jan 16, 2025 at 01:31:21PM +0100, Veronika Molnarova wrote:
> > > >
> > > >
> > > > On 12/10/24 16:20, Veronika Molnarova wrote:
> > > > >
> > > > >
> > > > > On 11/23/24 00:12, vmolnaro@redhat.com wrote:
> > > > >> From: Veronika Molnarova <vmolnaro@redhat.com>
> > > > >>
> > > > >> Test case "stat_all_pmu.sh" is not correctly checking 'perf stat' output
> > > > >> due to a poor design. Firstly, having the 'set -e' option with a trap
> > > > >> catching the sigexit causes the shell to exit immediately if 'perf stat' ends
> > > > >> with any non-zero value, which is then caught by the trap reporting an
> > > > >> unexpected signal. This causes events that should be parsed by the if-else
> > > > >> statement to be caught by the trap handler and are reported as errors:
> > > > >>
> > > > >>     $ perf test -vv "perf all pmu"
> > > > >>     Testing i915/actual-frequency/
> > > > >>     Unexpected signal in main
> > > > >>     Error:
> > > > >>     Access to performance monitoring and observability operations is limited.
> > > > >>
> > > > >> Secondly, the if-else branches are not exclusive as the checking if the
> > > > >> event is present in the output log covers also the "<not supported>"
> > > > >> events, which should be accepted, and also the "Bad name events", which
> > > > >> should be rejected.
> > > > >>
> > > > >> Remove the "set -e" option from the test case, correctly parse the
> > > > >> "perf stat" output log and check its return value. Add the missing
> > > > >> outputs for the 'perf stat' result and also add logs messages to
> > > > >> report the branch that parsed the event for more info.
> > > > >
> > > > > Hi Ian,
> > > > >
> > > > > is there anything that I am missing? If so would be great to know the idea
> > > > > of your patch. This issue is getting quite old so would be great to get a proper
> > > > > fix together.
> > > > >
> > > > > Thanks,
> > > > > Veronika
> > > >
> > > > Hello,
> > > >
> > > > just wanted to ping the patch. Would be great to sort it out and close the issue.
> > >
> > > Ian, are you ok with this?
> > >
> >
> > Just ping again for this patch, can this fix be acked?
> >
> > And I already tested this patch and it works well on the ppc64le system.
> >
> > Tested-by: Qiao Zhao <qzhao@redhat.com>
> 
> Revert the Tested-by, and when more tests on Intel EMR system, and show other
> "Bad event name" and " value too big for format (umask)" errors.
> This patch needs more tests and rewrites.

Thanks for the test, though!
Namhyung