Re: [PATCH v1] perf test: Ignore security failures in all PMU test

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Sep 26, 2024 at 08:24:30AM -0700, Ian Rogers wrote:
> On Wed, Sep 25, 2024 at 9:52 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, Sep 25, 2024 at 10:30:13AM -0700, Ian Rogers wrote:
> > > Refactor code to have some more error diagnosis on traps, etc. and to
> > > do less work on each line. Add an ignore situation for security failures.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/tests/shell/stat_all_pmu.sh | 52 ++++++++++++++++++++------
> > >  1 file changed, 40 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/tools/perf/tests/shell/stat_all_pmu.sh b/tools/perf/tests/shell/stat_all_pmu.sh
> > > index d2a3506e0d19..42456d89c5da 100755
> > > --- a/tools/perf/tests/shell/stat_all_pmu.sh
> > > +++ b/tools/perf/tests/shell/stat_all_pmu.sh
> > > @@ -1,23 +1,51 @@
> > > -#!/bin/sh
> > > +#!/bin/bash
> > >  # perf all PMU test
> > >  # SPDX-License-Identifier: GPL-2.0
> > >
> > >  set -e
> > > +err=0
> > > +result=""
> > > +
> > > +trap_cleanup() {
> > > +  echo "Unexpected signal in ${FUNCNAME[1]}"
> > > +  echo "$result"
> > > +  exit 1
> > > +}
> > > +trap trap_cleanup EXIT TERM INT
> > >
> > >  # Test all PMU events; however exclude parameterized ones (name contains '?')
> > > -for p in $(perf list --raw-dump pmu | sed 's/[[:graph:]]\+?[[:graph:]]\+[[:space:]]//g'); do
> > > +for p in $(perf list --raw-dump pmu | sed 's/[[:graph:]]\+?[[:graph:]]\+[[:space:]]//g')
> > > +do
> > >    echo "Testing $p"
> > >    result=$(perf stat -e "$p" true 2>&1)
> > > -  if ! echo "$result" | grep -q "$p" && ! echo "$result" | grep -q "<not supported>" ; then
> > > -    # We failed to see the event and it is supported. Possibly the workload was
> > > -    # too small so retry with something longer.
> > > -    result=$(perf stat -e "$p" perf bench internals synthesize 2>&1)
> > > -    if ! echo "$result" | grep -q "$p" ; then
> > > -      echo "Event '$p' not printed in:"
> > > -      echo "$result"
> > > -      exit 1
> > > -    fi
> > > +  if echo "$result" | grep -q "$p"
> > > +  then
> > > +    # Event seen in output.
> > > +    continue
> > > +  fi
> > > +  if echo "$result" | grep -q "<not supported>"
> > > +  then
> > > +    # Event not supported, so ignore.
> > > +    continue
> >
> > I'm curious about this case.  It'll show up even if it's not supported
> > right?  Then the first condition would match and it doesn't reach here.
> >
> >   $ perf stat -e LLC-loads true
> >
> >    Performance counter stats for 'true':
> >
> >      <not supported>      LLC-loads
> >
> >          0.001213558 seconds time elapsed
> >
> >          0.001373000 seconds user
> >          0.000000000 seconds sys
> 
> We carry this change at Google for an issue on older series kernels. I
> was resending the patch as a courtesy in the migration of development
> work to:
> https://github.com/googleprodkernel/linux-perf
> I'm okay with this being a Google only change.

I'm not sure if it's a kernel issue.  IIUC you want to run perf stat
again if some events are not counted, right?

I understand your patch just converted the existing logic and added the
check for security related failures.  So I'll pick it up for now but we
may want to revisit this later.

Thanks,
Namhyung