On Fri, Mar 22, 2024 at 3:26 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> Map base reference BTF type ids referenced in split BTF and their
> references to the base BTF passed in, and if the mapping succeeds,
> reparent the split BTF to the base BTF.
>
> Reconciliation rules are
>
> - base types must match exactly
> - enum[64] types should match all value name/value pairs, but the
> to-be-reconciled enum[64] can also define additional name/value pairs
> - named fwds match to the correspondingly-named struct/union/enum/enum64
yeah, but what about their size if they are embedded? Using FWD was a
nice trick, but it's not flexible enough for recording (optionally)
size... Probably emitting an empty (but named) struct/union/enum would
be a bit better (and actually make split BTF using base ref more valid
even without pre-processing).
> - anon struct/unions must have field names/offsets specified in base
> reference BTF matched by those in base BTF we are matching with
>
> Reconciliation can not recurse, since it will be used in-kernel also and
> we do not want to blow up the kernel stack when carrying out type
> compatibility checks. Hence we use a stack for reference type
> reconciliation rather then recursive function calls.
>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> tools/lib/bpf/Build | 2 +-
> tools/lib/bpf/btf.c | 58 ++++
> tools/lib/bpf/btf.h | 8 +
> tools/lib/bpf/btf_reconcile.c | 590 ++++++++++++++++++++++++++++++++
how wrong would it be to call this process "relocate" rather than "reconcile"?
> tools/lib/bpf/libbpf.map | 1 +
> tools/lib/bpf/libbpf_internal.h | 2 +
> 6 files changed, 660 insertions(+), 1 deletion(-)
> create mode 100644 tools/lib/bpf/btf_reconcile.c
>
[...]
> +/* Find next type after *id in base BTF that matches kind of type t passed in
> + * and name (if it is specified). Match fwd kinds to appropriate kind also.
> + */
> +static int btf_reconcile_find_next(struct btf_reconcile *r, const struct btf_type *t,
> + __u32 *id, const struct btf_type **tp)
I haven't grokked the whole patch logic just yet, doing a first pass,
so I might be asking stupid questions, sorry.
But it looks like we have these linear searches here to find matching
types, is that right? Wouldn't it be better to build an index first to
speed up search?
> +{
> + const struct btf_type *nt;
> + int kind, tkind = btf_kind(t);
> + int tkflag = btf_kflag(t);
> + __u32 i;
> +
[...]
> +static int btf_reconcile_int(struct btf_reconcile *r, const char *name,
> + const struct btf_type *t, const struct btf_type *bt)
> +{
> + __u32 *info = (__u32 *)(t + 1);
> + __u32 *binfo = (__u32 *)(bt + 1);
> +
> + if (t->size != bt->size) {
> + pr_warn("INT types '%s' disagree on size; reference base BTF says %d; base BTF says %d\n",
> + name, t->size, bt->size);
> + return -EINVAL;
> + }
> + if (BTF_INT_ENCODING(*info) != BTF_INT_ENCODING(*binfo)) {
> + pr_warn("INT types '%s' disagree on encoding; reference base BTF says '(%s/%s/%s); base BTF says '(%s/%s/%s)'\n",
> + name,
> + BTF_INT_ENCODING(*info) & BTF_INT_SIGNED ? "signed" : "unsigned",
> + BTF_INT_ENCODING(*info) & BTF_INT_CHAR ? "char" : "nonchar",
> + BTF_INT_ENCODING(*info) & BTF_INT_BOOL ? "bool" : "nonbool",
> + BTF_INT_ENCODING(*binfo) & BTF_INT_SIGNED ? "signed" : "unsigned",
> + BTF_INT_ENCODING(*binfo) & BTF_INT_CHAR ? "char" : "nonchar",
> + BTF_INT_ENCODING(*binfo) & BTF_INT_BOOL ? "bool" : "nonbool");
there is btf_int_encoding() helper, please use it
> + return -EINVAL;
> + }
> + if (BTF_INT_BITS(*info) != BTF_INT_BITS(*binfo)) {
btf_int_bits()
> + pr_warn("INT types '%s' disagree on bit size; reference base BTF says %d; base BTF says %d\n",
> + name, BTF_INT_BITS(*info), BTF_INT_BITS(*binfo));
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int btf_reconcile_float(struct btf_reconcile *r, const char *name,
> + const struct btf_type *t, const struct btf_type *bt)
> +{
> +
> + if (t->size != bt->size) {
> + pr_warn("float types '%s' disagree on size; reference base BTF says %d; base BTF says %d\n",
> + name, t->size, bt->size);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/* Ensure each enum value in type t has equivalent in base BTF and that values (if any) match. */
> +static int btf_reconcile_enum(struct btf_reconcile *r, const char *name,
> + const struct btf_type *t, const struct btf_type *bt)
> +{
should we care about compatibility between ENUM and ENUM64, they can
both represent the same values of the same size?
> + struct btf_enum *v = (struct btf_enum *)(t + 1);
> + struct btf_enum *bv = (struct btf_enum *)(bt + 1);
> + bool found, match;
> + int i, j;
> +
> + for (i = 0; i < btf_vlen(t); i++, v++) {
> + found = match = false;
> +
> + if (!v->name_off)
> + continue;
> + for (j = 0; j < btf_vlen(bt); j++, bv++) {
> + if (!bv->name_off)
> + continue;
> + if (strcmp(btf__name_by_offset(r->base_ref_btf, v->name_off),
> + btf__name_by_offset(r->base_btf, bv->name_off)) != 0)
nit: please use local vars to shorten these long if conditions, it
will be easier to read and follow
> + continue;
> + found = true;
> + match = (v->val == bv->val);
> + break;
> + }
[...]
> + while ((err = btf_reconcile_find_next(r, t, &base_id, &bt)) != -ENOENT) {
> + bt = btf_type_by_id(r->base_btf, base_id);
> + switch (btf_kind(t)) {
> + case BTF_KIND_INT:
> + err = btf_reconcile_int(r, name, t, bt);
> + break;
> + case BTF_KIND_ENUM:
> + err = btf_reconcile_enum(r, name, t, bt);
> + break;
> + case BTF_KIND_FLOAT:
> + err = btf_reconcile_float(r, name, t, bt);
> + break;
> + case BTF_KIND_ENUM64:
> + err = btf_reconcile_enum64(r, name, t, bt);
> + break;
> + case BTF_KIND_FWD:
well, FWD can be for enum vs struct, gotta check that
> + err = 0;
> + break;
> + default:
> + return 0;
> + }
> + if (!err) {
> + r->map[id] = base_id;
> + return 0;
> + }
> + }
> + return err;
> +}
[...]
> + if (err) {
> + pr_warn("could not find base BTF type for base reference type[%u]\n",
> + id);
> + return err;
> + }
> + } else {
> + if (btf_reconcile_push(r, id) < 0 ||
> + btf_reconcile_push(r, t->type) < 0)
> + return -ENOSPC;
I'm missing something, please help me understand. I don't get why we
need a recursive algorithm at all.
In my mind, we have this small "base ref" set of types referenced from
module's BTF (split BTF part), right? So all we should need is to map
every type from base ref set to vmlinux BTF.
What I don't yet fully get is why CONST/VOLATILE or PTR need to
postpone reconciliation via a queue. By the time we get to types in
split BTF all base ref types should be mapped, so all you need is to
remap t->type to resolved vmlinux BTF, no?
I suspect the answer might have something to do with those anonymous
structs/unions which you copy verbatim into base ref BTF?
But on the latter topic, I wonder if we at all need this? Why not keep
all those anon struct/union/enum in module's part of BTF? If they are
unnamed, I doubt they will ever be referenced from kfuncs or anything
like that, so their BTF ID isn't that important.
If base BTF is all named types, it would simplify the reconciliation
process significantly, I think.
But again, I only skimmed the overall algorithm, sorry for my
laziness, but I figured it would be good to discuss the above first
anyways.
> + }
> + break;
[...]
