| Message ID | 20240419205747.1102933-1-acme@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce --btf_features=+extra_features syntax | expand |
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. [..] Thanks, Daniel
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
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..
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. - Arnaldo Arnaldo Carvalho de Melo (2): pahole: Factor out routine to process "--btf_features=all" pahole: Allow asking for extra features using the '+' prefix in --btf_features man-pages/pahole.1 | 6 ++++++ pahole.c | 23 ++++++++++++++++------- tests/reproducible_build.sh | 2 +- 3 files changed, 23 insertions(+), 8 deletions(-)