Re: [PATCHES 0/2] Introduce --btf_features=+extra_features syntax

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Tue, Apr 23, 2024 at 10:02:29AM +0100, Alan Maguire wrote:
> On 23/04/2024 03:29, Daniel Xu wrote:
> > On Fri, Apr 19, 2024 at 05:57:43PM -0300, Arnaldo Carvalho de Melo wrote:
> >> 	Please take a look if you agree this is a more compact, less
> >> confusing way of asking for the set of standard BTF features + some
> >> extra features such as 'reproducible_build'.
> >>
> >> 	We have this in perf, for things like:
> >>
> >> ⬢[acme@toolbox pahole]$ perf report -h -F 
> >>
> >>  Usage: perf report [<options>]
> >>
> >>     -F, --fields <key[,keys...]>
> >>                           output field(s): overhead period sample  overhead overhead_sys
> >>                           overhead_us overhead_guest_sys overhead_guest_us overhead_children
> >>                           sample period weight1 weight2 weight3 ins_lat retire_lat
> >>                           p_stage_cyc pid comm dso symbol parent cpu socket
> >>                           srcline srcfile local_weight weight transaction trace
> >>                           symbol_size dso_size cgroup cgroup_id ipc_null time
> >>                           code_page_size local_ins_lat ins_lat local_p_stage_cyc
> >>                           p_stage_cyc addr local_retire_lat retire_lat simd
> >>                           type typeoff symoff dso_from dso_to symbol_from symbol_to
> >>                           mispredict abort in_tx cycles srcline_from srcline_to
> >>                           ipc_lbr addr_from addr_to symbol_daddr dso_daddr locked
> >>                           tlb mem snoop dcacheline symbol_iaddr phys_daddr data_page_size
> >>                           blocked
> >>
> >> ⬢[acme@toolbox pahole]$
> >>
> >> From the 'perf report' man page for '-F':
> >>
> >>         If the keys starts with a prefix '+', then it will append the specified
> >>         field(s) to the default field order. For example: perf report -F +period,sample.

> > I think for perf it makes sense to have compact representation b/c
> > folks might be doing a lot of ad-hoc work. But encoding BTF seems more
> > like a write-once, read mostly. So having `+` notation doesn't feel like

In the case where documentation style is prefered, i.e. a write once
read mostly, as you said, then use the most descriptive form.

> > it'd help that much.

> > As someone who's not seen that style of syntax before, it's not
> > immediately obvious what it does. But seeing `all`, I have a pretty
> > good idea.
 
> One thing we should probably bear in mind here is that for kernel builds
> we will always explicitly call out the set of features we want rather
> than use "all". So the "all" support is really more of a shortcut for
> developers who run pahole standalone for testing BTF encoding. It is
> still confusing though.

Agreed, multiple people agreed 'all' is confusing as not _all_ BTF
features are selected by it.
 
> The +/- approach seems fine to me especially if there are precedents in

Yeah, so we'll have a very compact way of adding (and removing, if we
feel the need, by prefixing a undesired feature that is present in the
'default' set with -) features, in addition to a more detailed way,
i.e. these will be equivalent:

	--btf_features=default,reproducible_build

and:

	--btf_features=+reproducible_build

> other tools; maybe we should also switch name to "default" instead of
> "all" at the same time tho? The notion of default values internal to

I'm ok with this, so please send a patch renaming 'all' to 'default', on
top of what is now in the 'next' branch.

- Arnaldo

> pahole (when BTF features aren't explicitly set) isn't exposed to the
> user, so I _think_ we can get away with using that term. We could
> probably do a bit of internal renaming - set_btf_features_default() ->
> set_btf_features_minimal() - to call these the minimal BTF features or
> something similar..