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..
