Re: [PATCH bpf-next v3] selftests/bpf: add BPF program dump in veristat

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 19, 2025 at 4:43 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>
> This patch adds support for dumping BPF program instructions directly
> from veristat.
> While it is already possible to inspect BPF program dump using bpftool,
> it requires multiple commands. During active development, it's common

not just that, veristat will load and unload program very fast, so you
don't have sufficient time to find prog ID and dump it. So it's not
realistically possible today, unless you artificially make veristat
pause.

> for developers to use veristat for testing verification. Integrating
> instruction dumping into veristat reduces the need to switch tools and
> simplifies the workflow.
> By making this information more readily accessible, this change aims
> to streamline the BPF development cycle and improve usability for
> developers.
> This implementation leverages bpftool, by running it directly via popen
> to avoid any code duplication and keep veristat simple.
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> ---
>  tools/testing/selftests/bpf/veristat.c | 34 +++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index d532dd82a3a8..a4ecbc12953e 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -181,6 +181,12 @@ struct var_preset {
>         bool applied;
>  };
>
> +enum dump_mode {
> +       NO_DUMP = 0,
> +       XLATED,
> +       JITED,
> +};

nit: DUMP_NONE, DUMP_XLATED, DUMP_JITED ?


and to think about it, why not support dumping both XLATED and
JITED?... Make this a set of bits?

> +
>  static struct env {
>         char **filenames;
>         int filename_cnt;
> @@ -227,6 +233,7 @@ static struct env {
>         char orig_cgroup[PATH_MAX];
>         char stat_cgroup[PATH_MAX];
>         int memory_peak_fd;
> +       enum dump_mode dump_mode;
>  } env;
>
>  static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va_list args)
> @@ -295,6 +302,7 @@ static const struct argp_option opts[] = {
>           "Force BPF verifier failure on register invariant violation (BPF_F_TEST_REG_INVARIANTS program flag)" },
>         { "top-src-lines", 'S', "N", 0, "Emit N most frequent source code lines" },
>         { "set-global-vars", 'G', "GLOBAL", 0, "Set global variables provided in the expression, for example \"var1 = 1\"" },
> +       { "dump", 'p', "DUMP_MODE", 0, "Print BPF program dump (xlated, jited)" },

"dump" and "p" don't have an obvious connection... let's just have
long-form --dump=xlated for now (and I'd default to --dump meaning
--dump=xlated to save typing for common case)?

>         {},
>  };
>
> @@ -427,6 +435,16 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>                         return err;
>                 }
>                 break;
> +       case 'p':
> +               if (strcmp(arg, "jited") == 0) {
> +                       env.dump_mode = JITED;
> +               } else if (strcmp(arg, "xlated") == 0) {

nit: make case-insensitive?

> +                       env.dump_mode = XLATED;
> +               } else {
> +                       fprintf(stderr, "Unrecognized dump mode '%s'\n", arg);
> +                       return -EINVAL;
> +               }
> +               break;
>         default:
>                 return ARGP_ERR_UNKNOWN;
>         }
> @@ -1554,6 +1572,17 @@ static int parse_rvalue(const char *val, struct rvalue *rvalue)
>         return 0;
>  }
>
> +static void dump(__u32 prog_id, const char *prog_name)
> +{
> +       char command[64];
> +
> +       snprintf(command, sizeof(command), "bpftool prog dump %s id %u",
> +                env.dump_mode == JITED ? "jited" : "xlated", prog_id);
> +       printf("Prog's '%s' dump:\n", prog_name);

let's make it a bit more apparent and follow PROCESSING styling:

<file>/<program> DUMP (XLATED|JITED):
...

> +       system(command);

Quick googling didn't answer this question, but with system() we make
assumption that system()'s stdout/stderr always goes into veristat's
stdout/stderr. It might be always true, but why rely on this if we can
just popen()? popen() also would allow us to do any processing we
might want to do (unlikely, but it's good to have an option, IMO).

Let's go with popen(), it's not much of a complication.

pw-bot: cr

> +       printf("\n");
> +}
> +
>  static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
>  {
>         const char *base_filename = basename(strdupa(filename));
> @@ -1630,8 +1659,11 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>
>         memset(&info, 0, info_len);
>         fd = bpf_program__fd(prog);
> -       if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0)
> +       if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0) {
>                 stats->stats[JITED_SIZE] = info.jited_prog_len;
> +               if (env.dump_mode != NO_DUMP)
> +                       dump(info.id, prog_name);
> +       }
>
>         parse_verif_log(buf, buf_sz, stats);
>
> --
> 2.50.1
>





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux