Re: [PATCH bpf-next v2 2/2] selftests/bpf: Use bpf_program__clone() in veristat

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

"Emil Tsalapatis" <emil@etsalapatis.com> writes:

> On Fri Feb 20, 2026 at 2:18 PM EST, Mykyta Yatsenko wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> Replace veristat's per-program object re-opening with
>> bpf_program__clone().
>>
>> Previously, veristat opened a separate bpf_object for every program in a
>> multi-program object file, iterated all programs to enable only the
>> target one, and then loaded the entire object.
>>
>> Use bpf_object__prepare() once, then call bpf_program__clone() for each
>> program individually. This lets veristat load programs one at a time
>> from a single prepared object.
>>
>> The caller now owns the returned fd and closes it after collecting stats.
>> Remove the special single-program fast path and the per-file early exit
>> in handle_verif_mode() so all files are always processed.
>>
>> Split fixup_obj() into fixup_obj_maps() for object-wide map fixups that
>> must run before bpf_object__prepare(), and fixup_obj() for per-program
>> fixups (struct_ops masking, freplace type guessing) that run before each
>> bpf_program__clone() call.
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>>  tools/testing/selftests/bpf/veristat.c | 104 ++++++++++++++-------------------
>>  1 file changed, 44 insertions(+), 60 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
>> index 1be1e353d40a..7d7eb56e04d0 100644
>> --- a/tools/testing/selftests/bpf/veristat.c
>> +++ b/tools/testing/selftests/bpf/veristat.c
>> @@ -1236,7 +1236,7 @@ static void mask_unrelated_struct_ops_progs(struct bpf_object *obj,
>>  	}
>>  }
>>  
>> -static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const char *filename)
>> +static void fixup_obj_maps(struct bpf_object *obj)
>>  {
>>  	struct bpf_map *map;
>>  
>> @@ -1251,15 +1251,23 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch
>>  		case BPF_MAP_TYPE_INODE_STORAGE:
>>  		case BPF_MAP_TYPE_CGROUP_STORAGE:
>>  		case BPF_MAP_TYPE_CGRP_STORAGE:
>> -			break;
>>  		case BPF_MAP_TYPE_STRUCT_OPS:
>> -			mask_unrelated_struct_ops_progs(obj, map, prog);
>>  			break;
>>  		default:
>>  			if (bpf_map__max_entries(map) == 0)
>>  				bpf_map__set_max_entries(map, 1);
>>  		}
>>  	}
>> +}
>> +
>> +static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const char *filename)
>> +{
>> +	struct bpf_map *map;
>> +
>> +	bpf_object__for_each_map(map, obj) {
>> +		if (bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS)
>> +			mask_unrelated_struct_ops_progs(obj, map, prog);
>> +	}
>>  
>>  	/* SEC(freplace) programs can't be loaded with veristat as is,
>>  	 * but we can try guessing their target program's expected type by
>> @@ -1608,6 +1616,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>>  	const char *base_filename = basename(strdupa(filename));
>>  	const char *prog_name = bpf_program__name(prog);
>>  	long mem_peak_a, mem_peak_b, mem_peak = -1;
>> +	LIBBPF_OPTS(bpf_prog_load_opts, opts);
>>  	char *buf;
>>  	int buf_sz, log_level;
>>  	struct verif_stats *stats;
>> @@ -1647,9 +1656,6 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>>  	}
>>  	verif_log_buf[0] = '\0';
>>  
>> -	bpf_program__set_log_buf(prog, buf, buf_sz);
>> -	bpf_program__set_log_level(prog, log_level);
>> -
>>  	/* increase chances of successful BPF object loading */
>>  	fixup_obj(obj, prog, base_filename);
>>  
>> @@ -1658,15 +1664,21 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>>  	if (env.force_reg_invariants)
>>  		bpf_program__set_flags(prog, bpf_program__flags(prog) | BPF_F_TEST_REG_INVARIANTS);
>>  
>> -	err = bpf_object__prepare(obj);
>> -	if (!err) {
>> -		cgroup_err = reset_stat_cgroup();
>> -		mem_peak_a = cgroup_memory_peak();
>> -		err = bpf_object__load(obj);
>> -		mem_peak_b = cgroup_memory_peak();
>> -		if (!cgroup_err && mem_peak_a >= 0 && mem_peak_b >= 0)
>> -			mem_peak = mem_peak_b - mem_peak_a;
>> +	opts.log_buf = buf;
>> +	opts.log_size = buf_sz;
>> +	opts.log_level = log_level;
>> +
>> +	cgroup_err = reset_stat_cgroup();
>> +	mem_peak_a = cgroup_memory_peak();
>> +	fd = bpf_program__clone(prog, &opts);
>> +	if (fd < 0) {
>> +		err = fd;
>> +		fprintf(stderr, "Failed to load program %s %d\n", prog_name, err);
>>  	}
>> +	mem_peak_b = cgroup_memory_peak();
>> +	if (!cgroup_err && mem_peak_a >= 0 && mem_peak_b >= 0)
>> +		mem_peak = mem_peak_b - mem_peak_a;
>> +
>
> AFAICT the meaning of mem_peak changes with this patch, right? Before it was the diff between 
> prepare() and load(), with the patch as it is now it is max(prepare,
> load) since mem_peak_a is reset right above. If we do not automatically
> load OBJ_PREPARED programs (see the review for patch 1/2) we can keep
> the old behavior by storing mem_peak_a after the first prepare and
> reading mem_peak_b after clone+load.
>
I'm not sure if I understand why the meaning of mem_peak changed. Before
it was:
   bpf_object__prepare()
   peak_reset()
   a = mem_peak()
   bpf_object__load();
   mem_peak = mem_peak() - a;

and now:
   bpf_object__prepare()
   ...
   peak_reset()
   a = mem_peak()
   bpf_program__clone();
   mem_peak = mem_peak() - a;

I understand this code is supposed to measure the memory overhead for
the loading a single program.

Could you please elaborate what the issue is? Maybe some example?
>>  	env.progs_processed++;
>>  
>>  	stats->file_name = strdup(base_filename);
>> @@ -1678,7 +1690,6 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>>  	stats->stats[MEMORY_PEAK] = mem_peak < 0 ? -1 : mem_peak / (1024 * 1024);
>>  
>>  	memset(&info, 0, info_len);
>> -	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;
>>  		if (env.dump_mode & DUMP_JITED)
>> @@ -1699,7 +1710,8 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
>>  
>>  	if (verif_log_buf != buf)
>>  		free(buf);
>> -
>> +	if (fd > 0)
>> +		close(fd);
>>  	return 0;
>>  }
>>  
>> @@ -2182,8 +2194,8 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
>>  static int process_obj(const char *filename)
>>  {
>>  	const char *base_filename = basename(strdupa(filename));
>> -	struct bpf_object *obj = NULL, *tobj;
>> -	struct bpf_program *prog, *tprog, *lprog;
>> +	struct bpf_object *obj = NULL;
>> +	struct bpf_program *prog;
>>  	libbpf_print_fn_t old_libbpf_print_fn;
>>  	LIBBPF_OPTS(bpf_object_open_opts, opts);
>>  	int err = 0, prog_cnt = 0;
>> @@ -2222,51 +2234,26 @@ static int process_obj(const char *filename)
>>  	env.files_processed++;
>>  
>>  	bpf_object__for_each_program(prog, obj) {
>> +		bpf_program__set_autoload(prog, true);
>>  		prog_cnt++;
>>  	}
>>  
>> -	if (prog_cnt == 1) {
>> -		prog = bpf_object__next_program(obj, NULL);
>> -		bpf_program__set_autoload(prog, true);
>> -		err = set_global_vars(obj, env.presets, env.npresets);
>> -		if (err) {
>> -			fprintf(stderr, "Failed to set global variables %d\n", err);
>> -			goto cleanup;
>> -		}
>> -		process_prog(filename, obj, prog);
>> +	fixup_obj_maps(obj);
>> +
>> +	err = set_global_vars(obj, env.presets, env.npresets);
>> +	if (err) {
>> +		fprintf(stderr, "Failed to set global variables %d\n", err);
>>  		goto cleanup;
>>  	}
>>  
>> -	bpf_object__for_each_program(prog, obj) {
>> -		const char *prog_name = bpf_program__name(prog);
>> -
>> -		tobj = bpf_object__open_file(filename, &opts);
>> -		if (!tobj) {
>> -			err = -errno;
>> -			fprintf(stderr, "Failed to open '%s': %d\n", filename, err);
>> -			goto cleanup;
>> -		}
>> -
>> -		err = set_global_vars(tobj, env.presets, env.npresets);
>> -		if (err) {
>> -			fprintf(stderr, "Failed to set global variables %d\n", err);
>> -			goto cleanup;
>> -		}
>> -
>> -		lprog = NULL;
>> -		bpf_object__for_each_program(tprog, tobj) {
>> -			const char *tprog_name = bpf_program__name(tprog);
>> -
>> -			if (strcmp(prog_name, tprog_name) == 0) {
>> -				bpf_program__set_autoload(tprog, true);
>> -				lprog = tprog;
>> -			} else {
>> -				bpf_program__set_autoload(tprog, false);
>> -			}
>> -		}
>> +	err = bpf_object__prepare(obj);
>> +	if (err) {
>> +		fprintf(stderr, "Failed to prepare BPF object for loading %d\n", err);
>> +		goto cleanup;
>> +	}
>>  
>> -		process_prog(filename, tobj, lprog);
>> -		bpf_object__close(tobj);
>> +	bpf_object__for_each_program(prog, obj) {
>> +		process_prog(filename, obj, prog);
>>  	}
>>  
>>  cleanup:
>> @@ -3264,17 +3251,14 @@ static int handle_verif_mode(void)
>>  	create_stat_cgroup();
>>  	for (i = 0; i < env.filename_cnt; i++) {
>>  		err = process_obj(env.filenames[i]);
>> -		if (err) {
>> +		if (err)
>>  			fprintf(stderr, "Failed to process '%s': %d\n", env.filenames[i], err);
>> -			goto out;
>> -		}
>
> This means we now do not stop on the first failure - this I assume is
> deliberate since it is an improvement over existing behavior imo, but
> still pointing it out in case it isn't.
>
>>  	}
>>  
>>  	qsort(env.prog_stats, env.prog_stat_cnt, sizeof(*env.prog_stats), cmp_prog_stats);
>>  
>>  	output_prog_stats();
>>  
>> -out:
>>  	destroy_stat_cgroup();
>>  	return err;
>>  }