perf test: Ignore security failures in all PMU test

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

Posted by Ian Rogers 2 months ago

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
+  fi
+  if echo "$result" | grep -q "Access to performance monitoring and observability operations is limited."
+  then
+    # Access is limited, so ignore.
+    continue
+  fi
+
+  # 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
+    # Event seen in output.
+    continue
   fi
+  echo "Error: event '$p' not printed in:"
+  echo "$result"
+  err=1
 done
 
-exit 0
+trap - EXIT TERM INT
+exit $err
-- 
2.46.0.792.g87dc391469-goog

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

Posted by Namhyung Kim 2 months ago

On Wed, 25 Sep 2024 10:30:13 -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.
> 
> 

Applied to perf-tools-next, thanks!

Best regards,
Namhyung

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

Posted by Namhyung Kim 2 months ago

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

> +  fi
> +  if echo "$result" | grep -q "Access to performance monitoring and observability operations is limited."
> +  then
> +    # Access is limited, so ignore.
> +    continue
> +  fi
> +
> +  # We failed to see the event and it is supported. Possibly the workload was
> +  # too small so retry with something longer.

Ok, maybe we want to check "<not counted>" events specifically.

Thanks,
Namhyung


> +  result=$(perf stat -e "$p" perf bench internals synthesize 2>&1)
> +  if echo "$result" | grep -q "$p"
> +  then
> +    # Event seen in output.
> +    continue
>    fi
> +  echo "Error: event '$p' not printed in:"
> +  echo "$result"
> +  err=1
>  done
>  
> -exit 0
> +trap - EXIT TERM INT
> +exit $err
> -- 
> 2.46.0.792.g87dc391469-goog
>

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

Posted by Ian Rogers 2 months ago

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.

Thanks,
Ian

> > +  fi
> > +  if echo "$result" | grep -q "Access to performance monitoring and observability operations is limited."
> > +  then
> > +    # Access is limited, so ignore.
> > +    continue
> > +  fi
> > +
> > +  # We failed to see the event and it is supported. Possibly the workload was
> > +  # too small so retry with something longer.
>
> Ok, maybe we want to check "<not counted>" events specifically.
>
> Thanks,
> Namhyung
>
>
> > +  result=$(perf stat -e "$p" perf bench internals synthesize 2>&1)
> > +  if echo "$result" | grep -q "$p"
> > +  then
> > +    # Event seen in output.
> > +    continue
> >    fi
> > +  echo "Error: event '$p' not printed in:"
> > +  echo "$result"
> > +  err=1
> >  done
> >
> > -exit 0
> > +trap - EXIT TERM INT
> > +exit $err
> > --
> > 2.46.0.792.g87dc391469-goog
> >

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

Posted by Namhyung Kim 2 months ago

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