[PATCH bpf-next v1 0/3] veristat: @files-list.txt notation for object files list

[PATCH bpf-next v1 0/3] veristat: @files-list.txt notation for object files list

[Eduard Zingerman]

A few small veristat improvements:
- It is possible to hit command line parameters number limit,
  e.g. when running veristat for all object files generated for
  test_progs. This patch-set adds an option to read objects files list
  from a file.
- Correct usage of strerror() function.
- Avoid printing log lines to CSV output.

Eduard Zingerman (3):
  veristat: @files-list.txt notation for object files list
  veristat: strerror expects positive number (errno)
  veristat: report program type guess results to sdterr

 tools/testing/selftests/bpf/veristat.c | 70 +++++++++++++++++++++-----
 1 file changed, 57 insertions(+), 13 deletions(-)

-- 
2.48.1

[PATCH bpf-next v1 1/3] veristat: @files-list.txt notation for object files list

[Eduard Zingerman]

Allow reading object file list from file.
E.g. the following command:

  ./veristat @list.txt

Is equivalent to the following invocation:

  ./veristat line-1 line-2 ... line-N

Where line-i corresponds to lines from list.txt.
Lines starting with '#' are ignored.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/veristat.c | 62 ++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 175a03e6c5ef..8bc462299290 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -268,10 +268,11 @@ static int append_filter(struct filter **filters, int *cnt, const char *str);
 static int append_filter_file(const char *path);
 static int append_var_preset(struct var_preset **presets, int *cnt, const char *expr);
 static int append_var_preset_file(const char *filename);
+static int append_file(const char *path);
+static int append_file_from_file(const char *path);
 
 static error_t parse_arg(int key, char *arg, struct argp_state *state)
 {
-	void *tmp;
 	int err;
 
 	switch (key) {
@@ -381,14 +382,14 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 		break;
 	}
 	case ARGP_KEY_ARG:
-		tmp = realloc(env.filenames, (env.filename_cnt + 1) * sizeof(*env.filenames));
-		if (!tmp)
-			return -ENOMEM;
-		env.filenames = tmp;
-		env.filenames[env.filename_cnt] = strdup(arg);
-		if (!env.filenames[env.filename_cnt])
-			return -ENOMEM;
-		env.filename_cnt++;
+		if (arg[0] == '@')
+			err = append_file_from_file(arg + 1);
+		else
+			err = append_file(arg);
+		if (err) {
+			fprintf(stderr, "Failed to collect BPF object files: %d\n", err);
+			return err;
+		}
 		break;
 	default:
 		return ARGP_ERR_UNKNOWN;
@@ -689,6 +690,49 @@ static const struct stat_specs default_output_spec = {
 	},
 };
 
+static int append_file(const char *path)
+{
+	void *tmp;
+
+	tmp = realloc(env.filenames, (env.filename_cnt + 1) * sizeof(*env.filenames));
+	if (!tmp)
+		return -ENOMEM;
+	env.filenames = tmp;
+	env.filenames[env.filename_cnt] = strdup(path);
+	if (!env.filenames[env.filename_cnt])
+		return -ENOMEM;
+	env.filename_cnt++;
+	return 0;
+}
+
+static int append_file_from_file(const char *path)
+{
+	char buf[1024];
+	int err = 0;
+	FILE *f;
+
+	f = fopen(path, "r");
+	if (!f) {
+		err = -errno;
+		fprintf(stderr, "Failed to open object files list in '%s': %s\n",
+			path, strerror(errno));
+		return err;
+	}
+
+	while (fscanf(f, " %1023[^\n]\n", buf) == 1) {
+		/* lines starting with # are comments, skip them */
+		if (buf[0] == '\0' || buf[0] == '#')
+			continue;
+		err = append_file(buf);
+		if (err)
+			goto cleanup;
+	}
+
+cleanup:
+	fclose(f);
+	return err;
+}
+
 static const struct stat_specs default_csv_output_spec = {
 	.spec_cnt = 14,
 	.ids = {
-- 
2.48.1

[PATCH bpf-next v1 2/3] veristat: strerror expects positive number (errno)

[Eduard Zingerman]

Before:

  ./veristat -G @foobar iters.bpf.o
  Failed to open presets in 'foobar': Unknown error -2
  ...

After:

  ./veristat -G @foobar iters.bpf.o
  Failed to open presets in 'foobar': No such file or directory
  ...

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/veristat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 8bc462299290..7d13b9234d2c 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -660,7 +660,7 @@ static int append_filter_file(const char *path)
 	f = fopen(path, "r");
 	if (!f) {
 		err = -errno;
-		fprintf(stderr, "Failed to open filters in '%s': %s\n", path, strerror(err));
+		fprintf(stderr, "Failed to open filters in '%s': %s\n", path, strerror(errno));
 		return err;
 	}
 
@@ -1422,7 +1422,7 @@ static int append_var_preset_file(const char *filename)
 	f = fopen(filename, "rt");
 	if (!f) {
 		err = -errno;
-		fprintf(stderr, "Failed to open presets in '%s': %s\n", filename, strerror(err));
+		fprintf(stderr, "Failed to open presets in '%s': %s\n", filename, strerror(errno));
 		return -EINVAL;
 	}
 
-- 
2.48.1

Re: [PATCH bpf-next v1 2/3] veristat: strerror expects positive number (errno)

[Andrii Nakryiko]

On Fri, Feb 28, 2025 at 11:13 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Before:
>
>   ./veristat -G @foobar iters.bpf.o
>   Failed to open presets in 'foobar': Unknown error -2
>   ...
>
> After:
>
>   ./veristat -G @foobar iters.bpf.o
>   Failed to open presets in 'foobar': No such file or directory
>   ...
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/testing/selftests/bpf/veristat.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 8bc462299290..7d13b9234d2c 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -660,7 +660,7 @@ static int append_filter_file(const char *path)
>         f = fopen(path, "r");
>         if (!f) {
>                 err = -errno;
> -               fprintf(stderr, "Failed to open filters in '%s': %s\n", path, strerror(err));
> +               fprintf(stderr, "Failed to open filters in '%s': %s\n", path, strerror(errno));

errno is fragile, -err would be more robust, IMO

>                 return err;
>         }
>
> @@ -1422,7 +1422,7 @@ static int append_var_preset_file(const char *filename)
>         f = fopen(filename, "rt");
>         if (!f) {
>                 err = -errno;
> -               fprintf(stderr, "Failed to open presets in '%s': %s\n", filename, strerror(err));
> +               fprintf(stderr, "Failed to open presets in '%s': %s\n", filename, strerror(errno));
>                 return -EINVAL;
>         }
>
> --
> 2.48.1
>

Re: [PATCH bpf-next v1 2/3] veristat: strerror expects positive number (errno)

[Eduard Zingerman]

On Fri, 2025-02-28 at 14:44 -0800, Andrii Nakryiko wrote:

[...]

> > diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> > index 8bc462299290..7d13b9234d2c 100644
> > --- a/tools/testing/selftests/bpf/veristat.c
> > +++ b/tools/testing/selftests/bpf/veristat.c
> > @@ -660,7 +660,7 @@ static int append_filter_file(const char *path)
> >         f = fopen(path, "r");
> >         if (!f) {
> >                 err = -errno;
> > -               fprintf(stderr, "Failed to open filters in '%s': %s\n", path, strerror(err));
> > +               fprintf(stderr, "Failed to open filters in '%s': %s\n", path, strerror(errno));
> 
> errno is fragile, -err would be more robust, IMO

Sure, I'll send v2.

[...]

[PATCH bpf-next v1 3/3] veristat: report program type guess results to sdterr

[Eduard Zingerman]

In order not to pollute CSV output, e.g.:

  $ ./veristat -o csv exceptions_ext.bpf.o > test.csv
  Using guessed program type 'sched_cls' for exceptions_ext.bpf.o/extension...
  Using guessed program type 'sched_cls' for exceptions_ext.bpf.o/throwing_extension...

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/veristat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 7d13b9234d2c..3a9cfc2bfae5 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -1234,13 +1234,13 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch
 			bpf_program__set_expected_attach_type(prog, attach_type);
 
 			if (!env.quiet) {
-				printf("Using guessed program type '%s' for %s/%s...\n",
+				fprintf(stderr, "Using guessed program type '%s' for %s/%s...\n",
 					libbpf_bpf_prog_type_str(prog_type),
 					filename, prog_name);
 			}
 		} else {
 			if (!env.quiet) {
-				printf("Failed to guess program type for freplace program with context type name '%s' for %s/%s. Consider using canonical type names to help veristat...\n",
+				fprintf(stderr, "Failed to guess program type for freplace program with context type name '%s' for %s/%s. Consider using canonical type names to help veristat...\n",
 					ctx_name, filename, prog_name);
 			}
 		}
-- 
2.48.1

Re: [PATCH bpf-next v1 0/3] veristat: @files-list.txt notation for object files list

[Andrii Nakryiko]

On Fri, Feb 28, 2025 at 11:13 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> A few small veristat improvements:
> - It is possible to hit command line parameters number limit,
>   e.g. when running veristat for all object files generated for
>   test_progs. This patch-set adds an option to read objects files list
>   from a file.
> - Correct usage of strerror() function.
> - Avoid printing log lines to CSV output.
>

All makes sense, and superficially LGTM, but I'd like Mykyta to take a
look when he gets a chance, as he's been working with veristat quite a
lot recently.

One thing I wanted to propose/ask. Do you think it would be useful to
allow <object>:<program> pattern to be specified to allow picking just
one program out of the object file? I normally do `veristat <object>
-f<program>` for this, but being able to do `veristat <obj1>:<prog1>
<obj2>:<prog2> ...` seems useful, no? (-f<program> would apply to all
objects, btw, which isn't a big problem in practice, but still). Oh,
and we could allow globbing in `veristat <obj>:<blah*>`.

Thoughts?

> Eduard Zingerman (3):
>   veristat: @files-list.txt notation for object files list
>   veristat: strerror expects positive number (errno)
>   veristat: report program type guess results to sdterr
>
>  tools/testing/selftests/bpf/veristat.c | 70 +++++++++++++++++++++-----
>  1 file changed, 57 insertions(+), 13 deletions(-)
>
> --
> 2.48.1
>

Re: [PATCH bpf-next v1 0/3] veristat: @files-list.txt notation for object files list

[Eduard Zingerman]

On Fri, 2025-02-28 at 14:47 -0800, Andrii Nakryiko wrote:
> On Fri, Feb 28, 2025 at 11:13 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > A few small veristat improvements:
> > - It is possible to hit command line parameters number limit,
> >   e.g. when running veristat for all object files generated for
> >   test_progs. This patch-set adds an option to read objects files list
> >   from a file.
> > - Correct usage of strerror() function.
> > - Avoid printing log lines to CSV output.
> > 
> 
> All makes sense, and superficially LGTM, but I'd like Mykyta to take a
> look when he gets a chance, as he's been working with veristat quite a
> lot recently.

Thanks. I'll wait for Mykytas comments before sending v2 with -err.

> One thing I wanted to propose/ask. Do you think it would be useful to
> allow <object>:<program> pattern to be specified to allow picking just
> one program out of the object file? I normally do `veristat <object>
> -f<program>` for this, but being able to do `veristat <obj1>:<prog1>
> <obj2>:<prog2> ...` seems useful, no? (-f<program> would apply to all
> objects, btw, which isn't a big problem in practice, but still). Oh,
> and we could allow globbing in `veristat <obj>:<blah*>`.
> 
> Thoughts?

Tbh I don't remember myself ever needing this, -f was sufficient.
Every time I used -f, it was to do <object>:<program> for a single program.
On the other hand, this looks like a nice generalization.
This does not seem to be too complicated, so I'd say lets add it,
the use case will find us eventually.

One thing I do want is multi-threading.
E.g. it takes about 2 minutes to process all .bpf.o from
selftests/bpf/cpuv4/, and it can be slashed to 10s of seconds.
Per-object this should be straightforward.
Per-program this would need to wait for Mykyta's work on prepare object,
as far as I understand.
I can add the per-object version over the weekend if you are ok with
such granularity.

Re: [PATCH bpf-next v1 0/3] veristat: @files-list.txt notation for object files list

[Andrii Nakryiko]

On Fri, Feb 28, 2025 at 3:00 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2025-02-28 at 14:47 -0800, Andrii Nakryiko wrote:
> > On Fri, Feb 28, 2025 at 11:13 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > A few small veristat improvements:
> > > - It is possible to hit command line parameters number limit,
> > >   e.g. when running veristat for all object files generated for
> > >   test_progs. This patch-set adds an option to read objects files list
> > >   from a file.
> > > - Correct usage of strerror() function.
> > > - Avoid printing log lines to CSV output.
> > >
> >
> > All makes sense, and superficially LGTM, but I'd like Mykyta to take a
> > look when he gets a chance, as he's been working with veristat quite a
> > lot recently.
>
> Thanks. I'll wait for Mykytas comments before sending v2 with -err.
>
> > One thing I wanted to propose/ask. Do you think it would be useful to
> > allow <object>:<program> pattern to be specified to allow picking just
> > one program out of the object file? I normally do `veristat <object>
> > -f<program>` for this, but being able to do `veristat <obj1>:<prog1>
> > <obj2>:<prog2> ...` seems useful, no? (-f<program> would apply to all
> > objects, btw, which isn't a big problem in practice, but still). Oh,
> > and we could allow globbing in `veristat <obj>:<blah*>`.
> >
> > Thoughts?
>
> Tbh I don't remember myself ever needing this, -f was sufficient.
> Every time I used -f, it was to do <object>:<program> for a single program.
> On the other hand, this looks like a nice generalization.
> This does not seem to be too complicated, so I'd say lets add it,
> the use case will find us eventually.
>
> One thing I do want is multi-threading.
> E.g. it takes about 2 minutes to process all .bpf.o from
> selftests/bpf/cpuv4/, and it can be slashed to 10s of seconds.
> Per-object this should be straightforward.
> Per-program this would need to wait for Mykyta's work on prepare object,
> as far as I understand.
> I can add the per-object version over the weekend if you are ok with
> such granularity.
>

Yeah, I was hoping for bpf_object__prepare() to be used by veristat to
speed up mass-processing. I think it's fine to parallelize, but this
will be awkward with verbose/error logging, so think how you'll handle
that, ok? Ideally we'll parallelize at program level, so you can start
with per-object parallelism, but just anticipate that it will actually
be prepared object + bpf_program eventually. Just write it in the way
that would accommodate that easily, once we have all the pieces in
libbpf.

Re: [PATCH bpf-next v1 0/3] veristat: @files-list.txt notation for object files list

[Mykyta Yatsenko]

On 28/02/2025 19:12, Eduard Zingerman wrote:
> A few small veristat improvements:
> - It is possible to hit command line parameters number limit,
>    e.g. when running veristat for all object files generated for
>    test_progs. This patch-set adds an option to read objects files list
>    from a file.
> - Correct usage of strerror() function.
> - Avoid printing log lines to CSV output.
>
> Eduard Zingerman (3):
>    veristat: @files-list.txt notation for object files list
>    veristat: strerror expects positive number (errno)
>    veristat: report program type guess results to sdterr
>
>   tools/testing/selftests/bpf/veristat.c | 70 +++++++++++++++++++++-----
>   1 file changed, 57 insertions(+), 13 deletions(-)
Patch set looks good to me.
Acked-by: Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>

Re: [PATCH bpf-next v1 0/3] veristat: @files-list.txt notation for object files list

[Eduard Zingerman]

On Fri, 2025-02-28 at 23:33 +0000, Mykyta Yatsenko wrote:
> On 28/02/2025 19:12, Eduard Zingerman wrote:
> > A few small veristat improvements:
> > - It is possible to hit command line parameters number limit,
> >    e.g. when running veristat for all object files generated for
> >    test_progs. This patch-set adds an option to read objects files list
> >    from a file.
> > - Correct usage of strerror() function.
> > - Avoid printing log lines to CSV output.
> > 
> > Eduard Zingerman (3):
> >    veristat: @files-list.txt notation for object files list
> >    veristat: strerror expects positive number (errno)
> >    veristat: report program type guess results to sdterr
> > 
> >   tools/testing/selftests/bpf/veristat.c | 70 +++++++++++++++++++++-----
> >   1 file changed, 57 insertions(+), 13 deletions(-)
> Patch set looks good to me.
> Acked-by: Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>
> 

Thank you for taking a look!