From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG
Date: Fri, 10 Sep 2021 09:04:15 -0700 [thread overview]
Message-ID: <d310362c-08b7-7c7b-c8e3-788ec56615b6@fb.com> (raw)
In-Reply-To: <CAEf4Bza5azi3mamL8geoCPJm-jxtKYsJ6+-Yv8uEg_pBkachNg@mail.gmail.com>
On 9/8/21 10:26 PM, Andrii Nakryiko wrote:
> On Tue, Sep 7, 2021 at 4:01 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add BTF_KIND_TAG support for parsing and dedup.
>> Also added sanitization for BTF_KIND_TAG. If BTF_KIND_TAG is not
>> supported in the kernel, sanitize it to INTs.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> tools/lib/bpf/btf.c | 61 +++++++++++++++++++++++++++++++++
>> tools/lib/bpf/btf.h | 13 +++++++
>> tools/lib/bpf/btf_dump.c | 3 ++
>> tools/lib/bpf/libbpf.c | 31 +++++++++++++++--
>> tools/lib/bpf/libbpf.map | 1 +
>> tools/lib/bpf/libbpf_internal.h | 2 ++
>> 6 files changed, 108 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>> index 7cb6ebf1be37..ed02b17aad17 100644
>> --- a/tools/lib/bpf/btf.c
>> +++ b/tools/lib/bpf/btf.c
>> @@ -304,6 +304,8 @@ static int btf_type_size(const struct btf_type *t)
>> return base_size + sizeof(struct btf_var);
>> case BTF_KIND_DATASEC:
>> return base_size + vlen * sizeof(struct btf_var_secinfo);
>> + case BTF_KIND_TAG:
>> + return base_size + sizeof(struct btf_tag);
>> default:
>> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
>> return -EINVAL;
>> @@ -376,6 +378,9 @@ static int btf_bswap_type_rest(struct btf_type *t)
>> v->size = bswap_32(v->size);
>> }
>> return 0;
>> + case BTF_KIND_TAG:
>> + btf_tag(t)->comp_id = bswap_32(btf_tag(t)->comp_id);
>> + return 0;
>> default:
>> pr_debug("Unsupported BTF_KIND:%u\n", btf_kind(t));
>> return -EINVAL;
>> @@ -586,6 +591,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
>> case BTF_KIND_CONST:
>> case BTF_KIND_RESTRICT:
>> case BTF_KIND_VAR:
>> + case BTF_KIND_TAG:
>> type_id = t->type;
>> break;
>> case BTF_KIND_ARRAY:
>> @@ -2440,6 +2446,41 @@ int btf__add_datasec_var_info(struct btf *btf, int var_type_id, __u32 offset, __
>> return 0;
>> }
>>
>> +int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id)
>
> Curious about the terminology here. The string recorded in bpf_tag, is
> that a "name" of the tag, or rather a "value" of the tag? We should
> reflect that in argument names for btf__add_tag.
We can use "value" as the argument.
>
> I'll also nitpick on order of arguments. ref_type_id is always
> specified, and it points to the entire type (struct/union/func), while
> comp_id might, optionally, point inside that type. So I think the
> order should be ref_type_id followed by comp_id.
Will switch and then the argument order follows ELF file format order.
>
> Please also add a comment describing inputs (especially the -1 comp_id
> case) and outputs, like all that other btf__add_xxx() APIs.
Will do.
>
>> +{
>> + bool for_ref_type = false;
>> + struct btf_type *t;
>> + int sz, name_off;
>> +
>> + if (!name || !name[0] || comp_id < -1)
>> + return libbpf_err(-EINVAL);
>> +
>> + if (validate_type_id(ref_type_id))
>> + return libbpf_err(-EINVAL);
>> +
>> + if (btf_ensure_modifiable(btf))
>> + return libbpf_err(-ENOMEM);
>> +
>> + sz = sizeof(struct btf_type) + sizeof(struct btf_tag);
>> + t = btf_add_type_mem(btf, sz);
>> + if (!t)
>> + return libbpf_err(-ENOMEM);
>> +
>> + name_off = btf__add_str(btf, name);
>> + if (name_off < 0)
>> + return name_off;
>> +
>> + t->name_off = name_off;
>> + t->type = ref_type_id;
>> +
>> + if (comp_id == -1)
>> + for_ref_type = true;
>> + t->info = btf_type_info(BTF_KIND_TAG, 0, for_ref_type);
>> + ((struct btf_tag *)(t + 1))->comp_id = for_ref_type ? 0 : comp_id;
>
> As I mentioned in the previous patch, it feels cleaner to just have
> this -1 special value and not utilize kflag at all. It will match
> libbpf API as you defined it naturally.
will do.
>
>> +
>> + return btf_commit_type(btf, sz);
>> +}
>> +
>
> [...]
>
>> case BTF_KIND_ARRAY: {
>> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
>> index 4a711f990904..a78cf8331d49 100644
>> --- a/tools/lib/bpf/btf.h
>> +++ b/tools/lib/bpf/btf.h
>> @@ -141,6 +141,9 @@ LIBBPF_API int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz
>> LIBBPF_API int btf__add_datasec_var_info(struct btf *btf, int var_type_id,
>> __u32 offset, __u32 byte_sz);
>>
>> +/* tag contruction API */
>
> typo: construction, but I'd put it after btf__add_restrict with no comment
ack.
>
>> +LIBBPF_API int btf__add_tag(struct btf *btf, const char *name, int comp_id, int ref_type_id);
>> +
>> struct btf_dedup_opts {
>> unsigned int dedup_table_size;
>> bool dont_resolve_fwds;
>> @@ -328,6 +331,11 @@ static inline bool btf_is_float(const struct btf_type *t)
>> return btf_kind(t) == BTF_KIND_FLOAT;
>> }
>>
>> +static inline bool btf_is_tag(const struct btf_type *t)
>> +{
>> + return btf_kind(t) == BTF_KIND_TAG;
>> +}
>> +
>> static inline __u8 btf_int_encoding(const struct btf_type *t)
>> {
>> return BTF_INT_ENCODING(*(__u32 *)(t + 1));
>> @@ -396,6 +404,11 @@ btf_var_secinfos(const struct btf_type *t)
>> return (struct btf_var_secinfo *)(t + 1);
>> }
>>
>
> please add `struct btf_tag;` forward reference for those users who are
> compiling with old UAPI headers.
okay.
>
>> +static inline struct btf_tag *btf_tag(const struct btf_type *t)
>> +{
>> + return (struct btf_tag *)(t + 1);
>> +}
>> +
>> #ifdef __cplusplus
>> } /* extern "C" */
>> #endif
>
> [...]
>
>> LIBBPF_0.5.0 {
>> global:
>> + btf__add_tag;
>> bpf_map__initial_value;
>> bpf_map__pin_path;
>> bpf_map_lookup_and_delete_elem_flags;
>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
>> index 533b0211f40a..7deb86d9af51 100644
>> --- a/tools/lib/bpf/libbpf_internal.h
>> +++ b/tools/lib/bpf/libbpf_internal.h
>> @@ -69,6 +69,8 @@
>> #define BTF_VAR_SECINFO_ENC(type, offset, size) (type), (offset), (size)
>> #define BTF_TYPE_FLOAT_ENC(name, sz) \
>> BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_FLOAT, 0, 0), sz)
>> +#define BTF_TYPE_TAG_KIND_ENC(name, type) \
>
> following other macro names, it should be BTF_TYPE_TAG_ENC, no?
This is to differentiate the case from attr to member/func_argument
which is implemented in selftest test_btf.h.
But with the new scheme, we just need BTF_TYPE_TAG_ENC. Will change.
>
>> + BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_TAG, 1, 0), type), (0)
>>
>> #ifndef likely
>> #define likely(x) __builtin_expect(!!(x), 1)
>> --
>> 2.30.2
>>
next prev parent reply other threads:[~2021-09-10 16:05 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-07 23:00 [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Yonghong Song
2021-09-07 23:00 ` [PATCH bpf-next 1/9] bpf: " Yonghong Song
2021-09-09 5:09 ` Andrii Nakryiko
2021-09-10 15:55 ` Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 2/9] libbpf: rename btf_{hash,equal}_int to btf_{hash,equal}_int_tag Yonghong Song
2021-09-09 5:10 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 3/9] libbpf: add support for BTF_KIND_TAG Yonghong Song
2021-09-09 5:26 ` Andrii Nakryiko
2021-09-10 16:04 ` Yonghong Song [this message]
2021-09-07 23:01 ` [PATCH bpf-next 4/9] bpftool: " Yonghong Song
2021-09-09 5:28 ` Andrii Nakryiko
2021-09-09 5:33 ` Andrii Nakryiko
2021-09-10 16:38 ` Yonghong Song
2021-09-10 18:05 ` Andrii Nakryiko
2021-09-10 16:04 ` Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 5/9] selftests/bpf: test libbpf API function btf__add_tag() Yonghong Song
2021-09-09 5:35 ` Andrii Nakryiko
2021-09-10 16:39 ` Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 6/9] selftests/bpf: add BTF_KIND_TAG unit tests Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 7/9] selftests/bpf: test BTF_KIND_TAG for deduplication Yonghong Song
2021-09-07 23:01 ` [PATCH bpf-next 8/9] selftests/bpf: add a test with a bpf program with btf_tag attributes Yonghong Song
2021-09-09 5:39 ` Andrii Nakryiko
2021-09-07 23:01 ` [PATCH bpf-next 9/9] docs/bpf: add documentation for BTF_KIND_TAG Yonghong Song
2021-09-09 5:42 ` Andrii Nakryiko
2021-09-10 16:40 ` Yonghong Song
2021-09-10 18:05 ` Andrii Nakryiko
2021-09-09 22:45 ` [PATCH bpf-next 0/9] bpf: add support for new btf kind BTF_KIND_TAG Jose E. Marchesi
2021-09-09 23:30 ` Yonghong Song
2021-09-10 2:19 ` Jose E. Marchesi
2021-09-10 7:04 ` Yonghong Song
2021-09-10 8:31 ` Jose E. Marchesi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d310362c-08b7-7c7b-c8e3-788ec56615b6@fb.com \
--to=yhs@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox