From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0E3D0C433FE for ; Tue, 15 Feb 2022 22:56:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244700AbiBOW46 (ORCPT ); Tue, 15 Feb 2022 17:56:58 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:33716 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244692AbiBOW44 (ORCPT ); Tue, 15 Feb 2022 17:56:56 -0500 Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 78E70C116D for ; Tue, 15 Feb 2022 14:56:44 -0800 (PST) Received: by mail-lj1-x22d.google.com with SMTP id b20so655982ljf.7 for ; Tue, 15 Feb 2022 14:56:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kinvolk.io; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=KgVZkUUbGUZQ31ZvNwCOIzIiGTf7WnYCysAE6/qgMwU=; b=jkpiSQy0pO5BRcFNR90Bqpn7JoS+4arc8FJd0S6Dam6/avw3TDN8o6vprTobszQt1Q /cGe7c9LVfx9qXrs1NGvIeo6YHcJHszV1ykPWhO6QR2hvIUg026GjgDOL7ip/FihwDvi rVOTR5gGVwJ58kA0IOlegJNi08TJr/CrqvWL4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=KgVZkUUbGUZQ31ZvNwCOIzIiGTf7WnYCysAE6/qgMwU=; b=oKI2Q12iDpL9ktmkN+Ke4+JpKkFOe+IgEW+Q8+BN+duywB3hJM4L5Cej7Qx/LZ+ufH wVMUSOHMO3Si8JeLkFkoR5Yz9m0r1nlaX7a48YQxw+O18sJy0O+HFVFn56D1XMkFD8ti nxdYKQ7D4NokL7oKm5z6ykWUb3Sy/8tPfR0z9WJLoLjmjIvYxuCeC4JWCOyBjl9G0RGt Ty/jGXTPLASRWVXXHjbvlvX48b5jORNCobF6DTneNo4pbz1muopf3aQSCe5jP/qsXYwS dpZfAprgGHr/kuPdfG6lAlSapxKocKDl9OAia5Dh0y3QyY1fRHWCqVnM2qdMaepwgu9D RA9g== X-Gm-Message-State: AOAM533GzHgBo5XGjD+tG5b0jh3dvrPM7Q93dNkoEx7K9w2GMDTspmns dQTCrViLnXRbSoyIFIRUdqXNaPCMCm6cjXErwWPHPQ== X-Google-Smtp-Source: ABdhPJzE4wAiM8xsgQMVU9mjRRGUb9KfcCe+Pj7DgIWMdz/n7DiwZkVB1IoZTu7jvNUymg/+ylaTOdvY519yYIh5M/E= X-Received: by 2002:a05:651c:990:b0:240:752f:a224 with SMTP id b16-20020a05651c099000b00240752fa224mr48937ljq.266.1644965802672; Tue, 15 Feb 2022 14:56:42 -0800 (PST) MIME-Version: 1.0 References: <20220209222646.348365-1-mauricio@kinvolk.io> <20220209222646.348365-6-mauricio@kinvolk.io> In-Reply-To: From: =?UTF-8?Q?Mauricio_V=C3=A1squez_Bernal?= Date: Tue, 15 Feb 2022 17:56:31 -0500 Message-ID: Subject: Re: [PATCH bpf-next v6 5/7] bpftool: Implement btfgen_get_btf() To: Andrii Nakryiko Cc: Networking , bpf , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Quentin Monnet , Rafael David Tinoco , Lorenzo Fontana , Leonardo Di Donato Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Fri, Feb 11, 2022 at 7:42 PM Andrii Nakryiko wrote: > > On Wed, Feb 9, 2022 at 2:27 PM Mauricio V=C3=A1squez wrote: > > > > The last part of the BTFGen algorithm is to create a new BTF object wit= h > > all the types that were recorded in the previous steps. > > > > This function performs two different steps: > > 1. Add the types to the new BTF object by using btf__add_type(). Some > > special logic around struct and unions is implemented to only add the > > members that are really used in the field-based relocations. The type > > ID on the new and old BTF objects is stored on a map. > > 2. Fix all the type IDs on the new BTF object by using the IDs saved in > > the previous step. > > > > Signed-off-by: Mauricio V=C3=A1squez > > Signed-off-by: Rafael David Tinoco > > Signed-off-by: Lorenzo Fontana > > Signed-off-by: Leonardo Di Donato > > --- > > tools/bpf/bpftool/gen.c | 136 +++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 135 insertions(+), 1 deletion(-) > > > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > > index c3e34db2ec8a..1efc7f3c64b2 100644 > > --- a/tools/bpf/bpftool/gen.c > > +++ b/tools/bpf/bpftool/gen.c > > @@ -1481,10 +1481,144 @@ static int btfgen_record_obj(struct btfgen_inf= o *info, const char *obj_path) > > return err; > > } > > > > +static unsigned int btfgen_get_id(struct hashmap *ids, unsigned int ol= d) > > +{ > > + uintptr_t new; > > + > > + if (!hashmap__find(ids, uint_as_hash_key(old), (void **)&new)) > > + /* return id for BTF_KIND_VOID as it's possible that th= e > > + * ID we're looking for is the type of a pointer that > > + * we're not adding. > > + */ > > + return 0; > > + > > + return (unsigned int)(uintptr_t)new; > > +} > > + > > +static int btfgen_add_id(struct hashmap *ids, unsigned int old, unsign= ed int new) > > +{ > > + return hashmap__add(ids, uint_as_hash_key(old), uint_as_hash_ke= y(new)); > > +} > > + > > +static int btfgen_remap_id(__u32 *type_id, void *ctx) > > +{ > > + struct hashmap *ids =3D ctx; > > + > > + *type_id =3D btfgen_get_id(ids, *type_id); > > + > > + return 0; > > +} > > + > > /* Generate BTF from relocation information previously recorded */ > > static struct btf *btfgen_get_btf(struct btfgen_info *info) > > { > > - return ERR_PTR(-EOPNOTSUPP); > > + struct btf *btf_new =3D NULL; > > + struct hashmap *ids =3D NULL; > > + unsigned int i; > > + int err =3D 0; > > + > > + btf_new =3D btf__new_empty(); > > + if (!btf_new) { > > + err =3D -errno; > > + goto err_out; > > + } > > + > > + ids =3D hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL); > > + if (IS_ERR(ids)) { > > + err =3D PTR_ERR(ids); > > + goto err_out; > > + } > > + > > + /* first pass: add all marked types to btf_new and add their ne= w ids to the ids map */ > > + for (i =3D 1; i < btf__type_cnt(info->marked_btf); i++) { > > small nit: why keep calling btf__type_cnt() on each iteration? store > it as n =3D btf__type_cnt(...) and do i < n ? Fixed > > > + const struct btf_type *cloned_type, *btf_type; > > + int new_id; > > + > > + cloned_type =3D btf__type_by_id(info->marked_btf, i); > > + > > + if (cloned_type->name_off !=3D MARKED) > > + continue; > > see, if you did > > #define MARKED (1<<31) > > and did > > t->name_off |=3D MARKED > > everywhere, then you wouldn't need src_btf anymore, as you'd just > restore original name_off right here with t->name_off &=3D ~MARKED. > > But it's fine, just wanted to point out why I wanted to use one bit, > so that original values are still available. I see, thanks for the explanation. In both cases a BTF copy to pass to libbpf is needed, hence I'd say there's not that much difference. > > > + > > + btf_type =3D btf__type_by_id(info->src_btf, i); > > + > > + /* add members for struct and union */ > > + if (btf_is_struct(btf_type) || btf_is_union(btf_type)) = { > > btf_is_composite(btf_type) > > > + struct btf_type *btf_type_cpy; > > + int nmembers =3D 0, idx_dst, idx_src; > > + size_t new_size; > > + > > + /* calculate nmembers */ > > + for (idx_src =3D 0; idx_src < btf_vlen(cloned_t= ype); idx_src++) { > > + struct btf_member *cloned_m =3D btf_mem= bers(cloned_type) + idx_src; > > a bit nicer pattern is: > > > struct btf_member *m =3D btf_members(cloned_type); > int vlen =3D btf_vlen(cloned_type) > > for (i =3D 0; i < vlen; i++, m++) { > } > > That way you don't have to re-calculate member > Reworked the code with the other suggestions below. > > + > > + if (cloned_m->name_off =3D=3D MARKED) > > + nmembers++; > > + } > > + > > + new_size =3D sizeof(struct btf_type) + nmembers= * sizeof(struct btf_member); > > + > > + btf_type_cpy =3D malloc(new_size); > > + if (!btf_type_cpy) > > + goto err_out; > > + > > + /* copy btf type */ > > + *btf_type_cpy =3D *btf_type; > > + > > + idx_dst =3D 0; > > + for (idx_src =3D 0; idx_src < btf_vlen(cloned_t= ype); idx_src++) { > > + struct btf_member *btf_member_src, *btf= _member_dst; > > + struct btf_member *cloned_m =3D btf_mem= bers(cloned_type) + idx_src; > > + > > + /* copy only members that are marked as= used */ > > + if (cloned_m->name_off !=3D MARKED) > > + continue; > > + > > + btf_member_src =3D btf_members(btf_type= ) + idx_src; > > + btf_member_dst =3D btf_members(btf_type= _cpy) + idx_dst; > > + > > + *btf_member_dst =3D *btf_member_src; > > + > > + idx_dst++; > > + } > > + > > + /* set new vlen */ > > + btf_type_cpy->info =3D btf_type_info(btf_kind(b= tf_type_cpy), nmembers, > > + btf_kflag(bt= f_type_cpy)); > > + > > + err =3D btf__add_type(btf_new, info->src_btf, b= tf_type_cpy); > > + free(btf_type_cpy); > > hmm.. this malloc and the rest still feels clunky... why not do it > explicitly with btf__add_struct()/btf__add_union() and then > btf__add_field() for each marked field? You also won't need to > pre-calculate the number of members (libbpf will adjust number of > members automatically, it's pretty nice API, try it). > You're right. Code looks better with this API. > You can also use err =3D err ?: btf__add_xxx() pattern to minimize error > handling conditionals > mmm, I didn't find a place where it could improve the code in this case. > > > > + } else { > > + err =3D btf__add_type(btf_new, info->src_btf, b= tf_type); > > + } > > + > > + if (err < 0) > > + goto err_out; > > + > > + new_id =3D err; > > + > > + /* add ID mapping */ > > + err =3D btfgen_add_id(ids, i, new_id); > > Why using clunky hashmap API if we are talking about mapping > sequential integers? Just allocate an array of btf__type_cnt() > integers and use that as a straightforward map? > Makes sense. Probably a hashmap will use a bit less memory but I think the readability improvement is worth it. > > > + if (err) > > + goto err_out; > > + } > > + > > + /* second pass: fix up type ids */ > > + for (i =3D 1; i < btf__type_cnt(btf_new); i++) { > > + struct btf_type *btf_type =3D (struct btf_type *) btf__= type_by_id(btf_new, i); > > + > > + err =3D btf_type_visit_type_ids(btf_type, btfgen_remap_= id, ids); > > + if (err) > > + goto err_out; > > + } > > + > > + hashmap__free(ids); > > + return btf_new; > > + > > +err_out: > > + btf__free(btf_new); > > + hashmap__free(ids); > > + errno =3D -err; > > + return NULL; > > } > > > > /* Create minimized BTF file for a set of BPF objects. > > -- > > 2.25.1 > >