On 23/04/2024 03:29, Daniel Xu wrote: > Hi Arnaldo, > > On Fri, Apr 19, 2024 at 05:57:43PM -0300, Arnaldo Carvalho de Melo wrote: >> Hi, >> >> 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 > 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. The +/- approach seems fine to me especially if there are precedents in 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 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.. > [..] > > Thanks, > Daniel
