On 17/06/2024 22:50, Andrii Nakryiko wrote:
> On Thu, Jun 13, 2024 at 2:50 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>>
>> Map distilled base 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.
>>
>> Relocation is done by first verifying that distilled base BTF
>> only consists of named INT, FLOAT, ENUM, FWD, STRUCT and
>> UNION kinds; then we sort these to speed lookups. Once sorted,
>> the base BTF is iterated, and for each relevant kind we check
>> for an equivalent in distilled base BTF. When found, the
>> mapping from distilled -> base BTF id and string offset is recorded.
>> In establishing mappings, we need to ensure we check STRUCT/UNION
>> size when the STRUCT/UNION is embedded in a split BTF STRUCT/UNION,
>> and when duplicate names exist for the same STRUCT/UNION. Otherwise
>> size is ignored in matching STRUCT/UNIONs.
>>
>> Once all mappings are established, we can update type ids
>> and string offsets in split BTF and reparent it to the new base.
>>
>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>> ---
>> tools/lib/bpf/Build | 2 +-
>> tools/lib/bpf/btf.c | 17 ++
>> tools/lib/bpf/btf.h | 14 +
>> tools/lib/bpf/btf_relocate.c | 506 ++++++++++++++++++++++++++++++++
>> tools/lib/bpf/libbpf.map | 1 +
>> tools/lib/bpf/libbpf_internal.h | 3 +
>> 6 files changed, 542 insertions(+), 1 deletion(-)
>> create mode 100644 tools/lib/bpf/btf_relocate.c
>>
>
> [...]
>
>> +/* Build a map from distilled base BTF ids to base BTF ids. To do so, iterate
>> + * through base BTF looking up distilled type (using binary search) equivalents.
>> + */
>> +static int btf_relocate_map_distilled_base(struct btf_relocate *r)
>> +{
>> + struct btf_name_info *dist_base_info_sorted, *dist_base_info_sorted_end;
>> + struct btf_type *base_t, *dist_t;
>> + __u8 *base_name_cnt = NULL;
>> + int err = 0;
>> + __u32 id;
>> +
>> + /* generate a sort index array of name/type ids sorted by name for
>> + * distilled base BTF to speed name-based lookups.
>> + */
>> + dist_base_info_sorted = calloc(r->nr_dist_base_types, sizeof(*dist_base_info_sorted));
>
> s/dist_base_info_sorted/infos/. Do we have any other info here? Not
> distilled base one? Not sorted? What's the point of these long verbose
> variable names besides making the rest of the code longer and more
> distracting?
>
When we discussed this earlier the concern was in distinguishing between
distilled and base BTF information. Later we have name info and
associated types from both base (the search key) and distilled base (the
sorted distilled base info). So we can use info here certainly, but I'd
propose using dist_info/base_info later in the function where we also
use dist_t/base_t.
>> + if (!dist_base_info_sorted) {
>> + err = -ENOMEM;
>> + goto done;
>> + }
>> + dist_base_info_sorted_end = dist_base_info_sorted + r->nr_dist_base_types;
>> + for (id = 0; id < r->nr_dist_base_types; id++) {
>> + dist_t = btf_type_by_id(r->dist_base_btf, id);
>> + dist_base_info_sorted[id].name = btf__name_by_offset(r->dist_base_btf,
>> + dist_t->name_off);
>> + dist_base_info_sorted[id].id = id;
>> + dist_base_info_sorted[id].size = dist_t->size;
>> + dist_base_info_sorted[id].needs_size = true;
>> + }
>> + qsort(dist_base_info_sorted, r->nr_dist_base_types, sizeof(*dist_base_info_sorted),
>> + cmp_btf_name_size);
>> +
>> + /* Mark distilled base struct/union members of split BTF structs/unions
>> + * in id_map with BTF_IS_EMBEDDED; this signals that these types
>> + * need to match both name and size, otherwise embeddding the base
>
> typo: embedding
>
will fix, thanks.
>> + * struct/union in the split type is invalid.
>> + */
>> + for (id = r->nr_dist_base_types; id < r->nr_split_types; id++) {
>> + err = btf_mark_embedded_composite_type_ids(r, id);
>> + if (err)
>> + goto done;
>> + }
>> +
>
> [...]
>
>> + /* iterate over all matching distilled base types */
>> + for (dist_name_info = search_btf_name_size(&base_name_info, dist_base_info_sorted,
>> + r->nr_dist_base_types);
>> + dist_name_info != NULL; dist_name_info = dist_name_info_next) {
>> + /* Are there more distilled matches to process after
>> + * this one?
>> + */
>> + dist_name_info_next = dist_name_info + 1;
>> + if (dist_name_info_next >= dist_base_info_sorted_end ||
>> + cmp_btf_name_size(&base_name_info, dist_name_info_next))
>> + dist_name_info_next = NULL;
>
> Goodness, does this have to be so verbose and ugly?...
>
> First, does "dist_name_info" give us much more information than just
> "info" or something like this?
>
I'd propose using dist_info here for the interator to distinguish it
from base_info (the search key) as we do for dist_t/base_t. More below..
> Second,
>
> for (info = search_btf_name_size(&.....);
> info && cmp_btf_name_size(...) == 0;
> info++) {
> ...
> }
>
> And there is no need for dist_name_info_next and this extra if with
> NULL-ing anything out.
>
There's a bit more complexity here unfortunately. The loop initializer
is straightforward - we do our search for most leftward match. However
from here we have to check a few things in the guard
1. has dist_info run off the end of the sort array? (dist_info <
info_end). We can add that into the guard.
2. does the current match match our search key? In the initializer case
we've already checked that, but for the next iteration we need to
cmp_btf_name_size(&base_info, dist_info)
So doing 2 is unneeded in the initial case (we've already confirmed the
distilled info matches via search_btf_name_size()). Setting the _next
value in the body of the loop therefore seemed like the easiest way to
handle this without the loop guard getting too unwieldy. Putting the
test in the loop guard would mean an extra unneeded comparison (that we
know will succeed for the first iteration), but if that's not too much
of a concern we could certainly do this:
for (dist_info = search_btf_name_size(&base_info, info,
r->nr_dist_base_types);
dist_info != NULL && dist_info < info_end &&
!cmp_btf_name_size(&base_info, dist_info);
dist_info++) {
> Please send a follow up with the clean up, this loop's conditions are
> hard to follow (I had to double check that we don't use
> dist_name_info_next for any decision making; but I shouldn't even
> care, it should be obvious if written as above)
>
>> +
>> + if (!dist_name_info->id || dist_name_info->id > r->nr_dist_base_types) {
>
> another off by one? Valid ID is always < number of types, and so `id
>> = nr_types` is the condition for invalid ID. Please fix in a follow
> up as well.
>
Will fix, thanks.
>> + pr_warn("base BTF id [%d] maps to invalid distilled base BTF id [%d]\n",
>> + id, dist_name_info->id);
>> + err = -EINVAL;
>> + goto done;
>> + }
>> + dist_t = btf_type_by_id(r->dist_base_btf, dist_name_info->id);
>> + dist_kind = btf_kind(dist_t);
>> +
>> + /* Validate that the found distilled type is compatible.
>> + * Do not error out on mismatch as another match may
>> + * occur for an identically-named type.
>> + */
>> + switch (dist_kind) {
>> + case BTF_KIND_FWD:
>> + switch (base_kind) {
>> + case BTF_KIND_FWD:
>> + if (btf_kflag(dist_t) != btf_kflag(base_t))
>> + continue;
>> + break;
>> + case BTF_KIND_STRUCT:
>> + if (btf_kflag(base_t))
>> + continue;
>> + break;
>> + case BTF_KIND_UNION:
>> + if (!btf_kflag(base_t))
>> + continue;
>> + break;
>> + default:
>> + continue;
>> + }
>> + break;
>
> I gotta say it's amazing that C allows this intermixing of breaks and
> continues to work at completely different "scopes" (switch vs for).
> Wonderful language :)
>
It's an odd feature alright. Let me know if you'd prefer an explicit
goto to make the logic clearer; I agree it looks strange.
Alan
