* [PATCH bpf-next] selftests/bpf: add more stats into veristat
@ 2024-12-05 19:34 Mykyta Yatsenko
2024-12-05 23:50 ` Andrii Nakryiko
0 siblings, 1 reply; 4+ messages in thread
From: Mykyta Yatsenko @ 2024-12-05 19:34 UTC (permalink / raw)
To: bpf, ast, andrii, daniel, kafai, kernel-team; +Cc: Mykyta Yatsenko
From: Mykyta Yatsenko <yatsenko@meta.com>
Extend veristat to collect and print more stats, namely:
- program size in instructions
- jited program size
- program type
- attach type
- stack depth
Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
tools/testing/selftests/bpf/veristat.c | 51 +++++++++++++++++++++++---
1 file changed, 46 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index e12ef953fba8..0d7fb00175e8 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -38,8 +38,14 @@ enum stat_id {
FILE_NAME,
PROG_NAME,
+ SIZE,
+ JITED_SIZE,
+ STACK,
+ PROG_TYPE,
+ ATTACH_TYPE,
+
ALL_STATS_CNT,
- NUM_STATS_CNT = FILE_NAME - VERDICT,
+ NUM_STATS_CNT = ATTACH_TYPE - VERDICT + 1,
};
/* In comparison mode each stat can specify up to four different values:
@@ -640,19 +646,22 @@ static int append_filter_file(const char *path)
}
static const struct stat_specs default_output_spec = {
- .spec_cnt = 7,
+ .spec_cnt = 12,
.ids = {
FILE_NAME, PROG_NAME, VERDICT, DURATION,
- TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
+ TOTAL_INSNS, TOTAL_STATES, PEAK_STATES, SIZE,
+ JITED_SIZE, PROG_TYPE, ATTACH_TYPE, STACK,
},
};
static const struct stat_specs default_csv_output_spec = {
- .spec_cnt = 9,
+ .spec_cnt = 14,
.ids = {
FILE_NAME, PROG_NAME, VERDICT, DURATION,
TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
MAX_STATES_PER_INSN, MARK_READ_MAX_LEN,
+ SIZE, JITED_SIZE, PROG_TYPE, ATTACH_TYPE,
+ STACK,
},
};
@@ -688,6 +697,11 @@ static struct stat_def {
[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"}, },
+ [SIZE] = { "Prog size", {"prog_size", "size"}, },
+ [JITED_SIZE] = { "Jited size", {"jited_size"}, },
+ [STACK] = {"Stack depth", {"stack_depth", "stack"}, },
+ [PROG_TYPE] = { "Program type", {"program_type", "prog_type"}, },
+ [ATTACH_TYPE] = { "Attach type", {"attach_type", }, },
};
static bool parse_stat_id_var(const char *name, size_t len, int *id,
@@ -853,13 +867,16 @@ static int parse_verif_log(char * const buf, size_t buf_sz, struct verif_stats *
if (1 == sscanf(cur, "verification time %ld usec\n", &s->stats[DURATION]))
continue;
- if (6 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
+ if (5 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
&s->stats[TOTAL_INSNS],
&s->stats[MAX_STATES_PER_INSN],
&s->stats[TOTAL_STATES],
&s->stats[PEAK_STATES],
&s->stats[MARK_READ_MAX_LEN]))
continue;
+
+ if (1 == sscanf(cur, "stack depth %ld", &s->stats[STACK]))
+ continue;
}
return 0;
@@ -1146,8 +1163,11 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
char *buf;
int buf_sz, log_level;
struct verif_stats *stats;
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
int err = 0;
void *tmp;
+ int fd;
if (!should_process_file_prog(base_filename, bpf_program__name(prog))) {
env.progs_skipped++;
@@ -1196,6 +1216,13 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
stats->file_name = strdup(base_filename);
stats->prog_name = strdup(bpf_program__name(prog));
stats->stats[VERDICT] = err == 0; /* 1 - success, 0 - failure */
+ stats->stats[SIZE] = bpf_program__insn_cnt(prog);
+ stats->stats[PROG_TYPE] = bpf_program__type(prog);
+ stats->stats[ATTACH_TYPE] = bpf_program__expected_attach_type(prog);
+ fd = bpf_program__fd(prog);
+ if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0)
+ stats->stats[JITED_SIZE] = info.jited_prog_len;
+
parse_verif_log(buf, buf_sz, stats);
if (env.verbose) {
@@ -1309,6 +1336,11 @@ static int cmp_stat(const struct verif_stats *s1, const struct verif_stats *s2,
case PROG_NAME:
cmp = strcmp(s1->prog_name, s2->prog_name);
break;
+ case ATTACH_TYPE:
+ case PROG_TYPE:
+ case SIZE:
+ case JITED_SIZE:
+ case STACK:
case VERDICT:
case DURATION:
case TOTAL_INSNS:
@@ -1523,12 +1555,21 @@ static void prepare_value(const struct verif_stats *s, enum stat_id id,
else
*str = s->stats[VERDICT] ? "success" : "failure";
break;
+ case ATTACH_TYPE:
+ *str = s ? libbpf_bpf_attach_type_str(s->stats[ATTACH_TYPE]) ? : "N/A" : "N/A";
+ break;
+ case PROG_TYPE:
+ *str = s ? libbpf_bpf_prog_type_str(s->stats[PROG_TYPE]) ? : "N/A" : "N/A";
+ break;
case DURATION:
case TOTAL_INSNS:
case TOTAL_STATES:
case PEAK_STATES:
case MAX_STATES_PER_INSN:
case MARK_READ_MAX_LEN:
+ case STACK:
+ case SIZE:
+ case JITED_SIZE:
*val = s ? s->stats[id] : 0;
break;
default:
--
2.47.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add more stats into veristat
2024-12-05 19:34 [PATCH bpf-next] selftests/bpf: add more stats into veristat Mykyta Yatsenko
@ 2024-12-05 23:50 ` Andrii Nakryiko
2024-12-06 9:54 ` Mykyta Yatsenko
2024-12-06 13:29 ` Mykyta Yatsenko
0 siblings, 2 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2024-12-05 23:50 UTC (permalink / raw)
To: Mykyta Yatsenko
Cc: bpf, ast, andrii, daniel, kafai, kernel-team, Mykyta Yatsenko
On Thu, Dec 5, 2024 at 11:34 AM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
> From: Mykyta Yatsenko <yatsenko@meta.com>
>
> Extend veristat to collect and print more stats, namely:
> - program size in instructions
> - jited program size
> - program type
> - attach type
> - stack depth
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---
> tools/testing/selftests/bpf/veristat.c | 51 +++++++++++++++++++++++---
> 1 file changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index e12ef953fba8..0d7fb00175e8 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -38,8 +38,14 @@ enum stat_id {
> FILE_NAME,
> PROG_NAME,
>
> + SIZE,
> + JITED_SIZE,
> + STACK,
> + PROG_TYPE,
> + ATTACH_TYPE,
> +
> ALL_STATS_CNT,
> - NUM_STATS_CNT = FILE_NAME - VERDICT,
> + NUM_STATS_CNT = ATTACH_TYPE - VERDICT + 1,
this doesn't sound right, because PROG_NAME isn't a number statistics
> };
>
> /* In comparison mode each stat can specify up to four different values:
> @@ -640,19 +646,22 @@ static int append_filter_file(const char *path)
> }
>
> static const struct stat_specs default_output_spec = {
> - .spec_cnt = 7,
> + .spec_cnt = 12,
> .ids = {
> FILE_NAME, PROG_NAME, VERDICT, DURATION,
> - TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
> + TOTAL_INSNS, TOTAL_STATES, PEAK_STATES, SIZE,
> + JITED_SIZE, PROG_TYPE, ATTACH_TYPE, STACK,
I think SIZE or JITED_SIZE might be good candidates for default view,
but not all of the above. I think we can also drop PEAK_STATES from
default, btw.
> },
> };
>
> static const struct stat_specs default_csv_output_spec = {
> - .spec_cnt = 9,
> + .spec_cnt = 14,
> .ids = {
> FILE_NAME, PROG_NAME, VERDICT, DURATION,
> TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
> MAX_STATES_PER_INSN, MARK_READ_MAX_LEN,
> + SIZE, JITED_SIZE, PROG_TYPE, ATTACH_TYPE,
> + STACK,
this is fine, we want everything in CSV, yep
> },
> };
>
> @@ -688,6 +697,11 @@ static struct stat_def {
> [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"}, },
> + [SIZE] = { "Prog size", {"prog_size", "size"}, },
drop "size" alias, it's too ambiguous?
> + [JITED_SIZE] = { "Jited size", {"jited_size"}, },
this probably should be prog_size_jited or something like that (I
know, verbose, but unambiguous)
> + [STACK] = {"Stack depth", {"stack_depth", "stack"}, },
> + [PROG_TYPE] = { "Program type", {"program_type", "prog_type"}, },
let's drop "program_type", verbose
> + [ATTACH_TYPE] = { "Attach type", {"attach_type", }, },
> };
>
> static bool parse_stat_id_var(const char *name, size_t len, int *id,
> @@ -853,13 +867,16 @@ static int parse_verif_log(char * const buf, size_t buf_sz, struct verif_stats *
>
> if (1 == sscanf(cur, "verification time %ld usec\n", &s->stats[DURATION]))
> continue;
> - if (6 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
> + if (5 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
is this a preexisting bug? why we didn't catch it before?
> &s->stats[TOTAL_INSNS],
> &s->stats[MAX_STATES_PER_INSN],
> &s->stats[TOTAL_STATES],
> &s->stats[PEAK_STATES],
> &s->stats[MARK_READ_MAX_LEN]))
> continue;
> +
> + if (1 == sscanf(cur, "stack depth %ld", &s->stats[STACK]))
heh, not so simple, actually. stack depth is actually a list of stack
sizes for main program and each subprogram. Try
sudo ./veristat test_subprogs.bpf.o -v
stack depth 8+8+0+0+8+0
so we have to make some choices here, actually... we either parse that
and add up, and/or we parse all that and associate it with individual
subprograms.
I think we can start with the former, but the latter is actually
useful and quite tricky for humans to figure out because that order
depends on libbpf-controlled order of subprograms (which veristat can
get from btf_ext, I believe). Not sure if we need/want to record
by-subprog breakdown into CSV, but it would be useful to have a more
detailed breakdown by subprog in some verbose mode. Let's think about
that.
> + continue;
> }
>
> return 0;
> @@ -1146,8 +1163,11 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
> char *buf;
> int buf_sz, log_level;
> struct verif_stats *stats;
> + struct bpf_prog_info info = {};
this should be initialized with memset(0)
> + __u32 info_len = sizeof(info);
> int err = 0;
> void *tmp;
> + int fd;
>
> if (!should_process_file_prog(base_filename, bpf_program__name(prog))) {
> env.progs_skipped++;
> @@ -1196,6 +1216,13 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
> stats->file_name = strdup(base_filename);
> stats->prog_name = strdup(bpf_program__name(prog));
> stats->stats[VERDICT] = err == 0; /* 1 - success, 0 - failure */
> + stats->stats[SIZE] = bpf_program__insn_cnt(prog);
> + stats->stats[PROG_TYPE] = bpf_program__type(prog);
> + stats->stats[ATTACH_TYPE] = bpf_program__expected_attach_type(prog);
> + fd = bpf_program__fd(prog);
> + if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0)
> + stats->stats[JITED_SIZE] = info.jited_prog_len;
> +
please check that this is total length including all the subprogs
> parse_verif_log(buf, buf_sz, stats);
>
> if (env.verbose) {
> @@ -1309,6 +1336,11 @@ static int cmp_stat(const struct verif_stats *s1, const struct verif_stats *s2,
> case PROG_NAME:
> cmp = strcmp(s1->prog_name, s2->prog_name);
> break;
> + case ATTACH_TYPE:
> + case PROG_TYPE:
> + case SIZE:
> + case JITED_SIZE:
> + case STACK:
> case VERDICT:
> case DURATION:
> case TOTAL_INSNS:
> @@ -1523,12 +1555,21 @@ static void prepare_value(const struct verif_stats *s, enum stat_id id,
> else
> *str = s->stats[VERDICT] ? "success" : "failure";
> break;
> + case ATTACH_TYPE:
> + *str = s ? libbpf_bpf_attach_type_str(s->stats[ATTACH_TYPE]) ? : "N/A" : "N/A";
> + break;
> + case PROG_TYPE:
> + *str = s ? libbpf_bpf_prog_type_str(s->stats[PROG_TYPE]) ? : "N/A" : "N/A";
let's not have x ? y ? z pattern, please do explicit outer if like we
do for VERDICT
pw-bot: cr
> + break;
> case DURATION:
> case TOTAL_INSNS:
> case TOTAL_STATES:
> case PEAK_STATES:
> case MAX_STATES_PER_INSN:
> case MARK_READ_MAX_LEN:
> + case STACK:
> + case SIZE:
> + case JITED_SIZE:
> *val = s ? s->stats[id] : 0;
> break;
> default:
> --
> 2.47.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add more stats into veristat
2024-12-05 23:50 ` Andrii Nakryiko
@ 2024-12-06 9:54 ` Mykyta Yatsenko
2024-12-06 13:29 ` Mykyta Yatsenko
1 sibling, 0 replies; 4+ messages in thread
From: Mykyta Yatsenko @ 2024-12-06 9:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, ast, andrii, daniel, kafai, kernel-team, Mykyta Yatsenko
On 05/12/2024 23:50, Andrii Nakryiko wrote:
> On Thu, Dec 5, 2024 at 11:34 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> Extend veristat to collect and print more stats, namely:
>> - program size in instructions
>> - jited program size
>> - program type
>> - attach type
>> - stack depth
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>> tools/testing/selftests/bpf/veristat.c | 51 +++++++++++++++++++++++---
>> 1 file changed, 46 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
>> index e12ef953fba8..0d7fb00175e8 100644
>> --- a/tools/testing/selftests/bpf/veristat.c
>> +++ b/tools/testing/selftests/bpf/veristat.c
>> @@ -38,8 +38,14 @@ enum stat_id {
>> FILE_NAME,
>> PROG_NAME,
>>
>> + SIZE,
>> + JITED_SIZE,
>> + STACK,
>> + PROG_TYPE,
>> + ATTACH_TYPE,
>> +
>> ALL_STATS_CNT,
>> - NUM_STATS_CNT = FILE_NAME - VERDICT,
>> + NUM_STATS_CNT = ATTACH_TYPE - VERDICT + 1,
> this doesn't sound right, because PROG_NAME isn't a number statistics
I did not realize NUM_STATS_CNT means count of number statistics, now
this makes sense, thanks.
>> };
>>
>> /* In comparison mode each stat can specify up to four different values:
>> @@ -640,19 +646,22 @@ static int append_filter_file(const char *path)
>> }
>>
>> static const struct stat_specs default_output_spec = {
>> - .spec_cnt = 7,
>> + .spec_cnt = 12,
>> .ids = {
>> FILE_NAME, PROG_NAME, VERDICT, DURATION,
>> - TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
>> + TOTAL_INSNS, TOTAL_STATES, PEAK_STATES, SIZE,
>> + JITED_SIZE, PROG_TYPE, ATTACH_TYPE, STACK,
> I think SIZE or JITED_SIZE might be good candidates for default view,
> but not all of the above. I think we can also drop PEAK_STATES from
> default, btw.
>
>> },
>> };
>>
>> static const struct stat_specs default_csv_output_spec = {
>> - .spec_cnt = 9,
>> + .spec_cnt = 14,
>> .ids = {
>> FILE_NAME, PROG_NAME, VERDICT, DURATION,
>> TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
>> MAX_STATES_PER_INSN, MARK_READ_MAX_LEN,
>> + SIZE, JITED_SIZE, PROG_TYPE, ATTACH_TYPE,
>> + STACK,
> this is fine, we want everything in CSV, yep
>
>> },
>> };
>>
>> @@ -688,6 +697,11 @@ static struct stat_def {
>> [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"}, },
>> + [SIZE] = { "Prog size", {"prog_size", "size"}, },
> drop "size" alias, it's too ambiguous?
>
>> + [JITED_SIZE] = { "Jited size", {"jited_size"}, },
> this probably should be prog_size_jited or something like that (I
> know, verbose, but unambiguous)
>
>> + [STACK] = {"Stack depth", {"stack_depth", "stack"}, },
>> + [PROG_TYPE] = { "Program type", {"program_type", "prog_type"}, },
> let's drop "program_type", verbose
>
>> + [ATTACH_TYPE] = { "Attach type", {"attach_type", }, },
>> };
>>
>> static bool parse_stat_id_var(const char *name, size_t len, int *id,
>> @@ -853,13 +867,16 @@ static int parse_verif_log(char * const buf, size_t buf_sz, struct verif_stats *
>>
>> if (1 == sscanf(cur, "verification time %ld usec\n", &s->stats[DURATION]))
>> continue;
>> - if (6 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
>> + if (5 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
> is this a preexisting bug? why we didn't catch it before?
Nothing is broken because of this, sscanf sets all 5 variables either
way. Currently we continue ongoing iteration of the loop, instead of jumping
to the next immediately. We can drop these checks at all, it's not going
to change correctness of this code.
>> &s->stats[TOTAL_INSNS],
>> &s->stats[MAX_STATES_PER_INSN],
>> &s->stats[TOTAL_STATES],
>> &s->stats[PEAK_STATES],
>> &s->stats[MARK_READ_MAX_LEN]))
>> continue;
>> +
>> + if (1 == sscanf(cur, "stack depth %ld", &s->stats[STACK]))
> heh, not so simple, actually. stack depth is actually a list of stack
> sizes for main program and each subprogram. Try
>
> sudo ./veristat test_subprogs.bpf.o -v
>
> stack depth 8+8+0+0+8+0
>
> so we have to make some choices here, actually... we either parse that
> and add up, and/or we parse all that and associate it with individual
> subprograms.
>
> I think we can start with the former, but the latter is actually
> useful and quite tricky for humans to figure out because that order
> depends on libbpf-controlled order of subprograms (which veristat can
> get from btf_ext, I believe). Not sure if we need/want to record
> by-subprog breakdown into CSV, but it would be useful to have a more
> detailed breakdown by subprog in some verbose mode. Let's think about
> that.
>
>> + continue;
>> }
>>
>> return 0;
>> @@ -1146,8 +1163,11 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>> char *buf;
>> int buf_sz, log_level;
>> struct verif_stats *stats;
>> + struct bpf_prog_info info = {};
> this should be initialized with memset(0)
>
>> + __u32 info_len = sizeof(info);
>> int err = 0;
>> void *tmp;
>> + int fd;
>>
>> if (!should_process_file_prog(base_filename, bpf_program__name(prog))) {
>> env.progs_skipped++;
>> @@ -1196,6 +1216,13 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>> stats->file_name = strdup(base_filename);
>> stats->prog_name = strdup(bpf_program__name(prog));
>> stats->stats[VERDICT] = err == 0; /* 1 - success, 0 - failure */
>> + stats->stats[SIZE] = bpf_program__insn_cnt(prog);
>> + stats->stats[PROG_TYPE] = bpf_program__type(prog);
>> + stats->stats[ATTACH_TYPE] = bpf_program__expected_attach_type(prog);
>> + fd = bpf_program__fd(prog);
>> + if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0)
>> + stats->stats[JITED_SIZE] = info.jited_prog_len;
>> +
> please check that this is total length including all the subprogs
>
>> parse_verif_log(buf, buf_sz, stats);
>>
>> if (env.verbose) {
>> @@ -1309,6 +1336,11 @@ static int cmp_stat(const struct verif_stats *s1, const struct verif_stats *s2,
>> case PROG_NAME:
>> cmp = strcmp(s1->prog_name, s2->prog_name);
>> break;
>> + case ATTACH_TYPE:
>> + case PROG_TYPE:
>> + case SIZE:
>> + case JITED_SIZE:
>> + case STACK:
>> case VERDICT:
>> case DURATION:
>> case TOTAL_INSNS:
>> @@ -1523,12 +1555,21 @@ static void prepare_value(const struct verif_stats *s, enum stat_id id,
>> else
>> *str = s->stats[VERDICT] ? "success" : "failure";
>> break;
>> + case ATTACH_TYPE:
>> + *str = s ? libbpf_bpf_attach_type_str(s->stats[ATTACH_TYPE]) ? : "N/A" : "N/A";
>> + break;
>> + case PROG_TYPE:
>> + *str = s ? libbpf_bpf_prog_type_str(s->stats[PROG_TYPE]) ? : "N/A" : "N/A";
> let's not have x ? y ? z pattern, please do explicit outer if like we
> do for VERDICT
>
> pw-bot: cr
>
>> + break;
>> case DURATION:
>> case TOTAL_INSNS:
>> case TOTAL_STATES:
>> case PEAK_STATES:
>> case MAX_STATES_PER_INSN:
>> case MARK_READ_MAX_LEN:
>> + case STACK:
>> + case SIZE:
>> + case JITED_SIZE:
>> *val = s ? s->stats[id] : 0;
>> break;
>> default:
>> --
>> 2.47.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: add more stats into veristat
2024-12-05 23:50 ` Andrii Nakryiko
2024-12-06 9:54 ` Mykyta Yatsenko
@ 2024-12-06 13:29 ` Mykyta Yatsenko
1 sibling, 0 replies; 4+ messages in thread
From: Mykyta Yatsenko @ 2024-12-06 13:29 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, ast, andrii, daniel, kafai, kernel-team, Mykyta Yatsenko
On 05/12/2024 23:50, Andrii Nakryiko wrote:
> On Thu, Dec 5, 2024 at 11:34 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> Extend veristat to collect and print more stats, namely:
>> - program size in instructions
>> - jited program size
>> - program type
>> - attach type
>> - stack depth
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>> tools/testing/selftests/bpf/veristat.c | 51 +++++++++++++++++++++++---
>> 1 file changed, 46 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
>> index e12ef953fba8..0d7fb00175e8 100644
>> --- a/tools/testing/selftests/bpf/veristat.c
>> +++ b/tools/testing/selftests/bpf/veristat.c
>> @@ -38,8 +38,14 @@ enum stat_id {
>> FILE_NAME,
>> PROG_NAME,
>>
>> + SIZE,
>> + JITED_SIZE,
>> + STACK,
>> + PROG_TYPE,
>> + ATTACH_TYPE,
>> +
>> ALL_STATS_CNT,
>> - NUM_STATS_CNT = FILE_NAME - VERDICT,
>> + NUM_STATS_CNT = ATTACH_TYPE - VERDICT + 1,
> this doesn't sound right, because PROG_NAME isn't a number statistics
>
>> };
>>
>> /* In comparison mode each stat can specify up to four different values:
>> @@ -640,19 +646,22 @@ static int append_filter_file(const char *path)
>> }
>>
>> static const struct stat_specs default_output_spec = {
>> - .spec_cnt = 7,
>> + .spec_cnt = 12,
>> .ids = {
>> FILE_NAME, PROG_NAME, VERDICT, DURATION,
>> - TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
>> + TOTAL_INSNS, TOTAL_STATES, PEAK_STATES, SIZE,
>> + JITED_SIZE, PROG_TYPE, ATTACH_TYPE, STACK,
> I think SIZE or JITED_SIZE might be good candidates for default view,
> but not all of the above. I think we can also drop PEAK_STATES from
> default, btw.
>
>> },
>> };
>>
>> static const struct stat_specs default_csv_output_spec = {
>> - .spec_cnt = 9,
>> + .spec_cnt = 14,
>> .ids = {
>> FILE_NAME, PROG_NAME, VERDICT, DURATION,
>> TOTAL_INSNS, TOTAL_STATES, PEAK_STATES,
>> MAX_STATES_PER_INSN, MARK_READ_MAX_LEN,
>> + SIZE, JITED_SIZE, PROG_TYPE, ATTACH_TYPE,
>> + STACK,
> this is fine, we want everything in CSV, yep
>
>> },
>> };
>>
>> @@ -688,6 +697,11 @@ static struct stat_def {
>> [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"}, },
>> + [SIZE] = { "Prog size", {"prog_size", "size"}, },
> drop "size" alias, it's too ambiguous?
>
>> + [JITED_SIZE] = { "Jited size", {"jited_size"}, },
> this probably should be prog_size_jited or something like that (I
> know, verbose, but unambiguous)
>
>> + [STACK] = {"Stack depth", {"stack_depth", "stack"}, },
>> + [PROG_TYPE] = { "Program type", {"program_type", "prog_type"}, },
> let's drop "program_type", verbose
>
>> + [ATTACH_TYPE] = { "Attach type", {"attach_type", }, },
>> };
>>
>> static bool parse_stat_id_var(const char *name, size_t len, int *id,
>> @@ -853,13 +867,16 @@ static int parse_verif_log(char * const buf, size_t buf_sz, struct verif_stats *
>>
>> if (1 == sscanf(cur, "verification time %ld usec\n", &s->stats[DURATION]))
>> continue;
>> - if (6 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
>> + if (5 == sscanf(cur, "processed %ld insns (limit %*d) max_states_per_insn %ld total_states %ld peak_states %ld mark_read %ld",
> is this a preexisting bug? why we didn't catch it before?
>
>> &s->stats[TOTAL_INSNS],
>> &s->stats[MAX_STATES_PER_INSN],
>> &s->stats[TOTAL_STATES],
>> &s->stats[PEAK_STATES],
>> &s->stats[MARK_READ_MAX_LEN]))
>> continue;
>> +
>> + if (1 == sscanf(cur, "stack depth %ld", &s->stats[STACK]))
> heh, not so simple, actually. stack depth is actually a list of stack
> sizes for main program and each subprogram. Try
>
> sudo ./veristat test_subprogs.bpf.o -v
>
> stack depth 8+8+0+0+8+0
>
> so we have to make some choices here, actually... we either parse that
> and add up, and/or we parse all that and associate it with individual
> subprograms.
>
> I think we can start with the former, but the latter is actually
> useful and quite tricky for humans to figure out because that order
> depends on libbpf-controlled order of subprograms (which veristat can
> get from btf_ext, I believe). Not sure if we need/want to record
> by-subprog breakdown into CSV, but it would be useful to have a more
> detailed breakdown by subprog in some verbose mode. Let's think about
> that.
>
>> + continue;
>> }
>>
>> return 0;
>> @@ -1146,8 +1163,11 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>> char *buf;
>> int buf_sz, log_level;
>> struct verif_stats *stats;
>> + struct bpf_prog_info info = {};
> this should be initialized with memset(0)
>
>> + __u32 info_len = sizeof(info);
>> int err = 0;
>> void *tmp;
>> + int fd;
>>
>> if (!should_process_file_prog(base_filename, bpf_program__name(prog))) {
>> env.progs_skipped++;
>> @@ -1196,6 +1216,13 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>> stats->file_name = strdup(base_filename);
>> stats->prog_name = strdup(bpf_program__name(prog));
>> stats->stats[VERDICT] = err == 0; /* 1 - success, 0 - failure */
>> + stats->stats[SIZE] = bpf_program__insn_cnt(prog);
>> + stats->stats[PROG_TYPE] = bpf_program__type(prog);
>> + stats->stats[ATTACH_TYPE] = bpf_program__expected_attach_type(prog);
>> + fd = bpf_program__fd(prog);
>> + if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0)
>> + stats->stats[JITED_SIZE] = info.jited_prog_len;
>> +
> please check that this is total length including all the subprogs
I think it does include subprogs. I checked this by loading program:
`bpftool prog load test_subprogs.bpf.o /sys/fs/bpf/test_subprogs`
then print its jited code:
`bpftool prog dump jited name prog1 test_subprogs.bpf.o`
summed sizes across all functions and compared with veristat output:
```
File Program Verdict Duration (us) Insns States Prog
size Jited size
------------------- ------- ------- ------------- ----- ------
--------- ----------
test_subprogs.bpf.o prog1 success 181 56 6
54 333
```
>> parse_verif_log(buf, buf_sz, stats);
>>
>> if (env.verbose) {
>> @@ -1309,6 +1336,11 @@ static int cmp_stat(const struct verif_stats *s1, const struct verif_stats *s2,
>> case PROG_NAME:
>> cmp = strcmp(s1->prog_name, s2->prog_name);
>> break;
>> + case ATTACH_TYPE:
>> + case PROG_TYPE:
>> + case SIZE:
>> + case JITED_SIZE:
>> + case STACK:
>> case VERDICT:
>> case DURATION:
>> case TOTAL_INSNS:
>> @@ -1523,12 +1555,21 @@ static void prepare_value(const struct verif_stats *s, enum stat_id id,
>> else
>> *str = s->stats[VERDICT] ? "success" : "failure";
>> break;
>> + case ATTACH_TYPE:
>> + *str = s ? libbpf_bpf_attach_type_str(s->stats[ATTACH_TYPE]) ? : "N/A" : "N/A";
>> + break;
>> + case PROG_TYPE:
>> + *str = s ? libbpf_bpf_prog_type_str(s->stats[PROG_TYPE]) ? : "N/A" : "N/A";
> let's not have x ? y ? z pattern, please do explicit outer if like we
> do for VERDICT
>
> pw-bot: cr
>
>> + break;
>> case DURATION:
>> case TOTAL_INSNS:
>> case TOTAL_STATES:
>> case PEAK_STATES:
>> case MAX_STATES_PER_INSN:
>> case MARK_READ_MAX_LEN:
>> + case STACK:
>> + case SIZE:
>> + case JITED_SIZE:
>> *val = s ? s->stats[id] : 0;
>> break;
>> default:
>> --
>> 2.47.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-12-06 13:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-05 19:34 [PATCH bpf-next] selftests/bpf: add more stats into veristat Mykyta Yatsenko
2024-12-05 23:50 ` Andrii Nakryiko
2024-12-06 9:54 ` Mykyta Yatsenko
2024-12-06 13:29 ` Mykyta Yatsenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox