| Message ID | 20230127181457.21389-4-aspsk@isovalent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | New benchmark for hashmap lookups | expand |
On Fri, Jan 27, 2023 at 10:14 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > To parse command line the bench utility uses the argp_parse() function. This > function takes as an argument a parent 'struct argp' structure which defines > common command line options and an array of children 'struct argp' structures > which defines additional command line options for particular benchmarks. This > implementation doesn't allow benchmarks to share option names, e.g., if two > benchmarks want to use, say, the --option option, then only one of them will > succeed (the first one encountered in the array). This will be convenient if > we could use the same option names in different benchmarks (with the same > semantics, e.g., --nr_loops=N). > > Fix this by calling the argp_parse() function twice. The first call is needed > to find the benchmark name. Given the name, we can patch the list of argp > children to only include the correct list of option. This way the right parser > will be executed. (If there's no a specific list of arguments, then only one > call is enough.) As was mentioned above, arguments with same names should have > the same semantics (which means in this case "taking a parameter or not"), but > can have different description and will have different parsers. > > As we now can share option names, this also makes sense to share the option ids. > Previously every benchmark defined a separate enum, like > > enum { > ARG_SMTH = 9000, > ARG_SMTH_ELSE = 9001, > }; > > These ids were later used to distinguish between command line options: > > static const struct argp_option opts[] = { > { "smth", ARG_SMTH, "SMTH", 0, > "Set number of smth to configure smth"}, > { "smth_else", ARG_SMTH_ELSE, "SMTH_ELSE", 0, > "Set number of smth_else to configure smth else"}, > ... > > Move arguments id definitions to bench.h such that they are visible to every > benchmark (and such that there's no need to grep if this number is defined > already or not). On the other hand, if each benchmark defines their own set of IDs and parser, that keeps benchmarks more self-contained. Is there a real need to centralize everything? I don't see much benefit, tbh. If we want to centralize, then we can just bypass all the child parser machinery and have one centralized list of arguments. But I think it's good to have each benchmark self-contained and independent from other benchmarks. > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com> > --- > tools/testing/selftests/bpf/bench.c | 98 +++++++++++++++++-- > tools/testing/selftests/bpf/bench.h | 20 ++++ > .../bpf/benchs/bench_bloom_filter_map.c | 6 -- > .../selftests/bpf/benchs/bench_bpf_loop.c | 4 - > .../bpf/benchs/bench_local_storage.c | 5 - > .../bench_local_storage_rcu_tasks_trace.c | 6 -- > .../selftests/bpf/benchs/bench_ringbufs.c | 8 -- > .../selftests/bpf/benchs/bench_strncmp.c | 4 - > 8 files changed, 110 insertions(+), 41 deletions(-) > [...] > +struct argp *bench_name_to_argp(const char *bench_name) > +{ > + > +#define _SCMP(NAME) (!strcmp(bench_name, NAME)) > + > + if (_SCMP("bloom-lookup") || > + _SCMP("bloom-update") || > + _SCMP("bloom-false-positive") || > + _SCMP("hashmap-without-bloom") || > + _SCMP("hashmap-with-bloom")) > + return &bench_bloom_map_argp; > + > + if (_SCMP("rb-libbpf") || > + _SCMP("rb-custom") || > + _SCMP("pb-libbpf") || > + _SCMP("pb-custom")) > + return &bench_ringbufs_argp; > + > + if (_SCMP("local-storage-cache-seq-get") || > + _SCMP("local-storage-cache-int-get") || > + _SCMP("local-storage-cache-hashmap-control")) > + return &bench_local_storage_argp; > + > + if (_SCMP("local-storage-tasks-trace")) > + return &bench_local_storage_rcu_tasks_trace_argp; > + > + if (_SCMP("strncmp-no-helper") || > + _SCMP("strncmp-helper")) > + return &bench_strncmp_argp; > + > + if (_SCMP("bpf-loop")) > + return &bench_bpf_loop_argp; > + > + /* no extra arguments */ > + if (_SCMP("count-global") || > + _SCMP("count-local") || > + _SCMP("rename-base") || > + _SCMP("rename-kprobe") || > + _SCMP("rename-kretprobe") || > + _SCMP("rename-rawtp") || > + _SCMP("rename-fentry") || > + _SCMP("rename-fexit") || > + _SCMP("trig-base") || > + _SCMP("trig-tp") || > + _SCMP("trig-rawtp") || > + _SCMP("trig-kprobe") || > + _SCMP("trig-fentry") || > + _SCMP("trig-fentry-sleep") || > + _SCMP("trig-fmodret") || > + _SCMP("trig-uprobe-base") || > + _SCMP("trig-uprobe-with-nop") || > + _SCMP("trig-uretprobe-with-nop") || > + _SCMP("trig-uprobe-without-nop") || > + _SCMP("trig-uretprobe-without-nop") || > + _SCMP("bpf-hashmap-full-update")) > + return NULL; > + > +#undef _SCMP > + it's not good to have to maintain a separate list of benchmark names here. Let's maybe extend struct bench to specify extra parser and use that to figure out if we need to run nested child parser? > + fprintf(stderr, "%s: bench %s is unknown\n", __func__, bench_name); > + exit(1); > +} > + > static void parse_cmdline_args(int argc, char **argv) > { > static const struct argp argp = { > @@ -367,12 +426,35 @@ static void parse_cmdline_args(int argc, char **argv) > .doc = argp_program_doc, > .children = bench_parsers, > }; > + static struct argp *bench_argp; nit: do you need static? > + > + /* Parse args for the first time to get bench name */ > if (argp_parse(&argp, argc, argv, 0, NULL, NULL)) > exit(1); > - if (!env.list && !env.bench_name) { > + > + if (env.list) > + return; > + > + if (!env.bench_name) { > argp_help(&argp, stderr, ARGP_HELP_DOC, "bench"); > exit(1); > } > + > + /* Now check if there are custom options available. If not, then > + * everything is done, if yes, then we need to patch bench_parsers > + * so that bench_parsers[0] points to the right 'struct argp', and > + * bench_parsers[1] terminates the list. > + */ > + bench_argp = bench_name_to_argp(env.bench_name); > + if (bench_argp) { > + bench_parsers[0].argp = bench_argp; > + bench_parsers[0].header = env.bench_name; > + memset(&bench_parsers[1], 0, sizeof(bench_parsers[1])); > + > + pos_args = 0; > + if (argp_parse(&argp, argc, argv, 0, NULL, NULL)) > + exit(1); > + } this also feels like a big hack, why can't you just construct a single-item array based on child parser, instead of overwriting global array? > } > > static void collect_measurements(long delta_ns); [...]
On 23/01/30 04:07, Andrii Nakryiko wrote: > On Fri, Jan 27, 2023 at 10:14 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > To parse command line the bench utility uses the argp_parse() function. This > > function takes as an argument a parent 'struct argp' structure which defines > > common command line options and an array of children 'struct argp' structures > > which defines additional command line options for particular benchmarks. This > > implementation doesn't allow benchmarks to share option names, e.g., if two > > benchmarks want to use, say, the --option option, then only one of them will > > succeed (the first one encountered in the array). This will be convenient if > > we could use the same option names in different benchmarks (with the same > > semantics, e.g., --nr_loops=N). > > > > Fix this by calling the argp_parse() function twice. The first call is needed > > to find the benchmark name. Given the name, we can patch the list of argp > > children to only include the correct list of option. This way the right parser > > will be executed. (If there's no a specific list of arguments, then only one > > call is enough.) As was mentioned above, arguments with same names should have > > the same semantics (which means in this case "taking a parameter or not"), but > > can have different description and will have different parsers. > > > > As we now can share option names, this also makes sense to share the option ids. > > Previously every benchmark defined a separate enum, like > > > > enum { > > ARG_SMTH = 9000, > > ARG_SMTH_ELSE = 9001, > > }; > > > > These ids were later used to distinguish between command line options: > > > > static const struct argp_option opts[] = { > > { "smth", ARG_SMTH, "SMTH", 0, > > "Set number of smth to configure smth"}, > > { "smth_else", ARG_SMTH_ELSE, "SMTH_ELSE", 0, > > "Set number of smth_else to configure smth else"}, > > ... > > > > Move arguments id definitions to bench.h such that they are visible to every > > benchmark (and such that there's no need to grep if this number is defined > > already or not). > > On the other hand, if each benchmark defines their own set of IDs and > parser, that keeps benchmarks more self-contained. Is there a real > need to centralize everything? I don't see much benefit, tbh. > > If we want to centralize, then we can just bypass all the child parser > machinery and have one centralized list of arguments. But I think it's > good to have each benchmark self-contained and independent from other > benchmarks. When I was adding a new bench, it looked simpler to just add IDs to a global list than to grep for what ID is not defined yet. This doesn't matter much though, I can switch back to local IDs. > > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com> > > --- > > tools/testing/selftests/bpf/bench.c | 98 +++++++++++++++++-- > > tools/testing/selftests/bpf/bench.h | 20 ++++ > > .../bpf/benchs/bench_bloom_filter_map.c | 6 -- > > .../selftests/bpf/benchs/bench_bpf_loop.c | 4 - > > .../bpf/benchs/bench_local_storage.c | 5 - > > .../bench_local_storage_rcu_tasks_trace.c | 6 -- > > .../selftests/bpf/benchs/bench_ringbufs.c | 8 -- > > .../selftests/bpf/benchs/bench_strncmp.c | 4 - > > 8 files changed, 110 insertions(+), 41 deletions(-) > > > > [...] > > > +struct argp *bench_name_to_argp(const char *bench_name) > > +{ > > + > > +#define _SCMP(NAME) (!strcmp(bench_name, NAME)) > > + > > + if (_SCMP("bloom-lookup") || > > + _SCMP("bloom-update") || > > + _SCMP("bloom-false-positive") || > > + _SCMP("hashmap-without-bloom") || > > + _SCMP("hashmap-with-bloom")) > > + return &bench_bloom_map_argp; > > + > > + if (_SCMP("rb-libbpf") || > > + _SCMP("rb-custom") || > > + _SCMP("pb-libbpf") || > > + _SCMP("pb-custom")) > > + return &bench_ringbufs_argp; > > + > > + if (_SCMP("local-storage-cache-seq-get") || > > + _SCMP("local-storage-cache-int-get") || > > + _SCMP("local-storage-cache-hashmap-control")) > > + return &bench_local_storage_argp; > > + > > + if (_SCMP("local-storage-tasks-trace")) > > + return &bench_local_storage_rcu_tasks_trace_argp; > > + > > + if (_SCMP("strncmp-no-helper") || > > + _SCMP("strncmp-helper")) > > + return &bench_strncmp_argp; > > + > > + if (_SCMP("bpf-loop")) > > + return &bench_bpf_loop_argp; > > + > > + /* no extra arguments */ > > + if (_SCMP("count-global") || > > + _SCMP("count-local") || > > + _SCMP("rename-base") || > > + _SCMP("rename-kprobe") || > > + _SCMP("rename-kretprobe") || > > + _SCMP("rename-rawtp") || > > + _SCMP("rename-fentry") || > > + _SCMP("rename-fexit") || > > + _SCMP("trig-base") || > > + _SCMP("trig-tp") || > > + _SCMP("trig-rawtp") || > > + _SCMP("trig-kprobe") || > > + _SCMP("trig-fentry") || > > + _SCMP("trig-fentry-sleep") || > > + _SCMP("trig-fmodret") || > > + _SCMP("trig-uprobe-base") || > > + _SCMP("trig-uprobe-with-nop") || > > + _SCMP("trig-uretprobe-with-nop") || > > + _SCMP("trig-uprobe-without-nop") || > > + _SCMP("trig-uretprobe-without-nop") || > > + _SCMP("bpf-hashmap-full-update")) > > + return NULL; > > + > > +#undef _SCMP > > + > > it's not good to have to maintain a separate list of benchmark names > here. Let's maybe extend struct bench to specify extra parser and use > that to figure out if we need to run nested child parser? Yes, right, so then I can do something like struct argp *bench_name_to_argp(const char *bench_name) { int i; for (i = 0; i < ARRAY_SIZE(benchs); i++) if (!strcmp(bench_name, benchs[i]->name)) return benchs[i]->argp; fprintf(stderr, "benchmark '%s' not found\n", bench_name); exit(1); } > > > + fprintf(stderr, "%s: bench %s is unknown\n", __func__, bench_name); > > + exit(1); > > +} > > + > > static void parse_cmdline_args(int argc, char **argv) > > { > > static const struct argp argp = { > > @@ -367,12 +426,35 @@ static void parse_cmdline_args(int argc, char **argv) > > .doc = argp_program_doc, > > .children = bench_parsers, > > }; > > + static struct argp *bench_argp; > > nit: do you need static? No, thanks for noting. > > > + > > + /* Parse args for the first time to get bench name */ > > if (argp_parse(&argp, argc, argv, 0, NULL, NULL)) > > exit(1); > > - if (!env.list && !env.bench_name) { Also, I will switch to a simpler mode here: just find the bench_name, then construct correct `struct argp`, then call argp_parse() once. > > + > > + if (env.list) > > + return; > > + > > + if (!env.bench_name) { > > argp_help(&argp, stderr, ARGP_HELP_DOC, "bench"); > > exit(1); > > } > > + > > + /* Now check if there are custom options available. If not, then > > + * everything is done, if yes, then we need to patch bench_parsers > > + * so that bench_parsers[0] points to the right 'struct argp', and > > + * bench_parsers[1] terminates the list. > > + */ > > + bench_argp = bench_name_to_argp(env.bench_name); > > + if (bench_argp) { > > + bench_parsers[0].argp = bench_argp; > > + bench_parsers[0].header = env.bench_name; > > + memset(&bench_parsers[1], 0, sizeof(bench_parsers[1])); > > + > > + pos_args = 0; > > + if (argp_parse(&argp, argc, argv, 0, NULL, NULL)) > > + exit(1); > > + } > > this also feels like a big hack, why can't you just construct a > single-item array based on child parser, instead of overwriting global > array? Sure, thanks. > > } > > > > static void collect_measurements(long delta_ns); > > [...]
diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c index c1f20a147462..ba93f1b268e1 100644 --- a/tools/testing/selftests/bpf/bench.c +++ b/tools/testing/selftests/bpf/bench.c @@ -249,11 +249,6 @@ const char argp_program_doc[] = " # run 'count-local' with 16 producer and 8 consumer thread, pinned to CPUs\n" " benchmark -p16 -c8 -a count-local\n"; -enum { - ARG_PROD_AFFINITY_SET = 1000, - ARG_CONS_AFFINITY_SET = 1001, -}; - static const struct argp_option opts[] = { { "list", 'l', NULL, 0, "List available benchmarks"}, { "duration", 'd', "SEC", 0, "Duration of benchmark, seconds"}, @@ -276,7 +271,7 @@ extern struct argp bench_local_storage_argp; extern struct argp bench_local_storage_rcu_tasks_trace_argp; extern struct argp bench_strncmp_argp; -static const struct argp_child bench_parsers[] = { +static struct argp_child bench_parsers[] = { { &bench_ringbufs_argp, 0, "Ring buffers benchmark", 0 }, { &bench_bloom_map_argp, 0, "Bloom filter map benchmark", 0 }, { &bench_bpf_loop_argp, 0, "bpf_loop helper benchmark", 0 }, @@ -287,9 +282,10 @@ static const struct argp_child bench_parsers[] = { {}, }; +static int pos_args; + static error_t parse_arg(int key, char *arg, struct argp_state *state) { - static int pos_args; switch (key) { case 'v': @@ -359,6 +355,69 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state) return 0; } +struct argp *bench_name_to_argp(const char *bench_name) +{ + +#define _SCMP(NAME) (!strcmp(bench_name, NAME)) + + if (_SCMP("bloom-lookup") || + _SCMP("bloom-update") || + _SCMP("bloom-false-positive") || + _SCMP("hashmap-without-bloom") || + _SCMP("hashmap-with-bloom")) + return &bench_bloom_map_argp; + + if (_SCMP("rb-libbpf") || + _SCMP("rb-custom") || + _SCMP("pb-libbpf") || + _SCMP("pb-custom")) + return &bench_ringbufs_argp; + + if (_SCMP("local-storage-cache-seq-get") || + _SCMP("local-storage-cache-int-get") || + _SCMP("local-storage-cache-hashmap-control")) + return &bench_local_storage_argp; + + if (_SCMP("local-storage-tasks-trace")) + return &bench_local_storage_rcu_tasks_trace_argp; + + if (_SCMP("strncmp-no-helper") || + _SCMP("strncmp-helper")) + return &bench_strncmp_argp; + + if (_SCMP("bpf-loop")) + return &bench_bpf_loop_argp; + + /* no extra arguments */ + if (_SCMP("count-global") || + _SCMP("count-local") || + _SCMP("rename-base") || + _SCMP("rename-kprobe") || + _SCMP("rename-kretprobe") || + _SCMP("rename-rawtp") || + _SCMP("rename-fentry") || + _SCMP("rename-fexit") || + _SCMP("trig-base") || + _SCMP("trig-tp") || + _SCMP("trig-rawtp") || + _SCMP("trig-kprobe") || + _SCMP("trig-fentry") || + _SCMP("trig-fentry-sleep") || + _SCMP("trig-fmodret") || + _SCMP("trig-uprobe-base") || + _SCMP("trig-uprobe-with-nop") || + _SCMP("trig-uretprobe-with-nop") || + _SCMP("trig-uprobe-without-nop") || + _SCMP("trig-uretprobe-without-nop") || + _SCMP("bpf-hashmap-full-update")) + return NULL; + +#undef _SCMP + + fprintf(stderr, "%s: bench %s is unknown\n", __func__, bench_name); + exit(1); +} + static void parse_cmdline_args(int argc, char **argv) { static const struct argp argp = { @@ -367,12 +426,35 @@ static void parse_cmdline_args(int argc, char **argv) .doc = argp_program_doc, .children = bench_parsers, }; + static struct argp *bench_argp; + + /* Parse args for the first time to get bench name */ if (argp_parse(&argp, argc, argv, 0, NULL, NULL)) exit(1); - if (!env.list && !env.bench_name) { + + if (env.list) + return; + + if (!env.bench_name) { argp_help(&argp, stderr, ARGP_HELP_DOC, "bench"); exit(1); } + + /* Now check if there are custom options available. If not, then + * everything is done, if yes, then we need to patch bench_parsers + * so that bench_parsers[0] points to the right 'struct argp', and + * bench_parsers[1] terminates the list. + */ + bench_argp = bench_name_to_argp(env.bench_name); + if (bench_argp) { + bench_parsers[0].argp = bench_argp; + bench_parsers[0].header = env.bench_name; + memset(&bench_parsers[1], 0, sizeof(bench_parsers[1])); + + pos_args = 0; + if (argp_parse(&argp, argc, argv, 0, NULL, NULL)) + exit(1); + } } static void collect_measurements(long delta_ns); diff --git a/tools/testing/selftests/bpf/bench.h b/tools/testing/selftests/bpf/bench.h index d748255877e2..316ba0589bf7 100644 --- a/tools/testing/selftests/bpf/bench.h +++ b/tools/testing/selftests/bpf/bench.h @@ -101,3 +101,23 @@ static inline long atomic_swap(long *value, long n) { return __atomic_exchange_n(value, n, __ATOMIC_RELAXED); } + +enum { + ARG_PROD_AFFINITY_SET = 1000, + ARG_CONS_AFFINITY_SET, + ARG_RB_BACK2BACK, + ARG_RB_USE_OUTPUT, + ARG_RB_BATCH_CNT, + ARG_RB_SAMPLED, + ARG_RB_SAMPLE_RATE, + ARG_NR_ENTRIES, + ARG_NR_HASH_FUNCS, + ARG_VALUE_SIZE, + ARG_NR_LOOPS, + ARG_CMP_STR_LEN, + ARG_NR_MAPS, + ARG_HASHMAP_NR_KEYS_USED, + ARG_NR_PROCS, + ARG_KTHREAD_PID, + ARG_QUIET, +}; diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c index 5bcb8a8cdeb2..bef8999d4a9e 100644 --- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c +++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c @@ -45,12 +45,6 @@ static struct { .value_size = 8, }; -enum { - ARG_NR_ENTRIES = 3000, - ARG_NR_HASH_FUNCS = 3001, - ARG_VALUE_SIZE = 3002, -}; - static const struct argp_option opts[] = { { "nr_entries", ARG_NR_ENTRIES, "NR_ENTRIES", 0, "Set number of expected unique entries in the bloom filter"}, diff --git a/tools/testing/selftests/bpf/benchs/bench_bpf_loop.c b/tools/testing/selftests/bpf/benchs/bench_bpf_loop.c index d0a6572bfab6..47630a9a0a4b 100644 --- a/tools/testing/selftests/bpf/benchs/bench_bpf_loop.c +++ b/tools/testing/selftests/bpf/benchs/bench_bpf_loop.c @@ -16,10 +16,6 @@ static struct { .nr_loops = 10, }; -enum { - ARG_NR_LOOPS = 4000, -}; - static const struct argp_option opts[] = { { "nr_loops", ARG_NR_LOOPS, "nr_loops", 0, "Set number of loops for the bpf_loop helper"}, diff --git a/tools/testing/selftests/bpf/benchs/bench_local_storage.c b/tools/testing/selftests/bpf/benchs/bench_local_storage.c index 5a378c84e81f..dd19ddf24fde 100644 --- a/tools/testing/selftests/bpf/benchs/bench_local_storage.c +++ b/tools/testing/selftests/bpf/benchs/bench_local_storage.c @@ -17,11 +17,6 @@ static struct { .hashmap_nr_keys_used = 1000, }; -enum { - ARG_NR_MAPS = 6000, - ARG_HASHMAP_NR_KEYS_USED = 6001, -}; - static const struct argp_option opts[] = { { "nr_maps", ARG_NR_MAPS, "NR_MAPS", 0, "Set number of local_storage maps"}, 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 43f109d93130..bcbcb8b90c61 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 @@ -19,12 +19,6 @@ static struct { .quiet = false, }; -enum { - ARG_NR_PROCS = 7000, - ARG_KTHREAD_PID = 7001, - ARG_QUIET = 7002, -}; - static const struct argp_option opts[] = { { "nr_procs", ARG_NR_PROCS, "NR_PROCS", 0, "Set number of user processes to spin up"}, diff --git a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c index c2554f9695ff..b31ae31d1fea 100644 --- a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c +++ b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c @@ -29,14 +29,6 @@ static struct { .perfbuf_sz = 128, }; -enum { - ARG_RB_BACK2BACK = 2000, - ARG_RB_USE_OUTPUT = 2001, - ARG_RB_BATCH_CNT = 2002, - ARG_RB_SAMPLED = 2003, - ARG_RB_SAMPLE_RATE = 2004, -}; - static const struct argp_option opts[] = { { "rb-b2b", ARG_RB_BACK2BACK, NULL, 0, "Back-to-back mode"}, { "rb-use-output", ARG_RB_USE_OUTPUT, NULL, 0, "Use bpf_ringbuf_output() instead of bpf_ringbuf_reserve()"}, diff --git a/tools/testing/selftests/bpf/benchs/bench_strncmp.c b/tools/testing/selftests/bpf/benchs/bench_strncmp.c index 494b591c0289..e7d48a65511b 100644 --- a/tools/testing/selftests/bpf/benchs/bench_strncmp.c +++ b/tools/testing/selftests/bpf/benchs/bench_strncmp.c @@ -14,10 +14,6 @@ static struct strncmp_args { .cmp_str_len = 32, }; -enum { - ARG_CMP_STR_LEN = 5000, -}; - static const struct argp_option opts[] = { { "cmp-str-len", ARG_CMP_STR_LEN, "CMP_STR_LEN", 0, "Set the length of compared string" },
To parse command line the bench utility uses the argp_parse() function. This function takes as an argument a parent 'struct argp' structure which defines common command line options and an array of children 'struct argp' structures which defines additional command line options for particular benchmarks. This implementation doesn't allow benchmarks to share option names, e.g., if two benchmarks want to use, say, the --option option, then only one of them will succeed (the first one encountered in the array). This will be convenient if we could use the same option names in different benchmarks (with the same semantics, e.g., --nr_loops=N). Fix this by calling the argp_parse() function twice. The first call is needed to find the benchmark name. Given the name, we can patch the list of argp children to only include the correct list of option. This way the right parser will be executed. (If there's no a specific list of arguments, then only one call is enough.) As was mentioned above, arguments with same names should have the same semantics (which means in this case "taking a parameter or not"), but can have different description and will have different parsers. As we now can share option names, this also makes sense to share the option ids. Previously every benchmark defined a separate enum, like enum { ARG_SMTH = 9000, ARG_SMTH_ELSE = 9001, }; These ids were later used to distinguish between command line options: static const struct argp_option opts[] = { { "smth", ARG_SMTH, "SMTH", 0, "Set number of smth to configure smth"}, { "smth_else", ARG_SMTH_ELSE, "SMTH_ELSE", 0, "Set number of smth_else to configure smth else"}, ... Move arguments id definitions to bench.h such that they are visible to every benchmark (and such that there's no need to grep if this number is defined already or not). Signed-off-by: Anton Protopopov <aspsk@isovalent.com> --- tools/testing/selftests/bpf/bench.c | 98 +++++++++++++++++-- tools/testing/selftests/bpf/bench.h | 20 ++++ .../bpf/benchs/bench_bloom_filter_map.c | 6 -- .../selftests/bpf/benchs/bench_bpf_loop.c | 4 - .../bpf/benchs/bench_local_storage.c | 5 - .../bench_local_storage_rcu_tasks_trace.c | 6 -- .../selftests/bpf/benchs/bench_ringbufs.c | 8 -- .../selftests/bpf/benchs/bench_strncmp.c | 4 - 8 files changed, 110 insertions(+), 41 deletions(-)