Re: [PATCH bpf-next v2 3/4] resolve_btfids: introduce enum btf_id_kind

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-11-27 at 10:52 -0800, Ihor Solodrai wrote:
> Instead of using multiple flags, make struct btf_id tagged with an
> enum value indicating its kind in the context of resolve_btfids.
>
> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> ---

Just a few nits, looks good to me overall.

>  tools/bpf/resolve_btfids/main.c | 62 ++++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 20 deletions(-)
>
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index b4caae1170dd..c60d303ca6ed 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -98,6 +98,13 @@
>  # error "Unknown machine endianness!"
>  #endif
>
> +enum btf_id_kind {
> +	BTF_ID_KIND_NONE,
> +	BTF_ID_KIND_SYM,
> +	BTF_ID_KIND_SET,
> +	BTF_ID_KIND_SET8
> +};
> +
>  struct btf_id {
>  	struct rb_node	 rb_node;
>  	char		*name;
> @@ -105,9 +112,8 @@ struct btf_id {
>  		int	 id;
>  		int	 cnt;
>  	};
> -	int		 addr_cnt;
> -	bool		 is_set;
> -	bool		 is_set8;
> +	enum btf_id_kind kind:8;
> +	int		 addr_cnt:8;
                                 ^^

Nit: these bitfields are not really necessary:

  $ pahole -C btf_id ./tools/bpf/resolve_btfids/resolve_btfids
  struct btf_id {
          struct rb_node             rb_node __attribute__((__aligned__(8))); /*     0    24 */
          char *                     name;                 /*    24     8 */
          union {
                  int                id;                   /*    32     4 */
                  int                cnt;                  /*    32     4 */
          };                                               /*    32     4 */
          enum btf_id_kind           kind:8;               /*    36: 0  4 */
          int                        addr_cnt:8;           /*    36: 8  4 */

          /* XXX 16 bits hole, try to pack */

          Elf64_Addr                 addr[100];            /*    40   800 */

          /* size: 840, cachelines: 14, members: 6 */
          /* sum members: 836 */
          /* sum bitfield members: 16 bits, bit holes: 1, sum bit holes: 16 bits */
          /* forced alignments: 1 */
          /* last cacheline: 8 bytes */
  } __attribute__((__aligned__(8)));

>  	Elf64_Addr	 addr[ADDR_CNT];
>  };
>
> @@ -260,26 +266,33 @@ static char *get_id(const char *prefix_end)
>  	return id;
>  }
>
> -static struct btf_id *add_set(struct object *obj, char *name, bool is_set8)
> +static struct btf_id *add_set(struct object *obj, char *name, enum btf_id_kind kind)
>  {
>  	/*
>  	 * __BTF_ID__set__name
>  	 * name =    ^
>  	 * id   =         ^
>  	 */
> -	char *id = name + (is_set8 ? sizeof(BTF_SET8 "__") : sizeof(BTF_SET "__")) - 1;
> +	int prefixlen = kind == BTF_ID_KIND_SET8 ? sizeof(BTF_SET8 "__") : sizeof(BTF_SET "__");

Nit: Should prefixlen be an input parameter as well? (Or adjust the 'name' at the callsite?)
     Otherwise the parameter is still a boolean logically.

> +	char *id = name + prefixlen - 1;
>  	int len = strlen(name);
> +	struct btf_id *btf_id;
>
>  	if (id >= name + len) {
>  		pr_err("FAILED to parse set name: %s\n", name);
>  		return NULL;
>  	}
>
> -	return btf_id__add(&obj->sets, id, true);
> +	btf_id = btf_id__add(&obj->sets, id, true);
> +	if (btf_id)
> +		btf_id->kind = kind;
> +
> +	return btf_id;
>  }
>
>  static struct btf_id *add_symbol(struct rb_root *root, char *name, size_t size)
>  {
> +	struct btf_id *btf_id;
>  	char *id;
>
>  	id = get_id(name + size);
> @@ -288,7 +301,11 @@ static struct btf_id *add_symbol(struct rb_root *root, char *name, size_t size)
>  		return NULL;
>  	}
>
> -	return btf_id__add(root, id, false);
> +	btf_id = btf_id__add(root, id, false);
> +	if (btf_id)
> +		btf_id->kind = BTF_ID_KIND_SYM;
> +
> +	return btf_id;

Agree with Andrii regarding 'kind' being a btf_id__add() parameter.

>  }
>
>  /* Older libelf.h and glibc elf.h might not yet define the ELF compression types. */
> @@ -491,28 +508,24 @@ static int symbols_collect(struct object *obj)
>  			id = add_symbol(&obj->funcs, prefix, sizeof(BTF_FUNC) - 1);
>  		/* set8 */
>  		} else if (!strncmp(prefix, BTF_SET8, sizeof(BTF_SET8) - 1)) {
> -			id = add_set(obj, prefix, true);
> +			id = add_set(obj, prefix, BTF_ID_KIND_SET8);
>  			/*
>  			 * SET8 objects store list's count, which is encoded
>  			 * in symbol's size, together with 'cnt' field hence
>  			 * that - 1.
>  			 */
> -			if (id) {
> +			if (id)
>  				id->cnt = sym.st_size / sizeof(uint64_t) - 1;
> -				id->is_set8 = true;
> -			}
>  		/* set */
>  		} else if (!strncmp(prefix, BTF_SET, sizeof(BTF_SET) - 1)) {
> -			id = add_set(obj, prefix, false);
> +			id = add_set(obj, prefix, BTF_ID_KIND_SET);
>  			/*
>  			 * SET objects store list's count, which is encoded
>  			 * in symbol's size, together with 'cnt' field hence
>  			 * that - 1.
>  			 */
> -			if (id) {
> +			if (id)
>  				id->cnt = sym.st_size / sizeof(int) - 1;
> -				id->is_set = true;
> -			}
>  		} else {
>  			pr_err("FAILED unsupported prefix %s\n", prefix);
>  			return -1;
> @@ -643,7 +656,7 @@ static int id_patch(struct object *obj, struct btf_id *id)
>  	int i;
>
>  	/* For set, set8, id->id may be 0 */
> -	if (!id->id && !id->is_set && !id->is_set8) {
> +	if (!id->id && id->kind == BTF_ID_KIND_SYM) {
>  		pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name);
>  		warnings++;
>  	}
> @@ -696,6 +709,7 @@ static int sets_patch(struct object *obj)
>  {
>  	Elf_Data *data = obj->efile.idlist;
>  	struct rb_node *next;
> +	int cnt;
>
>  	next = rb_first(&obj->sets);
>  	while (next) {
> @@ -715,11 +729,15 @@ static int sets_patch(struct object *obj)
>  			return -1;
>  		}
>
> -		if (id->is_set) {
> +		switch (id->kind) {
> +		case BTF_ID_KIND_SET:
>  			set = data->d_buf + off;
> +			cnt = set->cnt;
>  			qsort(set->ids, set->cnt, sizeof(set->ids[0]), cmp_id);
> -		} else {
> +			break;
> +		case BTF_ID_KIND_SET8:
>  			set8 = data->d_buf + off;
> +			cnt = set8->cnt;
>  			/*
>  			 * Make sure id is at the beginning of the pairs
>  			 * struct, otherwise the below qsort would not work.
> @@ -744,10 +762,14 @@ static int sets_patch(struct object *obj)
>  						bswap_32(set8->pairs[i].flags);
>  				}
>  			}
> +			break;
> +		case BTF_ID_KIND_SYM:
> +		default:

Nit: just default, no need for `case BTF_ID_KIND_SYM:`?

> +			pr_err("Unexpected btf_id_kind %d for set '%s'\n", id->kind, id->name);
> +			return -1;
>  		}
>
> -		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
> -			 off, id->is_set ? set->cnt : set8->cnt, id->name);
> +		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n", off, cnt, id->name);
>
>  		next = rb_next(next);
>  	}