From: Alan Maguire <alan.maguire@oracle.com>
To: Quentin Monnet <quentin@isovalent.com>,
ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
jolsa@kernel.org
Cc: eddyz87@gmail.com, martin.lau@linux.dev, song@kernel.org,
yonghong.song@linux.dev, john.fastabend@gmail.com,
kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
masahiroy@kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH v4 bpf-next 10/17] bpftool: add BTF dump "format meta" to dump header/metadata
Date: Thu, 16 Nov 2023 09:16:21 +0000 [thread overview]
Message-ID: <0ce1e802-d0dc-36bc-0342-e87fcc6914ad@oracle.com> (raw)
In-Reply-To: <F129A42C-3E63-4B94-BBB6-5B13AFC9A73A@isovalent.com>
On 15/11/2023 14:51, Quentin Monnet wrote:
>
>
> On 15 November 2023 03:45:41 GMT-05:00, Alan Maguire <alan.maguire@oracle.com> wrote:
>> On 14/11/2023 05:10, Quentin Monnet wrote:
>>> 2023-11-12 12:49 UTC+0000 ~ Alan Maguire <alan.maguire@oracle.com>
>>>> Provide a way to dump BTF metadata info via bpftool; this
>>>> consists of BTF size, header fields and kind layout info
>>>> (if available); for example
>>>>
>>>> $ bpftool btf dump file vmlinux format meta
>>>> size 5161076
>>>> magic 0xeb9f
>>>> version 1
>>>> flags 0x1
>>>> hdr_len 40
>>>> type_len 3036368
>>>> type_off 0
>>>> str_len 2124588
>>>> str_off 3036368
>>>> kind_layout_len 80
>>>> kind_layout_off 5160956
>>>> crc 0x64af901b
>>>> base_crc 0x0
>>>> kind 0 UNKNOWN flags 0x0 info_sz 0 elem_sz 0
>>>> kind 1 INT flags 0x0 info_sz 0 elem_sz 0
>>>> kind 2 PTR flags 0x0 info_sz 0 elem_sz 0
>>>> kind 3 ARRAY flags 0x0 info_sz 0 elem_sz 0
>>>> kind 4 STRUCT flags 0x35 info_sz 0 elem_sz 0
>>>> ...
>>>>
>>>> JSON output is also supported:
>>>>
>>>> $ bpftool -j btf dump file vmlinux format meta
>>>> {"size":5161076,"header":{"magic":60319,"version":1,"flags":1,"hdr_len":40,"type_len":3036368,"type_off":0,"str_len":2124588,"str_off":3036368,"kind_layout_len":80,"kind_layout_offset":5160956,"crc":1689227291,"base_crc":0},"kind_layouts":[{"kind":0,"name":"UNKNOWN","flags":0,"info_sz":0,"elem_sz":0},{"kind":1,"name":"INT","flags":0,"info_sz":0,"elem_sz":0},{"kind":2,"name":"PTR","flags":0,"info_sz":0,"elem_sz":0},{"kind":3,"name":"ARRAY","flags":0,"info_sz":0,"elem_sz":0},{"kind":4,"name":"STRUCT","flags":53,"info_sz":0,"elem_sz":0},{"kind":5,"name":"UNION","flags":0,"info_sz":0,"elem_sz":0},{"kind":6,"name":"ENUM","flags":60319,"info_sz":1,"elem_sz":1},{"kind":7,"name":"FWD","flags":40,"info_sz":0,"elem_sz":0},{"kind":8,"name":"TYPEDEF","flags":0,"info_sz":0,"elem_sz":0},{"kind":9,"name":"VOLATILE","flags":0,"info_sz":0,"elem_sz":0},{"kind":10,"name":"CONST","flags":0,"info_sz":0,"elem_sz":0},{"kind":11,"name":"RESTRICT","flags":1,"info_sz":0,"elem_sz":0},{"kind":12,"name":"FUNC","flags":0,"info_sz":0,"elem_sz":0},{"kind":13,"name":"FUNC_PROTO","flags":80,"info_sz":0,"elem_sz":0},{"kind":14,"name":"VAR","flags":0,"info_sz":0,"elem_sz":0},{"kind":15,"name":"DATASEC","flags":0,"info_sz":0,"elem_sz":0},{"kind":16,"name":"FLOAT","flags":53,"info_sz":0,"elem_sz":0},{"kind":17,"name":"DECL_TAG","flags":0,"info_sz":0,"elem_sz":0},{"kind":18,"name":"TYPE_TAG","flags":11441,"info_sz":3,"elem_sz":0},{"kind":19,"name":"ENUM64","flags":0,"info_sz":0,"elem_sz":0}]}
>>>>
>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>> ---
>>>> tools/bpf/bpftool/bash-completion/bpftool | 2 +-
>>>> tools/bpf/bpftool/btf.c | 91 ++++++++++++++++++++++-
>>>> 2 files changed, 90 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
>>>> index 6e4f7ce6bc01..157c3afd8247 100644
>>>> --- a/tools/bpf/bpftool/bash-completion/bpftool
>>>> +++ b/tools/bpf/bpftool/bash-completion/bpftool
>>>> @@ -937,7 +937,7 @@ _bpftool()
>>>> return 0
>>>> ;;
>>>> format)
>>>> - COMPREPLY=( $( compgen -W "c raw" -- "$cur" ) )
>>>> + COMPREPLY=( $( compgen -W "c raw meta" -- "$cur" ) )
>>>> ;;
>>>> *)
>>>> # emit extra options
>>>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>>>> index 91fcb75babe3..208f3a587534 100644
>>>> --- a/tools/bpf/bpftool/btf.c
>>>> +++ b/tools/bpf/bpftool/btf.c
>>>> @@ -504,6 +504,88 @@ static int dump_btf_c(const struct btf *btf,
>>>> return err;
>>>> }
>>>>
>>>> +static int dump_btf_meta(const struct btf *btf)
>>>> +{
>>>> + const struct btf_header *hdr;
>>>> + const struct btf_kind_layout *k;
>>>> + __u8 i, nr_kinds = 0;
>>>> + const void *data;
>>>> + __u32 data_sz;
>>>> +
>>>> + data = btf__raw_data(btf, &data_sz);
>>>> + if (!data)
>>>> + return -ENOMEM;
>>>> + hdr = data;
>>>> + if (json_output) {
>>>> + jsonw_start_object(json_wtr); /* btf metadata object */
>>>
>>> Nit: Please make sure to be consistent when aligning these comments:
>>> there are several occurrences with spaces (here three spaces), several
>>> ones with tabs. For these, I'd prefer tabs to align the start and end
>>> comments for a given object/array, although I don't really using a
>>> single space as well as long as we keep it consistent.
>>>
>>>> + jsonw_uint_field(json_wtr, "size", data_sz);
>>>> + jsonw_name(json_wtr, "header");
>>>> + jsonw_start_object(json_wtr); /* btf header object */
>>>> + jsonw_uint_field(json_wtr, "magic", hdr->magic);
>>>> + jsonw_uint_field(json_wtr, "version", hdr->version);
>>>> + jsonw_uint_field(json_wtr, "flags", hdr->flags);
>>>> + jsonw_uint_field(json_wtr, "hdr_len", hdr->hdr_len);
>>>> + jsonw_uint_field(json_wtr, "type_len", hdr->type_len);
>>>> + jsonw_uint_field(json_wtr, "type_off", hdr->type_off);
>>>> + jsonw_uint_field(json_wtr, "str_len", hdr->str_len);
>>>> + jsonw_uint_field(json_wtr, "str_off", hdr->str_off);
>>>> + } else {
>>>> + printf("size %-10d\n", data_sz);
>>>> + printf("magic 0x%-10x\nversion %-10d\nflags 0x%-10x\nhdr_len %-10d\n",
>>>> + hdr->magic, hdr->version, hdr->flags, hdr->hdr_len);
>>>> + printf("type_len %-10d\ntype_off %-10d\n", hdr->type_len, hdr->type_off);
>>>> + printf("str_len %-10d\nstr_off %-10d\n", hdr->str_len, hdr->str_off);
>>>> + }
>>>> +
>>>> + if (hdr->hdr_len < sizeof(struct btf_header)) {
>>>> + if (json_output) {
>>>> + jsonw_end_object(json_wtr); /* header object */
>>>> + jsonw_end_object(json_wtr); /* metadata object */
>>>
>>> Similarly, can you please keep consistent comment strings? "metadata
>>> object" here vs. "end metadata" below.
>>>
>>
>> Sure, I'll fix indent consistency/naming and the docs issue in the
>> next revision. Thanks!
>>
>>>> + }
>>>> + return 0;
>>>> + }
>>>> + if (hdr->kind_layout_len > 0 && hdr->kind_layout_off > 0) {
>>>> + k = (void *)hdr + hdr->hdr_len + hdr->kind_layout_off;
>>>> + nr_kinds = hdr->kind_layout_len / sizeof(*k);
>>>> + }
>>>> + if (json_output) {
>>>> + jsonw_uint_field(json_wtr, "kind_layout_len", hdr->kind_layout_len);
>>>> + jsonw_uint_field(json_wtr, "kind_layout_offset", hdr->kind_layout_off);
>>>> + jsonw_uint_field(json_wtr, "crc", hdr->crc);
>>>> + jsonw_uint_field(json_wtr, "base_crc", hdr->base_crc);
>>>> + jsonw_end_object(json_wtr); /* end header object */
>>>> +
>>>> + if (nr_kinds > 0) {
>>>> + jsonw_name(json_wtr, "kind_layouts");
>>>> + jsonw_start_array(json_wtr);
>>>> + for (i = 0; i < nr_kinds; i++) {
>>>> + jsonw_start_object(json_wtr);
>>>> + jsonw_uint_field(json_wtr, "kind", i);
>>>> + if (i < NR_BTF_KINDS)
>>>> + jsonw_string_field(json_wtr, "name", btf_kind_str[i]);
>>>
>>> I prefer to avoid conditional fields in JSON, especially in an array
>>> it's easier to process the JSON if all items have the same structure.
>>> Would it make sense to keep the "name" field, but use jsonw_null() (or
>>> "UNKNOWN") for the value when there's no name to print?
>>>
>>
>> The only thing about UNKNOWN is that there is a BTF_KIND_UNKN that
>> is displayed as UNKNOWN; what about "?" to be consistent with the
>> non-json output (or if there's another option of course, we could
>> use that for both)? Thanks!
>
> Right, sorry, I realised just after sending my message.
> In that case we could just use a 'null' value in JSON with jsonw_null()? The object '{"name": null}' is valid. The question mark is another possibility but requires comparing strings when parsing the JSON.
>
Great idea! I'll use jsonw_null() for the next version.
I think given the fact we're not sure about the way forward on CRCs and
standalone BTF I'll probably just separate out the kind layout stuff +
bpftool dump meta parts since I _think_ at this point those pieces are
relatively uncontroversial.
Thanks again for the reviews Quentin!
Alan
> Thanks,
> Quentin
next prev parent reply other threads:[~2023-11-16 9:16 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-12 12:48 [PATCH v4 bpf-next 00/17] Add kind layout, CRCs to BTF Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 01/17] btf: add kind layout encoding, crcs to UAPI Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 02/17] libbpf: support kind layout section handling in BTF Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 03/17] libbpf: use kind layout to compute an unknown kind size Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 04/17] libbpf: add kind layout encoding, crc support Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 05/17] libbpf: BTF validation can use kind layout for unknown kinds Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 06/17] btf: support kernel parsing of BTF with kind layout Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 07/17] bpf: add BTF CRC verification where present Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 08/17] bpf: verify base BTF CRC to ensure it matches module BTF Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 09/17] kbuild, bpf: switch to --btf_features, add crc,kind_layout features Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 10/17] bpftool: add BTF dump "format meta" to dump header/metadata Alan Maguire
2023-11-14 5:10 ` Quentin Monnet
2023-11-15 8:45 ` Alan Maguire
2023-11-15 14:51 ` Quentin Monnet
2023-11-16 9:16 ` Alan Maguire [this message]
2023-11-12 12:48 ` [PATCH v4 bpf-next 11/17] bpftool: update doc to describe bpftool btf dump .. format meta Alan Maguire
2023-11-14 5:12 ` Quentin Monnet
2023-11-12 12:48 ` [PATCH v4 bpf-next 12/17] selftests/bpf: test kind encoding/decoding Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 13/17] bpf: support standalone BTF in modules Alan Maguire
2023-11-12 15:35 ` kernel test robot
2023-11-12 20:00 ` kernel test robot
2023-11-12 12:48 ` [PATCH v4 bpf-next 14/17] kbuild, bpf: allow opt-out from using split BTF for modules Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 15/17] selftests/bpf: generalize module load to support specifying a module name Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 16/17] selftests/bpf: build separate bpf_testmod module with standalone BTF Alan Maguire
2023-11-12 12:48 ` [PATCH v4 bpf-next 17/17] selftests/bpf: update btf_module test to ensure standalone BTF works Alan Maguire
2023-11-14 20:16 ` [PATCH v4 bpf-next 00/17] Add kind layout, CRCs to BTF Alan Maguire
2023-11-21 19:44 ` Andrii Nakryiko
2023-11-22 17:00 ` Alan Maguire
2023-11-22 17:42 ` Andrii Nakryiko
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=0ce1e802-d0dc-36bc-0342-e87fcc6914ad@oracle.com \
--to=alan.maguire@oracle.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=martin.lau@linux.dev \
--cc=masahiroy@kernel.org \
--cc=quentin@isovalent.com \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/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