linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters
@ 2021-06-17 18:42 Ian Rogers
  2021-06-17 18:42 ` [PATCH 2/4] perf test: Pass the verbose option to shell tests Ian Rogers
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Ian Rogers @ 2021-06-17 18:42 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, linux-perf-users, linux-kernel, bpf
  Cc: Ian Rogers

$(( .. )) is a bash feature but the test's interpreter is !/bin/sh,
switch the code to use expr.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/stat_bpf_counters.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/stat_bpf_counters.sh b/tools/perf/tests/shell/stat_bpf_counters.sh
index 22eb31e48ca7..2f9948b3d943 100755
--- a/tools/perf/tests/shell/stat_bpf_counters.sh
+++ b/tools/perf/tests/shell/stat_bpf_counters.sh
@@ -11,9 +11,9 @@ compare_number()
        second_num=$2
 
        # upper bound is first_num * 110%
-       upper=$(( $first_num + $first_num / 10 ))
+       upper=$(expr $first_num + $first_num / 10 )
        # lower bound is first_num * 90%
-       lower=$(( $first_num - $first_num / 10 ))
+       lower=$(expr $first_num - $first_num / 10 )
 
        if [ $second_num -gt $upper ] || [ $second_num -lt $lower ]; then
                echo "The difference between $first_num and $second_num are greater than 10%."
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] perf test: Pass the verbose option to shell tests
  2021-06-17 18:42 [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Ian Rogers
@ 2021-06-17 18:42 ` Ian Rogers
  2021-06-17 19:19   ` Arnaldo Carvalho de Melo
  2021-06-17 18:42 ` [PATCH 3/4] perf test: Add verbose skip output for bpf counters Ian Rogers
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Ian Rogers @ 2021-06-17 18:42 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, linux-perf-users, linux-kernel, bpf
  Cc: Ian Rogers

Having a verbose option will allow shell tests to provide extra failure
details when the fail or skip.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index cbbfe48ab802..a8160b1684de 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -577,11 +577,14 @@ struct shell_test {
 static int shell_test__run(struct test *test, int subdir __maybe_unused)
 {
 	int err;
-	char script[PATH_MAX];
+	char script[PATH_MAX + 3];
 	struct shell_test *st = test->priv;
 
 	path__join(script, sizeof(script), st->dir, st->file);
 
+	if (verbose)
+		strncat(script, " -v", sizeof(script));
+
 	err = system(script);
 	if (!err)
 		return TEST_OK;
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] perf test: Add verbose skip output for bpf counters
  2021-06-17 18:42 [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Ian Rogers
  2021-06-17 18:42 ` [PATCH 2/4] perf test: Pass the verbose option to shell tests Ian Rogers
@ 2021-06-17 18:42 ` Ian Rogers
  2021-06-17 19:20   ` Arnaldo Carvalho de Melo
  2021-06-17 18:42 ` [PATCH 4/4] perf test: Make stat bpf counters test more robust Ian Rogers
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Ian Rogers @ 2021-06-17 18:42 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, linux-perf-users, linux-kernel, bpf
  Cc: Ian Rogers

Provide additional context for when the stat bpf counters test skips.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/stat_bpf_counters.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/stat_bpf_counters.sh b/tools/perf/tests/shell/stat_bpf_counters.sh
index 2f9948b3d943..81d61b6e1208 100755
--- a/tools/perf/tests/shell/stat_bpf_counters.sh
+++ b/tools/perf/tests/shell/stat_bpf_counters.sh
@@ -22,7 +22,13 @@ compare_number()
 }
 
 # skip if --bpf-counters is not supported
-perf stat --bpf-counters true > /dev/null 2>&1 || exit 2
+if ! perf stat --bpf-counters true > /dev/null 2>&1; then
+	if [ "$1" == "-v" ]; then
+		echo "Skipping: --bpf-counters not supported"
+		perf --no-pager stat --bpf-counters true || true
+	fi
+	exit 2
+fi
 
 base_cycles=$(perf stat --no-big-num -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
 bpf_cycles=$(perf stat --no-big-num --bpf-counters -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] perf test: Make stat bpf counters test more robust
  2021-06-17 18:42 [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Ian Rogers
  2021-06-17 18:42 ` [PATCH 2/4] perf test: Pass the verbose option to shell tests Ian Rogers
  2021-06-17 18:42 ` [PATCH 3/4] perf test: Add verbose skip output for bpf counters Ian Rogers
@ 2021-06-17 18:42 ` Ian Rogers
  2021-06-17 19:21   ` Arnaldo Carvalho de Melo
  2021-06-17 19:18 ` [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Arnaldo Carvalho de Melo
  2021-06-20 21:55 ` Stephen Rothwell
  4 siblings, 1 reply; 13+ messages in thread
From: Ian Rogers @ 2021-06-17 18:42 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, linux-perf-users, linux-kernel, bpf
  Cc: Ian Rogers

If the test is run on a hypervisor then the cycles event may not be
counted, skip the test in this situation. Fail the test if cycles are
not counted in the subsequent bpf counter run.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/stat_bpf_counters.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/tests/shell/stat_bpf_counters.sh b/tools/perf/tests/shell/stat_bpf_counters.sh
index 81d61b6e1208..2aed20dc2262 100755
--- a/tools/perf/tests/shell/stat_bpf_counters.sh
+++ b/tools/perf/tests/shell/stat_bpf_counters.sh
@@ -31,7 +31,15 @@ if ! perf stat --bpf-counters true > /dev/null 2>&1; then
 fi
 
 base_cycles=$(perf stat --no-big-num -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
+if [ "$base_cycles" == "<not" ]; then
+	echo "Skipping: cycles event not counted"
+	exit 2
+fi
 bpf_cycles=$(perf stat --no-big-num --bpf-counters -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
+if [ "$bpf_cycles" == "<not" ]; then
+	echo "Failed: cycles not counted with --bpf-counters"
+	exit 1
+fi
 
 compare_number $base_cycles $bpf_cycles
 exit 0
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters
  2021-06-17 18:42 [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Ian Rogers
                   ` (2 preceding siblings ...)
  2021-06-17 18:42 ` [PATCH 4/4] perf test: Make stat bpf counters test more robust Ian Rogers
@ 2021-06-17 19:18 ` Arnaldo Carvalho de Melo
  2021-06-20 21:55 ` Stephen Rothwell
  4 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-06-17 19:18 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, linux-perf-users,
	linux-kernel, bpf

Em Thu, Jun 17, 2021 at 11:42:13AM -0700, Ian Rogers escreveu:
> $(( .. )) is a bash feature but the test's interpreter is !/bin/sh,
> switch the code to use expr.


Thanks, applied to perf/urgent.

- Arnaldo

 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/shell/stat_bpf_counters.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/stat_bpf_counters.sh b/tools/perf/tests/shell/stat_bpf_counters.sh
> index 22eb31e48ca7..2f9948b3d943 100755
> --- a/tools/perf/tests/shell/stat_bpf_counters.sh
> +++ b/tools/perf/tests/shell/stat_bpf_counters.sh
> @@ -11,9 +11,9 @@ compare_number()
>         second_num=$2
>  
>         # upper bound is first_num * 110%
> -       upper=$(( $first_num + $first_num / 10 ))
> +       upper=$(expr $first_num + $first_num / 10 )
>         # lower bound is first_num * 90%
> -       lower=$(( $first_num - $first_num / 10 ))
> +       lower=$(expr $first_num - $first_num / 10 )
>  
>         if [ $second_num -gt $upper ] || [ $second_num -lt $lower ]; then
>                 echo "The difference between $first_num and $second_num are greater than 10%."
> -- 
> 2.32.0.288.g62a8d224e6-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] perf test: Pass the verbose option to shell tests
  2021-06-17 18:42 ` [PATCH 2/4] perf test: Pass the verbose option to shell tests Ian Rogers
@ 2021-06-17 19:19   ` Arnaldo Carvalho de Melo
  2021-06-18 16:49     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-06-17 19:19 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, linux-perf-users,
	linux-kernel, bpf

Em Thu, Jun 17, 2021 at 11:42:14AM -0700, Ian Rogers escreveu:
> Having a verbose option will allow shell tests to provide extra failure
> details when the fail or skip.
 

Thanks, applied to perf/core.

- Arnaldo

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/builtin-test.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index cbbfe48ab802..a8160b1684de 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -577,11 +577,14 @@ struct shell_test {
>  static int shell_test__run(struct test *test, int subdir __maybe_unused)
>  {
>  	int err;
> -	char script[PATH_MAX];
> +	char script[PATH_MAX + 3];
>  	struct shell_test *st = test->priv;
>  
>  	path__join(script, sizeof(script), st->dir, st->file);
>  
> +	if (verbose)
> +		strncat(script, " -v", sizeof(script));
> +
>  	err = system(script);
>  	if (!err)
>  		return TEST_OK;
> -- 
> 2.32.0.288.g62a8d224e6-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] perf test: Add verbose skip output for bpf counters
  2021-06-17 18:42 ` [PATCH 3/4] perf test: Add verbose skip output for bpf counters Ian Rogers
@ 2021-06-17 19:20   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-06-17 19:20 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, linux-perf-users,
	linux-kernel, bpf

Em Thu, Jun 17, 2021 at 11:42:15AM -0700, Ian Rogers escreveu:
> Provide additional context for when the stat bpf counters test skips.

Thanks, applied.

- Arnaldo

 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/shell/stat_bpf_counters.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/shell/stat_bpf_counters.sh b/tools/perf/tests/shell/stat_bpf_counters.sh
> index 2f9948b3d943..81d61b6e1208 100755
> --- a/tools/perf/tests/shell/stat_bpf_counters.sh
> +++ b/tools/perf/tests/shell/stat_bpf_counters.sh
> @@ -22,7 +22,13 @@ compare_number()
>  }
>  
>  # skip if --bpf-counters is not supported
> -perf stat --bpf-counters true > /dev/null 2>&1 || exit 2
> +if ! perf stat --bpf-counters true > /dev/null 2>&1; then
> +	if [ "$1" == "-v" ]; then
> +		echo "Skipping: --bpf-counters not supported"
> +		perf --no-pager stat --bpf-counters true || true
> +	fi
> +	exit 2
> +fi
>  
>  base_cycles=$(perf stat --no-big-num -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
>  bpf_cycles=$(perf stat --no-big-num --bpf-counters -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
> -- 
> 2.32.0.288.g62a8d224e6-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] perf test: Make stat bpf counters test more robust
  2021-06-17 18:42 ` [PATCH 4/4] perf test: Make stat bpf counters test more robust Ian Rogers
@ 2021-06-17 19:21   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-06-17 19:21 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, linux-perf-users,
	linux-kernel, bpf

Em Thu, Jun 17, 2021 at 11:42:16AM -0700, Ian Rogers escreveu:
> If the test is run on a hypervisor then the cycles event may not be
> counted, skip the test in this situation. Fail the test if cycles are
> not counted in the subsequent bpf counter run.

Thanks, applied.

- Arnaldo

 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/shell/stat_bpf_counters.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/perf/tests/shell/stat_bpf_counters.sh b/tools/perf/tests/shell/stat_bpf_counters.sh
> index 81d61b6e1208..2aed20dc2262 100755
> --- a/tools/perf/tests/shell/stat_bpf_counters.sh
> +++ b/tools/perf/tests/shell/stat_bpf_counters.sh
> @@ -31,7 +31,15 @@ if ! perf stat --bpf-counters true > /dev/null 2>&1; then
>  fi
>  
>  base_cycles=$(perf stat --no-big-num -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
> +if [ "$base_cycles" == "<not" ]; then
> +	echo "Skipping: cycles event not counted"
> +	exit 2
> +fi
>  bpf_cycles=$(perf stat --no-big-num --bpf-counters -e cycles -- perf bench sched messaging -g 1 -l 100 -t 2>&1 | awk '/cycles/ {print $1}')
> +if [ "$bpf_cycles" == "<not" ]; then
> +	echo "Failed: cycles not counted with --bpf-counters"
> +	exit 1
> +fi
>  
>  compare_number $base_cycles $bpf_cycles
>  exit 0
> -- 
> 2.32.0.288.g62a8d224e6-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] perf test: Pass the verbose option to shell tests
  2021-06-17 19:19   ` Arnaldo Carvalho de Melo
@ 2021-06-18 16:49     ` Arnaldo Carvalho de Melo
  2021-06-19  4:45       ` Ian Rogers
  0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-06-18 16:49 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, linux-perf-users,
	linux-kernel, bpf

Em Thu, Jun 17, 2021 at 04:19:06PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jun 17, 2021 at 11:42:14AM -0700, Ian Rogers escreveu:
> > Having a verbose option will allow shell tests to provide extra failure
> > details when the fail or skip.
>  
> 
> Thanks, applied to perf/core.
> 
> - Arnaldo
> 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/tests/builtin-test.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index cbbfe48ab802..a8160b1684de 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -577,11 +577,14 @@ struct shell_test {
> >  static int shell_test__run(struct test *test, int subdir __maybe_unused)
> >  {
> >  	int err;
> > -	char script[PATH_MAX];
> > +	char script[PATH_MAX + 3];
> >  	struct shell_test *st = test->priv;
> >  
> >  	path__join(script, sizeof(script), st->dir, st->file);

probably you need to add a  '- 3' after the sizeof above, right?

> >  
> > +	if (verbose)
> > +		strncat(script, " -v", sizeof(script));
> > +

Seemed simple enough, but gcc knows better, I'm removing this one:

    tests/builtin-test.c:586:26: error: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Werror,-Wstrncat-size]
                    strncat(script, " -v", sizeof(script));
                                           ^~~~~~~~~~~~~~
    tests/builtin-test.c:586:26: note: change the argument to be the free space in the destination buffer minus the terminating null byte
                    strncat(script, " -v", sizeof(script));
                                           ^~~~~~~~~~~~~~
                                           sizeof(script) - strlen(script) - 1
    1 error generated.
    make[3]: *** [/git/perf-5.13.0-rc4/tools/build/Makefile.build:139: tests] Error 2
  77    31.98 ubuntu:21.04                  : FAIL gcc version 10.3.0 (Ubuntu 10.3.0-1ubuntu1)
    tests/builtin-test.c:586:26: error: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Werror,-Wstrncat-size]
                    strncat(script, " -v", sizeof(script));
                                           ^~~~~~~~~~~~~~
    tests/builtin-test.c:586:26: note: change the argument to be the free space in the destination buffer minus the terminating null byte
                    strncat(script, " -v", sizeof(script));
                                           ^~~~~~~~~~~~~~
                                           sizeof(script) - strlen(script) - 1
    1 error generated.
    make[3]: *** [/git/perf-5.13.0-rc4/tools/build/Makefile.build:139: tests] Error 2


> >  	err = system(script);
> >  	if (!err)
> >  		return TEST_OK;
> > -- 
> > 2.32.0.288.g62a8d224e6-goog
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] perf test: Pass the verbose option to shell tests
  2021-06-18 16:49     ` Arnaldo Carvalho de Melo
@ 2021-06-19  4:45       ` Ian Rogers
  2021-06-19 13:03         ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 13+ messages in thread
From: Ian Rogers @ 2021-06-19  4:45 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, linux-perf-users,
	linux-kernel, bpf

On Fri, Jun 18, 2021 at 9:49 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Jun 17, 2021 at 04:19:06PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jun 17, 2021 at 11:42:14AM -0700, Ian Rogers escreveu:
> > > Having a verbose option will allow shell tests to provide extra failure
> > > details when the fail or skip.
> >
> >
> > Thanks, applied to perf/core.
> >
> > - Arnaldo
> >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/tests/builtin-test.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > > index cbbfe48ab802..a8160b1684de 100644
> > > --- a/tools/perf/tests/builtin-test.c
> > > +++ b/tools/perf/tests/builtin-test.c
> > > @@ -577,11 +577,14 @@ struct shell_test {
> > >  static int shell_test__run(struct test *test, int subdir __maybe_unused)
> > >  {
> > >     int err;
> > > -   char script[PATH_MAX];
> > > +   char script[PATH_MAX + 3];
> > >     struct shell_test *st = test->priv;
> > >
> > >     path__join(script, sizeof(script), st->dir, st->file);
>
> probably you need to add a  '- 3' after the sizeof above, right?

Either way is fine, but -3 is ok with me.

> > >
> > > +   if (verbose)
> > > +           strncat(script, " -v", sizeof(script));
> > > +
>
> Seemed simple enough, but gcc knows better, I'm removing this one:
>
>     tests/builtin-test.c:586:26: error: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Werror,-Wstrncat-size]
>                     strncat(script, " -v", sizeof(script));
>                                            ^~~~~~~~~~~~~~
>     tests/builtin-test.c:586:26: note: change the argument to be the free space in the destination buffer minus the terminating null byte
>                     strncat(script, " -v", sizeof(script));
>                                            ^~~~~~~~~~~~~~
>                                            sizeof(script) - strlen(script) - 1
>     1 error generated.
>     make[3]: *** [/git/perf-5.13.0-rc4/tools/build/Makefile.build:139: tests] Error 2
>   77    31.98 ubuntu:21.04                  : FAIL gcc version 10.3.0 (Ubuntu 10.3.0-1ubuntu1)
>     tests/builtin-test.c:586:26: error: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Werror,-Wstrncat-size]
>                     strncat(script, " -v", sizeof(script));
>                                            ^~~~~~~~~~~~~~
>     tests/builtin-test.c:586:26: note: change the argument to be the free space in the destination buffer minus the terminating null byte
>                     strncat(script, " -v", sizeof(script));
>                                            ^~~~~~~~~~~~~~
>                                            sizeof(script) - strlen(script) - 1
>     1 error generated.
>     make[3]: *** [/git/perf-5.13.0-rc4/tools/build/Makefile.build:139: tests] Error 2

Thanks gcc :-) Do you want me to resend the patch?

Ian

> > >     err = system(script);
> > >     if (!err)
> > >             return TEST_OK;
> > > --
> > > 2.32.0.288.g62a8d224e6-goog
> > >
> >
> > --
> >
> > - Arnaldo
>
> --
>
> - Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] perf test: Pass the verbose option to shell tests
  2021-06-19  4:45       ` Ian Rogers
@ 2021-06-19 13:03         ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-06-19 13:03 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, linux-perf-users,
	linux-kernel, bpf

Em Fri, Jun 18, 2021 at 09:45:05PM -0700, Ian Rogers escreveu:
> On Fri, Jun 18, 2021 at 9:49 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Thu, Jun 17, 2021 at 04:19:06PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Thu, Jun 17, 2021 at 11:42:14AM -0700, Ian Rogers escreveu:
> > > > Having a verbose option will allow shell tests to provide extra failure
> > > > details when the fail or skip.
> > >
> > >
> > > Thanks, applied to perf/core.
> > >
> > > - Arnaldo
> > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  tools/perf/tests/builtin-test.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > > > index cbbfe48ab802..a8160b1684de 100644
> > > > --- a/tools/perf/tests/builtin-test.c
> > > > +++ b/tools/perf/tests/builtin-test.c
> > > > @@ -577,11 +577,14 @@ struct shell_test {
> > > >  static int shell_test__run(struct test *test, int subdir __maybe_unused)
> > > >  {
> > > >     int err;
> > > > -   char script[PATH_MAX];
> > > > +   char script[PATH_MAX + 3];
> > > >     struct shell_test *st = test->priv;
> > > >
> > > >     path__join(script, sizeof(script), st->dir, st->file);
> >
> > probably you need to add a  '- 3' after the sizeof above, right?
> 
> Either way is fine, but -3 is ok with me.
> 
> > > >
> > > > +   if (verbose)
> > > > +           strncat(script, " -v", sizeof(script));
> > > > +
> >
> > Seemed simple enough, but gcc knows better, I'm removing this one:
> >
> >     tests/builtin-test.c:586:26: error: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Werror,-Wstrncat-size]
> >                     strncat(script, " -v", sizeof(script));
> >                                            ^~~~~~~~~~~~~~
> >     tests/builtin-test.c:586:26: note: change the argument to be the free space in the destination buffer minus the terminating null byte
> >                     strncat(script, " -v", sizeof(script));
> >                                            ^~~~~~~~~~~~~~
> >                                            sizeof(script) - strlen(script) - 1
> >     1 error generated.
> >     make[3]: *** [/git/perf-5.13.0-rc4/tools/build/Makefile.build:139: tests] Error 2
> >   77    31.98 ubuntu:21.04                  : FAIL gcc version 10.3.0 (Ubuntu 10.3.0-1ubuntu1)
> >     tests/builtin-test.c:586:26: error: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Werror,-Wstrncat-size]
> >                     strncat(script, " -v", sizeof(script));
> >                                            ^~~~~~~~~~~~~~
> >     tests/builtin-test.c:586:26: note: change the argument to be the free space in the destination buffer minus the terminating null byte
> >                     strncat(script, " -v", sizeof(script));
> >                                            ^~~~~~~~~~~~~~
> >                                            sizeof(script) - strlen(script) - 1
> >     1 error generated.
> >     make[3]: *** [/git/perf-5.13.0-rc4/tools/build/Makefile.build:139: tests] Error 2
> 
> Thanks gcc :-) Do you want me to resend the patch?

In such cases please do,

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters
  2021-06-17 18:42 [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Ian Rogers
                   ` (3 preceding siblings ...)
  2021-06-17 19:18 ` [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Arnaldo Carvalho de Melo
@ 2021-06-20 21:55 ` Stephen Rothwell
  2021-06-21 21:30   ` Ian Rogers
  4 siblings, 1 reply; 13+ messages in thread
From: Stephen Rothwell @ 2021-06-20 21:55 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, linux-perf-users, linux-kernel, bpf

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

Hi Ian,

On Thu, 17 Jun 2021 11:42:13 -0700 Ian Rogers <irogers@google.com> wrote:
>
> $(( .. )) is a bash feature but the test's interpreter is !/bin/sh,
> switch the code to use expr.

The $(( .. )) syntax is specified in POSIX (see
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_04),
so unless this caused an actual problem, this change is unnecessary.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters
  2021-06-20 21:55 ` Stephen Rothwell
@ 2021-06-21 21:30   ` Ian Rogers
  0 siblings, 0 replies; 13+ messages in thread
From: Ian Rogers @ 2021-06-21 21:30 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, linux-perf-users, linux-kernel, bpf

On Sun, Jun 20, 2021 at 2:55 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Ian,
>
> On Thu, 17 Jun 2021 11:42:13 -0700 Ian Rogers <irogers@google.com> wrote:
> >
> > $(( .. )) is a bash feature but the test's interpreter is !/bin/sh,
> > switch the code to use expr.
>
> The $(( .. )) syntax is specified in POSIX (see
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_04),
> so unless this caused an actual problem, this change is unnecessary.

Agreed. The issue I was seeing was:

./tests/shell/stat_bpf_counters.sh: line 14: <not + <not / 10 : syntax
error: operand expected (error token is "<not + <not / 10 ")

but that syntax error is caused by running the test within a
hypervisor. I'll resend the patch set with this one dropped.

Thanks,
Ian

> --
> Cheers,
> Stephen Rothwell

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-21 21:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 18:42 [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Ian Rogers
2021-06-17 18:42 ` [PATCH 2/4] perf test: Pass the verbose option to shell tests Ian Rogers
2021-06-17 19:19   ` Arnaldo Carvalho de Melo
2021-06-18 16:49     ` Arnaldo Carvalho de Melo
2021-06-19  4:45       ` Ian Rogers
2021-06-19 13:03         ` Arnaldo Carvalho de Melo
2021-06-17 18:42 ` [PATCH 3/4] perf test: Add verbose skip output for bpf counters Ian Rogers
2021-06-17 19:20   ` Arnaldo Carvalho de Melo
2021-06-17 18:42 ` [PATCH 4/4] perf test: Make stat bpf counters test more robust Ian Rogers
2021-06-17 19:21   ` Arnaldo Carvalho de Melo
2021-06-17 19:18 ` [PATCH 1/4] perf test: Fix non-bash issue with stat bpf counters Arnaldo Carvalho de Melo
2021-06-20 21:55 ` Stephen Rothwell
2021-06-21 21:30   ` Ian Rogers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).