Re: [PATCH bpf-next v3 3/6] resolve_btfids: Introduce enum btf_id_kind

Eduard Zingerman
2025-12-16T02:38:57Z

On Mon, 2025-12-15 at 18:31 -0800, Ihor Solodrai wrote:
> On 12/11/25 11:09 PM, Eduard Zingerman wrote:
> > On Fri, 2025-12-05 at 14:30 -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>
> > > ---
> >
> > Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> >
> > (But see a question below).
> >
> > > @@ -213,14 +218,19 @@ btf_id__add(struct rb_root *root, char *name, bool unique)
> > > p = &(*p)->rb_left;
> > > else if (cmp > 0)
> > > p = &(*p)->rb_right;
> > > - else
> > > - return unique ? NULL : id;
> > > + else if (kind == BTF_ID_KIND_SYM && id->kind == BTF_ID_KIND_SYM)
> >
> > Nit: I'd keep the 'unique' parameter alongside 'kind' and resolve this
> > condition on the function callsite.
>
> I don't like the boolean args, they're always opaque on the callsite.
>
> We want to allow duplicates for _KIND_SYM and forbid for other kinds.
> Since we are passing the kind from outside, I think it makes sense to
> check for this inside the function. It makes the usage simpler.

On the contrary, the callsite knows exactly what it wants:
unique or non-unique entries. Here you need additional logic
to figure out the intent.

Arguably the uniqueness is associated not with entry type,
but with a particular tree the entry is added to.
And that is a property of the callsite.

> > > + return id;
> > > + else {
> > > + pr_err("Unexpected duplicate symbol %s of kind %d\n", name, id->kind);
> > > + return NULL;
> > > + }
> >
> > [...]
> >
> > > @@ -491,28 +515,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)
> >
> > Current patch is not a culprit, but shouldn't resolve_btfids fail if
> > `id` cannot be added? (here and in a hunk above).
>
> By the existing design, resolve_btfids generally fails if
> CONFIG_WERROR is set and `warnings > 0`.
>
> And in this particular place it would fails with -ENOMEM a bit below:
>
> [...]
> } else if (!strncmp(prefix, BTF_SET, sizeof(BTF_SET) - 1)) {
> 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)
> id->cnt = sym.st_size / sizeof(int) - 1;
> } else {
> pr_err("FAILED unsupported prefix %s\n", prefix);
> return -1;
> }
>
> /* --> */ if (!id)
> return -ENOMEM;
>
> So I think an error code change may be appropriate, and that's about it.

Oh, ok, sorry, didn't notice that.

>
> >
> > > id->cnt = sym.st_size / sizeof(int) - 1;
> > > - id->is_set = true;
> > > - }
> > > } else {
> > > pr_err("FAILED unsupported prefix %s\n", prefix);
> > > return -1;
> >
> > [...]
> >