Re: [PATCH bpf-next] selftests/bpf: add more stats into veristat

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

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
>>