From 9b5e3536c898b23143748b8cc01c64f868ec7078 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:52:55 -0700 Subject: [PATCH 01/10] selftests/bpf: add veristat replay mode Replay mode allow to parse previously stored CSV file with verification results and present it in desired output (presumable human-readable table, but CSV to CSV convertion is supported as well). While doing that, it's possible to use veristat's sorting rules, specify subset of columns, and filter by file and program name. In subsequent patches veristat's filtering capabilities will just grow making replay mode even more useful in practice for post-processing results. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-2-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 126 +++++++++++++++++-------- 1 file changed, 88 insertions(+), 38 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 973cbf6af3230..7e1432c06e0cb 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -67,6 +67,7 @@ static struct env { int log_level; enum resfmt out_fmt; bool comparison_mode; + bool replay_mode; struct verif_stats *prog_stats; int prog_stat_cnt; @@ -115,6 +116,7 @@ static const struct argp_option opts[] = { { "sort", 's', "SPEC", 0, "Specify sort order" }, { "output-format", 'o', "FMT", 0, "Result output format (table, csv), default is table." }, { "compare", 'C', NULL, 0, "Comparison mode" }, + { "replay", 'R', NULL, 0, "Replay mode" }, { "filter", 'f', "FILTER", 0, "Filter expressions (or @filename for file with expressions)." }, {}, }; @@ -169,6 +171,9 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state) case 'C': env.comparison_mode = true; break; + case 'R': + env.replay_mode = true; + break; case 'f': if (arg[0] == '@') err = append_filter_file(arg + 1); @@ -841,42 +846,6 @@ static void output_stats(const struct verif_stats *s, enum resfmt fmt, bool last } } -static int handle_verif_mode(void) -{ - int i, err; - - if (env.filename_cnt == 0) { - fprintf(stderr, "Please provide path to BPF object file!\n"); - argp_help(&argp, stderr, ARGP_HELP_USAGE, "veristat"); - return -EINVAL; - } - - for (i = 0; i < env.filename_cnt; i++) { - err = process_obj(env.filenames[i]); - if (err) { - fprintf(stderr, "Failed to process '%s': %d\n", env.filenames[i], err); - return err; - } - } - - qsort(env.prog_stats, env.prog_stat_cnt, sizeof(*env.prog_stats), cmp_prog_stats); - - if (env.out_fmt == RESFMT_TABLE) { - /* calculate column widths */ - output_headers(RESFMT_TABLE_CALCLEN); - for (i = 0; i < env.prog_stat_cnt; i++) - output_stats(&env.prog_stats[i], RESFMT_TABLE_CALCLEN, false); - } - - /* actually output the table */ - output_headers(env.out_fmt); - for (i = 0; i < env.prog_stat_cnt; i++) { - output_stats(&env.prog_stats[i], env.out_fmt, i == env.prog_stat_cnt - 1); - } - - return 0; -} - static int parse_stat_value(const char *str, enum stat_id id, struct verif_stats *st) { switch (id) { @@ -1206,7 +1175,7 @@ static int handle_comparison_mode(void) int err, i, j; if (env.filename_cnt != 2) { - fprintf(stderr, "Comparison mode expects exactly two input CSV files!\n"); + fprintf(stderr, "Comparison mode expects exactly two input CSV files!\n\n"); argp_help(&argp, stderr, ARGP_HELP_USAGE, "veristat"); return -EINVAL; } @@ -1307,6 +1276,79 @@ static int handle_comparison_mode(void) return 0; } +static void output_prog_stats(void) +{ + const struct verif_stats *stats; + int i, last_stat_idx = 0; + + if (env.out_fmt == RESFMT_TABLE) { + /* calculate column widths */ + output_headers(RESFMT_TABLE_CALCLEN); + for (i = 0; i < env.prog_stat_cnt; i++) { + stats = &env.prog_stats[i]; + output_stats(stats, RESFMT_TABLE_CALCLEN, false); + last_stat_idx = i; + } + } + + /* actually output the table */ + output_headers(env.out_fmt); + for (i = 0; i < env.prog_stat_cnt; i++) { + stats = &env.prog_stats[i]; + output_stats(stats, env.out_fmt, i == last_stat_idx); + } +} + +static int handle_verif_mode(void) +{ + int i, err; + + if (env.filename_cnt == 0) { + fprintf(stderr, "Please provide path to BPF object file!\n\n"); + argp_help(&argp, stderr, ARGP_HELP_USAGE, "veristat"); + return -EINVAL; + } + + for (i = 0; i < env.filename_cnt; i++) { + err = process_obj(env.filenames[i]); + if (err) { + fprintf(stderr, "Failed to process '%s': %d\n", env.filenames[i], err); + return err; + } + } + + qsort(env.prog_stats, env.prog_stat_cnt, sizeof(*env.prog_stats), cmp_prog_stats); + + output_prog_stats(); + + return 0; +} + +static int handle_replay_mode(void) +{ + struct stat_specs specs = {}; + int err; + + if (env.filename_cnt != 1) { + fprintf(stderr, "Replay mode expects exactly one input CSV file!\n\n"); + argp_help(&argp, stderr, ARGP_HELP_USAGE, "veristat"); + return -EINVAL; + } + + err = parse_stats_csv(env.filenames[0], &specs, + &env.prog_stats, &env.prog_stat_cnt); + if (err) { + fprintf(stderr, "Failed to parse stats from '%s': %d\n", env.filenames[0], err); + return err; + } + + qsort(env.prog_stats, env.prog_stat_cnt, sizeof(*env.prog_stats), cmp_prog_stats); + + output_prog_stats(); + + return 0; +} + int main(int argc, char **argv) { int err = 0, i; @@ -1315,7 +1357,7 @@ int main(int argc, char **argv) return 1; if (env.verbose && env.quiet) { - fprintf(stderr, "Verbose and quiet modes are incompatible, please specify just one or neither!\n"); + fprintf(stderr, "Verbose and quiet modes are incompatible, please specify just one or neither!\n\n"); argp_help(&argp, stderr, ARGP_HELP_USAGE, "veristat"); return 1; } @@ -1327,8 +1369,16 @@ int main(int argc, char **argv) if (env.sort_spec.spec_cnt == 0) env.sort_spec = default_sort_spec; + if (env.comparison_mode && env.replay_mode) { + fprintf(stderr, "Can't specify replay and comparison mode at the same time!\n\n"); + argp_help(&argp, stderr, ARGP_HELP_USAGE, "veristat"); + return 1; + } + if (env.comparison_mode) err = handle_comparison_mode(); + else if (env.replay_mode) + err = handle_replay_mode(); else err = handle_verif_mode(); From 62d2c08bb91cc3fc26319c571000cacac0312426 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:52:56 -0700 Subject: [PATCH 02/10] selftests/bpf: shorten "Total insns/states" column names in veristat In comparison mode the "Total " part is pretty useless, but takes a considerable amount of horizontal space. Drop the "Total " parts. Also make sure that table headers for numerical columns are aligned in the same fashion as integer values in those columns. This looks better and is now more obvious with shorter "Insns" and "States" column headers. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-3-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 7e1432c06e0cb..d553f38a6ceea 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -405,13 +405,14 @@ static struct stat_def { const char *header; const char *names[4]; bool asc_by_default; + bool left_aligned; } stat_defs[] = { - [FILE_NAME] = { "File", {"file_name", "filename", "file"}, true /* asc */ }, - [PROG_NAME] = { "Program", {"prog_name", "progname", "prog"}, true /* asc */ }, - [VERDICT] = { "Verdict", {"verdict"}, true /* asc: failure, success */ }, + [FILE_NAME] = { "File", {"file_name", "filename", "file"}, true /* asc */, true /* left */ }, + [PROG_NAME] = { "Program", {"prog_name", "progname", "prog"}, true /* asc */, true /* left */ }, + [VERDICT] = { "Verdict", {"verdict"}, true /* asc: failure, success */, true /* left */ }, [DURATION] = { "Duration (us)", {"duration", "dur"}, }, - [TOTAL_INSNS] = { "Total insns", {"total_insns", "insns"}, }, - [TOTAL_STATES] = { "Total states", {"total_states", "states"}, }, + [TOTAL_INSNS] = { "Insns", {"total_insns", "insns"}, }, + [TOTAL_STATES] = { "States", {"total_states", "states"}, }, [PEAK_STATES] = { "Peak states", {"peak_states"}, }, [MAX_STATES_PER_INSN] = { "Max states per insn", {"max_states_per_insn"}, }, [MARK_READ_MAX_LEN] = { "Max mark read length", {"max_mark_read_len", "mark_read"}, }, @@ -743,6 +744,7 @@ static void output_header_underlines(void) static void output_headers(enum resfmt fmt) { + const char *fmt_str; int i, len; for (i = 0; i < env.output_spec.spec_cnt; i++) { @@ -756,7 +758,8 @@ static void output_headers(enum resfmt fmt) *max_len = len; break; case RESFMT_TABLE: - printf("%s%-*s", i == 0 ? "" : COLUMN_SEP, *max_len, stat_defs[id].header); + fmt_str = stat_defs[id].left_aligned ? "%s%-*s" : "%s%*s"; + printf(fmt_str, i == 0 ? "" : COLUMN_SEP, *max_len, stat_defs[id].header); if (i == env.output_spec.spec_cnt - 1) printf("\n"); break; From 10b1b3f3e56a6a3586356bf8cc77d7753ba8fcc9 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:52:57 -0700 Subject: [PATCH 03/10] selftests/bpf: consolidate and improve file/prog filtering in veristat Slightly change rules of specifying file/prog glob filters. In practice it's quite often inconvenient to do `*/` if that program glob is unique enough and won't accidentally match any file names. This patch changes the rules so that `-f ` will apply specified glob to both file and program names. User still has all the control by doing '*/' or '' and ' Link: https://lore.kernel.org/r/20221103055304.2904589-4-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 127 +++++++++++++------------ 1 file changed, 65 insertions(+), 62 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index d553f38a6ceea..f6f6a24904896 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -55,6 +55,7 @@ enum resfmt { }; struct filter { + char *any_glob; char *file_glob; char *prog_glob; }; @@ -231,28 +232,6 @@ static bool glob_matches(const char *str, const char *pat) return !*str && !*pat; } -static bool should_process_file(const char *filename) -{ - int i; - - if (env.deny_filter_cnt > 0) { - for (i = 0; i < env.deny_filter_cnt; i++) { - if (glob_matches(filename, env.deny_filters[i].file_glob)) - return false; - } - } - - if (env.allow_filter_cnt == 0) - return true; - - for (i = 0; i < env.allow_filter_cnt; i++) { - if (glob_matches(filename, env.allow_filters[i].file_glob)) - return true; - } - - return false; -} - static bool is_bpf_obj_file(const char *path) { Elf64_Ehdr *ehdr; int fd, err = -EINVAL; @@ -285,38 +264,46 @@ static bool is_bpf_obj_file(const char *path) { return err == 0; } -static bool should_process_prog(const char *path, const char *prog_name) +static bool should_process_file_prog(const char *filename, const char *prog_name) { - const char *filename = basename(path); - int i; + struct filter *f; + int i, allow_cnt = 0; - if (env.deny_filter_cnt > 0) { - for (i = 0; i < env.deny_filter_cnt; i++) { - if (glob_matches(filename, env.deny_filters[i].file_glob)) - return false; - if (!env.deny_filters[i].prog_glob) - continue; - if (glob_matches(prog_name, env.deny_filters[i].prog_glob)) - return false; - } - } + for (i = 0; i < env.deny_filter_cnt; i++) { + f = &env.deny_filters[i]; - if (env.allow_filter_cnt == 0) - return true; + if (f->any_glob && glob_matches(filename, f->any_glob)) + return false; + if (f->any_glob && prog_name && glob_matches(prog_name, f->any_glob)) + return false; + if (f->file_glob && glob_matches(filename, f->file_glob)) + return false; + if (f->prog_glob && prog_name && glob_matches(prog_name, f->prog_glob)) + return false; + } for (i = 0; i < env.allow_filter_cnt; i++) { - if (!glob_matches(filename, env.allow_filters[i].file_glob)) - continue; - /* if filter specifies only filename glob part, it implicitly - * allows all progs within that file - */ - if (!env.allow_filters[i].prog_glob) - return true; - if (glob_matches(prog_name, env.allow_filters[i].prog_glob)) + f = &env.allow_filters[i]; + allow_cnt++; + + if (f->any_glob) { + if (glob_matches(filename, f->any_glob)) + return true; + if (prog_name && glob_matches(prog_name, f->any_glob)) + return true; + } else { + if (f->file_glob && !glob_matches(filename, f->file_glob)) + continue; + if (f->prog_glob && prog_name && !glob_matches(prog_name, f->prog_glob)) + continue; return true; + } } - return false; + /* if there are no file/prog name allow filters, allow all progs, + * unless they are denied earlier explicitly + */ + return allow_cnt == 0; } static int append_filter(struct filter **filters, int *cnt, const char *str) @@ -331,26 +318,40 @@ static int append_filter(struct filter **filters, int *cnt, const char *str) *filters = tmp; f = &(*filters)[*cnt]; - f->file_glob = f->prog_glob = NULL; - - /* filter can be specified either as "" or "/" */ + memset(f, 0, sizeof(*f)); + + /* File/prog filter can be specified either as '' or + * '/'. In the former case is applied to + * both file and program names. This seems to be way more useful in + * practice. If user needs full control, they can use '/' + * form to glob just program name, or '/' to glob only file + * name. But usually common seems to be the most useful and + * ergonomic way. + */ p = strchr(str, '/'); if (!p) { - f->file_glob = strdup(str); - if (!f->file_glob) + f->any_glob = strdup(str); + if (!f->any_glob) return -ENOMEM; } else { - f->file_glob = strndup(str, p - str); - f->prog_glob = strdup(p + 1); - if (!f->file_glob || !f->prog_glob) { - free(f->file_glob); - free(f->prog_glob); - f->file_glob = f->prog_glob = NULL; - return -ENOMEM; + if (str != p) { + /* non-empty file glob */ + f->file_glob = strndup(str, p - str); + if (!f->file_glob) + return -ENOMEM; + } + if (strlen(p + 1) > 0) { + /* non-empty prog glob */ + f->prog_glob = strdup(p + 1); + if (!f->prog_glob) { + free(f->file_glob); + f->file_glob = NULL; + return -ENOMEM; + } } } - *cnt = *cnt + 1; + *cnt += 1; return 0; } @@ -546,7 +547,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf int err = 0; void *tmp; - if (!should_process_prog(filename, bpf_program__name(prog))) { + if (!should_process_file_prog(basename(filename), bpf_program__name(prog))) { env.progs_skipped++; return 0; } @@ -602,7 +603,7 @@ static int process_obj(const char *filename) LIBBPF_OPTS(bpf_object_open_opts, opts); int err = 0, prog_cnt = 0; - if (!should_process_file(basename(filename))) { + if (!should_process_file_prog(basename(filename), NULL)) { if (env.verbose) printf("Skipping '%s' due to filters...\n", filename); env.files_skipped++; @@ -980,7 +981,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs, * parsed entire line; if row should be ignored we pretend we * never parsed it */ - if (!should_process_prog(st->file_name, st->prog_name)) { + if (!should_process_file_prog(st->file_name, st->prog_name)) { free(st->file_name); free(st->prog_name); *stat_cntp -= 1; @@ -1391,11 +1392,13 @@ int main(int argc, char **argv) free(env.filenames[i]); free(env.filenames); for (i = 0; i < env.allow_filter_cnt; i++) { + free(env.allow_filters[i].any_glob); free(env.allow_filters[i].file_glob); free(env.allow_filters[i].prog_glob); } free(env.allow_filters); for (i = 0; i < env.deny_filter_cnt; i++) { + free(env.deny_filters[i].any_glob); free(env.deny_filters[i].file_glob); free(env.deny_filters[i].prog_glob); } From b9670b904a59808ef4222179a255978384bcc119 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:52:58 -0700 Subject: [PATCH 04/10] selftests/bpf: ensure we always have non-ambiguous sorting in veristat Always fall back to unique file/prog comparison if user's custom order specs are ambiguous. This ensures stable output no matter what. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-5-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index f6f6a24904896..0da3ecf6ed525 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -723,7 +723,11 @@ static int cmp_prog_stats(const void *v1, const void *v2) return cmp; } - return 0; + /* always disambiguate with file+prog, which are unique */ + cmp = strcmp(s1->file_name, s2->file_name); + if (cmp != 0) + return cmp; + return strcmp(s1->prog_name, s2->prog_name); } #define HEADER_CHAR '-' From d68c07e2dd91c3e8bc451ecb218b77da2635cdd4 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:52:59 -0700 Subject: [PATCH 05/10] selftests/bpf: allow to define asc/desc ordering for sort specs in veristat Allow to specify '^' at the end of stat name to designate that it should be sorted in ascending order. Similarly, allow any of 'v', 'V', '.', '!', or '_' suffix "symbols" to designate descending order. It's such a zoo for descending order because there is no single intuitive symbol that could be used (using 'v' looks pretty weird in practice), so few symbols that are "downwards leaning or pointing" were chosen. Either way, it shouldn't cause any troubles in practice. This new feature allows to customize sortering order to match user's needs. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-6-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 63 ++++++++++++++++++++------ 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 0da3ecf6ed525..56ba55156abbe 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -419,32 +419,65 @@ static struct stat_def { [MARK_READ_MAX_LEN] = { "Max mark read length", {"max_mark_read_len", "mark_read"}, }, }; +static bool parse_stat_id(const char *name, size_t len, int *id) +{ + int i, j; + + for (i = 0; i < ARRAY_SIZE(stat_defs); i++) { + struct stat_def *def = &stat_defs[i]; + + for (j = 0; j < ARRAY_SIZE(stat_defs[i].names); j++) { + + if (!def->names[j] || + strlen(def->names[j]) != len || + strncmp(def->names[j], name, len) != 0) + continue; + + *id = i; + return true; + } + } + + return false; +} + +static bool is_asc_sym(char c) +{ + return c == '^'; +} + +static bool is_desc_sym(char c) +{ + return c == 'v' || c == 'V' || c == '.' || c == '!' || c == '_'; +} + static int parse_stat(const char *stat_name, struct stat_specs *specs) { - int id, i; + int id; + bool has_order = false, is_asc = false; + size_t len = strlen(stat_name); if (specs->spec_cnt >= ARRAY_SIZE(specs->ids)) { fprintf(stderr, "Can't specify more than %zd stats\n", ARRAY_SIZE(specs->ids)); return -E2BIG; } - for (id = 0; id < ARRAY_SIZE(stat_defs); id++) { - struct stat_def *def = &stat_defs[id]; - - for (i = 0; i < ARRAY_SIZE(stat_defs[id].names); i++) { - if (!def->names[i] || strcmp(def->names[i], stat_name) != 0) - continue; - - specs->ids[specs->spec_cnt] = id; - specs->asc[specs->spec_cnt] = def->asc_by_default; - specs->spec_cnt++; + if (len > 1 && (is_asc_sym(stat_name[len - 1]) || is_desc_sym(stat_name[len - 1]))) { + has_order = true; + is_asc = is_asc_sym(stat_name[len - 1]); + len -= 1; + } - return 0; - } + if (!parse_stat_id(stat_name, len, &id)) { + fprintf(stderr, "Unrecognized stat name '%s'\n", stat_name); + return -ESRCH; } - fprintf(stderr, "Unrecognized stat name '%s'\n", stat_name); - return -ESRCH; + specs->ids[specs->spec_cnt] = id; + specs->asc[specs->spec_cnt] = has_order ? is_asc : stat_defs[id].asc_by_default; + specs->spec_cnt++; + + return 0; } static int parse_stats(const char *stats_str, struct stat_specs *specs) From 1bb4ec815015609c9458d5ffeb5c8cc95b7d44d6 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:53:00 -0700 Subject: [PATCH 06/10] selftests/bpf: support simple filtering of stats in veristat Define simple expressions to filter not just by file and program name, but also by resulting values of collected stats. Support usual equality and inequality operators. Verdict, which is a boolean-like field can be also filtered either as 0/1, failure/success (with f/s, fail/succ, and failure/success aliases) symbols, or as false/true (f/t). Aliases are case insensitive. Currently this filtering is honored only in verification and replay modes. Comparison mode support will be added in next patch. Here's an example of verifying a bunch of BPF object files and emitting only results for successfully validated programs that have more than 100 total instructions processed by BPF verifier, sorted by number of instructions in ascending order: $ sudo ./veristat *.bpf.o -s insns^ -f 'insns>100' There can be many filters (both allow and deny flavors), all of them are combined. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-7-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 158 ++++++++++++++++++++++++- 1 file changed, 157 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 56ba55156abbe..37e512d233a70 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -54,10 +54,30 @@ enum resfmt { RESFMT_CSV, }; +enum filter_kind { + FILTER_NAME, + FILTER_STAT, +}; + +enum operator_kind { + OP_EQ, /* == or = */ + OP_NEQ, /* != or <> */ + OP_LT, /* < */ + OP_LE, /* <= */ + OP_GT, /* > */ + OP_GE, /* >= */ +}; + struct filter { + enum filter_kind kind; + /* FILTER_NAME */ char *any_glob; char *file_glob; char *prog_glob; + /* FILTER_STAT */ + enum operator_kind op; + int stat_id; + long value; }; static struct env { @@ -271,6 +291,8 @@ static bool should_process_file_prog(const char *filename, const char *prog_name for (i = 0; i < env.deny_filter_cnt; i++) { f = &env.deny_filters[i]; + if (f->kind != FILTER_NAME) + continue; if (f->any_glob && glob_matches(filename, f->any_glob)) return false; @@ -284,8 +306,10 @@ static bool should_process_file_prog(const char *filename, const char *prog_name for (i = 0; i < env.allow_filter_cnt; i++) { f = &env.allow_filters[i]; - allow_cnt++; + if (f->kind != FILTER_NAME) + continue; + allow_cnt++; if (f->any_glob) { if (glob_matches(filename, f->any_glob)) return true; @@ -306,11 +330,32 @@ static bool should_process_file_prog(const char *filename, const char *prog_name return allow_cnt == 0; } +static struct { + enum operator_kind op_kind; + const char *op_str; +} operators[] = { + /* Order of these definitions matter to avoid situations like '<' + * matching part of what is actually a '<>' operator. That is, + * substrings should go last. + */ + { OP_EQ, "==" }, + { OP_NEQ, "!=" }, + { OP_NEQ, "<>" }, + { OP_LE, "<=" }, + { OP_LT, "<" }, + { OP_GE, ">=" }, + { OP_GT, ">" }, + { OP_EQ, "=" }, +}; + +static bool parse_stat_id(const char *name, size_t len, int *id); + static int append_filter(struct filter **filters, int *cnt, const char *str) { struct filter *f; void *tmp; const char *p; + int i; tmp = realloc(*filters, (*cnt + 1) * sizeof(**filters)); if (!tmp) @@ -320,6 +365,67 @@ static int append_filter(struct filter **filters, int *cnt, const char *str) f = &(*filters)[*cnt]; memset(f, 0, sizeof(*f)); + /* First, let's check if it's a stats filter of the following form: + * is one of supported numerical stats (verdict is also + * considered numerical, failure == 0, success == 1); + * - is comparison operator (see `operators` definitions); + * - is an integer (or failure/success, or false/true as + * special aliases for 0 and 1, respectively). + * If the form doesn't match what user provided, we assume file/prog + * glob filter. + */ + for (i = 0; i < ARRAY_SIZE(operators); i++) { + int id; + long val; + const char *end = str; + const char *op_str; + + op_str = operators[i].op_str; + p = strstr(str, op_str); + if (!p) + continue; + + if (!parse_stat_id(str, p - str, &id)) { + fprintf(stderr, "Unrecognized stat name in '%s'!\n", str); + return -EINVAL; + } + if (id >= FILE_NAME) { + fprintf(stderr, "Non-integer stat is specified in '%s'!\n", str); + return -EINVAL; + } + + p += strlen(op_str); + + if (strcasecmp(p, "true") == 0 || + strcasecmp(p, "t") == 0 || + strcasecmp(p, "success") == 0 || + strcasecmp(p, "succ") == 0 || + strcasecmp(p, "s") == 0) { + val = 1; + } else if (strcasecmp(p, "false") == 0 || + strcasecmp(p, "f") == 0 || + strcasecmp(p, "failure") == 0 || + strcasecmp(p, "fail") == 0) { + val = 0; + } else { + errno = 0; + val = strtol(p, (char **)&end, 10); + if (errno || end == p || *end != '\0' ) { + fprintf(stderr, "Invalid integer value in '%s'!\n", str); + return -EINVAL; + } + } + + f->kind = FILTER_STAT; + f->stat_id = id; + f->op = operators[i].op_kind; + f->value = val; + + *cnt += 1; + return 0; + } + /* File/prog filter can be specified either as '' or * '/'. In the former case is applied to * both file and program names. This seems to be way more useful in @@ -328,6 +434,7 @@ static int append_filter(struct filter **filters, int *cnt, const char *str) * name. But usually common seems to be the most useful and * ergonomic way. */ + f->kind = FILTER_NAME; p = strchr(str, '/'); if (!p) { f->any_glob = strdup(str); @@ -1317,6 +1424,51 @@ static int handle_comparison_mode(void) return 0; } +static bool is_stat_filter_matched(struct filter *f, const struct verif_stats *stats) +{ + long value = stats->stats[f->stat_id]; + + switch (f->op) { + case OP_EQ: return value == f->value; + case OP_NEQ: return value != f->value; + case OP_LT: return value < f->value; + case OP_LE: return value <= f->value; + case OP_GT: return value > f->value; + case OP_GE: return value >= f->value; + } + + fprintf(stderr, "BUG: unknown filter op %d!\n", f->op); + return false; +} + +static bool should_output_stats(const struct verif_stats *stats) +{ + struct filter *f; + int i, allow_cnt = 0; + + for (i = 0; i < env.deny_filter_cnt; i++) { + f = &env.deny_filters[i]; + if (f->kind != FILTER_STAT) + continue; + + if (is_stat_filter_matched(f, stats)) + return false; + } + + for (i = 0; i < env.allow_filter_cnt; i++) { + f = &env.allow_filters[i]; + if (f->kind != FILTER_STAT) + continue; + allow_cnt++; + + if (is_stat_filter_matched(f, stats)) + return true; + } + + /* if there are no stat allowed filters, pass everything through */ + return allow_cnt == 0; +} + static void output_prog_stats(void) { const struct verif_stats *stats; @@ -1327,6 +1479,8 @@ static void output_prog_stats(void) output_headers(RESFMT_TABLE_CALCLEN); for (i = 0; i < env.prog_stat_cnt; i++) { stats = &env.prog_stats[i]; + if (!should_output_stats(stats)) + continue; output_stats(stats, RESFMT_TABLE_CALCLEN, false); last_stat_idx = i; } @@ -1336,6 +1490,8 @@ static void output_prog_stats(void) output_headers(env.out_fmt); for (i = 0; i < env.prog_stat_cnt; i++) { stats = &env.prog_stats[i]; + if (!should_output_stats(stats)) + continue; output_stats(stats, env.out_fmt, i == last_stat_idx); } } From 77534401d69c2a35d2a53e599fafb5f0f604e45d Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:53:01 -0700 Subject: [PATCH 07/10] selftests/bpf: make veristat emit all stats in CSV mode by default Make veristat distinguish between table and CSV output formats and use different default set of stats (columns) that are emitted. While for human-readable table output it doesn't make sense to output all known stats, it is very useful for CSV mode to record all possible data, so that it can later be queried and filtered in replay or comparison mode. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-8-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 37e512d233a70..ec1a8ba7791c7 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -501,6 +501,15 @@ static const struct stat_specs default_output_spec = { }, }; +static const struct stat_specs default_csv_output_spec = { + .spec_cnt = 9, + .ids = { + FILE_NAME, PROG_NAME, VERDICT, DURATION, + TOTAL_INSNS, TOTAL_STATES, PEAK_STATES, + MAX_STATES_PER_INSN, MARK_READ_MAX_LEN, + }, +}; + static const struct stat_specs default_sort_spec = { .spec_cnt = 2, .ids = { @@ -1561,8 +1570,12 @@ int main(int argc, char **argv) if (env.verbose && env.log_level == 0) env.log_level = 1; - if (env.output_spec.spec_cnt == 0) - env.output_spec = default_output_spec; + if (env.output_spec.spec_cnt == 0) { + if (env.out_fmt == RESFMT_CSV) + env.output_spec = default_csv_output_spec; + else + env.output_spec = default_output_spec; + } if (env.sort_spec.spec_cnt == 0) env.sort_spec = default_sort_spec; From a5710848d824168b5f7f02aa3689e648c46b2e46 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:53:02 -0700 Subject: [PATCH 08/10] selftests/bpf: handle missing records in comparison mode better in veristat When comparing two datasets, if either side is missing corresponding record with the same file and prog name, currently veristat emits misleading zeros/failures, and even tried to calculate a difference, even though there is no data to compare against. This patch improves internal logic of handling such situations. Now we'll emit "N/A" in places where data is missing and comparison is non-sensical. As an example, in an artificially truncated and mismatched Cilium results, the output looks like below: $ ./veristat -e file,prog,verdict,insns -C ~/base.csv ~/comp.csv File Program Verdict (A) Verdict (B) Verdict (DIFF) Insns (A) Insns (B) Insns (DIFF) ------------------ ------------------------------ ----------- ----------- -------------- --------- --------- -------------- bpf_alignchecker.o __send_drop_notify success N/A N/A 53 N/A N/A bpf_alignchecker.o tail_icmp6_handle_ns failure failure MATCH 33 33 +0 (+0.00%) bpf_alignchecker.o tail_icmp6_send_echo_reply N/A failure N/A N/A 74 N/A bpf_host.o __send_drop_notify success N/A N/A 53 N/A N/A bpf_host.o cil_from_host success N/A N/A 762 N/A N/A bpf_xdp.o __send_drop_notify success success MATCH 151 151 +0 (+0.00%) bpf_xdp.o cil_xdp_entry success success MATCH 423 423 +0 (+0.00%) bpf_xdp.o tail_handle_nat_fwd_ipv4 success success MATCH 21547 20920 -627 (-2.91%) bpf_xdp.o tail_handle_nat_fwd_ipv6 success success MATCH 16974 17039 +65 (+0.38%) bpf_xdp.o tail_lb_ipv4 success success MATCH 71736 73430 +1694 (+2.36%) bpf_xdp.o tail_lb_ipv6 N/A success N/A N/A 151895 N/A bpf_xdp.o tail_nodeport_ipv4_dsr N/A success N/A N/A 1162 N/A bpf_xdp.o tail_nodeport_ipv6_dsr N/A success N/A N/A 1206 N/A bpf_xdp.o tail_nodeport_nat_egress_ipv4 N/A success N/A N/A 15619 N/A bpf_xdp.o tail_nodeport_nat_ingress_ipv4 success success MATCH 7658 7713 +55 (+0.72%) bpf_xdp.o tail_nodeport_nat_ingress_ipv6 success success MATCH 6405 6397 -8 (-0.12%) bpf_xdp.o tail_nodeport_nat_ipv6_egress failure failure MATCH 752 752 +0 (+0.00%) bpf_xdp.o tail_rev_nodeport_lb4 success success MATCH 7126 6934 -192 (-2.69%) bpf_xdp.o tail_rev_nodeport_lb6 success success MATCH 17954 17905 -49 (-0.27%) ------------------ ------------------------------ ----------- ----------- -------------- --------- --------- -------------- Internally veristat now separates joining two datasets and remembering the join, and actually emitting a comparison view. This will come handy when we add support for filtering and custom ordering in comparison mode. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-9-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 147 ++++++++++++++++++------- 1 file changed, 110 insertions(+), 37 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index ec1a8ba7791c7..5a9568a8c0bfb 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -41,6 +41,15 @@ struct verif_stats { long stats[NUM_STATS_CNT]; }; +/* joined comparison mode stats */ +struct verif_stats_join { + char *file_name; + char *prog_name; + + const struct verif_stats *stats_a; + const struct verif_stats *stats_b; +}; + struct stat_specs { int spec_cnt; enum stat_id ids[ALL_STATS_CNT]; @@ -97,6 +106,9 @@ static struct env { struct verif_stats *baseline_stats; int baseline_stat_cnt; + struct verif_stats_join *join_stats; + int join_stat_cnt; + struct stat_specs output_spec; struct stat_specs sort_spec; @@ -518,6 +530,15 @@ static const struct stat_specs default_sort_spec = { .asc = { true, true, }, }; +/* sorting for comparison mode to join two data sets */ +static const struct stat_specs join_sort_spec = { + .spec_cnt = 2, + .ids = { + FILE_NAME, PROG_NAME, + }, + .asc = { true, true, }, +}; + static struct stat_def { const char *header; const char *names[4]; @@ -934,13 +955,16 @@ static void prepare_value(const struct verif_stats *s, enum stat_id id, { switch (id) { case FILE_NAME: - *str = s->file_name; + *str = s ? s->file_name : "N/A"; break; case PROG_NAME: - *str = s->prog_name; + *str = s ? s->prog_name : "N/A"; break; case VERDICT: - *str = s->stats[VERDICT] ? "success" : "failure"; + if (!s) + *str = "N/A"; + else + *str = s->stats[VERDICT] ? "success" : "failure"; break; case DURATION: case TOTAL_INSNS: @@ -948,7 +972,7 @@ static void prepare_value(const struct verif_stats *s, enum stat_id id, case PEAK_STATES: case MAX_STATES_PER_INSN: case MARK_READ_MAX_LEN: - *val = s->stats[id]; + *val = s ? s->stats[id] : 0; break; default: fprintf(stderr, "Unrecognized stat #%d\n", id); @@ -1223,9 +1247,11 @@ static void output_comp_headers(enum resfmt fmt) output_comp_header_underlines(); } -static void output_comp_stats(const struct verif_stats *base, const struct verif_stats *comp, +static void output_comp_stats(const struct verif_stats_join *join_stats, enum resfmt fmt, bool last) { + const struct verif_stats *base = join_stats->stats_a; + const struct verif_stats *comp = join_stats->stats_b; char base_buf[1024] = {}, comp_buf[1024] = {}, diff_buf[1024] = {}; int i; @@ -1243,33 +1269,45 @@ static void output_comp_stats(const struct verif_stats *base, const struct verif /* normalize all the outputs to be in string buffers for simplicity */ if (is_key_stat(id)) { /* key stats (file and program name) are always strings */ - if (base != &fallback_stats) + if (base) snprintf(base_buf, sizeof(base_buf), "%s", base_str); else snprintf(base_buf, sizeof(base_buf), "%s", comp_str); } else if (base_str) { snprintf(base_buf, sizeof(base_buf), "%s", base_str); snprintf(comp_buf, sizeof(comp_buf), "%s", comp_str); - if (strcmp(base_str, comp_str) == 0) + if (!base || !comp) + snprintf(diff_buf, sizeof(diff_buf), "%s", "N/A"); + else if (strcmp(base_str, comp_str) == 0) snprintf(diff_buf, sizeof(diff_buf), "%s", "MATCH"); else snprintf(diff_buf, sizeof(diff_buf), "%s", "MISMATCH"); } else { double p = 0.0; - snprintf(base_buf, sizeof(base_buf), "%ld", base_val); - snprintf(comp_buf, sizeof(comp_buf), "%ld", comp_val); + if (base) + snprintf(base_buf, sizeof(base_buf), "%ld", base_val); + else + snprintf(base_buf, sizeof(base_buf), "%s", "N/A"); + if (comp) + snprintf(comp_buf, sizeof(comp_buf), "%ld", comp_val); + else + snprintf(comp_buf, sizeof(comp_buf), "%s", "N/A"); diff_val = comp_val - base_val; - if (base == &fallback_stats || comp == &fallback_stats || base_val == 0) { - if (comp_val == base_val) - p = 0.0; /* avoid +0 (+100%) case */ - else - p = comp_val < base_val ? -100.0 : 100.0; + if (!base || !comp) { + snprintf(diff_buf, sizeof(diff_buf), "%s", "N/A"); } else { - p = diff_val * 100.0 / base_val; + if (base_val == 0) { + if (comp_val == base_val) + p = 0.0; /* avoid +0 (+100%) case */ + else + p = comp_val < base_val ? -100.0 : 100.0; + } else { + p = diff_val * 100.0 / base_val; + } + snprintf(diff_buf, sizeof(diff_buf), "%+ld (%+.2lf%%)", diff_val, p); } - snprintf(diff_buf, sizeof(diff_buf), "%+ld (%+.2lf%%)", diff_val, p); } switch (fmt) { @@ -1328,6 +1366,7 @@ static int cmp_stats_key(const struct verif_stats *base, const struct verif_stat static int handle_comparison_mode(void) { struct stat_specs base_specs = {}, comp_specs = {}; + struct stat_specs tmp_sort_spec; enum resfmt cur_fmt; int err, i, j; @@ -1370,31 +1409,26 @@ static int handle_comparison_mode(void) } } + /* Replace user-specified sorting spec with file+prog sorting rule to + * be able to join two datasets correctly. Once we are done, we will + * restore the original sort spec. + */ + tmp_sort_spec = env.sort_spec; + env.sort_spec = join_sort_spec; qsort(env.prog_stats, env.prog_stat_cnt, sizeof(*env.prog_stats), cmp_prog_stats); qsort(env.baseline_stats, env.baseline_stat_cnt, sizeof(*env.baseline_stats), cmp_prog_stats); + env.sort_spec = tmp_sort_spec; - /* for human-readable table output we need to do extra pass to - * calculate column widths, so we substitute current output format - * with RESFMT_TABLE_CALCLEN and later revert it back to RESFMT_TABLE - * and do everything again. - */ - if (env.out_fmt == RESFMT_TABLE) - cur_fmt = RESFMT_TABLE_CALCLEN; - else - cur_fmt = env.out_fmt; - -one_more_time: - output_comp_headers(cur_fmt); - - /* If baseline and comparison datasets have different subset of rows - * (we match by 'object + prog' as a unique key) then assume - * empty/missing/zero value for rows that are missing in the opposite - * data set + /* Join two datasets together. If baseline and comparison datasets + * have different subset of rows (we match by 'object + prog' as + * a unique key) then assume empty/missing/zero value for rows that + * are missing in the opposite data set. */ i = j = 0; while (i < env.baseline_stat_cnt || j < env.prog_stat_cnt) { - bool last = (i == env.baseline_stat_cnt - 1) || (j == env.prog_stat_cnt - 1); const struct verif_stats *base, *comp; + struct verif_stats_join *join; + void *tmp; int r; base = i < env.baseline_stat_cnt ? &env.baseline_stats[i] : &fallback_stats; @@ -1411,18 +1445,56 @@ static int handle_comparison_mode(void) return -EINVAL; } + tmp = realloc(env.join_stats, (env.join_stat_cnt + 1) * sizeof(*env.join_stats)); + if (!tmp) + return -ENOMEM; + env.join_stats = tmp; + + join = &env.join_stats[env.join_stat_cnt]; + memset(join, 0, sizeof(*join)); + r = cmp_stats_key(base, comp); if (r == 0) { - output_comp_stats(base, comp, cur_fmt, last); + join->file_name = base->file_name; + join->prog_name = base->prog_name; + join->stats_a = base; + join->stats_b = comp; i++; j++; } else if (comp == &fallback_stats || r < 0) { - output_comp_stats(base, &fallback_stats, cur_fmt, last); + join->file_name = base->file_name; + join->prog_name = base->prog_name; + join->stats_a = base; + join->stats_b = NULL; i++; } else { - output_comp_stats(&fallback_stats, comp, cur_fmt, last); + join->file_name = comp->file_name; + join->prog_name = comp->prog_name; + join->stats_a = NULL; + join->stats_b = comp; j++; } + env.join_stat_cnt += 1; + } + + /* for human-readable table output we need to do extra pass to + * calculate column widths, so we substitute current output format + * with RESFMT_TABLE_CALCLEN and later revert it back to RESFMT_TABLE + * and do everything again. + */ + if (env.out_fmt == RESFMT_TABLE) + cur_fmt = RESFMT_TABLE_CALCLEN; + else + cur_fmt = env.out_fmt; + +one_more_time: + output_comp_headers(cur_fmt); + + for (i = 0; i < env.join_stat_cnt; i++) { + const struct verif_stats_join *join = &env.join_stats[i]; + bool last = i == env.join_stat_cnt - 1; + + output_comp_stats(join, cur_fmt, last); } if (cur_fmt == RESFMT_TABLE_CALCLEN) { @@ -1594,6 +1666,7 @@ int main(int argc, char **argv) free_verif_stats(env.prog_stats, env.prog_stat_cnt); free_verif_stats(env.baseline_stats, env.baseline_stat_cnt); + free(env.join_stats); for (i = 0; i < env.filename_cnt; i++) free(env.filenames[i]); free(env.filenames); From fa9bb590c2895b14c9da46ba1860d06efba55657 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:53:03 -0700 Subject: [PATCH 09/10] selftests/bpf: support stats ordering in comparison mode in veristat Introduce the concept of "stat variant", by which it's possible to specify whether to use the value from A (baseline) side, B (comparison or control) side, the absolute difference value or relative (percentage) difference value. To support specifying this, veristat recognizes `_a`, `_b`, `_diff`, `_pct` suffixes, which can be appended to stat name(s). In non-comparison mode variants are ignored (there is only `_a` variant effectively), if no variant suffix is provided, `_b` is assumed, as control group is of primary interest in comparison mode. These stat variants can be flexibly combined with asc/desc orders. Here's an example of ordering results first by verdict match/mismatch (or n/a if one of the sides is missing; n/a is always considered to be the lowest value), and within each match/mismatch/n/a group further sort by number of instructions in B side. In this case we don't have MISMATCH cases, but N/A are split from MATCH, demonstrating this custom ordering. $ ./veristat -e file,prog,verdict,insns -s verdict_diff,insns_b_ -C ~/base.csv ~/comp.csv File Program Verdict (A) Verdict (B) Verdict (DIFF) Insns (A) Insns (B) Insns (DIFF) ------------------ ------------------------------ ----------- ----------- -------------- --------- --------- -------------- bpf_xdp.o tail_lb_ipv6 N/A success N/A N/A 151895 N/A bpf_xdp.o tail_nodeport_nat_egress_ipv4 N/A success N/A N/A 15619 N/A bpf_xdp.o tail_nodeport_ipv6_dsr N/A success N/A N/A 1206 N/A bpf_xdp.o tail_nodeport_ipv4_dsr N/A success N/A N/A 1162 N/A bpf_alignchecker.o tail_icmp6_send_echo_reply N/A failure N/A N/A 74 N/A bpf_alignchecker.o __send_drop_notify success N/A N/A 53 N/A N/A bpf_host.o __send_drop_notify success N/A N/A 53 N/A N/A bpf_host.o cil_from_host success N/A N/A 762 N/A N/A bpf_xdp.o tail_lb_ipv4 success success MATCH 71736 73430 +1694 (+2.36%) bpf_xdp.o tail_handle_nat_fwd_ipv4 success success MATCH 21547 20920 -627 (-2.91%) bpf_xdp.o tail_rev_nodeport_lb6 success success MATCH 17954 17905 -49 (-0.27%) bpf_xdp.o tail_handle_nat_fwd_ipv6 success success MATCH 16974 17039 +65 (+0.38%) bpf_xdp.o tail_nodeport_nat_ingress_ipv4 success success MATCH 7658 7713 +55 (+0.72%) bpf_xdp.o tail_rev_nodeport_lb4 success success MATCH 7126 6934 -192 (-2.69%) bpf_xdp.o tail_nodeport_nat_ingress_ipv6 success success MATCH 6405 6397 -8 (-0.12%) bpf_xdp.o tail_nodeport_nat_ipv6_egress failure failure MATCH 752 752 +0 (+0.00%) bpf_xdp.o cil_xdp_entry success success MATCH 423 423 +0 (+0.00%) bpf_xdp.o __send_drop_notify success success MATCH 151 151 +0 (+0.00%) bpf_alignchecker.o tail_icmp6_handle_ns failure failure MATCH 33 33 +0 (+0.00%) ------------------ ------------------------------ ----------- ----------- -------------- --------- --------- -------------- Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-10-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 192 +++++++++++++++++++++++-- 1 file changed, 182 insertions(+), 10 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 5a9568a8c0bfb..f2ea825ee80a9 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -17,6 +17,7 @@ #include #include #include +#include enum stat_id { VERDICT, @@ -34,6 +35,45 @@ enum stat_id { NUM_STATS_CNT = FILE_NAME - VERDICT, }; +/* In comparison mode each stat can specify up to four different values: + * - A side value; + * - B side value; + * - absolute diff value; + * - relative (percentage) diff value. + * + * When specifying stat specs in comparison mode, user can use one of the + * following variant suffixes to specify which exact variant should be used for + * ordering or filtering: + * - `_a` for A side value; + * - `_b` for B side value; + * - `_diff` for absolute diff value; + * - `_pct` for relative (percentage) diff value. + * + * If no variant suffix is provided, then `_b` (control data) is assumed. + * + * As an example, let's say instructions stat has the following output: + * + * Insns (A) Insns (B) Insns (DIFF) + * --------- --------- -------------- + * 21547 20920 -627 (-2.91%) + * + * Then: + * - 21547 is A side value (insns_a); + * - 20920 is B side value (insns_b); + * - -627 is absolute diff value (insns_diff); + * - -2.91% is relative diff value (insns_pct). + * + * For verdict there is no verdict_pct variant. + * For file and program name, _a and _b variants are equivalent and there are + * no _diff or _pct variants. + */ +enum stat_variant { + VARIANT_A, + VARIANT_B, + VARIANT_DIFF, + VARIANT_PCT, +}; + struct verif_stats { char *file_name; char *prog_name; @@ -53,6 +93,7 @@ struct verif_stats_join { struct stat_specs { int spec_cnt; enum stat_id ids[ALL_STATS_CNT]; + enum stat_variant variants[ALL_STATS_CNT]; bool asc[ALL_STATS_CNT]; int lens[ALL_STATS_CNT * 3]; /* 3x for comparison mode */ }; @@ -86,6 +127,7 @@ struct filter { /* FILTER_STAT */ enum operator_kind op; int stat_id; + enum stat_variant stat_var; long value; }; @@ -360,7 +402,7 @@ static struct { { OP_EQ, "=" }, }; -static bool parse_stat_id(const char *name, size_t len, int *id); +static bool parse_stat_id_var(const char *name, size_t len, int *id, enum stat_variant *var); static int append_filter(struct filter **filters, int *cnt, const char *str) { @@ -388,6 +430,7 @@ static int append_filter(struct filter **filters, int *cnt, const char *str) * glob filter. */ for (i = 0; i < ARRAY_SIZE(operators); i++) { + enum stat_variant var; int id; long val; const char *end = str; @@ -398,7 +441,7 @@ static int append_filter(struct filter **filters, int *cnt, const char *str) if (!p) continue; - if (!parse_stat_id(str, p - str, &id)) { + if (!parse_stat_id_var(str, p - str, &id, &var)) { fprintf(stderr, "Unrecognized stat name in '%s'!\n", str); return -EINVAL; } @@ -431,6 +474,7 @@ static int append_filter(struct filter **filters, int *cnt, const char *str) f->kind = FILTER_STAT; f->stat_id = id; + f->stat_var = var; f->op = operators[i].op_kind; f->value = val; @@ -556,22 +600,52 @@ static struct stat_def { [MARK_READ_MAX_LEN] = { "Max mark read length", {"max_mark_read_len", "mark_read"}, }, }; -static bool parse_stat_id(const char *name, size_t len, int *id) +static bool parse_stat_id_var(const char *name, size_t len, int *id, enum stat_variant *var) { - int i, j; + static const char *var_sfxs[] = { + [VARIANT_A] = "_a", + [VARIANT_B] = "_b", + [VARIANT_DIFF] = "_diff", + [VARIANT_PCT] = "_pct", + }; + int i, j, k; for (i = 0; i < ARRAY_SIZE(stat_defs); i++) { struct stat_def *def = &stat_defs[i]; + size_t alias_len, sfx_len; + const char *alias; for (j = 0; j < ARRAY_SIZE(stat_defs[i].names); j++) { + alias = def->names[j]; + if (!alias) + continue; - if (!def->names[j] || - strlen(def->names[j]) != len || - strncmp(def->names[j], name, len) != 0) + alias_len = strlen(alias); + if (strncmp(name, alias, alias_len) != 0) continue; - *id = i; - return true; + if (alias_len == len) { + /* If no variant suffix is specified, we + * assume control group (just in case we are + * in comparison mode. Variant is ignored in + * non-comparison mode. + */ + *var = VARIANT_B; + *id = i; + return true; + } + + for (k = 0; k < ARRAY_SIZE(var_sfxs); k++) { + sfx_len = strlen(var_sfxs[k]); + if (alias_len + sfx_len != len) + continue; + + if (strncmp(name + alias_len, var_sfxs[k], sfx_len) == 0) { + *var = (enum stat_variant)k; + *id = i; + return true; + } + } } } @@ -593,6 +667,7 @@ static int parse_stat(const char *stat_name, struct stat_specs *specs) int id; bool has_order = false, is_asc = false; size_t len = strlen(stat_name); + enum stat_variant var; if (specs->spec_cnt >= ARRAY_SIZE(specs->ids)) { fprintf(stderr, "Can't specify more than %zd stats\n", ARRAY_SIZE(specs->ids)); @@ -605,12 +680,13 @@ static int parse_stat(const char *stat_name, struct stat_specs *specs) len -= 1; } - if (!parse_stat_id(stat_name, len, &id)) { + if (!parse_stat_id_var(stat_name, len, &id, &var)) { fprintf(stderr, "Unrecognized stat name '%s'\n", stat_name); return -ESRCH; } specs->ids[specs->spec_cnt] = id; + specs->variants[specs->spec_cnt] = var; specs->asc[specs->spec_cnt] = has_order ? is_asc : stat_defs[id].asc_by_default; specs->spec_cnt++; @@ -900,6 +976,99 @@ static int cmp_prog_stats(const void *v1, const void *v2) return strcmp(s1->prog_name, s2->prog_name); } +static void fetch_join_stat_value(const struct verif_stats_join *s, + enum stat_id id, enum stat_variant var, + const char **str_val, + double *num_val) +{ + long v1, v2; + + if (id == FILE_NAME) { + *str_val = s->file_name; + return; + } + if (id == PROG_NAME) { + *str_val = s->prog_name; + return; + } + + v1 = s->stats_a ? s->stats_a->stats[id] : 0; + v2 = s->stats_b ? s->stats_b->stats[id] : 0; + + switch (var) { + case VARIANT_A: + if (!s->stats_a) + *num_val = -DBL_MAX; + else + *num_val = s->stats_a->stats[id]; + return; + case VARIANT_B: + if (!s->stats_b) + *num_val = -DBL_MAX; + else + *num_val = s->stats_b->stats[id]; + return; + case VARIANT_DIFF: + if (!s->stats_a || !s->stats_b) + *num_val = -DBL_MAX; + else + *num_val = (double)(v2 - v1); + return; + case VARIANT_PCT: + if (!s->stats_a || !s->stats_b) { + *num_val = -DBL_MAX; + } else if (v1 == 0) { + if (v1 == v2) + *num_val = 0.0; + else + *num_val = v2 < v1 ? -100.0 : 100.0; + } else { + *num_val = (v2 - v1) * 100.0 / v1; + } + return; + } +} + +static int cmp_join_stat(const struct verif_stats_join *s1, + const struct verif_stats_join *s2, + enum stat_id id, enum stat_variant var, bool asc) +{ + const char *str1 = NULL, *str2 = NULL; + double v1, v2; + int cmp = 0; + + fetch_join_stat_value(s1, id, var, &str1, &v1); + fetch_join_stat_value(s2, id, var, &str2, &v2); + + if (str1) + cmp = strcmp(str1, str2); + else if (v1 != v2) + cmp = v1 < v2 ? -1 : 1; + + return asc ? cmp : -cmp; +} + +static int cmp_join_stats(const void *v1, const void *v2) +{ + const struct verif_stats_join *s1 = v1, *s2 = v2; + int i, cmp; + + for (i = 0; i < env.sort_spec.spec_cnt; i++) { + cmp = cmp_join_stat(s1, s2, + env.sort_spec.ids[i], + env.sort_spec.variants[i], + env.sort_spec.asc[i]); + if (cmp != 0) + return cmp; + } + + /* always disambiguate with file+prog, which are unique */ + cmp = strcmp(s1->file_name, s2->file_name); + if (cmp != 0) + return cmp; + return strcmp(s1->prog_name, s2->prog_name); +} + #define HEADER_CHAR '-' #define COLUMN_SEP " " @@ -1477,6 +1646,9 @@ static int handle_comparison_mode(void) env.join_stat_cnt += 1; } + /* now sort joined results accorsing to sort spec */ + qsort(env.join_stats, env.join_stat_cnt, sizeof(*env.join_stats), cmp_join_stats); + /* for human-readable table output we need to do extra pass to * calculate column widths, so we substitute current output format * with RESFMT_TABLE_CALCLEN and later revert it back to RESFMT_TABLE From d5ce4b89234156d66ac8a59bddbc341667aadf86 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Wed, 2 Nov 2022 22:53:04 -0700 Subject: [PATCH 10/10] selftests/bpf: support stat filtering in comparison mode in veristat Finally add support for filtering stats values, similar to non-comparison mode filtering. For comparison mode 4 variants of stats are important for filtering, as they allow to filter either A or B side, but even more importantly they allow to filter based on value difference, and for verdict stat value difference is MATCH/MISMATCH classification. So with these changes it's finally possible to easily check if there were any mismatches between failure/success outcomes on two separate data sets. Like in an example below: $ ./veristat -e file,prog,verdict,insns -C ~/baseline-results.csv ~/shortest-results.csv -f verdict_diff=mismatch File Program Verdict (A) Verdict (B) Verdict (DIFF) Insns (A) Insns (B) Insns (DIFF) ------------------------------------- --------------------- ----------- ----------- -------------- --------- --------- ------------------- dynptr_success.bpf.linked1.o test_data_slice success failure MISMATCH 85 0 -85 (-100.00%) dynptr_success.bpf.linked1.o test_read_write success failure MISMATCH 1992 0 -1992 (-100.00%) dynptr_success.bpf.linked1.o test_ringbuf success failure MISMATCH 74 0 -74 (-100.00%) kprobe_multi.bpf.linked1.o test_kprobe failure success MISMATCH 0 246 +246 (+100.00%) kprobe_multi.bpf.linked1.o test_kprobe_manual failure success MISMATCH 0 246 +246 (+100.00%) kprobe_multi.bpf.linked1.o test_kretprobe failure success MISMATCH 0 248 +248 (+100.00%) kprobe_multi.bpf.linked1.o test_kretprobe_manual failure success MISMATCH 0 248 +248 (+100.00%) kprobe_multi.bpf.linked1.o trigger failure success MISMATCH 0 2 +2 (+100.00%) netcnt_prog.bpf.linked1.o bpf_nextcnt failure success MISMATCH 0 56 +56 (+100.00%) pyperf600_nounroll.bpf.linked1.o on_event success failure MISMATCH 568128 1000001 +431873 (+76.02%) ringbuf_bench.bpf.linked1.o bench_ringbuf success failure MISMATCH 8 0 -8 (-100.00%) strobemeta.bpf.linked1.o on_event success failure MISMATCH 557149 1000001 +442852 (+79.49%) strobemeta_nounroll1.bpf.linked1.o on_event success failure MISMATCH 57240 1000001 +942761 (+1647.03%) strobemeta_nounroll2.bpf.linked1.o on_event success failure MISMATCH 501725 1000001 +498276 (+99.31%) strobemeta_subprogs.bpf.linked1.o on_event success failure MISMATCH 65420 1000001 +934581 (+1428.59%) test_map_in_map_invalid.bpf.linked1.o xdp_noop0 success failure MISMATCH 2 0 -2 (-100.00%) test_mmap.bpf.linked1.o test_mmap success failure MISMATCH 46 0 -46 (-100.00%) test_verif_scale3.bpf.linked1.o balancer_ingress success failure MISMATCH 845499 1000001 +154502 (+18.27%) ------------------------------------- --------------------- ----------- ----------- -------------- --------- --------- ------------------- Note that by filtering on verdict_diff=mismatch, it's now extremely easy and fast to see any changes in verdict. Example above showcases both failure -> success transitions (which are generally surprising) and success -> failure transitions (which are expected if bugs are present). Given veristat allows to query relative percent difference values, internal logic for comparison mode is based on floating point numbers, so requires a bit of epsilon precision logic, deviating from typical integer simple handling rules. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20221103055304.2904589-11-andrii@kernel.org Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/veristat.c | 70 ++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 5 deletions(-) diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index f2ea825ee80a9..9e3811ab48660 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -456,12 +456,16 @@ static int append_filter(struct filter **filters, int *cnt, const char *str) strcasecmp(p, "t") == 0 || strcasecmp(p, "success") == 0 || strcasecmp(p, "succ") == 0 || - strcasecmp(p, "s") == 0) { + strcasecmp(p, "s") == 0 || + strcasecmp(p, "match") == 0 || + strcasecmp(p, "m") == 0) { val = 1; } else if (strcasecmp(p, "false") == 0 || strcasecmp(p, "f") == 0 || strcasecmp(p, "failure") == 0 || - strcasecmp(p, "fail") == 0) { + strcasecmp(p, "fail") == 0 || + strcasecmp(p, "mismatch") == 0 || + strcasecmp(p, "mis") == 0) { val = 0; } else { errno = 0; @@ -1011,6 +1015,8 @@ static void fetch_join_stat_value(const struct verif_stats_join *s, case VARIANT_DIFF: if (!s->stats_a || !s->stats_b) *num_val = -DBL_MAX; + else if (id == VERDICT) + *num_val = v1 == v2 ? 1.0 /* MATCH */ : 0.0 /* MISMATCH */; else *num_val = (double)(v2 - v1); return; @@ -1532,12 +1538,61 @@ static int cmp_stats_key(const struct verif_stats *base, const struct verif_stat return strcmp(base->prog_name, comp->prog_name); } +static bool is_join_stat_filter_matched(struct filter *f, const struct verif_stats_join *stats) +{ + static const double eps = 1e-9; + const char *str = NULL; + double value = 0.0; + + fetch_join_stat_value(stats, f->stat_id, f->stat_var, &str, &value); + + switch (f->op) { + case OP_EQ: return value > f->value - eps && value < f->value + eps; + case OP_NEQ: return value < f->value - eps || value > f->value + eps; + case OP_LT: return value < f->value - eps; + case OP_LE: return value <= f->value + eps; + case OP_GT: return value > f->value + eps; + case OP_GE: return value >= f->value - eps; + } + + fprintf(stderr, "BUG: unknown filter op %d!\n", f->op); + return false; +} + +static bool should_output_join_stats(const struct verif_stats_join *stats) +{ + struct filter *f; + int i, allow_cnt = 0; + + for (i = 0; i < env.deny_filter_cnt; i++) { + f = &env.deny_filters[i]; + if (f->kind != FILTER_STAT) + continue; + + if (is_join_stat_filter_matched(f, stats)) + return false; + } + + for (i = 0; i < env.allow_filter_cnt; i++) { + f = &env.allow_filters[i]; + if (f->kind != FILTER_STAT) + continue; + allow_cnt++; + + if (is_join_stat_filter_matched(f, stats)) + return true; + } + + /* if there are no stat allowed filters, pass everything through */ + return allow_cnt == 0; +} + static int handle_comparison_mode(void) { struct stat_specs base_specs = {}, comp_specs = {}; struct stat_specs tmp_sort_spec; enum resfmt cur_fmt; - int err, i, j; + int err, i, j, last_idx; if (env.filename_cnt != 2) { fprintf(stderr, "Comparison mode expects exactly two input CSV files!\n\n"); @@ -1664,9 +1719,14 @@ static int handle_comparison_mode(void) for (i = 0; i < env.join_stat_cnt; i++) { const struct verif_stats_join *join = &env.join_stats[i]; - bool last = i == env.join_stat_cnt - 1; - output_comp_stats(join, cur_fmt, last); + if (!should_output_join_stats(join)) + continue; + + if (cur_fmt == RESFMT_TABLE_CALCLEN) + last_idx = i; + + output_comp_stats(join, cur_fmt, i == last_idx); } if (cur_fmt == RESFMT_TABLE_CALCLEN) {