[PATCH v2] perf test: Workload test of metric and metricgroups

[PATCH v2] perf test: Workload test of metric and metricgroups

[Ian Rogers]

Test every metric and metricgroup with 'true' as a workload.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/stat_all_metricgroups.sh | 12 ++++++++++++
 tools/perf/tests/shell/stat_all_metrics.sh      | 16 ++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100755 tools/perf/tests/shell/stat_all_metricgroups.sh
 create mode 100755 tools/perf/tests/shell/stat_all_metrics.sh

diff --git a/tools/perf/tests/shell/stat_all_metricgroups.sh b/tools/perf/tests/shell/stat_all_metricgroups.sh
new file mode 100755
index 000000000000..de24d374ce24
--- /dev/null
+++ b/tools/perf/tests/shell/stat_all_metricgroups.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+# perf all metricgroups test
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+for m in $(perf list --raw-dump metricgroups); do
+  echo "Testing $m"
+  perf stat -M "$m" true
+done
+
+exit 0
diff --git a/tools/perf/tests/shell/stat_all_metrics.sh b/tools/perf/tests/shell/stat_all_metrics.sh
new file mode 100755
index 000000000000..81b19ba27e68
--- /dev/null
+++ b/tools/perf/tests/shell/stat_all_metrics.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+# perf all metrics test
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+for m in `perf list --raw-dump metrics`; do
+  echo "Testing $m"
+  result=$(perf stat -M "$m" true)
+  if [[ "$result" =~ "$m" ]]; then
+    echo "Metric not printed: $m"
+    exit 1
+  fi
+done
+
+exit 0
-- 
2.33.0.309.g3052b89438-goog

Re: [PATCH v2] perf test: Workload test of metric and metricgroups

[Jiri Olsa]

On Wed, Sep 15, 2021 at 11:05:25PM -0700, Ian Rogers wrote:
> Test every metric and metricgroup with 'true' as a workload.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> ---
>  tools/perf/tests/shell/stat_all_metricgroups.sh | 12 ++++++++++++
>  tools/perf/tests/shell/stat_all_metrics.sh      | 16 ++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100755 tools/perf/tests/shell/stat_all_metricgroups.sh
>  create mode 100755 tools/perf/tests/shell/stat_all_metrics.sh
> 
> diff --git a/tools/perf/tests/shell/stat_all_metricgroups.sh b/tools/perf/tests/shell/stat_all_metricgroups.sh
> new file mode 100755
> index 000000000000..de24d374ce24
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat_all_metricgroups.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +# perf all metricgroups test
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +for m in $(perf list --raw-dump metricgroups); do
> +  echo "Testing $m"
> +  perf stat -M "$m" true
> +done
> +
> +exit 0
> diff --git a/tools/perf/tests/shell/stat_all_metrics.sh b/tools/perf/tests/shell/stat_all_metrics.sh
> new file mode 100755
> index 000000000000..81b19ba27e68
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat_all_metrics.sh
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +# perf all metrics test
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +for m in `perf list --raw-dump metrics`; do
> +  echo "Testing $m"
> +  result=$(perf stat -M "$m" true)
> +  if [[ "$result" =~ "$m" ]]; then
> +    echo "Metric not printed: $m"
> +    exit 1
> +  fi
> +done
> +
> +exit 0
> -- 
> 2.33.0.309.g3052b89438-goog
> 

Re: [PATCH v2] perf test: Workload test of metric and metricgroups

[John Garry]

On 16/09/2021 07:05, Ian Rogers wrote:
> Test every metric and metricgroup with 'true' as a workload.
> 
> Signed-off-by: Ian Rogers<irogers@google.com>

Reviewed-by: John Garry <john.garry@huawei.com>

Note that I also had a local test for pmu events:
for e in `$PERF list --raw-dump pmu`; do
   echo "Testing $e"
   result=$($PERF stat -v -e "$e" perf bench internals synthesize)
   if [[ "$result" =~ "$e" ]]; then
     echo "Event not printed: $e"
     exit 1
   fi
done

Is there any value in upstreaming this? I could not see same already 
there. Or else make your new script generic, so that it accepts an 
argument whether to test events or metrics or metricgroups

Re: [PATCH v2] perf test: Workload test of metric and metricgroups

[Ian Rogers]

On Thu, Sep 16, 2021 at 12:37 AM John Garry <john.garry@huawei.com> wrote:
>
> On 16/09/2021 07:05, Ian Rogers wrote:
> > Test every metric and metricgroup with 'true' as a workload.
> >
> > Signed-off-by: Ian Rogers<irogers@google.com>
>
> Reviewed-by: John Garry <john.garry@huawei.com>
>
> Note that I also had a local test for pmu events:
> for e in `$PERF list --raw-dump pmu`; do
>    echo "Testing $e"
>    result=$($PERF stat -v -e "$e" perf bench internals synthesize)
>    if [[ "$result" =~ "$e" ]]; then
>      echo "Event not printed: $e"
>      exit 1
>    fi
> done
>
> Is there any value in upstreaming this? I could not see same already
> there. Or else make your new script generic, so that it accepts an
> argument whether to test events or metrics or metricgroups

It is not easy to make a generic script with the current shell test
infrastructure. I made a variant of this test:
https://lore.kernel.org/linux-perf-users/20210917184240.2181186-2-irogers@google.com/T/#u
For skylake it ran for 1m15s and so it may be too slow. Perhaps we
need to add to the test infrastructure with some kind of speed flag.

Thanks,
Ian

Re: [PATCH v2] perf test: Workload test of metric and metricgroups

[John Garry]

On 17/09/2021 20:16, Ian Rogers wrote:
> On Thu, Sep 16, 2021 at 12:37 AM John Garry<john.garry@huawei.com>  wrote:
>> On 16/09/2021 07:05, Ian Rogers wrote:
>>> Test every metric and metricgroup with 'true' as a workload.
>>>
>>> Signed-off-by: Ian Rogers<irogers@google.com>
>> Reviewed-by: John Garry<john.garry@huawei.com>
>>
>> Note that I also had a local test for pmu events:
>> for e in `$PERF list --raw-dump pmu`; do
>>     echo "Testing $e"
>>     result=$($PERF stat -v -e "$e" perf bench internals synthesize)
>>     if [[ "$result" =~ "$e" ]]; then
>>       echo "Event not printed: $e"
>>       exit 1
>>     fi
>> done
>>
>> Is there any value in upstreaming this? I could not see same already
>> there. Or else make your new script generic, so that it accepts an
>> argument whether to test events or metrics or metricgroups
> It is not easy to make a generic script with the current shell test
> infrastructure. I made a variant of this test:
> https://lore.kernel.org/linux-perf-users/20210917184240.2181186-2-irogers@google.com/T/#u
> For skylake it ran for 1m15s and so it may be too slow. Perhaps we
> need to add to the test infrastructure with some kind of speed flag.

Hi Ian,

I suggested this before I realized that it would be called from "perf test".

You think that 1m15s could be considered too slow, but I think that it 
could be much slower to now run "perf test" on some other systems. Like 
my arm64 system - see series 
https://lore.kernel.org/linux-perf-users/1631795665-240946-1-git-send-email-john.garry@huawei.com/T/#t 
- where I mention that we have >700 HW PMU events (before applying that 
series to take advantage of the event merging). And each of those events 
would be tested individually - slow...

So firstly maybe a speed or test level flag could be added before we try 
this. Sorry for any inconvenience caused.

Thanks,
John

Re: [PATCH v2] perf test: Workload test of metric and metricgroups

[Ian Rogers]

On Mon, Sep 20, 2021 at 3:00 AM John Garry <john.garry@huawei.com> wrote:
>
> On 17/09/2021 20:16, Ian Rogers wrote:
> > On Thu, Sep 16, 2021 at 12:37 AM John Garry<john.garry@huawei.com>  wrote:
> >> On 16/09/2021 07:05, Ian Rogers wrote:
> >>> Test every metric and metricgroup with 'true' as a workload.
> >>>
> >>> Signed-off-by: Ian Rogers<irogers@google.com>
> >> Reviewed-by: John Garry<john.garry@huawei.com>
> >>
> >> Note that I also had a local test for pmu events:
> >> for e in `$PERF list --raw-dump pmu`; do
> >>     echo "Testing $e"
> >>     result=$($PERF stat -v -e "$e" perf bench internals synthesize)
> >>     if [[ "$result" =~ "$e" ]]; then
> >>       echo "Event not printed: $e"
> >>       exit 1
> >>     fi
> >> done
> >>
> >> Is there any value in upstreaming this? I could not see same already
> >> there. Or else make your new script generic, so that it accepts an
> >> argument whether to test events or metrics or metricgroups
> > It is not easy to make a generic script with the current shell test
> > infrastructure. I made a variant of this test:
> > https://lore.kernel.org/linux-perf-users/20210917184240.2181186-2-irogers@google.com/T/#u
> > For skylake it ran for 1m15s and so it may be too slow. Perhaps we
> > need to add to the test infrastructure with some kind of speed flag.
>
> Hi Ian,
>
> I suggested this before I realized that it would be called from "perf test".
>
> You think that 1m15s could be considered too slow, but I think that it
> could be much slower to now run "perf test" on some other systems. Like
> my arm64 system - see series
> https://lore.kernel.org/linux-perf-users/1631795665-240946-1-git-send-email-john.garry@huawei.com/T/#t
> - where I mention that we have >700 HW PMU events (before applying that
> series to take advantage of the event merging). And each of those events
> would be tested individually - slow...
>
> So firstly maybe a speed or test level flag could be added before we try
> this. Sorry for any inconvenience caused.

Hi John,

I think a flag would be best. I'll look to add a notion of test sizes,
as that mirrors what works well at Google. We can then tag a test as
small, medium, large and default to just say running small and medium
tests.

Thanks,
Ian

> Thanks,
> John

Re: [PATCH v2] perf test: Workload test of metric and metricgroups

[Paul A. Clarke]

On Wed, Sep 15, 2021 at 11:05:25PM -0700, Ian Rogers wrote:
> Test every metric and metricgroup with 'true' as a workload.

Good idea!  (However...)

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/shell/stat_all_metricgroups.sh | 12 ++++++++++++
>  tools/perf/tests/shell/stat_all_metrics.sh      | 16 ++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100755 tools/perf/tests/shell/stat_all_metricgroups.sh
>  create mode 100755 tools/perf/tests/shell/stat_all_metrics.sh
> 
> diff --git a/tools/perf/tests/shell/stat_all_metricgroups.sh b/tools/perf/tests/shell/stat_all_metricgroups.sh
> new file mode 100755
> index 000000000000..de24d374ce24
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat_all_metricgroups.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +# perf all metricgroups test
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +for m in $(perf list --raw-dump metricgroups); do
> +  echo "Testing $m"
> +  perf stat -M "$m" true
> +done
> +
> +exit 0

This always succeeds. Is that what you want?
Maybe check the return code from "perf", at least?

> diff --git a/tools/perf/tests/shell/stat_all_metrics.sh b/tools/perf/tests/shell/stat_all_metrics.sh
> new file mode 100755
> index 000000000000..81b19ba27e68
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat_all_metrics.sh
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +# perf all metrics test
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +for m in `perf list --raw-dump metrics`; do
> +  echo "Testing $m"
> +  result=$(perf stat -M "$m" true)

I don't think this is doing what you want it to do, as it just captures the
output of "true", which is always empty.

> +  if [[ "$result" =~ "$m" ]]; then

So this always fails to match, and you'll never fail here, either. :-)

> +    echo "Metric not printed: $m"
> +    exit 1
> +  fi
> +done
> +
> +exit 0

You may want to redirect the output of the "perf" command to a temporary file,
then grep within that.  And, you'll need to remove the file before running the
perf command, because if it fails, it will leave any existing file untouched.

Thanks! :-)

PC

Re: [PATCH v2] perf test: Workload test of metric and metricgroups

[Ian Rogers]

On Thu, Sep 16, 2021 at 5:04 AM Paul A. Clarke <pc@us.ibm.com> wrote:
>
> On Wed, Sep 15, 2021 at 11:05:25PM -0700, Ian Rogers wrote:
> > Test every metric and metricgroup with 'true' as a workload.
>
> Good idea!  (However...)
>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/tests/shell/stat_all_metricgroups.sh | 12 ++++++++++++
> >  tools/perf/tests/shell/stat_all_metrics.sh      | 16 ++++++++++++++++
> >  2 files changed, 28 insertions(+)
> >  create mode 100755 tools/perf/tests/shell/stat_all_metricgroups.sh
> >  create mode 100755 tools/perf/tests/shell/stat_all_metrics.sh
> >
> > diff --git a/tools/perf/tests/shell/stat_all_metricgroups.sh b/tools/perf/tests/shell/stat_all_metricgroups.sh
> > new file mode 100755
> > index 000000000000..de24d374ce24
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/stat_all_metricgroups.sh
> > @@ -0,0 +1,12 @@
> > +#!/bin/sh
> > +# perf all metricgroups test
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -e
> > +
> > +for m in $(perf list --raw-dump metricgroups); do
> > +  echo "Testing $m"
> > +  perf stat -M "$m" true
> > +done
> > +
> > +exit 0
>
> This always succeeds. Is that what you want?
> Maybe check the return code from "perf", at least?

The "set -e" above means that if the perf command fails then the test exits.

> > diff --git a/tools/perf/tests/shell/stat_all_metrics.sh b/tools/perf/tests/shell/stat_all_metrics.sh
> > new file mode 100755
> > index 000000000000..81b19ba27e68
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/stat_all_metrics.sh
> > @@ -0,0 +1,16 @@
> > +#!/bin/sh
> > +# perf all metrics test
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -e
> > +
> > +for m in `perf list --raw-dump metrics`; do
> > +  echo "Testing $m"
> > +  result=$(perf stat -M "$m" true)
>
> I don't think this is doing what you want it to do, as it just captures the
> output of "true", which is always empty.
>
> > +  if [[ "$result" =~ "$m" ]]; then
>
> So this always fails to match, and you'll never fail here, either. :-)

Doh! Thanks for catching this!

> > +    echo "Metric not printed: $m"
> > +    exit 1
> > +  fi
> > +done
> > +
> > +exit 0
>
> You may want to redirect the output of the "perf" command to a temporary file,
> then grep within that.  And, you'll need to remove the file before running the
> perf command, because if it fails, it will leave any existing file untouched.

I fixed it by redirecting stderr.

Thanks!
Ian

> Thanks! :-)
>
> PC