On Fri, Feb 28, 2025 at 9:53 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>
> Introduce bpf_object__prepare API: additional intermediate step,
> executing all steps that bpf_object__load is running before the actual
> loading of BPF programs.
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 161 +++++++++++++++++++++++++++------------
> tools/lib/bpf/libbpf.h | 9 +++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 121 insertions(+), 50 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 9ced1ce2334c..dd2f64903c3b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4858,7 +4858,7 @@ bool bpf_map__autocreate(const struct bpf_map *map)
>
> int bpf_map__set_autocreate(struct bpf_map *map, bool autocreate)
> {
> - if (map->obj->state >= OBJ_LOADED)
> + if (map->obj->state >= OBJ_PREPARED)
ah, ok, so you've decided to change that in this patch... that works
as well, I guess
> return libbpf_err(-EBUSY);
>
> map->autocreate = autocreate;
> @@ -4952,7 +4952,7 @@ struct bpf_map *bpf_map__inner_map(struct bpf_map *map)
>
> int bpf_map__set_max_entries(struct bpf_map *map, __u32 max_entries)
> {
> - if (map->obj->state >= OBJ_LOADED)
> + if (map->obj->state >= OBJ_PREPARED)
> return libbpf_err(-EBUSY);
>
> map->def.max_entries = max_entries;
> @@ -5199,7 +5199,7 @@ static void bpf_map__destroy(struct bpf_map *map);
>
> static bool map_is_created(const struct bpf_map *map)
we should use this helper in bpf_map__set_max_entries() and other
setters like that. We better error out when someone is trying to set
different value size for the map that was explicitly set from FD with
bpf_map__reuse_fd()... Can you please add a patch that would fix this
up (before all this OBJ_PREPARED refactoring)?
> {
> - return map->obj->state >= OBJ_LOADED || map->reused;
> + return map->obj->state >= OBJ_PREPARED || map->reused;
> }
>
> static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, bool is_inner)
> @@ -7901,13 +7901,6 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
> size_t i;
> int err;
>
> - for (i = 0; i < obj->nr_programs; i++) {
> - prog = &obj->programs[i];
> - err = bpf_object__sanitize_prog(obj, prog);
> - if (err)
> - return err;
> - }
> -
> for (i = 0; i < obj->nr_programs; i++) {
> prog = &obj->programs[i];
> if (prog_is_subprog(obj, prog))
> @@ -7933,6 +7926,21 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
> return 0;
> }
>
> +static int bpf_object_prepare_progs(struct bpf_object *obj)
> +{
> + struct bpf_program *prog;
> + size_t i;
> + int err;
> +
> + for (i = 0; i < obj->nr_programs; i++) {
> + prog = &obj->programs[i];
> + err = bpf_object__sanitize_prog(obj, prog);
> + if (err)
> + return err;
> + }
> + return 0;
> +}
> +
> static const struct bpf_sec_def *find_sec_def(const char *sec_name);
>
> static int bpf_object_init_progs(struct bpf_object *obj, const struct bpf_object_open_opts *opts)
> @@ -8549,9 +8557,72 @@ static int bpf_object_prepare_struct_ops(struct bpf_object *obj)
> return 0;
> }
>
> +static void bpf_object_unpin(struct bpf_object *obj)
> +{
> + int i;
> +
> + /* unpin any maps that were auto-pinned during load */
> + for (i = 0; i < obj->nr_maps; i++)
> + if (obj->maps[i].pinned && !obj->maps[i].reused)
> + bpf_map__unpin(&obj->maps[i], NULL);
> +}
> +
> +static void bpf_object_post_load_cleanup(struct bpf_object *obj)
> +{
> + int i;
> +
> + /* clean up fd_array */
> + zfree(&obj->fd_array);
> +
> + /* clean up module BTFs */
> + for (i = 0; i < obj->btf_module_cnt; i++) {
> + close(obj->btf_modules[i].fd);
> + btf__free(obj->btf_modules[i].btf);
> + free(obj->btf_modules[i].name);
> + }
> + free(obj->btf_modules);
> +
> + /* clean up vmlinux BTF */
> + btf__free(obj->btf_vmlinux);
> + obj->btf_vmlinux = NULL;
> +}
> +
> +static int bpf_object_prepare(struct bpf_object *obj, const char *target_btf_path)
> +{
> + int err;
> +
> + if (!obj)
> + return -EINVAL;
meh, drop this please, can't be NULL
> +
> + if (obj->state >= OBJ_PREPARED) {
> + pr_warn("object '%s': prepare loading can't be attempted twice\n", obj->name);
> + return -EINVAL;
> + }
> +
> + err = bpf_object_prepare_token(obj);
> + err = err ? : bpf_object__probe_loading(obj);
> + err = err ? : bpf_object__load_vmlinux_btf(obj, false);
> + err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
> + err = err ? : bpf_object__sanitize_maps(obj);
> + err = err ? : bpf_object__init_kern_struct_ops_maps(obj);
> + err = err ? : bpf_object_adjust_struct_ops_autoload(obj);
> + err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : target_btf_path);
> + err = err ? : bpf_object__sanitize_and_load_btf(obj);
> + err = err ? : bpf_object__create_maps(obj);
> + err = err ? : bpf_object_prepare_progs(obj);
> + obj->state = OBJ_PREPARED;
> +
> + if (err) {
> + bpf_object_unpin(obj);
> + bpf_object_unload(obj);
> + return err;
> + }
> + return 0;
> +}
> +
> static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const char *target_btf_path)
> {
> - int err, i;
> + int err;
>
> if (!obj)
> return libbpf_err(-EINVAL);
> @@ -8571,17 +8642,12 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch
> return libbpf_err(-LIBBPF_ERRNO__ENDIAN);
> }
>
> - err = bpf_object_prepare_token(obj);
> - err = err ? : bpf_object__probe_loading(obj);
> - err = err ? : bpf_object__load_vmlinux_btf(obj, false);
> - err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
> - err = err ? : bpf_object__sanitize_maps(obj);
> - err = err ? : bpf_object__init_kern_struct_ops_maps(obj);
> - err = err ? : bpf_object_adjust_struct_ops_autoload(obj);
> - err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : target_btf_path);
> - err = err ? : bpf_object__sanitize_and_load_btf(obj);
> - err = err ? : bpf_object__create_maps(obj);
> - err = err ? : bpf_object__load_progs(obj, extra_log_level);
> + if (obj->state < OBJ_PREPARED) {
> + err = bpf_object_prepare(obj, target_btf_path);
> + if (err)
> + return libbpf_err(err);
> + }
> + err = bpf_object__load_progs(obj, extra_log_level);
> err = err ? : bpf_object_init_prog_arrays(obj);
> err = err ? : bpf_object_prepare_struct_ops(obj);
>
> @@ -8593,35 +8659,22 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch
> err = bpf_gen__finish(obj->gen_loader, obj->nr_programs, obj->nr_maps);
> }
>
> - /* clean up fd_array */
> - zfree(&obj->fd_array);
> + bpf_object_post_load_cleanup(obj);
> + obj->state = OBJ_LOADED;/* doesn't matter if successfully or not */
>
> - /* clean up module BTFs */
> - for (i = 0; i < obj->btf_module_cnt; i++) {
> - close(obj->btf_modules[i].fd);
> - btf__free(obj->btf_modules[i].btf);
> - free(obj->btf_modules[i].name);
> + if (err) {
> + bpf_object_unpin(obj);
> + bpf_object_unload(obj);
> + pr_warn("failed to load object '%s'\n", obj->path);
> + return libbpf_err(err);
> }
> - free(obj->btf_modules);
> -
> - /* clean up vmlinux BTF */
> - btf__free(obj->btf_vmlinux);
> - obj->btf_vmlinux = NULL;
> -
> - obj->state = OBJ_LOADED;/* doesn't matter if successfully or not */
> - if (err)
> - goto out;
>
> return 0;
> -out:
> - /* unpin any maps that were auto-pinned during load */
> - for (i = 0; i < obj->nr_maps; i++)
> - if (obj->maps[i].pinned && !obj->maps[i].reused)
> - bpf_map__unpin(&obj->maps[i], NULL);
> +}
>
> - bpf_object_unload(obj);
> - pr_warn("failed to load object '%s'\n", obj->path);
> - return libbpf_err(err);
> +int bpf_object__prepare(struct bpf_object *obj)
> +{
> + return libbpf_err(bpf_object_prepare(obj, NULL));
> }
>
> int bpf_object__load(struct bpf_object *obj)
> @@ -8871,8 +8924,8 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
> if (!obj)
> return libbpf_err(-ENOENT);
>
> - if (obj->state < OBJ_LOADED) {
> - pr_warn("object not yet loaded; load it first\n");
> + if (obj->state < OBJ_PREPARED) {
> + pr_warn("object not yet loaded/prepared; load/prepare it first\n");
let's keep the original simpler message, this is way too much of
either/or. Most users will never care about bpf_object__prepare()
anyways.
> return libbpf_err(-ENOENT);
> }
>
> @@ -9069,6 +9122,14 @@ void bpf_object__close(struct bpf_object *obj)
> if (IS_ERR_OR_NULL(obj))
> return;
>
> + /*
> + * if user called bpf_object__prepare() without ever getting to
> + * bpf_object__load(), we need to clean up stuff that is normally
> + * cleaned up at the end of loading step
> + */
> + if (obj->state == OBJ_PREPARED)
> + bpf_object_post_load_cleanup(obj);
> +
let's make bpf_object_post_load_cleanup() idempotent, so calling it
multiple times won't hurt, it should be pretty simple (just set
obj->btf_module_cnt to zero)
> usdt_manager_free(obj->usdt_man);
> obj->usdt_man = NULL;
>
> @@ -10304,7 +10365,7 @@ static int map_btf_datasec_resize(struct bpf_map *map, __u32 size)
>
> int bpf_map__set_value_size(struct bpf_map *map, __u32 size)
> {
> - if (map->obj->state >= OBJ_LOADED || map->reused)
> + if (map->obj->state >= OBJ_PREPARED || map->reused)
> return libbpf_err(-EBUSY);
>
> if (map->mmaped) {
> @@ -10350,7 +10411,7 @@ int bpf_map__set_initial_value(struct bpf_map *map,
> {
> size_t actual_sz;
>
> - if (map->obj->state >= OBJ_LOADED || map->reused)
> + if (map->obj->state >= OBJ_PREPARED || map->reused)
> return libbpf_err(-EBUSY);
>
> if (!map->mmaped || map->libbpf_type == LIBBPF_MAP_KCONFIG)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 3020ee45303a..09e87998c64e 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -241,6 +241,15 @@ LIBBPF_API struct bpf_object *
> bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
> const struct bpf_object_open_opts *opts);
>
> +/**
> + * @brief **bpf_object__prepare()** prepares BPF object for loading.
a bit too brief. Please add that preparation step performs ELF
processing, relocations, prepares final state of BPF program
instructions (accessible with bpf_program__insns()), creates and
(potentially) pins maps, and stops short of loading BPF programs.
This is an important aspect that veristat and others will be relying
on, so it should be documented properly
> + * @param obj Pointer to a valid BPF object instance returned by
> + * **bpf_object__open*()** API
> + * @return 0, on success; negative error code, otherwise, error code is
> + * stored in errno
> + */
> +int bpf_object__prepare(struct bpf_object *obj);
> +
> /**
> * @brief **bpf_object__load()** loads BPF object into kernel.
> * @param obj Pointer to a valid BPF object instance returned by
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index b5a838de6f47..22edde0bf85e 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -438,4 +438,5 @@ LIBBPF_1.6.0 {
> bpf_linker__new_fd;
> btf__add_decl_attr;
> btf__add_type_attr;
> + bpf_object__prepare;
this is sorted list
pw-bot: cr
> } LIBBPF_1.5.0;
> --
> 2.48.1
>
