On 23/01/30 04:07, Andrii Nakryiko wrote:
> On Fri, Jan 27, 2023 at 10:14 AM Anton Protopopov <aspsk@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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);
>
> [...]
