Re: [PATCH v10 0/4] Introduce perf check subcommand

[Thomas Richter <tmricht@linux.ibm.com>]

On 6/26/24 15:16, 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-v10
> 
> Changelog
> =========
> 
> 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 (3):
>   perf check: introduce check subcommand
>   perf version: update --build-options to use 'supported_features' array
>   perf tests task_analyzer: use perf check for libtraceevent support
> 
> Athira Rajeev (1):
>   tools/perf/tests: Update probe_vfs_getname.sh script to use perf check
>     feature
> 
>  tools/perf/Build                              |   1 +
>  tools/perf/Documentation/perf-check.txt       |  79 ++++++++
>  tools/perf/builtin-check.c                    | 179 ++++++++++++++++++
>  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 +-
>  10 files changed, 297 insertions(+), 41 deletions(-)
>  create mode 100644 tools/perf/Documentation/perf-check.txt
>  create mode 100644 tools/perf/builtin-check.c
> 

I have a running linux-next kernel on fedora 40.
I applied your patches onto linux-next head and got these results:

 # git am ~/gupta1.patch
 Applying: perf check: introduce check subcommand
 .git/rebase-apply/patch:110: new blank line at EOF.
 +
 warning: 1 line adds whitespace errors.
 # git am ~/gupta2.patch

The other patches apply cleanly.
The compile of perf fails with this error:
 # make
 ...
 builtin-check.c: In function ‘cmd_check’:
 builtin-check.c:167:44: error: passing argument 5 of
       ‘parse_options_subcommand’ discards ‘const’ qualifier
       from pointer target type [-Werror=discarded-qualifiers]
  167 |                         check_subcommands, check_usage, 0);
      |                                            ^~~~~~~~~~~
 In file included from builtin-check.c:10:
 /root/mirror-linux-next/tools/perf/libsubcmd/include/subcmd/
       parse-options.h:180:45: note: expected ‘const char **’
       but argument is of type ‘const char * const*’
  180 |                                 const char *usagestr[], int flags);
      |                                 ~~~~~~~~~~~~^~~~~~~~~~
  CC      builtin-timechart.o
  CC      builtin-sched.o
 cc1: all warnings being treated as errors
 make[3]: *** [/root/mirror-linux-next/tools/build/Makefile.build:106: builtin-check.o] Error 1
 make[3]: *** Waiting for unfinished jobs....
 make[2]: *** [Makefile.perf:728: perf-in.o] Error 2
 make[1]: *** [Makefile.perf:265: sub-make] Error 2
 make: *** [Makefile:70: all] Error 2
 #

Hope this helps...

-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
IBM Deutschland Research & Development GmbH

Vorsitzender des Aufsichtsrats: Wolfgang Wendt

Geschäftsführung: David Faller

Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294