Re: [PATCH v13 0/7] Introduce perf check subcommand

[Aditya Gupta <adityag@linux.ibm.com>]

Linux-ci build test results for all patches (all green, except some 
g5_defconfig in kernel+qemu, which is red irrespective of this series):  
https://github.com/adi-g15-ibm/linux-ci/actions


Thanks,

Aditya Gupta


On 18/07/24 14:29, Aditya Gupta wrote:
> The Problem
> ===========
>
> Currently the presence of a feature is checked with a combination of
> perf version --build-options and greps, such as:
>
>      perf version --build-options | grep " on .* HAVE_FEATURE"
>
> This relies on the output of perf version, and is a common pattern in tests.
>
> Proposed solution
> =================
>
> As suggested by contributors in:
> https://lore.kernel.org/linux-perf-users/ZMPWk5K63tadmDlU@kernel.org/
>
> Introduce a subcommand "perf check feature", with which
> scripts can test for presence of a feature or multiple features, such as:
>
>      perf check feature HAVE_LIBTRACEEVENT  (feature macro)
>
>      or
>
>      perf check feature libtraceevent       (feature name)
>
>      or
>
>      perf check feature LibTraceEvent       (case-insensitive)
>
>      or
>
>      perf check feature libtraceevent,bpf   (multiple features)
>
> The usage of "perf version --build-options | grep" has been replaced in two
> tests, with "perf check feature" command
>
> Also, to not duplicate the same feature list at multiple places, a new global
> 'supported_features' array has been introduced in builtin.h, so both commands
> 'perf check feature' and 'perf version --build-options' use the same array
>
> 'supported_features' feature is an array of 'struct feature_support', which
> also has the name of the feature, macro used to test it's presence, and a
> is_builtin member, which will be 0 if feature not built-in, and 1 if built-in
>
> Architectures Tested
> ====================
> * x86_64
> * ppc64le
>
> Commands ran for testing (Fedora & RHEL):
>
>      sudo dnf install -y libtraceevent-devel
>      make clean
>      make -j$(nproc)
>      ./perf check feature libtraceevent,bpf; echo Return Code: $?
>      ./perf check feature libtraceevent; echo Return Code: $?
>      
>      sudo ./perf test -v "task-analyzer"
>      sudo ./perf test -v "probe libc's inet_pton & backtrace it with ping"
>      sudo ./perf test -v "Use vfs_getname probe to get syscall args filenames"
>      
>      sudo dnf remove -y libtraceevent-devel
>      make clean
>      make NO_LIBTRACEEVENT=1 -j$(nproc)
>      ./perf check feature libtraceevent,bpf; echo Return Code: $?
>      ./perf check feature libtraceevent; echo Return Code: $?
>      
>      sudo ./perf test -v "task-analyzer"
>      sudo ./perf test -v "probe libc's inet_pton & backtrace it with ping"
>      sudo ./perf test -v "Use vfs_getname probe to get syscall args filenames"
>
> Git tree
> ========
>
> Git tree with this patch series applied for testing:
> https://github.com/adi-g15-ibm/linux/tree/perf-check-feature-v13
>
> Changelog
> =========
>
> v13:
> + patch #1: add fix for parse_options_subcommand not setting usage string
> + patch #2: if unknown feature name passed, print "please use 'perf version
>    --build-options' to see which ones are available"
> + patch #6: fix inconsistency in feature names (dwarf-unwind-support & get_cpuid)
> + patch #7: add more features to feature_list
>
> v12
> + patch #1: fix comment to mention argv[0] instead of argv[1]
> + patch #2: fix alignment
>
> v11
> + patch #1: fix build error due to const *const instead of const*
>
> v10
> + patch #1: use 'strdup' instead of 'malloc+memcpy'
> + patch #1: replace '-q' with '--quiet' in doc
> + patch #1: add usage for perf check
>
> V9
> + make 'feature' a subcommand instead of an option
> + make feature name/macro check case-insensitive
> + rename 'FEATURE_SUPPORT' as 'FEATURE_STATUS'
> + rebase on upstream perf-tools-next
>
> V8
> + handle return value of 'malloc' in patch #1
> + fix error due to strncpy depending on source string's length
>
> V7
> + modified patch #1 to fix compile issue, and add feature to allow
>    multiple comma-separated features
>
> V6
> + rebased to perf-tools-next/perf-tools-next
> + modified patch #1 to include FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL)
>
> V5
> + invert return value of 'has_support', but return value of perf check --feature
> according to shell convention
>
> V4
> + invert return value of perf check --feature
>
> V3
> + simplified has_support code in builtin-check.c (patch #1)
> + modified patch #3 and patch #4 according to change in return value in patch #1
>
> V2
> + improved the patch series with suggestions from Namhyung
> + fix incorrect return value, added -q option, and moved array definition to
>    perf-check.c
>
> V1
> + changed subcommand name to 'perf check --feature'
> + added documentation for perf check
> + support both macro (eg. HAVE_LIBTRACEEVENT), and name (eg. libtraceevent) as
>    input to 'perf check --feature'
> + change subject and descriptions of all patch mentioning perf check instead of
>    perf build
>
> V0: Previous patch series: https://lore.kernel.org/linux-perf-users/20230825061125.24312-1-adityag@linux.ibm.com/
>
> Aditya Gupta (6):
>    tools/lib/subcmd: Don't free the usage string
>    perf check: Introduce 'check' subcommand
>    perf version: Update --build-options to use 'supported_features' array
>    tools/perf/tests: Update test_task_analyzer.sh to use perf check
>      feature
>    perf: Fix inconsistencies in feature names
>    perf: Add more features to supported_features list
>
> Athira Rajeev (1):
>    tools/perf/tests: Update probe_vfs_getname.sh script to use perf check
>      feature
>
>   tools/lib/subcmd/parse-options.c              |   8 +-
>   tools/perf/Build                              |   1 +
>   tools/perf/Documentation/perf-check.txt       | 106 +++++++++
>   tools/perf/builtin-check.c                    | 205 ++++++++++++++++++
>   tools/perf/builtin-kmem.c                     |   2 +
>   tools/perf/builtin-kvm.c                      |   3 +
>   tools/perf/builtin-kwork.c                    |   3 +
>   tools/perf/builtin-lock.c                     |   3 +
>   tools/perf/builtin-mem.c                      |   3 +
>   tools/perf/builtin-sched.c                    |   3 +
>   tools/perf/builtin-version.c                  |  43 +---
>   tools/perf/builtin.h                          |  17 ++
>   tools/perf/perf.c                             |   1 +
>   .../perf/tests/shell/lib/probe_vfs_getname.sh |   4 +-
>   .../shell/record+probe_libc_inet_pton.sh      |   5 +-
>   .../shell/record+script_probe_vfs_getname.sh  |   5 +-
>   tools/perf/tests/shell/test_task_analyzer.sh  |   4 +-
>   17 files changed, 370 insertions(+), 46 deletions(-)
>   create mode 100644 tools/perf/Documentation/perf-check.txt
>   create mode 100644 tools/perf/builtin-check.c
>