diff mbox series

[bpf-next,4/6] selftest/bpf/benchs: make quiet option common

Message ID 20230127181457.21389-5-aspsk@isovalent.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series New benchmark for hashmap lookups | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 11 maintainers not CCed: sdf@google.com kpsingh@kernel.org jolsa@kernel.org mykolal@fb.com davemarchevsky@fb.com song@kernel.org linux-kselftest@vger.kernel.org shuah@kernel.org haoluo@google.com yhs@fb.com paulmck@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 87 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-2 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain }}
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 fail Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-8 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-9 success Logs for set-matrix

Commit Message

Anton Protopopov Jan. 27, 2023, 6:14 p.m. UTC 
The "local-storage-tasks-trace" benchmark has a `--quiet` option. Move it to
the list of common options, so that the main code and other benchmarks can use
(now global) env.quiet as well.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 tools/testing/selftests/bpf/bench.c               | 15 +++++++++++++++
 tools/testing/selftests/bpf/bench.h               |  1 +
 .../benchs/bench_local_storage_rcu_tasks_trace.c  | 14 +-------------
 3 files changed, 17 insertions(+), 13 deletions(-)

Comments

Andrii Nakryiko Jan. 31, 2023, 12:10 a.m. UTC | #1 
On Fri, Jan 27, 2023 at 10:14 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> The "local-storage-tasks-trace" benchmark has a `--quiet` option. Move it to
> the list of common options, so that the main code and other benchmarks can use
> (now global) env.quiet as well.
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
>  tools/testing/selftests/bpf/bench.c               | 15 +++++++++++++++
>  tools/testing/selftests/bpf/bench.h               |  1 +
>  .../benchs/bench_local_storage_rcu_tasks_trace.c  | 14 +-------------
>  3 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> index ba93f1b268e1..42bf41a9385e 100644
> --- a/tools/testing/selftests/bpf/bench.c
> +++ b/tools/testing/selftests/bpf/bench.c
> @@ -16,6 +16,7 @@ struct env env = {
>         .warmup_sec = 1,
>         .duration_sec = 5,
>         .affinity = false,
> +       .quiet = false,
>         .consumer_cnt = 1,
>         .producer_cnt = 1,
>  };
> @@ -257,6 +258,7 @@ static const struct argp_option opts[] = {
>         { "consumers", 'c', "NUM", 0, "Number of consumer threads"},
>         { "verbose", 'v', NULL, 0, "Verbose debug output"},
>         { "affinity", 'a', NULL, 0, "Set consumer/producer thread affinity"},
> +       { "quiet", 'q', "{0,1}", OPTION_ARG_OPTIONAL, "If true, be quiet"},

given the default is not quiet, why add 0 or 1? -q for quiet, no "-q"
for not quiet? Keeping it simple?

>         { "prod-affinity", ARG_PROD_AFFINITY_SET, "CPUSET", 0,
>           "Set of CPUs for producer threads; implies --affinity"},
>         { "cons-affinity", ARG_CONS_AFFINITY_SET, "CPUSET", 0,

[...]
Anton Protopopov Jan. 31, 2023, 10:57 a.m. UTC | #2 
On 23/01/30 04:10, Andrii Nakryiko wrote:
> On Fri, Jan 27, 2023 at 10:14 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > The "local-storage-tasks-trace" benchmark has a `--quiet` option. Move it to
> > the list of common options, so that the main code and other benchmarks can use
> > (now global) env.quiet as well.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> >  tools/testing/selftests/bpf/bench.c               | 15 +++++++++++++++
> >  tools/testing/selftests/bpf/bench.h               |  1 +
> >  .../benchs/bench_local_storage_rcu_tasks_trace.c  | 14 +-------------
> >  3 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> > index ba93f1b268e1..42bf41a9385e 100644
> > --- a/tools/testing/selftests/bpf/bench.c
> > +++ b/tools/testing/selftests/bpf/bench.c
> > @@ -16,6 +16,7 @@ struct env env = {
> >         .warmup_sec = 1,
> >         .duration_sec = 5,
> >         .affinity = false,
> > +       .quiet = false,
> >         .consumer_cnt = 1,
> >         .producer_cnt = 1,
> >  };
> > @@ -257,6 +258,7 @@ static const struct argp_option opts[] = {
> >         { "consumers", 'c', "NUM", 0, "Number of consumer threads"},
> >         { "verbose", 'v', NULL, 0, "Verbose debug output"},
> >         { "affinity", 'a', NULL, 0, "Set consumer/producer thread affinity"},
> > +       { "quiet", 'q', "{0,1}", OPTION_ARG_OPTIONAL, "If true, be quiet"},
> 
> given the default is not quiet, why add 0 or 1? -q for quiet, no "-q"
> for not quiet? Keeping it simple?

The local-storage-tasks-trace benchmark expected 0 or 1 there, so I didn't want
to break any script which utilize this option.

The new parser accepts the old --quiet=0|1 for consistency, but also -q|--quiet
without value, as you've suggested (I pass OPTION_ARG_OPTIONAL and set
quiet=true if arg is NULL in the new parser).

> >         { "prod-affinity", ARG_PROD_AFFINITY_SET, "CPUSET", 0,
> >           "Set of CPUs for producer threads; implies --affinity"},
> >         { "cons-affinity", ARG_CONS_AFFINITY_SET, "CPUSET", 0,
> 
> [...]
Andrii Nakryiko Jan. 31, 2023, 6:51 p.m. UTC | #3 
On Tue, Jan 31, 2023 at 2:57 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On 23/01/30 04:10, Andrii Nakryiko wrote:
> > On Fri, Jan 27, 2023 at 10:14 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > The "local-storage-tasks-trace" benchmark has a `--quiet` option. Move it to
> > > the list of common options, so that the main code and other benchmarks can use
> > > (now global) env.quiet as well.
> > >
> > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > ---
> > >  tools/testing/selftests/bpf/bench.c               | 15 +++++++++++++++
> > >  tools/testing/selftests/bpf/bench.h               |  1 +
> > >  .../benchs/bench_local_storage_rcu_tasks_trace.c  | 14 +-------------
> > >  3 files changed, 17 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> > > index ba93f1b268e1..42bf41a9385e 100644
> > > --- a/tools/testing/selftests/bpf/bench.c
> > > +++ b/tools/testing/selftests/bpf/bench.c
> > > @@ -16,6 +16,7 @@ struct env env = {
> > >         .warmup_sec = 1,
> > >         .duration_sec = 5,
> > >         .affinity = false,
> > > +       .quiet = false,
> > >         .consumer_cnt = 1,
> > >         .producer_cnt = 1,
> > >  };
> > > @@ -257,6 +258,7 @@ static const struct argp_option opts[] = {
> > >         { "consumers", 'c', "NUM", 0, "Number of consumer threads"},
> > >         { "verbose", 'v', NULL, 0, "Verbose debug output"},
> > >         { "affinity", 'a', NULL, 0, "Set consumer/producer thread affinity"},
> > > +       { "quiet", 'q', "{0,1}", OPTION_ARG_OPTIONAL, "If true, be quiet"},
> >
> > given the default is not quiet, why add 0 or 1? -q for quiet, no "-q"
> > for not quiet? Keeping it simple?
>
> The local-storage-tasks-trace benchmark expected 0 or 1 there, so I didn't want
> to break any script which utilize this option.
>
> The new parser accepts the old --quiet=0|1 for consistency, but also -q|--quiet
> without value, as you've suggested (I pass OPTION_ARG_OPTIONAL and set
> quiet=true if arg is NULL in the new parser).
>

I think it was mostly due to copy/pasting some other integer-based
argument handling. We don't need that for boolean flags. Let's just
fix benchs/run_bench_local_storage_rcu_tasks_trace.sh to do -q and
keep it simple?

> > >         { "prod-affinity", ARG_PROD_AFFINITY_SET, "CPUSET", 0,
> > >           "Set of CPUs for producer threads; implies --affinity"},
> > >         { "cons-affinity", ARG_CONS_AFFINITY_SET, "CPUSET", 0,
> >
> > [...]
Anton Protopopov Jan. 31, 2023, 6:57 p.m. UTC | #4 
On 23/01/31 11:57, Anton Protopopov wrote:
> On 23/01/30 04:10, Andrii Nakryiko wrote:
> > On Fri, Jan 27, 2023 at 10:14 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > The "local-storage-tasks-trace" benchmark has a `--quiet` option. Move it to
> > > the list of common options, so that the main code and other benchmarks can use
> > > (now global) env.quiet as well.
> > >
> > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > ---
> > >  tools/testing/selftests/bpf/bench.c               | 15 +++++++++++++++
> > >  tools/testing/selftests/bpf/bench.h               |  1 +
> > >  .../benchs/bench_local_storage_rcu_tasks_trace.c  | 14 +-------------
> > >  3 files changed, 17 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
> > > index ba93f1b268e1..42bf41a9385e 100644
> > > --- a/tools/testing/selftests/bpf/bench.c
> > > +++ b/tools/testing/selftests/bpf/bench.c
> > > @@ -16,6 +16,7 @@ struct env env = {
> > >         .warmup_sec = 1,
> > >         .duration_sec = 5,
> > >         .affinity = false,
> > > +       .quiet = false,
> > >         .consumer_cnt = 1,
> > >         .producer_cnt = 1,
> > >  };
> > > @@ -257,6 +258,7 @@ static const struct argp_option opts[] = {
> > >         { "consumers", 'c', "NUM", 0, "Number of consumer threads"},
> > >         { "verbose", 'v', NULL, 0, "Verbose debug output"},
> > >         { "affinity", 'a', NULL, 0, "Set consumer/producer thread affinity"},
> > > +       { "quiet", 'q', "{0,1}", OPTION_ARG_OPTIONAL, "If true, be quiet"},
> > 
> > given the default is not quiet, why add 0 or 1? -q for quiet, no "-q"
> > for not quiet? Keeping it simple?
> 
> The local-storage-tasks-trace benchmark expected 0 or 1 there, so I didn't want
> to break any script which utilize this option.
> 
> The new parser accepts the old --quiet=0|1 for consistency, but also -q|--quiet
> without value, as you've suggested (I pass OPTION_ARG_OPTIONAL and set
> quiet=true if arg is NULL in the new parser).

Sure will do, for me --quiet=1 looks weird as well.

> > >         { "prod-affinity", ARG_PROD_AFFINITY_SET, "CPUSET", 0,
> > >           "Set of CPUs for producer threads; implies --affinity"},
> > >         { "cons-affinity", ARG_CONS_AFFINITY_SET, "CPUSET", 0,
> > 
> > [...]
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c
index ba93f1b268e1..42bf41a9385e 100644
--- a/tools/testing/selftests/bpf/bench.c
+++ b/tools/testing/selftests/bpf/bench.c
@@ -16,6 +16,7 @@  struct env env = {
 	.warmup_sec = 1,
 	.duration_sec = 5,
 	.affinity = false,
+	.quiet = false,
 	.consumer_cnt = 1,
 	.producer_cnt = 1,
 };
@@ -257,6 +258,7 @@  static const struct argp_option opts[] = {
 	{ "consumers", 'c', "NUM", 0, "Number of consumer threads"},
 	{ "verbose", 'v', NULL, 0, "Verbose debug output"},
 	{ "affinity", 'a', NULL, 0, "Set consumer/producer thread affinity"},
+	{ "quiet", 'q', "{0,1}", OPTION_ARG_OPTIONAL, "If true, be quiet"},
 	{ "prod-affinity", ARG_PROD_AFFINITY_SET, "CPUSET", 0,
 	  "Set of CPUs for producer threads; implies --affinity"},
 	{ "cons-affinity", ARG_CONS_AFFINITY_SET, "CPUSET", 0,
@@ -286,6 +288,7 @@  static int pos_args;
 
 static error_t parse_arg(int key, char *arg, struct argp_state *state)
 {
+	long ret;
 
 	switch (key) {
 	case 'v':
@@ -325,6 +328,18 @@  static error_t parse_arg(int key, char *arg, struct argp_state *state)
 	case 'a':
 		env.affinity = true;
 		break;
+	case 'q':
+		if (!arg) {
+			env.quiet = true;
+		} else {
+			ret = strtol(arg, NULL, 10);
+			if (ret < 0 || ret > 1) {
+				fprintf(stderr, "invalid quiet %ld\n", ret);
+				argp_usage(state);
+			}
+			env.quiet = ret;
+		}
+		break;
 	case ARG_PROD_AFFINITY_SET:
 		env.affinity = true;
 		if (parse_num_list(arg, &env.prod_cpus.cpus,
diff --git a/tools/testing/selftests/bpf/bench.h b/tools/testing/selftests/bpf/bench.h
index 316ba0589bf7..e322654b5e8a 100644
--- a/tools/testing/selftests/bpf/bench.h
+++ b/tools/testing/selftests/bpf/bench.h
@@ -24,6 +24,7 @@  struct env {
 	bool verbose;
 	bool list;
 	bool affinity;
+	bool quiet;
 	int consumer_cnt;
 	int producer_cnt;
 	struct cpu_set prod_cpus;
diff --git a/tools/testing/selftests/bpf/benchs/bench_local_storage_rcu_tasks_trace.c b/tools/testing/selftests/bpf/benchs/bench_local_storage_rcu_tasks_trace.c
index bcbcb8b90c61..51d4b1a690f9 100644
--- a/tools/testing/selftests/bpf/benchs/bench_local_storage_rcu_tasks_trace.c
+++ b/tools/testing/selftests/bpf/benchs/bench_local_storage_rcu_tasks_trace.c
@@ -12,11 +12,9 @@ 
 static struct {
 	__u32 nr_procs;
 	__u32 kthread_pid;
-	bool quiet;
 } args = {
 	.nr_procs = 1000,
 	.kthread_pid = 0,
-	.quiet = false,
 };
 
 static const struct argp_option opts[] = {
@@ -24,8 +22,6 @@  static const struct argp_option opts[] = {
 		"Set number of user processes to spin up"},
 	{ "kthread_pid", ARG_KTHREAD_PID, "PID", 0,
 		"Pid of rcu_tasks_trace kthread for ticks tracking"},
-	{ "quiet", ARG_QUIET, "{0,1}", 0,
-		"If true, don't report progress"},
 	{},
 };
 
@@ -50,14 +46,6 @@  static error_t parse_arg(int key, char *arg, struct argp_state *state)
 		}
 		args.kthread_pid = ret;
 		break;
-	case ARG_QUIET:
-		ret = strtol(arg, NULL, 10);
-		if (ret < 0 || ret > 1) {
-			fprintf(stderr, "invalid quiet %ld\n", ret);
-			argp_usage(state);
-		}
-		args.quiet = ret;
-		break;
 break;
 	default:
 		return ARGP_ERR_UNKNOWN;
@@ -224,7 +212,7 @@  static void report_progress(int iter, struct bench_res *res, long delta_ns)
 		exit(1);
 	}
 
-	if (args.quiet)
+	if (env.quiet)
 		return;
 
 	printf("Iter %d\t avg tasks_trace grace period latency\t%lf ns\n",