On Sun, Nov 21, 2021 at 5:55 AM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote:
>
> Support static initialization of BPF_MAP_TYPE_PROG_ARRAY with a
> syntax similar to map-in-map initialization ([0]):
>
> SEC("socket")
> int tailcall_1(void *ctx)
> {
> return 0;
> }
>
> struct {
> __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> __uint(max_entries, 2);
> __uint(key_size, sizeof(__u32));
> __array(values, int (void *));
> } prog_array_init SEC(".maps") = {
> .values = {
> [1] = (void *)&tailcall_1,
> },
> };
>
> Here's the relevant part of libbpf debug log showing what's
> going on with prog-array initialization:
>
> libbpf: sec '.relsocket': collecting relocation for section(3) 'socket'
> libbpf: sec '.relsocket': relo #0: insn #2 against 'prog_array_init'
> libbpf: prog 'entry': found map 0 (prog_array_init, sec 4, off 0) for insn #0
> libbpf: .maps relo #0: for 3 value 0 rel->r_offset 32 name 53 ('tailcall_1')
> libbpf: .maps relo #0: map 'prog_array_init' slot [1] points to prog 'tailcall_1'
> libbpf: map 'prog_array_init': created successfully, fd=5
> libbpf: map 'prog_array_init': slot [1] set to prog 'tailcall_1' fd=6
>
> [0] Closes: https://github.com/libbpf/libbpf/issues/354
>
> Signed-off-by: Hengqi Chen <hengqi.chen@xxxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 146 ++++++++++++++++++++++++++++++++++-------
> 1 file changed, 122 insertions(+), 24 deletions(-)
>
Just a few nits and suggestions below, but it looks great overall, thanks!
[...]
> t = skip_mods_and_typedefs(btf, btf_array(t)->type, NULL);
> if (!btf_is_ptr(t)) {
> - pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> - map_name, btf_kind_str(t));
> + if (is_map_in_map)
> + pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> + map_name, btf_kind_str(t));
> + else
> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
maybe let's do
const char *desc = is_map_in_map ? "map-in-map inner" : "prog-array value";
and use desc in those three pr_warn() messages?
> + map_name, btf_kind_str(t));
> return -EINVAL;
> }
> t = skip_mods_and_typedefs(btf, t->type, NULL);
> - if (!btf_is_struct(t)) {
> + if (is_prog_array) {
> + if (btf_is_func_proto(t))
> + return 0;
you can't return on success here, there could technically be other
fields after "values". Can you please also invert the condition so
that error handling happens first and then we continue:
if (!btf_is_func_proto(t)) {
pr_warn(..);
return -EINVAl;
}
continue;
It's more consistent with the other logic in this function
> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
> + map_name, btf_kind_str(t));
> + return -EINVAL;
> + }
> + if (is_map_in_map && !btf_is_struct(t)) {
well, it can't be anything else, so I guess drop the is_map_in_map check?
> pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> map_name, btf_kind_str(t));
> return -EINVAL;
> @@ -4964,12 +4985,16 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> unsigned int i;
> int fd, err = 0;
>
> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY)
> + return 0;
let's leave a comment that PROG_ARRAY can only be initialized once all
the programs are loaded, so this will be done later
Better still, it would be good to also rename init_map_slots to
init_map_in_map_slots and do the PROG_ARRAY check outside, in
bpf_object__create_maps(). This creates a nice symmetry with
init_prog_array (should it be named init_prog_array_slots for
consistency?). WDYT?
> +
> for (i = 0; i < map->init_slots_sz; i++) {
> if (!map->init_slots[i])
> continue;
>
> targ_map = map->init_slots[i];
> fd = bpf_map__fd(targ_map);
> +
> if (obj->gen_loader) {
> pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n",
> map - obj->maps, i, targ_map - obj->maps);
> @@ -4980,8 +5005,7 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> if (err) {
> err = -errno;
> pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
> - map->name, i, targ_map->name,
> - fd, err);
> + map->name, i, targ_map->name, fd, err);
> return err;
> }
> pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
> @@ -4994,6 +5018,60 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
> return 0;
> }
>
> +static int init_prog_array(struct bpf_object *obj, struct bpf_map *map)
> +{
> + const struct bpf_program *targ_prog;
> + unsigned int i;
> + int fd, err = 0;
you always set err, no need to zero-initialize it
> +
> + for (i = 0; i < map->init_slots_sz; i++) {
> + if (!map->init_slots[i])
> + continue;
> +
> + targ_prog = map->init_slots[i];
> + fd = bpf_program__fd(targ_prog);
> +
> + if (obj->gen_loader) {
> + return -ENOTSUP;
> + } else {
> + err = bpf_map_update_elem(map->fd, &i, &fd, 0);
> + }
> + if (err) {
> + err = -errno;
> + pr_warn("map '%s': failed to initialize slot [%d] to prog '%s' fd=%d: %d\n",
> + map->name, i, targ_prog->name, fd, err);
> + return err;
> + }
> + pr_debug("map '%s': slot [%d] set to prog '%s' fd=%d\n",
> + map->name, i, targ_prog->name, fd);
> + }
> +
> + zfree(&map->init_slots);
> + map->init_slots_sz = 0;
> +
> + return 0;
> +}
> +
> +static int bpf_object_init_prog_array(struct bpf_object *obj)
s/prog_array/prog_arrays/
> +{
> + struct bpf_map *map;
> + int i, err;
> +
> + for (i = 0; i < obj->nr_maps; i++) {
> + map = &obj->maps[i];
> +
> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY &&
> + map->init_slots_sz) {
nit: longer single line is fine here (but you can also invert the
condition and continue early to avoid extra level of nestedness)
> + err = init_prog_array(obj, map);
> + if (err < 0) {
> + zclose(map->fd);
> + return err;
> + }
> + }
> + }
> + return 0;
> +}
> +
> static int
> bpf_object__create_maps(struct bpf_object *obj)
> {
> @@ -6174,7 +6252,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> int i, j, nrels, new_sz;
> const struct btf_var_secinfo *vi = NULL;
> const struct btf_type *sec, *var, *def;
> - struct bpf_map *map = NULL, *targ_map;
> + struct bpf_map *map = NULL, *targ_map = NULL;
> + struct bpf_program *targ_prog = NULL;
> + bool is_prog_array, is_map_in_map;
> const struct btf_member *member;
> const char *name, *mname;
> unsigned int moff;
> @@ -6203,11 +6283,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> return -LIBBPF_ERRNO__FORMAT;
> }
> name = elf_sym_str(obj, sym->st_name) ?: "<?>";
> - if (sym->st_shndx != obj->efile.btf_maps_shndx) {
> - pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
> - i, name);
> - return -LIBBPF_ERRNO__RELOC;
> - }
>
> pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n",
> i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value,
> @@ -6229,8 +6304,20 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> return -EINVAL;
> }
>
> - if (!bpf_map_type__is_map_in_map(map->def.type))
> + is_map_in_map = bpf_map_type__is_map_in_map(map->def.type);
> + is_prog_array = map->def.type == BPF_MAP_TYPE_PROG_ARRAY;
> + if (!is_map_in_map && !is_prog_array)
> return -EINVAL;
> + if (is_map_in_map && sym->st_shndx != obj->efile.btf_maps_shndx) {
> + pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
> + i, name);
> + return -LIBBPF_ERRNO__RELOC;
> + }
> + if (is_prog_array && !bpf_object__find_program_by_name(obj, name)) {
let's do an additional check on the program you found with find_program_by_name:
1. prog->sec_idx == sym->st_shndx
2. prog->sec_insn_off * 8 == sym->st_value
3. !prog_is_subprog(obj, prog)
This will make sure you have the correct entry-point BPF program (not
a subprog) and we point to its beginning (no offset into the program).
> + pr_warn(".maps relo #%d: '%s' isn't a BPF program\n",
"entry-point" is an important distinction, please mention that. You
can't put sub-programs into PROG_ARRAY.
> + i, name);
> + return -LIBBPF_ERRNO__RELOC;
> + }
> if (map->def.type == BPF_MAP_TYPE_HASH_OF_MAPS &&
> map->def.key_size != sizeof(int)) {
> pr_warn(".maps relo #%d: hash-of-maps '%s' should have key size %zu.\n",
> @@ -6238,9 +6325,15 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> return -EINVAL;
> }
>
> - targ_map = bpf_object__find_map_by_name(obj, name);
> - if (!targ_map)
> - return -ESRCH;
> + if (is_map_in_map) {
> + targ_map = bpf_object__find_map_by_name(obj, name);
> + if (!targ_map)
> + return -ESRCH;
> + } else {
> + targ_prog = bpf_object__find_program_by_name(obj, name);
> + if (!targ_prog)
> + return -ESRCH;
> + }
>
> var = btf__type_by_id(obj->btf, vi->type);
> def = skip_mods_and_typedefs(obj->btf, var->type, NULL);
> @@ -6272,10 +6365,14 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> (new_sz - map->init_slots_sz) * host_ptr_sz);
> map->init_slots_sz = new_sz;
> }
> - map->init_slots[moff] = targ_map;
> + map->init_slots[moff] = is_map_in_map ? (void *)targ_map : (void *)targ_prog;
>
> - pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
> - i, map->name, moff, name);
> + if (is_map_in_map)
> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n",
> + i, map->name, moff, name);
> + else
> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to prog '%s'\n",
> + i, map->name, moff, name);
similar as above `is_map_in_map ? "map" : "prog"` will keep it short
and not duplicated
> }
>
> return 0;
> @@ -7293,6 +7390,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> err = err ? : bpf_object__create_maps(obj);
> err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path);
> err = err ? : bpf_object__load_progs(obj, attr->log_level);
> + err = err ? : bpf_object_init_prog_array(obj);
>
> if (obj->gen_loader) {
> /* reset FDs */
> --
> 2.30.2
