* [PATCH bpf-next 00/10] veristat: replay, filtering, sorting
@ 2022-11-03 5:52 Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 01/10] selftests/bpf: add veristat replay mode Andrii Nakryiko
` (10 more replies)
0 siblings, 11 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:52 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
This patch set adds a bunch of new featurs and improvements that were sorely
missing during recent active use of veristat to develop BPF verifier precision
changes. Individual patches provide justification, explanation and often
examples showing how new capabilities can be used.
Andrii Nakryiko (10):
selftests/bpf: add veristat replay mode
selftests/bpf: shorten "Total insns/states" column names in veristat
selftests/bpf: consolidate and improve file/prog filtering in veristat
selftests/bpf: ensure we always have non-ambiguous sorting in veristat
selftests/bpf: allow to define asc/desc ordering for sort specs in
veristat
selftests/bpf: support simple filtering of stats in veristat
selftests/bpf: make veristat emit all stats in CSV mode by default
selftests/bpf: handle missing records in comparison mode better in
veristat
selftests/bpf: support stats ordering in comparison mode in veristat
selftests/bpf: support stat filtering in comparison mode in veristat
tools/testing/selftests/bpf/veristat.c | 887 ++++++++++++++++++++-----
1 file changed, 727 insertions(+), 160 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH bpf-next 01/10] selftests/bpf: add veristat replay mode
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
@ 2022-11-03 5:52 ` Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 02/10] selftests/bpf: shorten "Total insns/states" column names in veristat Andrii Nakryiko
` (9 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:52 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 973cbf6af323..7e1432c06e0c 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();
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 02/10] selftests/bpf: shorten "Total insns/states" column names in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 01/10] selftests/bpf: add veristat replay mode Andrii Nakryiko
@ 2022-11-03 5:52 ` Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 03/10] selftests/bpf: consolidate and improve file/prog filtering " Andrii Nakryiko
` (8 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:52 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 7e1432c06e0c..d553f38a6cee 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;
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 03/10] selftests/bpf: consolidate and improve file/prog filtering in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 01/10] selftests/bpf: add veristat replay mode Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 02/10] selftests/bpf: shorten "Total insns/states" column names in veristat Andrii Nakryiko
@ 2022-11-03 5:52 ` Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 04/10] selftests/bpf: ensure we always have non-ambiguous sorting " Andrii Nakryiko
` (7 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:52 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
Slightly change rules of specifying file/prog glob filters. In practice
it's quite often inconvenient to do `*/<prog-glob>` if that program glob
is unique enough and won't accidentally match any file names.
This patch changes the rules so that `-f <glob>` will apply specified
glob to both file and program names. User still has all the control by
doing '*/<prog-only-glob>' or '<file-only-glob/*'. We also now allow
'/<prog-glob>' and '<file-glob/' (all matching wildcard is assumed if
missing).
Also, internally unify file-only and file+prog checks
(should_process_file and should_process_prog are now
should_process_file_prog that can handle prog name as optional). This
makes maintaining and extending this code easier.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
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 d553f38a6cee..f6f6a2490489 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 "<obj-glob>" or "<obj-glob>/<prog-glob>" */
+ memset(f, 0, sizeof(*f));
+
+ /* File/prog filter can be specified either as '<glob>' or
+ * '<file-glob>/<prog-glob>'. In the former case <glob> 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 '/<prog-glob>'
+ * form to glob just program name, or '<file-glob>/' to glob only file
+ * name. But usually common <glob> 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);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 04/10] selftests/bpf: ensure we always have non-ambiguous sorting in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (2 preceding siblings ...)
2022-11-03 5:52 ` [PATCH bpf-next 03/10] selftests/bpf: consolidate and improve file/prog filtering " Andrii Nakryiko
@ 2022-11-03 5:52 ` Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 05/10] selftests/bpf: allow to define asc/desc ordering for sort specs " Andrii Nakryiko
` (6 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:52 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 f6f6a2490489..0da3ecf6ed52 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 '-'
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 05/10] selftests/bpf: allow to define asc/desc ordering for sort specs in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (3 preceding siblings ...)
2022-11-03 5:52 ` [PATCH bpf-next 04/10] selftests/bpf: ensure we always have non-ambiguous sorting " Andrii Nakryiko
@ 2022-11-03 5:52 ` Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 06/10] selftests/bpf: support simple filtering of stats " Andrii Nakryiko
` (5 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:52 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 0da3ecf6ed52..56ba55156abb 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)
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 06/10] selftests/bpf: support simple filtering of stats in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (4 preceding siblings ...)
2022-11-03 5:52 ` [PATCH bpf-next 05/10] selftests/bpf: allow to define asc/desc ordering for sort specs " Andrii Nakryiko
@ 2022-11-03 5:53 ` Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 07/10] selftests/bpf: make veristat emit all stats in CSV mode by default Andrii Nakryiko
` (4 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:53 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 56ba55156abb..37e512d233a7 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:
+ * <stat><op><value, where:
+ * - <stat> is one of supported numerical stats (verdict is also
+ * considered numerical, failure == 0, success == 1);
+ * - <op> is comparison operator (see `operators` definitions);
+ * - <value> 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 '<glob>' or
* '<file-glob>/<prog-glob>'. In the former case <glob> 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 <glob> 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);
}
}
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 07/10] selftests/bpf: make veristat emit all stats in CSV mode by default
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (5 preceding siblings ...)
2022-11-03 5:53 ` [PATCH bpf-next 06/10] selftests/bpf: support simple filtering of stats " Andrii Nakryiko
@ 2022-11-03 5:53 ` Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 08/10] selftests/bpf: handle missing records in comparison mode better in veristat Andrii Nakryiko
` (3 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:53 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 37e512d233a7..ec1a8ba7791c 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;
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 08/10] selftests/bpf: handle missing records in comparison mode better in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (6 preceding siblings ...)
2022-11-03 5:53 ` [PATCH bpf-next 07/10] selftests/bpf: make veristat emit all stats in CSV mode by default Andrii Nakryiko
@ 2022-11-03 5:53 ` Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 09/10] selftests/bpf: support stats ordering in comparison mode " Andrii Nakryiko
` (2 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:53 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 ec1a8ba7791c..5a9568a8c0bf 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);
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 09/10] selftests/bpf: support stats ordering in comparison mode in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (7 preceding siblings ...)
2022-11-03 5:53 ` [PATCH bpf-next 08/10] selftests/bpf: handle missing records in comparison mode better in veristat Andrii Nakryiko
@ 2022-11-03 5:53 ` Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 10/10] selftests/bpf: support stat filtering " Andrii Nakryiko
2022-11-04 5:10 ` [PATCH bpf-next 00/10] veristat: replay, filtering, sorting patchwork-bot+netdevbpf
10 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:53 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 5a9568a8c0bf..f2ea825ee80a 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -17,6 +17,7 @@
#include <bpf/libbpf.h>
#include <libelf.h>
#include <gelf.h>
+#include <float.h>
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
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 10/10] selftests/bpf: support stat filtering in comparison mode in veristat
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (8 preceding siblings ...)
2022-11-03 5:53 ` [PATCH bpf-next 09/10] selftests/bpf: support stats ordering in comparison mode " Andrii Nakryiko
@ 2022-11-03 5:53 ` Andrii Nakryiko
2022-11-03 16:46 ` Alexei Starovoitov
2022-11-04 5:10 ` [PATCH bpf-next 00/10] veristat: replay, filtering, sorting patchwork-bot+netdevbpf
10 siblings, 1 reply; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 5:53 UTC (permalink / raw)
To: bpf, ast, daniel, netdev, kuba; +Cc: andrii, kernel-team
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 <andrii@kernel.org>
---
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 f2ea825ee80a..9e3811ab4866 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) {
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 10/10] selftests/bpf: support stat filtering in comparison mode in veristat
2022-11-03 5:53 ` [PATCH bpf-next 10/10] selftests/bpf: support stat filtering " Andrii Nakryiko
@ 2022-11-03 16:46 ` Alexei Starovoitov
2022-11-03 18:25 ` Andrii Nakryiko
0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2022-11-03 16:46 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, ast, daniel, netdev, kuba; +Cc: kernel-team
On 11/2/22 10:53 PM, Andrii Nakryiko wrote:
> 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
All these improvements to veristat look great.
What is the way to do negative filter ?
In other words what is the way to avoid using " | grep -v '+0' " ?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 10/10] selftests/bpf: support stat filtering in comparison mode in veristat
2022-11-03 16:46 ` Alexei Starovoitov
@ 2022-11-03 18:25 ` Andrii Nakryiko
0 siblings, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2022-11-03 18:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, ast, daniel, netdev, kuba, kernel-team
On Thu, Nov 3, 2022 at 9:46 AM Alexei Starovoitov <ast@meta.com> wrote:
>
> On 11/2/22 10:53 PM, Andrii Nakryiko wrote:
> > 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
>
> All these improvements to veristat look great.
> What is the way to do negative filter ?
> In other words what is the way to avoid using " | grep -v '+0' " ?
>
for grep -v '+0' replacement you can do `-f 'insns_diff>0'`.
But what I meant by negative filtering is adding '!' in front of the
filter to make it a "deny filter". E.g., -f '!*blah*.bpf.o' will
exclude any file matching "*blah*.bpf.o" naming glob. Similar for the
stat conditions, you should be able to do `-f '!insns_diff=0'`, which
in this case is equivalent to just `-f 'insns_diff>0'`.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 00/10] veristat: replay, filtering, sorting
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
` (9 preceding siblings ...)
2022-11-03 5:53 ` [PATCH bpf-next 10/10] selftests/bpf: support stat filtering " Andrii Nakryiko
@ 2022-11-04 5:10 ` patchwork-bot+netdevbpf
10 siblings, 0 replies; 14+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-11-04 5:10 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, netdev, kuba, kernel-team
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Wed, 2 Nov 2022 22:52:54 -0700 you wrote:
> This patch set adds a bunch of new featurs and improvements that were sorely
> missing during recent active use of veristat to develop BPF verifier precision
> changes. Individual patches provide justification, explanation and often
> examples showing how new capabilities can be used.
>
> Andrii Nakryiko (10):
> selftests/bpf: add veristat replay mode
> selftests/bpf: shorten "Total insns/states" column names in veristat
> selftests/bpf: consolidate and improve file/prog filtering in veristat
> selftests/bpf: ensure we always have non-ambiguous sorting in veristat
> selftests/bpf: allow to define asc/desc ordering for sort specs in
> veristat
> selftests/bpf: support simple filtering of stats in veristat
> selftests/bpf: make veristat emit all stats in CSV mode by default
> selftests/bpf: handle missing records in comparison mode better in
> veristat
> selftests/bpf: support stats ordering in comparison mode in veristat
> selftests/bpf: support stat filtering in comparison mode in veristat
>
> [...]
Here is the summary with links:
- [bpf-next,01/10] selftests/bpf: add veristat replay mode
https://git.kernel.org/bpf/bpf-next/c/9b5e3536c898
- [bpf-next,02/10] selftests/bpf: shorten "Total insns/states" column names in veristat
https://git.kernel.org/bpf/bpf-next/c/62d2c08bb91c
- [bpf-next,03/10] selftests/bpf: consolidate and improve file/prog filtering in veristat
https://git.kernel.org/bpf/bpf-next/c/10b1b3f3e56a
- [bpf-next,04/10] selftests/bpf: ensure we always have non-ambiguous sorting in veristat
https://git.kernel.org/bpf/bpf-next/c/b9670b904a59
- [bpf-next,05/10] selftests/bpf: allow to define asc/desc ordering for sort specs in veristat
https://git.kernel.org/bpf/bpf-next/c/d68c07e2dd91
- [bpf-next,06/10] selftests/bpf: support simple filtering of stats in veristat
https://git.kernel.org/bpf/bpf-next/c/1bb4ec815015
- [bpf-next,07/10] selftests/bpf: make veristat emit all stats in CSV mode by default
https://git.kernel.org/bpf/bpf-next/c/77534401d69c
- [bpf-next,08/10] selftests/bpf: handle missing records in comparison mode better in veristat
https://git.kernel.org/bpf/bpf-next/c/a5710848d824
- [bpf-next,09/10] selftests/bpf: support stats ordering in comparison mode in veristat
https://git.kernel.org/bpf/bpf-next/c/fa9bb590c289
- [bpf-next,10/10] selftests/bpf: support stat filtering in comparison mode in veristat
https://git.kernel.org/bpf/bpf-next/c/d5ce4b892341
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-11-04 5:10 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-03 5:52 [PATCH bpf-next 00/10] veristat: replay, filtering, sorting Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 01/10] selftests/bpf: add veristat replay mode Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 02/10] selftests/bpf: shorten "Total insns/states" column names in veristat Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 03/10] selftests/bpf: consolidate and improve file/prog filtering " Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 04/10] selftests/bpf: ensure we always have non-ambiguous sorting " Andrii Nakryiko
2022-11-03 5:52 ` [PATCH bpf-next 05/10] selftests/bpf: allow to define asc/desc ordering for sort specs " Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 06/10] selftests/bpf: support simple filtering of stats " Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 07/10] selftests/bpf: make veristat emit all stats in CSV mode by default Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 08/10] selftests/bpf: handle missing records in comparison mode better in veristat Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 09/10] selftests/bpf: support stats ordering in comparison mode " Andrii Nakryiko
2022-11-03 5:53 ` [PATCH bpf-next 10/10] selftests/bpf: support stat filtering " Andrii Nakryiko
2022-11-03 16:46 ` Alexei Starovoitov
2022-11-03 18:25 ` Andrii Nakryiko
2022-11-04 5:10 ` [PATCH bpf-next 00/10] veristat: replay, filtering, sorting patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).