Re: [PATCH bpf-next v4 3/4] bpftool: btf: Support dumping a specific types from file

[Quentin Monnet]

2024-12-13 09:45 UTC-0700 ~ Daniel Xu <dxu@xxxxxxxxx>
> Hi Quentin,
> 
> On Fri, Dec 13, 2024 at 03:17:36PM GMT, Quentin Monnet wrote:
>> 2024-12-12 18:24 UTC-0700 ~ Daniel Xu <dxu@xxxxxxxxx>
>>> Some projects, for example xdp-tools [0], prefer to check in a minimized
>>> vmlinux.h rather than the complete file which can get rather large.
>>>
>>> However, when you try to add a minimized version of a complex struct (eg
>>> struct xfrm_state), things can get quite complex if you're trying to
>>> manually untangle and deduplicate the dependencies.
>>>
>>> This commit teaches bpftool to do a minimized dump of a specific types by
>>> providing a optional root_id argument(s).
>>>
>>> Example usage:
>>>
>>>     $ ./bpftool btf dump file ~/dev/linux/vmlinux | rg "STRUCT 'xfrm_state'"
>>>     [12643] STRUCT 'xfrm_state' size=912 vlen=58
>>>
>>>     $ ./bpftool btf dump file ~/dev/linux/vmlinux root_id 12643 format c
>>>     #ifndef __VMLINUX_H__
>>>     #define __VMLINUX_H__
>>>
>>>     [..]
>>>
>>>     struct xfrm_type_offload;
>>>
>>>     struct xfrm_sec_ctx;
>>>
>>>     struct xfrm_state {
>>>             possible_net_t xs_net;
>>>             union {
>>>                     struct hlist_node gclist;
>>>                     struct hlist_node bydst;
>>>             };
>>>             union {
>>>                     struct hlist_node dev_gclist;
>>>                     struct hlist_node bysrc;
>>>             };
>>>             struct hlist_node byspi;
>>>     [..]
>>>
>>> [0]: https://github.com/xdp-project/xdp-tools/blob/master/headers/bpf/vmlinux.h
>>>
>>> Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
>>> ---
>>>  .../bpf/bpftool/Documentation/bpftool-btf.rst |  8 +++-
>>>  tools/bpf/bpftool/btf.c                       | 39 ++++++++++++++++++-
>>>  2 files changed, 43 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-btf.rst b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
>>> index 245569f43035..dbe6d6d94e4c 100644
>>> --- a/tools/bpf/bpftool/Documentation/bpftool-btf.rst
>>> +++ b/tools/bpf/bpftool/Documentation/bpftool-btf.rst
>>> @@ -24,7 +24,7 @@ BTF COMMANDS
>>>  =============
>>>  
>>>  | **bpftool** **btf** { **show** | **list** } [**id** *BTF_ID*]
>>> -| **bpftool** **btf dump** *BTF_SRC* [**format** *FORMAT*]
>>> +| **bpftool** **btf dump** *BTF_SRC* [**format** *FORMAT*] [**root_id** *ROOT_ID*]
>>>  | **bpftool** **btf help**
>>>  |
>>>  | *BTF_SRC* := { **id** *BTF_ID* | **prog** *PROG* | **map** *MAP* [{**key** | **value** | **kv** | **all**}] | **file** *FILE* }
>>> @@ -43,7 +43,7 @@ bpftool btf { show | list } [id *BTF_ID*]
>>>      that hold open file descriptors (FDs) against BTF objects. On such kernels
>>>      bpftool will automatically emit this information as well.
>>>  
>>> -bpftool btf dump *BTF_SRC* [format *FORMAT*]
>>> +bpftool btf dump *BTF_SRC* [format *FORMAT*] [root_id *ROOT_ID*]
>>>      Dump BTF entries from a given *BTF_SRC*.
>>>  
>>>      When **id** is specified, BTF object with that ID will be loaded and all
>>> @@ -67,6 +67,10 @@ bpftool btf dump *BTF_SRC* [format *FORMAT*]
>>>      formatting, the output is sorted by default. Use the **unsorted** option
>>>      to avoid sorting the output.
>>>  
>>> +    **root_id** option can be used to filter a dump to a single type and all
>>> +    its dependent types. It cannot be used with any other types of filtering.
>>> +    It can be passed multiple times to dump multiple types.
>>> +
>>>  bpftool btf help
>>>      Print short help message.
>>>  
>>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>>> index 3e995faf9efa..2636655ac180 100644
>>> --- a/tools/bpf/bpftool/btf.c
>>> +++ b/tools/bpf/bpftool/btf.c
>>> @@ -27,6 +27,8 @@
>>>  #define KFUNC_DECL_TAG		"bpf_kfunc"
>>>  #define FASTCALL_DECL_TAG	"bpf_fastcall"
>>>  
>>> +#define MAX_ROOT_IDS		16
>>> +
>>>  static const char * const btf_kind_str[NR_BTF_KINDS] = {
>>>  	[BTF_KIND_UNKN]		= "UNKNOWN",
>>>  	[BTF_KIND_INT]		= "INT",
>>> @@ -880,7 +882,8 @@ static int do_dump(int argc, char **argv)
>>>  {
>>>  	bool dump_c = false, sort_dump_c = true;
>>>  	struct btf *btf = NULL, *base = NULL;
>>> -	__u32 root_type_ids[2];
>>> +	__u32 root_type_ids[MAX_ROOT_IDS];
>>> +	bool have_id_filtering;
>>>  	int root_type_cnt = 0;
>>>  	__u32 btf_id = -1;
>>>  	const char *src;
>>> @@ -974,6 +977,8 @@ static int do_dump(int argc, char **argv)
>>>  		goto done;
>>>  	}
>>>  
>>> +	have_id_filtering = !!root_type_cnt;
>>> +
>>>  	while (argc) {
>>>  		if (is_prefix(*argv, "format")) {
>>>  			NEXT_ARG();
>>> @@ -993,6 +998,36 @@ static int do_dump(int argc, char **argv)
>>>  				goto done;
>>>  			}
>>>  			NEXT_ARG();
>>> +		} else if (is_prefix(*argv, "root_id")) {
>>> +			__u32 root_id;
>>> +			char *end;
>>> +
>>> +			if (have_id_filtering) {
>>> +				p_err("cannot use root_id with other type filtering");
>>> +				err = -EINVAL;
>>> +				goto done;
>>> +			} else if (root_type_cnt == MAX_ROOT_IDS) {
>>> +				p_err("only %d root_id are supported", MAX_ROOT_IDS);
>>
>>
>> I doubt users will often reach this limit, but if they do, the message
>> can be confusing, because MAX_ROOT_IDS also accounts for root_type_ids[]
>> cells used when we pass map arguments ("key" or "value" or "kv"), so you
>> could pass 15 "root_id" on the command line and get a message telling
>> only 16 are supported.
>>
>> Maybe add a counter to tell how many were defined from the rest of the
>> command line, and adjust the value in the error message?
> 
> The above `if (have_id_filtering)` check prevents mixing key/value/kv
> map args with root_id. That ought to prevent overcounting, right?

Ah, you're right, you even mentioned it in the docs, sorry. All good on
that side, then.

Regarding the restriction, would you mind making the mention from the
man page a bit more explicit, please? Something along:

    [...] It cannot be used with any other types of filtering, such as
    the "key", "value", or "kv" arguments when dumping BTF for a map.
    **root_id** can be passed multiple times...

Thanks,
Quentin