[PATCHSET 0/6] perf test: Improve perf record tests (v1)

[PATCHSET 0/6] perf test: Improve perf record tests (v1)

[Namhyung Kim]

Hello,

This patchset improves the perf record tests to check more cases so
that it can find problems early.  The motivation is a failure in
per-thread mmap with multi-threaded targets which Adrian is working on
the fix.

I added a custom test program and more combinations like system-wide
and command line workload (in per-process mode) testing with
multi-threaded recording mode.

Currently it fails on per-thread and register capture tests.  The
system-wide test was skipped since I ran it as a normal user.  We can
use this to verify Adirian's fix and future works.

  $ ./perf test -v 86
   86: perf record tests                                               :
  --- start ---
  test child forked, pid 1190747
  Build a test program
  Basic --per-thread mode test
  Per-thread record [Failed record]
  Register capture test
  Register capture test [Failed missing output]
  Basic --system-wide mode test
  System-wide record [Skipped not supported]
  Basic target workload test
  Basic target workload test [Success]
  test child finished with -1
  ---- end ----
  perf record tests: FAILED!


You can find it in 'perf/record-test-v1' branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (6):
  perf test: Do not use instructions:u explicitly
  perf test: Use a test program in perf record tests
  perf test: Test record with --threads option
  perf test: Add system-wide mode in perf record tests
  perf test: Add target workload test in perf record tests
  perf test: Do not set TEST_SKIP for record subtests

 tools/perf/tests/shell/record.sh | 150 +++++++++++++++++++++++++++----
 1 file changed, 133 insertions(+), 17 deletions(-)


base-commit: 6c3bd8d3e01d9014312caa52e4ef1c29d5249648
-- 
2.37.2.789.g6183377224-goog

[PATCH 1/6] perf test: Do not use instructions:u explicitly

[Namhyung Kim]

I think it's to support non-root user tests.  But perf record can handle
the case and fall back to a software event (cpu-clock).  Practically this
would affect when it's run on a VM, but it seems no reason to prevent running
the test in the guest.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/record.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 00c7285ce1ac..40b087bfdb76 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -21,18 +21,18 @@ trap trap_cleanup exit term int
 
 test_per_thread() {
   echo "Basic --per-thread mode test"
-  if ! perf record -e instructions:u -o ${perfdata} --quiet true 2> /dev/null
+  if ! perf record -o /dev/null --quiet true 2> /dev/null
   then
-    echo "Per-thread record [Skipped instructions:u not supported]"
+    echo "Per-thread record [Skipped event not supported]"
     if [ $err -ne 1 ]
     then
       err=2
     fi
     return
   fi
-  if ! perf record -e instructions:u --per-thread -o ${perfdata} true 2> /dev/null
+  if ! perf record --per-thread -o ${perfdata} true 2> /dev/null
   then
-    echo "Per-thread record of instructions:u [Failed]"
+    echo "Per-thread record [Failed record]"
     err=1
     return
   fi
@@ -49,7 +49,7 @@ test_register_capture() {
   echo "Register capture test"
   if ! perf list | egrep -q 'br_inst_retired.near_call'
   then
-    echo "Register capture test [Skipped missing instruction]"
+    echo "Register capture test [Skipped missing event]"
     if [ $err -ne 1 ]
     then
       err=2
-- 
2.37.2.789.g6183377224-goog

[PATCH 2/6] perf test: Use a test program in perf record tests

[Namhyung Kim]

If the system has cc it could build a test program with two threads
and then use it for more detailed testing.  Also it adds initial delay
of 3ms to profile a multi-threaded target.  This change make the test
failing but that's what we want to check for now.

If cc is not found, it falls back to use the default value 'true'.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/record.sh | 58 +++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 40b087bfdb76..cea3c7b7e2cd 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -6,10 +6,18 @@ set -e
 
 err=0
 perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
+testprog=$(mktemp /tmp/__perf_test.prog.XXXXXX)
+testsym="test_loop"
+testopt="-D 3"
 
 cleanup() {
   rm -f ${perfdata}
   rm -f ${perfdata}.old
+
+  if [ "${testprog}" != "true" ]; then
+    rm -f ${testprog}
+  fi
+
   trap - exit term int
 }
 
@@ -19,9 +27,49 @@ trap_cleanup() {
 }
 trap trap_cleanup exit term int
 
+build_test_program() {
+  if ! [ -x "$(command -v cc)" ]; then
+    # No CC found. Fall back to 'true'
+    testprog=true
+    testsym=true
+    testopt=''
+    return
+  fi
+
+  echo "Build a test program"
+  cat <<EOF | cc -o ${testprog} -xc - -pthread
+#include <stdio.h>
+#include <unistd.h>
+#include <pthread.h>
+
+void test_loop(void) {
+  volatile int count = 1000000;
+
+  // wait for perf record
+  usleep(5000);
+
+  while (count--)
+    continue;
+}
+
+void *thfunc(void *arg) {
+  test_loop();
+  return NULL;
+}
+
+int main(void) {
+  pthread_t th;
+  pthread_create(&th, NULL, thfunc, NULL);
+  test_loop();
+  pthread_join(th, NULL);
+  return 0;
+}
+EOF
+}
+
 test_per_thread() {
   echo "Basic --per-thread mode test"
-  if ! perf record -o /dev/null --quiet true 2> /dev/null
+  if ! perf record -o /dev/null --quiet ${testprog} 2> /dev/null
   then
     echo "Per-thread record [Skipped event not supported]"
     if [ $err -ne 1 ]
@@ -30,13 +78,13 @@ test_per_thread() {
     fi
     return
   fi
-  if ! perf record --per-thread -o ${perfdata} true 2> /dev/null
+  if ! perf record --per-thread ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
   then
     echo "Per-thread record [Failed record]"
     err=1
     return
   fi
-  if ! perf report -i ${perfdata} -q | egrep -q true
+  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
   then
     echo "Per-thread record [Failed missing output]"
     err=1
@@ -62,7 +110,7 @@ test_register_capture() {
     return
   fi
   if ! perf record -o - --intr-regs=di,r8,dx,cx -e cpu/br_inst_retired.near_call/p \
-    -c 1000 --per-thread true 2> /dev/null \
+    -c 1000 --per-thread ${testopt} ${testprog} 2> /dev/null \
     | perf script -F ip,sym,iregs -i - 2> /dev/null \
     | egrep -q "DI:"
   then
@@ -73,6 +121,8 @@ test_register_capture() {
   echo "Register capture test [Success]"
 }
 
+build_test_program
+
 test_per_thread
 test_register_capture
 
-- 
2.37.2.789.g6183377224-goog

[PATCH 3/6] perf test: Test record with --threads option

[Namhyung Kim]

To verify per-thread mode is working with multi-thread recording.
Use two software events for testing to check event set-output handling.
Also update the cleanup routine because threads recording produces data
in a directory.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/record.sh | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index cea3c7b7e2cd..3331fb092654 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -11,8 +11,8 @@ testsym="test_loop"
 testopt="-D 3"
 
 cleanup() {
-  rm -f ${perfdata}
-  rm -f ${perfdata}.old
+  rm -rf ${perfdata}
+  rm -rf ${perfdata}.old
 
   if [ "${testprog}" != "true" ]; then
     rm -f ${testprog}
@@ -90,6 +90,19 @@ test_per_thread() {
     err=1
     return
   fi
+  if ! perf record -e cpu-clock,cs --per-thread --threads=core ${testopt} \
+    -o ${perfdata} ${testprog} 2> /dev/null
+  then
+    echo "Per-thread record with --threads [Failed]"
+    err=1
+    return
+  fi
+  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
+  then
+    echo "Per-thread record with --threads [Failed missing output]"
+    err=1
+    return
+  fi
   echo "Basic --per-thread mode test [Success]"
 }
 
-- 
2.37.2.789.g6183377224-goog

[PATCH 4/6] perf test: Add system-wide mode in perf record tests

[Namhyung Kim]

Add system wide recording test with the same pattern.  It'd skip the
test when it failes to run perf record.  For system-wide mode, it needs
to avoid build-id collection and synthesis because the test only cares
about the test program and kernel would generates necessary events as
the process starts.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/record.sh | 34 ++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 3331fb092654..bd4ef60948bd 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -134,10 +134,44 @@ test_register_capture() {
   echo "Register capture test [Success]"
 }
 
+test_system_wide() {
+  echo "Basic --system-wide mode test"
+  if ! perf record -aB --synth=no ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
+  then
+    echo "System-wide record [Skipped not supported]"
+    if [ $err -ne 1 ]
+    then
+      err=2
+    fi
+    return
+  fi
+  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
+  then
+    echo "System-wide record [Failed missing output]"
+    err=1
+    return
+  fi
+  if ! perf record -aB --synth=no -e cpu-clock,cs --threads=cpu ${testopt} \
+    -o ${perfdata} ${testprog} 2> /dev/null
+  then
+    echo "System-wide test [Failed recording with threads]"
+    err=1
+    return
+  fi
+  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
+  then
+    echo "System-wide record [Failed missing output]"
+    err=1
+    return
+  fi
+  echo "Basic --system-wide mode test [Success]"
+}
+
 build_test_program
 
 test_per_thread
 test_register_capture
+test_system_wide
 
 cleanup
 exit $err
-- 
2.37.2.789.g6183377224-goog

[PATCH 5/6] perf test: Add target workload test in perf record tests

[Namhyung Kim]

Add a subtest which profiles the given workload on the command line.
As it's a minimal requirement, test should run ok so it doesn't skip
the test even if it failed to run the perf record.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/record.sh | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index bd4ef60948bd..ff66e58f3a26 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -167,11 +167,42 @@ test_system_wide() {
   echo "Basic --system-wide mode test [Success]"
 }
 
+test_workload() {
+  echo "Basic target workload test"
+  if ! perf record ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
+  then
+    echo "Workload record [Failed record]"
+    err=1
+    return
+  fi
+  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
+  then
+    echo "Workload record [Failed missing output]"
+    err=1
+    return
+  fi
+  if ! perf record -e cpu-clock,cs --threads=package ${testopt} \
+    -o ${perfdata} ${testprog} 2> /dev/null
+  then
+    echo "Workload record [Failed recording with threads]"
+    err=1
+    return
+  fi
+  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
+  then
+    echo "Workload record [Failed missing output]"
+    err=1
+    return
+  fi
+  echo "Basic target workload test [Success]"
+}
+
 build_test_program
 
 test_per_thread
 test_register_capture
 test_system_wide
+test_workload
 
 cleanup
 exit $err
-- 
2.37.2.789.g6183377224-goog

[PATCH 6/6] perf test: Do not set TEST_SKIP for record subtests

[Namhyung Kim]

It now has 4 sub tests and one of them should run at least.
But once TEST_SKIP (= 2) return value is set, it won't be overwritten
unless there's a failure.  I think we should return success when one
or more tested are skipped but the remaining subtests are passed.

So update the test code not to set the err variable when it skips
the test.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/record.sh | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index ff66e58f3a26..487981767455 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -72,10 +72,6 @@ test_per_thread() {
   if ! perf record -o /dev/null --quiet ${testprog} 2> /dev/null
   then
     echo "Per-thread record [Skipped event not supported]"
-    if [ $err -ne 1 ]
-    then
-      err=2
-    fi
     return
   fi
   if ! perf record --per-thread ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
@@ -111,10 +107,6 @@ test_register_capture() {
   if ! perf list | egrep -q 'br_inst_retired.near_call'
   then
     echo "Register capture test [Skipped missing event]"
-    if [ $err -ne 1 ]
-    then
-      err=2
-    fi
     return
   fi
   if ! perf record --intr-regs=\? 2>&1 | egrep -q 'available registers: AX BX CX DX SI DI BP SP IP FLAGS CS SS R8 R9 R10 R11 R12 R13 R14 R15'
@@ -139,10 +131,6 @@ test_system_wide() {
   if ! perf record -aB --synth=no ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
   then
     echo "System-wide record [Skipped not supported]"
-    if [ $err -ne 1 ]
-    then
-      err=2
-    fi
     return
   fi
   if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 1/6] perf test: Do not use instructions:u explicitly

[Adrian Hunter]

On 7/09/22 09:46, Namhyung Kim wrote:
> I think it's to support non-root user tests.  But perf record can handle
> the case and fall back to a software event (cpu-clock).  Practically this
> would affect when it's run on a VM, but it seems no reason to prevent running
> the test in the guest.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/tests/shell/record.sh | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index 00c7285ce1ac..40b087bfdb76 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -21,18 +21,18 @@ trap trap_cleanup exit term int
>  
>  test_per_thread() {
>    echo "Basic --per-thread mode test"
> -  if ! perf record -e instructions:u -o ${perfdata} --quiet true 2> /dev/null
> +  if ! perf record -o /dev/null --quiet true 2> /dev/null
>    then
> -    echo "Per-thread record [Skipped instructions:u not supported]"
> +    echo "Per-thread record [Skipped event not supported]"
>      if [ $err -ne 1 ]
>      then
>        err=2
>      fi
>      return
>    fi
> -  if ! perf record -e instructions:u --per-thread -o ${perfdata} true 2> /dev/null
> +  if ! perf record --per-thread -o ${perfdata} true 2> /dev/null
>    then
> -    echo "Per-thread record of instructions:u [Failed]"
> +    echo "Per-thread record [Failed record]"
>      err=1
>      return
>    fi
> @@ -49,7 +49,7 @@ test_register_capture() {
>    echo "Register capture test"
>    if ! perf list | egrep -q 'br_inst_retired.near_call'
>    then
> -    echo "Register capture test [Skipped missing instruction]"
> +    echo "Register capture test [Skipped missing event]"
>      if [ $err -ne 1 ]
>      then
>        err=2

Re: [PATCH 2/6] perf test: Use a test program in perf record tests

[Adrian Hunter]

On 7/09/22 09:46, Namhyung Kim wrote:
> If the system has cc it could build a test program with two threads
> and then use it for more detailed testing.  Also it adds initial delay
> of 3ms to profile a multi-threaded target.  This change make the test
> failing but that's what we want to check for now.

So the delay is just to get a separate dummy event?

I hit the issue from this patch:

https://lore.kernel.org/lkml/20220711180706.3418612-1-kan.liang@linux.intel.com/

That is, if I apply the patch above then the test passes, otherwise it fails always.

> 
> If cc is not found, it falls back to use the default value 'true'.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/shell/record.sh | 58 +++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index 40b087bfdb76..cea3c7b7e2cd 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -6,10 +6,18 @@ set -e
>  
>  err=0
>  perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +testprog=$(mktemp /tmp/__perf_test.prog.XXXXXX)
> +testsym="test_loop"
> +testopt="-D 3"
>  
>  cleanup() {
>    rm -f ${perfdata}
>    rm -f ${perfdata}.old
> +
> +  if [ "${testprog}" != "true" ]; then
> +    rm -f ${testprog}
> +  fi
> +
>    trap - exit term int
>  }
>  
> @@ -19,9 +27,49 @@ trap_cleanup() {
>  }
>  trap trap_cleanup exit term int
>  
> +build_test_program() {
> +  if ! [ -x "$(command -v cc)" ]; then
> +    # No CC found. Fall back to 'true'
> +    testprog=true
> +    testsym=true
> +    testopt=''
> +    return
> +  fi
> +
> +  echo "Build a test program"
> +  cat <<EOF | cc -o ${testprog} -xc - -pthread
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <pthread.h>
> +
> +void test_loop(void) {
> +  volatile int count = 1000000;
> +
> +  // wait for perf record
> +  usleep(5000);
> +
> +  while (count--)
> +    continue;
> +}
> +
> +void *thfunc(void *arg) {
> +  test_loop();
> +  return NULL;
> +}
> +
> +int main(void) {
> +  pthread_t th;
> +  pthread_create(&th, NULL, thfunc, NULL);
> +  test_loop();
> +  pthread_join(th, NULL);
> +  return 0;
> +}
> +EOF
> +}
> +
>  test_per_thread() {
>    echo "Basic --per-thread mode test"
> -  if ! perf record -o /dev/null --quiet true 2> /dev/null
> +  if ! perf record -o /dev/null --quiet ${testprog} 2> /dev/null
>    then
>      echo "Per-thread record [Skipped event not supported]"
>      if [ $err -ne 1 ]
> @@ -30,13 +78,13 @@ test_per_thread() {
>      fi
>      return
>    fi
> -  if ! perf record --per-thread -o ${perfdata} true 2> /dev/null
> +  if ! perf record --per-thread ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
>    then
>      echo "Per-thread record [Failed record]"
>      err=1
>      return
>    fi
> -  if ! perf report -i ${perfdata} -q | egrep -q true
> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
>    then
>      echo "Per-thread record [Failed missing output]"
>      err=1
> @@ -62,7 +110,7 @@ test_register_capture() {
>      return
>    fi
>    if ! perf record -o - --intr-regs=di,r8,dx,cx -e cpu/br_inst_retired.near_call/p \
> -    -c 1000 --per-thread true 2> /dev/null \
> +    -c 1000 --per-thread ${testopt} ${testprog} 2> /dev/null \
>      | perf script -F ip,sym,iregs -i - 2> /dev/null \
>      | egrep -q "DI:"
>    then
> @@ -73,6 +121,8 @@ test_register_capture() {
>    echo "Register capture test [Success]"
>  }
>  
> +build_test_program
> +
>  test_per_thread
>  test_register_capture
>  

Re: [PATCH 2/6] perf test: Use a test program in perf record tests

[Adrian Hunter]

On 7/09/22 09:46, Namhyung Kim wrote:
> If the system has cc it could build a test program with two threads
> and then use it for more detailed testing.  Also it adds initial delay
> of 3ms to profile a multi-threaded target.  This change make the test
> failing but that's what we want to check for now.
> 
> If cc is not found, it falls back to use the default value 'true'.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/shell/record.sh | 58 +++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index 40b087bfdb76..cea3c7b7e2cd 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -6,10 +6,18 @@ set -e
>  
>  err=0
>  perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +testprog=$(mktemp /tmp/__perf_test.prog.XXXXXX)
> +testsym="test_loop"
> +testopt="-D 3"
>  
>  cleanup() {
>    rm -f ${perfdata}
>    rm -f ${perfdata}.old
> +
> +  if [ "${testprog}" != "true" ]; then
> +    rm -f ${testprog}
> +  fi
> +
>    trap - exit term int
>  }
>  
> @@ -19,9 +27,49 @@ trap_cleanup() {
>  }
>  trap trap_cleanup exit term int
>  
> +build_test_program() {
> +  if ! [ -x "$(command -v cc)" ]; then
> +    # No CC found. Fall back to 'true'
> +    testprog=true
> +    testsym=true
> +    testopt=''
> +    return
> +  fi
> +
> +  echo "Build a test program"
> +  cat <<EOF | cc -o ${testprog} -xc - -pthread
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <pthread.h>
> +
> +void test_loop(void) {
> +  volatile int count = 1000000;
> +
> +  // wait for perf record
> +  usleep(5000);
> +
> +  while (count--)
> +    continue;
> +}
> +
> +void *thfunc(void *arg) {
> +  test_loop();
> +  return NULL;
> +}
> +
> +int main(void) {
> +  pthread_t th;
> +  pthread_create(&th, NULL, thfunc, NULL);
> +  test_loop();
> +  pthread_join(th, NULL);
> +  return 0;
> +}
> +EOF
> +}
> +
>  test_per_thread() {
>    echo "Basic --per-thread mode test"
> -  if ! perf record -o /dev/null --quiet true 2> /dev/null
> +  if ! perf record -o /dev/null --quiet ${testprog} 2> /dev/null
>    then
>      echo "Per-thread record [Skipped event not supported]"
>      if [ $err -ne 1 ]
> @@ -30,13 +78,13 @@ test_per_thread() {
>      fi
>      return
>    fi
> -  if ! perf record --per-thread -o ${perfdata} true 2> /dev/null
> +  if ! perf record --per-thread ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
>    then
>      echo "Per-thread record [Failed record]"
>      err=1
>      return
>    fi
> -  if ! perf report -i ${perfdata} -q | egrep -q true
> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
>    then
>      echo "Per-thread record [Failed missing output]"
>      err=1
> @@ -62,7 +110,7 @@ test_register_capture() {
>      return
>    fi
>    if ! perf record -o - --intr-regs=di,r8,dx,cx -e cpu/br_inst_retired.near_call/p \
> -    -c 1000 --per-thread true 2> /dev/null \
> +    -c 1000 --per-thread ${testopt} ${testprog} 2> /dev/null \
>      | perf script -F ip,sym,iregs -i - 2> /dev/null \

With the kernel patch:

https://lore.kernel.org/lkml/20220711180706.3418612-1-kan.liang@linux.intel.com/

I get:

Samples for 'dummy:HG' event do not have IREGS attribute set. Cannot print 'iregs' field.

>      | egrep -q "DI:"
>    then
> @@ -73,6 +121,8 @@ test_register_capture() {
>    echo "Register capture test [Success]"
>  }
>  
> +build_test_program
> +
>  test_per_thread
>  test_register_capture
>  

Re: [PATCH 2/6] perf test: Use a test program in perf record tests

[Adrian Hunter]

On 7/09/22 16:16, Adrian Hunter wrote:
> On 7/09/22 09:46, Namhyung Kim wrote:
>> If the system has cc it could build a test program with two threads
>> and then use it for more detailed testing.  Also it adds initial delay
>> of 3ms to profile a multi-threaded target.  This change make the test
>> failing but that's what we want to check for now.
>>
>> If cc is not found, it falls back to use the default value 'true'.
>>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/tests/shell/record.sh | 58 +++++++++++++++++++++++++++++---
>>  1 file changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
>> index 40b087bfdb76..cea3c7b7e2cd 100755
>> --- a/tools/perf/tests/shell/record.sh
>> +++ b/tools/perf/tests/shell/record.sh
>> @@ -6,10 +6,18 @@ set -e
>>  
>>  err=0
>>  perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
>> +testprog=$(mktemp /tmp/__perf_test.prog.XXXXXX)
>> +testsym="test_loop"
>> +testopt="-D 3"
>>  
>>  cleanup() {
>>    rm -f ${perfdata}
>>    rm -f ${perfdata}.old
>> +
>> +  if [ "${testprog}" != "true" ]; then
>> +    rm -f ${testprog}
>> +  fi
>> +
>>    trap - exit term int
>>  }
>>  
>> @@ -19,9 +27,49 @@ trap_cleanup() {
>>  }
>>  trap trap_cleanup exit term int
>>  
>> +build_test_program() {
>> +  if ! [ -x "$(command -v cc)" ]; then
>> +    # No CC found. Fall back to 'true'
>> +    testprog=true
>> +    testsym=true
>> +    testopt=''
>> +    return
>> +  fi
>> +
>> +  echo "Build a test program"
>> +  cat <<EOF | cc -o ${testprog} -xc - -pthread
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <pthread.h>
>> +
>> +void test_loop(void) {
>> +  volatile int count = 1000000;
>> +
>> +  // wait for perf record
>> +  usleep(5000);
>> +
>> +  while (count--)
>> +    continue;
>> +}
>> +
>> +void *thfunc(void *arg) {
>> +  test_loop();
>> +  return NULL;
>> +}
>> +
>> +int main(void) {
>> +  pthread_t th;
>> +  pthread_create(&th, NULL, thfunc, NULL);
>> +  test_loop();
>> +  pthread_join(th, NULL);
>> +  return 0;
>> +}
>> +EOF
>> +}
>> +
>>  test_per_thread() {
>>    echo "Basic --per-thread mode test"
>> -  if ! perf record -o /dev/null --quiet true 2> /dev/null
>> +  if ! perf record -o /dev/null --quiet ${testprog} 2> /dev/null
>>    then
>>      echo "Per-thread record [Skipped event not supported]"
>>      if [ $err -ne 1 ]
>> @@ -30,13 +78,13 @@ test_per_thread() {
>>      fi
>>      return
>>    fi
>> -  if ! perf record --per-thread -o ${perfdata} true 2> /dev/null
>> +  if ! perf record --per-thread ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
>>    then
>>      echo "Per-thread record [Failed record]"
>>      err=1
>>      return
>>    fi
>> -  if ! perf report -i ${perfdata} -q | egrep -q true
>> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
>>    then
>>      echo "Per-thread record [Failed missing output]"
>>      err=1
>> @@ -62,7 +110,7 @@ test_register_capture() {
>>      return
>>    fi
>>    if ! perf record -o - --intr-regs=di,r8,dx,cx -e cpu/br_inst_retired.near_call/p \
>> -    -c 1000 --per-thread true 2> /dev/null \
>> +    -c 1000 --per-thread ${testopt} ${testprog} 2> /dev/null \
>>      | perf script -F ip,sym,iregs -i - 2> /dev/null \
> 
> With the kernel patch:
> 
> https://lore.kernel.org/lkml/20220711180706.3418612-1-kan.liang@linux.intel.com/
> 
> I get:
> 
> Samples for 'dummy:HG' event do not have IREGS attribute set. Cannot print 'iregs' field.

We seem to need:


diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 13580a9c50b8d..959291903936a 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -411,7 +411,7 @@ static int evsel__do_check_stype(struct evsel *evsel, u64 sample_type, const cha
       int type = output_type(attr->type);
       const char *evname;
 
-       if (attr->sample_type & sample_type)
+       if (evsel__is_dummy_event(evsel) || attr->sample_type & sample_type)
               return 0;
 
       if (output[type].user_set_fields & field) {
diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh





> 
>>      | egrep -q "DI:"
>>    then
>> @@ -73,6 +121,8 @@ test_register_capture() {
>>    echo "Register capture test [Success]"
>>  }
>>  
>> +build_test_program
>> +
>>  test_per_thread
>>  test_register_capture
>>  
> 

Re: [PATCH 3/6] perf test: Test record with --threads option

[Adrian Hunter]

On 7/09/22 09:46, Namhyung Kim wrote:
> To verify per-thread mode is working with multi-thread recording.
> Use two software events for testing to check event set-output handling.
> Also update the cleanup routine because threads recording produces data
> in a directory.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/shell/record.sh | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index cea3c7b7e2cd..3331fb092654 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -11,8 +11,8 @@ testsym="test_loop"
>  testopt="-D 3"
>  
>  cleanup() {
> -  rm -f ${perfdata}
> -  rm -f ${perfdata}.old
> +  rm -rf ${perfdata}
> +  rm -rf ${perfdata}.old
>  
>    if [ "${testprog}" != "true" ]; then
>      rm -f ${testprog}
> @@ -90,6 +90,19 @@ test_per_thread() {
>      err=1
>      return
>    fi
> +  if ! perf record -e cpu-clock,cs --per-thread --threads=core ${testopt} \
> +    -o ${perfdata} ${testprog} 2> /dev/null

This does not seem to be possible, because it gives the error:

--per-thread option is mutually exclusive to parallel streaming mode.
Failed to initialize parallel data streaming masks 

> +  then
> +    echo "Per-thread record with --threads [Failed]"
> +    err=1
> +    return
> +  fi
> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
> +  then
> +    echo "Per-thread record with --threads [Failed missing output]"
> +    err=1
> +    return
> +  fi
>    echo "Basic --per-thread mode test [Success]"
>  }
>  

Re: [PATCH 4/6] perf test: Add system-wide mode in perf record tests

[Adrian Hunter]

On 7/09/22 09:46, Namhyung Kim wrote:
> Add system wide recording test with the same pattern.  It'd skip the
> test when it failes to run perf record.  For system-wide mode, it needs
> to avoid build-id collection and synthesis because the test only cares
> about the test program and kernel would generates necessary events as
> the process starts.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/tests/shell/record.sh | 34 ++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index 3331fb092654..bd4ef60948bd 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -134,10 +134,44 @@ test_register_capture() {
>    echo "Register capture test [Success]"
>  }
>  
> +test_system_wide() {
> +  echo "Basic --system-wide mode test"
> +  if ! perf record -aB --synth=no ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
> +  then
> +    echo "System-wide record [Skipped not supported]"
> +    if [ $err -ne 1 ]
> +    then
> +      err=2
> +    fi
> +    return
> +  fi
> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
> +  then
> +    echo "System-wide record [Failed missing output]"
> +    err=1
> +    return
> +  fi
> +  if ! perf record -aB --synth=no -e cpu-clock,cs --threads=cpu ${testopt} \
> +    -o ${perfdata} ${testprog} 2> /dev/null
> +  then
> +    echo "System-wide test [Failed recording with threads]"
> +    err=1
> +    return
> +  fi
> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
> +  then
> +    echo "System-wide record [Failed missing output]"
> +    err=1
> +    return
> +  fi
> +  echo "Basic --system-wide mode test [Success]"
> +}
> +
>  build_test_program
>  
>  test_per_thread
>  test_register_capture
> +test_system_wide
>  
>  cleanup
>  exit $err

Re: [PATCH 2/6] perf test: Use a test program in perf record tests

[Namhyung Kim]

Hi Adrian,

On Wed, Sep 7, 2022 at 3:45 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/09/22 09:46, Namhyung Kim wrote:
> > If the system has cc it could build a test program with two threads
> > and then use it for more detailed testing.  Also it adds initial delay
> > of 3ms to profile a multi-threaded target.  This change make the test
> > failing but that's what we want to check for now.
>
> So the delay is just to get a separate dummy event?

No, it's actually to wait for the new thread.

>
> I hit the issue from this patch:
>
> https://lore.kernel.org/lkml/20220711180706.3418612-1-kan.liang@linux.intel.com/
>
> That is, if I apply the patch above then the test passes, otherwise it fails always.

It's good to find/verify the bug with this :)

Thanks,
Namhyung

Re: [PATCH 2/6] perf test: Use a test program in perf record tests

[Namhyung Kim]

On Wed, Sep 7, 2022 at 6:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/09/22 16:16, Adrian Hunter wrote:
> > On 7/09/22 09:46, Namhyung Kim wrote:
> >>    if ! perf record -o - --intr-regs=di,r8,dx,cx -e cpu/br_inst_retired.near_call/p \
> >> -    -c 1000 --per-thread true 2> /dev/null \
> >> +    -c 1000 --per-thread ${testopt} ${testprog} 2> /dev/null \
> >>      | perf script -F ip,sym,iregs -i - 2> /dev/null \
> >
> > With the kernel patch:
> >
> > https://lore.kernel.org/lkml/20220711180706.3418612-1-kan.liang@linux.intel.com/
> >
> > I get:
> >
> > Samples for 'dummy:HG' event do not have IREGS attribute set. Cannot print 'iregs' field.
>
> We seem to need:

Yeah, I think we have a discussion already.

Thanks,
Namhyung


>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 13580a9c50b8d..959291903936a 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -411,7 +411,7 @@ static int evsel__do_check_stype(struct evsel *evsel, u64 sample_type, const cha
>        int type = output_type(attr->type);
>        const char *evname;
>
> -       if (attr->sample_type & sample_type)
> +       if (evsel__is_dummy_event(evsel) || attr->sample_type & sample_type)
>                return 0;
>
>        if (output[type].user_set_fields & field) {
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh

Re: [PATCH 3/6] perf test: Test record with --threads option

[Namhyung Kim]

On Wed, Sep 7, 2022 at 6:33 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/09/22 09:46, Namhyung Kim wrote:
> > @@ -90,6 +90,19 @@ test_per_thread() {
> >      err=1
> >      return
> >    fi
> > +  if ! perf record -e cpu-clock,cs --per-thread --threads=core ${testopt} \
> > +    -o ${perfdata} ${testprog} 2> /dev/null
>
> This does not seem to be possible, because it gives the error:
>
> --per-thread option is mutually exclusive to parallel streaming mode.
> Failed to initialize parallel data streaming masks

Oh, ok.  Let's drop this change then.

Thanks,
Namhyung

Re: [PATCH 5/6] perf test: Add target workload test in perf record tests

[Adrian Hunter]

On 7/09/22 09:46, Namhyung Kim wrote:
> Add a subtest which profiles the given workload on the command line.
> As it's a minimal requirement, test should run ok so it doesn't skip
> the test even if it failed to run the perf record.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/shell/record.sh | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index bd4ef60948bd..ff66e58f3a26 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -167,11 +167,42 @@ test_system_wide() {
>    echo "Basic --system-wide mode test [Success]"
>  }
>  
> +test_workload() {
> +  echo "Basic target workload test"
> +  if ! perf record ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
> +  then
> +    echo "Workload record [Failed record]"
> +    err=1
> +    return
> +  fi
> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
> +  then
> +    echo "Workload record [Failed missing output]"
> +    err=1
> +    return
> +  fi
> +  if ! perf record -e cpu-clock,cs --threads=package ${testopt} \
> +    -o ${perfdata} ${testprog} 2> /dev/null
> +  then
> +    echo "Workload record [Failed recording with threads]"
> +    err=1
> +    return
> +  fi
> +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
> +  then
> +    echo "Workload record [Failed missing output]"

When testing, this was happening always until changing the delay
from 3 to 1 ms.  It might be a bit racy to work consistently on
different machines.

Another approach is to wait for threads to appear in /proc/N/task
like wait_for_threads() here:

	https://lore.kernel.org/linux-perf-users/20220912083412.7058-12-adrian.hunter@intel.com/T/#u


> +    err=1
> +    return
> +  fi
> +  echo "Basic target workload test [Success]"
> +}
> +
>  build_test_program
>  
>  test_per_thread
>  test_register_capture
>  test_system_wide
> +test_workload
>  
>  cleanup
>  exit $err

Re: [PATCH 6/6] perf test: Do not set TEST_SKIP for record subtests

[Adrian Hunter]

On 7/09/22 09:46, Namhyung Kim wrote:
> It now has 4 sub tests and one of them should run at least.
> But once TEST_SKIP (= 2) return value is set, it won't be overwritten
> unless there's a failure.  I think we should return success when one
> or more tested are skipped but the remaining subtests are passed.
> 
> So update the test code not to set the err variable when it skips
> the test.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

I tend to agree that the perf's default event should work.

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  tools/perf/tests/shell/record.sh | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index ff66e58f3a26..487981767455 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -72,10 +72,6 @@ test_per_thread() {
>    if ! perf record -o /dev/null --quiet ${testprog} 2> /dev/null
>    then
>      echo "Per-thread record [Skipped event not supported]"
> -    if [ $err -ne 1 ]
> -    then
> -      err=2
> -    fi
>      return
>    fi
>    if ! perf record --per-thread ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
> @@ -111,10 +107,6 @@ test_register_capture() {
>    if ! perf list | egrep -q 'br_inst_retired.near_call'
>    then
>      echo "Register capture test [Skipped missing event]"
> -    if [ $err -ne 1 ]
> -    then
> -      err=2
> -    fi
>      return
>    fi
>    if ! perf record --intr-regs=\? 2>&1 | egrep -q 'available registers: AX BX CX DX SI DI BP SP IP FLAGS CS SS R8 R9 R10 R11 R12 R13 R14 R15'
> @@ -139,10 +131,6 @@ test_system_wide() {
>    if ! perf record -aB --synth=no ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
>    then
>      echo "System-wide record [Skipped not supported]"
> -    if [ $err -ne 1 ]
> -    then
> -      err=2
> -    fi
>      return
>    fi
>    if ! perf report -i ${perfdata} -q | egrep -q ${testsym}

Re: [PATCH 5/6] perf test: Add target workload test in perf record tests

[Namhyung Kim]

Hi Adrian,

On Tue, Sep 13, 2022 at 4:03 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/09/22 09:46, Namhyung Kim wrote:
> > Add a subtest which profiles the given workload on the command line.
> > As it's a minimal requirement, test should run ok so it doesn't skip
> > the test even if it failed to run the perf record.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/tests/shell/record.sh | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> > index bd4ef60948bd..ff66e58f3a26 100755
> > --- a/tools/perf/tests/shell/record.sh
> > +++ b/tools/perf/tests/shell/record.sh
> > @@ -167,11 +167,42 @@ test_system_wide() {
> >    echo "Basic --system-wide mode test [Success]"
> >  }
> >
> > +test_workload() {
> > +  echo "Basic target workload test"
> > +  if ! perf record ${testopt} -o ${perfdata} ${testprog} 2> /dev/null
> > +  then
> > +    echo "Workload record [Failed record]"
> > +    err=1
> > +    return
> > +  fi
> > +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
> > +  then
> > +    echo "Workload record [Failed missing output]"
> > +    err=1
> > +    return
> > +  fi
> > +  if ! perf record -e cpu-clock,cs --threads=package ${testopt} \
> > +    -o ${perfdata} ${testprog} 2> /dev/null
> > +  then
> > +    echo "Workload record [Failed recording with threads]"
> > +    err=1
> > +    return
> > +  fi
> > +  if ! perf report -i ${perfdata} -q | egrep -q ${testsym}
> > +  then
> > +    echo "Workload record [Failed missing output]"
>
> When testing, this was happening always until changing the delay
> from 3 to 1 ms.  It might be a bit racy to work consistently on
> different machines.
>
> Another approach is to wait for threads to appear in /proc/N/task
> like wait_for_threads() here:
>
>         https://lore.kernel.org/linux-perf-users/20220912083412.7058-12-adrian.hunter@intel.com/T/#u

Looks good.  Maybe we can move it to tests/shell/lib ?

Thanks,
Namhyung