[PATCH v11 0/4] Introduce perf check subcommand

[PATCH v11 0/4] Introduce perf check subcommand

[Aditya Gupta]

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

Changelog
=========

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

-- 
2.45.2

[PATCH v11 1/4] perf check: introduce check subcommand

[Aditya Gupta]

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"

Instead of this, introduce a subcommand "perf check feature", with which
scripts can test for presence of a feature, such as:

    perf check feature HAVE_FEATURE

'perf check feature' command is expected to have exit status of 0 if
feature is built-in, and 1 if it's not built-in or if feature is not known.

Multiple features can also be passed as a comma-separated list, in which
case the exit status will be 1 only if all of the passed features are
built-in. For example, with below command, it will have exit status of 0
only if both libtraceevent and bpf are enabled, else 1 in all other cases

    perf check feature libtraceevent,bpf

The arguments are case-insensitive.
An array 'supported_features' has also been introduced that can be used by
other commands like 'perf version --build-options', so that new features
can be added in one place, with the array

Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/Build                        |   1 +
 tools/perf/Documentation/perf-check.txt |  78 +++++++++++
 tools/perf/builtin-check.c              | 178 ++++++++++++++++++++++++
 tools/perf/builtin.h                    |  17 +++
 tools/perf/perf.c                       |   1 +
 5 files changed, 275 insertions(+)
 create mode 100644 tools/perf/Documentation/perf-check.txt
 create mode 100644 tools/perf/builtin-check.c

diff --git a/tools/perf/Build b/tools/perf/Build
index b0cb7ad8e6ac..7635cc823565 100644
--- a/tools/perf/Build
+++ b/tools/perf/Build
@@ -1,5 +1,6 @@
 perf-y += builtin-bench.o
 perf-y += builtin-annotate.o
+perf-y += builtin-check.o
 perf-y += builtin-config.o
 perf-y += builtin-diff.o
 perf-y += builtin-evlist.o
diff --git a/tools/perf/Documentation/perf-check.txt b/tools/perf/Documentation/perf-check.txt
new file mode 100644
index 000000000000..3c4a92251026
--- /dev/null
+++ b/tools/perf/Documentation/perf-check.txt
@@ -0,0 +1,78 @@
+perf-check(1)
+===============
+
+NAME
+----
+perf-check - check features in perf
+
+SYNOPSIS
+--------
+[verse]
+'perf check' [<options>]
+'perf check' {feature <feature_list>} [<options>]
+
+DESCRIPTION
+-----------
+With no options given, the 'perf check' just prints the perf version
+on the standard output.
+
+If the subcommand 'feature' is used, then status of feature is printed
+on the standard output (unless '-q' is also passed), ie. whether it is
+compiled-in/built-in or not.
+Also, 'perf check feature' returns with exit status 0 if the feature
+is built-in, otherwise returns with exit status 1.
+
+SUBCOMMANDS
+-----------
+
+feature::
+
+        Print whether feature(s) is compiled-in or not, and also returns with an
+        exit status of 0, if passed feature(s) are compiled-in, else 1.
+
+        It expects a feature list as an argument. There can be a single feature
+        name/macro, or multiple features can also be passed as a comma-separated
+        list, in which case the exit status will be 0 only if all of the passed
+        features are compiled-in.
+
+        The feature names/macros are case-insensitive.
+
+        Example Usage:
+                perf check feature libtraceevent
+                perf check feature HAVE_LIBTRACEEVENT
+                perf check feature libtraceevent,bpf
+
+        Supported feature names/macro:
+                aio                     /  HAVE_AIO_SUPPORT
+                bpf                     /  HAVE_LIBBPF_SUPPORT
+                bpf_skeletons           /  HAVE_BPF_SKEL
+                debuginfod              /  HAVE_DEBUGINFOD_SUPPORT
+                dwarf                   /  HAVE_DWARF_SUPPORT
+                dwarf_getlocations      /  HAVE_DWARF_GETLOCATIONS_SUPPORT
+                get_cpuid               /  HAVE_AUXTRACE_SUPPORT
+                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
+                libaudit                /  HAVE_LIBAUDIT_SUPPORT
+                libbfd                  /  HAVE_LIBBFD_SUPPORT
+                libelf                  /  HAVE_LIBELF_SUPPORT
+                libcrypto               /  HAVE_LIBCRYPTO_SUPPORT
+                libdw-dwarf-unwind      /  HAVE_DWARF_SUPPORT
+                libnuma                 /  HAVE_LIBNUMA_SUPPORT
+                libperl                 /  HAVE_LIBPERL_SUPPORT
+                libpfm4                 /  HAVE_LIBPFM
+                libpython               /  HAVE_LIBPYTHON_SUPPORT
+                libslang                /  HAVE_SLANG_SUPPORT
+                libtraceevent           /  HAVE_LIBTRACEEVENT
+                libunwind               /  HAVE_LIBUNWIND_SUPPORT
+                lzma                    /  HAVE_LZMA_SUPPORT
+                syscall_table           /  HAVE_SYSCALL_TABLE_SUPPORT
+                zlib                    /  HAVE_ZLIB_SUPPORT
+                zstd                    /  HAVE_ZSTD_SUPPORT
+
+OPTIONS
+-------
+--quiet::
+        Do not print any messages or warnings
+
+        This can be used along with subcommands such as 'perf check feature'
+        to hide unnecessary output in test scripts, eg.
+        'perf check feature --quiet libtraceevent'
diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
new file mode 100644
index 000000000000..fe2d4468fd69
--- /dev/null
+++ b/tools/perf/builtin-check.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "builtin.h"
+#include "color.h"
+#include "util/debug.h"
+#include "util/header.h"
+#include <tools/config.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+#include <subcmd/parse-options.h>
+
+static const char * const check_subcommands[] = { "feature", NULL };
+static struct option check_options[] = {
+	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
+	OPT_END()
+};
+static struct option check_feature_options[] = { OPT_END() };
+
+static const char *check_usage[] = {
+	"perf check [<subcommand>] [<options>]",
+	NULL
+};
+static const char *check_feature_usage[] = {
+	"perf check feature <feature_list>",
+	NULL
+};
+
+struct feature_status supported_features[] = {
+	FEATURE_STATUS("aio", HAVE_AIO_SUPPORT),
+	FEATURE_STATUS("bpf", HAVE_LIBBPF_SUPPORT),
+	FEATURE_STATUS("bpf_skeletons", HAVE_BPF_SKEL),
+	FEATURE_STATUS("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
+	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
+	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
+	FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
+	FEATURE_STATUS("get_cpuid", HAVE_AUXTRACE_SUPPORT),
+	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
+	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
+	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
+	FEATURE_STATUS("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
+	FEATURE_STATUS("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
+	FEATURE_STATUS("libelf", HAVE_LIBELF_SUPPORT),
+	FEATURE_STATUS("libnuma", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_STATUS("libopencsd", HAVE_CSTRACE_SUPPORT),
+	FEATURE_STATUS("libperl", HAVE_LIBPERL_SUPPORT),
+	FEATURE_STATUS("libpfm4", HAVE_LIBPFM),
+	FEATURE_STATUS("libpython", HAVE_LIBPYTHON_SUPPORT),
+	FEATURE_STATUS("libslang", HAVE_SLANG_SUPPORT),
+	FEATURE_STATUS("libtraceevent", HAVE_LIBTRACEEVENT),
+	FEATURE_STATUS("libunwind", HAVE_LIBUNWIND_SUPPORT),
+	FEATURE_STATUS("lzma", HAVE_LZMA_SUPPORT),
+	FEATURE_STATUS("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_STATUS("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
+	FEATURE_STATUS("zlib", HAVE_ZLIB_SUPPORT),
+	FEATURE_STATUS("zstd", HAVE_ZSTD_SUPPORT),
+
+	/* this should remain at end, to know the array end */
+	FEATURE_STATUS(NULL, _)
+};
+
+static void on_off_print(const char *status)
+{
+	printf("[ ");
+
+	if (!strcmp(status, "OFF"))
+		color_fprintf(stdout, PERF_COLOR_RED, "%-3s", status);
+	else
+		color_fprintf(stdout, PERF_COLOR_GREEN, "%-3s", status);
+
+	printf(" ]");
+}
+
+/* Helper function to print status of a feature along with name/macro */
+static void status_print(const char *name, const char *macro,
+			 const char *status)
+{
+	printf("%22s: ", name);
+	on_off_print(status);
+	printf("  # %s\n", macro);
+}
+
+#define STATUS(feature)                                   \
+do {                                                      \
+	if (feature.is_builtin)                               \
+		status_print(feature.name, feature.macro, "on");  \
+	else                                                  \
+		status_print(feature.name, feature.macro, "OFF"); \
+} while (0)
+
+/**
+ * check whether "feature" is built-in with perf
+ *
+ * returns:
+ *    0: NOT built-in or Feature not known
+ *    1: Built-in
+ */
+static int has_support(const char *feature)
+{
+	for (int i = 0; supported_features[i].name; ++i) {
+		if ((strcasecmp(feature, supported_features[i].name) == 0) ||
+		    (strcasecmp(feature, supported_features[i].macro) == 0)) {
+			if (!quiet)
+				STATUS(supported_features[i]);
+			return supported_features[i].is_builtin;
+		}
+	}
+
+	if (!quiet)
+		pr_err("Feature not known: '%s'\n", feature);
+
+	return 0;
+}
+
+
+/**
+ * Usage: 'perf check feature <feature_list>'
+ *
+ * <feature_list> can be a single feature name/macro, or a comma-separated list
+ * of feature names/macros
+ * eg. argument can be "libtraceevent" or "libtraceevent,bpf" etc
+ *
+ * In case of a comma-separated list, feature_enabled will be 1, only if
+ * all features passed in the string are supported
+ *
+ * Note that argv will get modified
+ */
+static int subcommand_feature(int argc, const char **argv)
+{
+	char *feature_list;
+	char *feature_name;
+	int feature_enabled;
+
+	argc = parse_options(argc, argv, check_feature_options,
+			check_feature_usage, 0);
+
+	if (!argc)
+		usage_with_options(check_feature_usage, check_feature_options);
+
+	if (argc > 1) {
+		pr_err("Too many arguments passed to 'perf check feature'\n");
+		return -1;
+	}
+
+	feature_enabled = 1;
+	/* feature_list is a non-const copy of 'argv[1]' */
+	feature_list = strdup(argv[0]);
+	if (!feature_list) {
+		pr_err("ERROR: failed to allocate memory for feature list\n");
+		return -1;
+	}
+
+	feature_name = strtok(feature_list, ",");
+
+	while (feature_name) {
+		feature_enabled &= has_support(feature_name);
+		feature_name = strtok(NULL, ",");
+	}
+
+	free(feature_list);
+
+	return !feature_enabled;
+}
+
+int cmd_check(int argc, const char **argv)
+{
+	argc = parse_options_subcommand(argc, argv, check_options,
+			check_subcommands, check_usage, 0);
+
+	if (!argc)
+		usage_with_options(check_usage, check_options);
+
+	if (strcmp(argv[0], "feature") == 0)
+		return subcommand_feature(argc, argv);
+
+	/* If no subcommand matched above, print usage help */
+	usage_with_options(check_usage, check_options);
+	return 0;
+}
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index f4375deabfa3..0cdb11b9efec 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -2,6 +2,22 @@
 #ifndef BUILTIN_H
 #define BUILTIN_H
 
+#include <stddef.h>
+#include <linux/compiler.h>
+#include <tools/config.h>
+
+struct feature_status {
+	const char *name;
+	const char *macro;
+	int is_builtin;
+};
+
+#define FEATURE_STATUS(name_, macro_) { \
+	.name = name_,                       \
+	.macro = #macro_,                    \
+	.is_builtin = IS_BUILTIN(macro_) }
+
+extern struct feature_status supported_features[];
 struct cmdnames;
 
 void list_common_cmds_help(void);
@@ -11,6 +27,7 @@ int cmd_annotate(int argc, const char **argv);
 int cmd_bench(int argc, const char **argv);
 int cmd_buildid_cache(int argc, const char **argv);
 int cmd_buildid_list(int argc, const char **argv);
+int cmd_check(int argc, const char **argv);
 int cmd_config(int argc, const char **argv);
 int cmd_c2c(int argc, const char **argv);
 int cmd_diff(int argc, const char **argv);
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index bd3f80b5bb46..4def800f4089 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -52,6 +52,7 @@ static struct cmd_struct commands[] = {
 	{ "archive",	NULL,	0 },
 	{ "buildid-cache", cmd_buildid_cache, 0 },
 	{ "buildid-list", cmd_buildid_list, 0 },
+	{ "check",	cmd_check,	0 },
 	{ "config",	cmd_config,	0 },
 	{ "c2c",	cmd_c2c,	0 },
 	{ "diff",	cmd_diff,	0 },
-- 
2.45.2

[PATCH v11 2/4] perf version: update --build-options to use 'supported_features' array

[Aditya Gupta]

Now that the feature list has been duplicated in a global
'supported_features' array, use that array instead of manually checking
status of built-in features.

This helps in being consistent with commands such as 'perf check feature',
so commands can use the same array, and any new feature can be added at
one place, in the 'supported_features' array

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/builtin-version.c | 43 +++++++-----------------------------
 1 file changed, 8 insertions(+), 35 deletions(-)

diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
index 398aa53e9e2e..e149d96c6dc5 100644
--- a/tools/perf/builtin-version.c
+++ b/tools/perf/builtin-version.c
@@ -46,45 +46,18 @@ static void status_print(const char *name, const char *macro,
 	printf("  # %s\n", macro);
 }
 
-#define STATUS(__d, __m)				\
-do {							\
-	if (IS_BUILTIN(__d))				\
-		status_print(#__m, #__d, "on");		\
-	else						\
-		status_print(#__m, #__d, "OFF");	\
+#define STATUS(feature)                                   \
+do {                                                      \
+	if (feature.is_builtin)                               \
+		status_print(feature.name, feature.macro, "on");  \
+	else                                                  \
+		status_print(feature.name, feature.macro, "OFF"); \
 } while (0)
 
 static void library_status(void)
 {
-	STATUS(HAVE_DWARF_SUPPORT, dwarf);
-	STATUS(HAVE_DWARF_GETLOCATIONS_SUPPORT, dwarf_getlocations);
-#ifndef HAVE_SYSCALL_TABLE_SUPPORT
-	STATUS(HAVE_LIBAUDIT_SUPPORT, libaudit);
-#endif
-	STATUS(HAVE_SYSCALL_TABLE_SUPPORT, syscall_table);
-	STATUS(HAVE_LIBBFD_SUPPORT, libbfd);
-	STATUS(HAVE_DEBUGINFOD_SUPPORT, debuginfod);
-	STATUS(HAVE_LIBELF_SUPPORT, libelf);
-	STATUS(HAVE_LIBNUMA_SUPPORT, libnuma);
-	STATUS(HAVE_LIBNUMA_SUPPORT, numa_num_possible_cpus);
-	STATUS(HAVE_LIBPERL_SUPPORT, libperl);
-	STATUS(HAVE_LIBPYTHON_SUPPORT, libpython);
-	STATUS(HAVE_SLANG_SUPPORT, libslang);
-	STATUS(HAVE_LIBCRYPTO_SUPPORT, libcrypto);
-	STATUS(HAVE_LIBUNWIND_SUPPORT, libunwind);
-	STATUS(HAVE_DWARF_SUPPORT, libdw-dwarf-unwind);
-	STATUS(HAVE_LIBCAPSTONE_SUPPORT, libcapstone);
-	STATUS(HAVE_ZLIB_SUPPORT, zlib);
-	STATUS(HAVE_LZMA_SUPPORT, lzma);
-	STATUS(HAVE_AUXTRACE_SUPPORT, get_cpuid);
-	STATUS(HAVE_LIBBPF_SUPPORT, bpf);
-	STATUS(HAVE_AIO_SUPPORT, aio);
-	STATUS(HAVE_ZSTD_SUPPORT, zstd);
-	STATUS(HAVE_LIBPFM, libpfm4);
-	STATUS(HAVE_LIBTRACEEVENT, libtraceevent);
-	STATUS(HAVE_BPF_SKEL, bpf_skeletons);
-	STATUS(HAVE_DWARF_UNWIND_SUPPORT, dwarf-unwind-support);
-	STATUS(HAVE_CSTRACE_SUPPORT, libopencsd);
+	for (int i = 0; supported_features[i].name; ++i)
+		STATUS(supported_features[i]);
 }
 
 int cmd_version(int argc, const char **argv)
-- 
2.45.2

[PATCH v11 3/4] perf tests task_analyzer: use perf check for libtraceevent support

[Aditya Gupta]

Currently we use output of 'perf version --build-options', to check
whether perf was built with libtraceevent support.

Instead, use 'perf check feature libtraceevent' to check for
libtraceevent support.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/tests/shell/test_task_analyzer.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/test_task_analyzer.sh b/tools/perf/tests/shell/test_task_analyzer.sh
index 92d15154ba79..456665ba465e 100755
--- a/tools/perf/tests/shell/test_task_analyzer.sh
+++ b/tools/perf/tests/shell/test_task_analyzer.sh
@@ -52,8 +52,8 @@ find_str_or_fail() {
 
 # check if perf is compiled with libtraceevent support
 skip_no_probe_record_support() {
-	perf version --build-options | grep -q " OFF .* HAVE_LIBTRACEEVENT" && return 2
-	return 0
+	perf check feature -q libtraceevent && return 0
+	return 2
 }
 
 prepare_perf_data() {
-- 
2.45.2

[PATCH v11 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check feature

[Aditya Gupta]

From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

In probe_vfs_getname.sh, current we use "perf record --dry-run"
to check for libtraceevent and skip the test if perf is not
build with libtraceevent. Change the check to use "perf check feature"
option

Acked-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/lib/probe_vfs_getname.sh           | 4 ++--
 tools/perf/tests/shell/record+probe_libc_inet_pton.sh     | 5 ++++-
 tools/perf/tests/shell/record+script_probe_vfs_getname.sh | 5 ++++-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/shell/lib/probe_vfs_getname.sh b/tools/perf/tests/shell/lib/probe_vfs_getname.sh
index bf4c1fb71c4b..24cca0fbfe31 100644
--- a/tools/perf/tests/shell/lib/probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/lib/probe_vfs_getname.sh
@@ -27,7 +27,7 @@ skip_if_no_debuginfo() {
 # check if perf is compiled with libtraceevent support
 skip_no_probe_record_support() {
 	if [ $had_vfs_getname -eq 1 ] ; then
-		perf record --dry-run -e $1 2>&1 | grep "libtraceevent is necessary for tracepoint support" && return 2
-		return 1
+		perf check feature -q libtraceevent && return 1
+		return 2
 	fi
 }
diff --git a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
index 72c65570db37..f38c8ead0b03 100755
--- a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
+++ b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
@@ -63,7 +63,10 @@ trace_libc_inet_pton_backtrace() {
 
 	# Check presence of libtraceevent support to run perf record
 	skip_no_probe_record_support "$event_name/$eventattr/"
-	[ $? -eq 2 ] && return 2
+	if [ $? -eq 2 ]; then
+		echo "WARN: Skipping test trace_libc_inet_pton_backtrace. No libtraceevent support."
+		return 2
+	fi
 
 	perf record -e $event_name/$eventattr/ -o $perf_data ping -6 -c 1 ::1 > /dev/null 2>&1
 	# check if perf data file got created in above step.
diff --git a/tools/perf/tests/shell/record+script_probe_vfs_getname.sh b/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
index 5eedbe29bba1..9a61928e3c9a 100755
--- a/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
@@ -21,7 +21,10 @@ record_open_file() {
 	echo "Recording open file:"
 	# Check presence of libtraceevent support to run perf record
 	skip_no_probe_record_support "probe:vfs_getname*"
-	[ $? -eq 2 ] && return 2
+	if [ $? -eq 2 ]; then
+		echo "WARN: Skipping test record_open_file. No libtraceevent support"
+		return 2
+	fi
 	perf record -o ${perfdata} -e probe:vfs_getname\* touch $file
 }
 
-- 
2.45.2

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Arnaldo Carvalho de Melo]

Some minor patch format comments, the commiter could fix those up while
applying, but I don't think its a strict req for merging the patch
series:

  Subject: Re: [PATCH v11 1/4] perf check: introduce check subcommand

Please capitalize the first letter in the summary sentence:

  Subject: Re: [PATCH v11 1/4] perf check: Introduce check subcommand

And when talking about some special name, quote it:

  Subject: Re: [PATCH v11 1/4] perf check: Introduce 'check' subcommand

On Thu, Jun 27, 2024 at 03:36:41PM +0530, Aditya Gupta wrote:
> 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"
> 
> Instead of this, introduce a subcommand "perf check feature", with which
> scripts can test for presence of a feature, such as:
> 
>     perf check feature HAVE_FEATURE
> 
> 'perf check feature' command is expected to have exit status of 0 if
> feature is built-in, and 1 if it's not built-in or if feature is not known.
> 
> Multiple features can also be passed as a comma-separated list, in which
> case the exit status will be 1 only if all of the passed features are
> built-in. For example, with below command, it will have exit status of 0
> only if both libtraceevent and bpf are enabled, else 1 in all other cases
> 
>     perf check feature libtraceevent,bpf
> 
> The arguments are case-insensitive.
> An array 'supported_features' has also been introduced that can be used by
> other commands like 'perf version --build-options', so that new features
> can be added in one place, with the array
> 
> Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
>  tools/perf/Build                        |   1 +
>  tools/perf/Documentation/perf-check.txt |  78 +++++++++++
>  tools/perf/builtin-check.c              | 178 ++++++++++++++++++++++++
>  tools/perf/builtin.h                    |  17 +++
>  tools/perf/perf.c                       |   1 +
>  5 files changed, 275 insertions(+)
>  create mode 100644 tools/perf/Documentation/perf-check.txt
>  create mode 100644 tools/perf/builtin-check.c
> 
> diff --git a/tools/perf/Build b/tools/perf/Build
> index b0cb7ad8e6ac..7635cc823565 100644
> --- a/tools/perf/Build
> +++ b/tools/perf/Build
> @@ -1,5 +1,6 @@
>  perf-y += builtin-bench.o
>  perf-y += builtin-annotate.o
> +perf-y += builtin-check.o
>  perf-y += builtin-config.o
>  perf-y += builtin-diff.o
>  perf-y += builtin-evlist.o
> diff --git a/tools/perf/Documentation/perf-check.txt b/tools/perf/Documentation/perf-check.txt
> new file mode 100644
> index 000000000000..3c4a92251026
> --- /dev/null
> +++ b/tools/perf/Documentation/perf-check.txt
> @@ -0,0 +1,78 @@
> +perf-check(1)
> +===============
> +
> +NAME
> +----
> +perf-check - check features in perf


perf-check - check if features are present

> +
> +SYNOPSIS
> +--------
> +[verse]
> +'perf check' [<options>]
> +'perf check' {feature <feature_list>} [<options>]
> +
> +DESCRIPTION
> +-----------
> +With no options given, the 'perf check' just prints the perf version
                                        subcommand
> +on the standard output.
> +
> +If the subcommand 'feature' is used, then status of feature is printed
> +on the standard output (unless '-q' is also passed), ie. whether it is
> +compiled-in/built-in or not.
> +Also, 'perf check feature' returns with exit status 0 if the feature
> +is built-in, otherwise returns with exit status 1.
> +
> +SUBCOMMANDS
> +-----------
> +
> +feature::
> +
> +        Print whether feature(s) is compiled-in or not, and also returns with an
> +        exit status of 0, if passed feature(s) are compiled-in, else 1.
> +
> +        It expects a feature list as an argument. There can be a single feature
> +        name/macro, or multiple features can also be passed as a comma-separated
> +        list, in which case the exit status will be 0 only if all of the passed
> +        features are compiled-in.
> +
> +        The feature names/macros are case-insensitive.
> +
> +        Example Usage:
> +                perf check feature libtraceevent
> +                perf check feature HAVE_LIBTRACEEVENT
> +                perf check feature libtraceevent,bpf
> +
> +        Supported feature names/macro:
> +                aio                     /  HAVE_AIO_SUPPORT
> +                bpf                     /  HAVE_LIBBPF_SUPPORT
> +                bpf_skeletons           /  HAVE_BPF_SKEL
> +                debuginfod              /  HAVE_DEBUGINFOD_SUPPORT
> +                dwarf                   /  HAVE_DWARF_SUPPORT
> +                dwarf_getlocations      /  HAVE_DWARF_GETLOCATIONS_SUPPORT
> +                get_cpuid               /  HAVE_AUXTRACE_SUPPORT
> +                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
> +                libaudit                /  HAVE_LIBAUDIT_SUPPORT
> +                libbfd                  /  HAVE_LIBBFD_SUPPORT
> +                libelf                  /  HAVE_LIBELF_SUPPORT
> +                libcrypto               /  HAVE_LIBCRYPTO_SUPPORT
> +                libdw-dwarf-unwind      /  HAVE_DWARF_SUPPORT
> +                libnuma                 /  HAVE_LIBNUMA_SUPPORT
> +                libperl                 /  HAVE_LIBPERL_SUPPORT
> +                libpfm4                 /  HAVE_LIBPFM
> +                libpython               /  HAVE_LIBPYTHON_SUPPORT
> +                libslang                /  HAVE_SLANG_SUPPORT
> +                libtraceevent           /  HAVE_LIBTRACEEVENT
> +                libunwind               /  HAVE_LIBUNWIND_SUPPORT
> +                lzma                    /  HAVE_LZMA_SUPPORT
> +                syscall_table           /  HAVE_SYSCALL_TABLE_SUPPORT
> +                zlib                    /  HAVE_ZLIB_SUPPORT
> +                zstd                    /  HAVE_ZSTD_SUPPORT

There are actually more features that are detected and that probably
would be great to support:

⬢[acme@toolbox perf-tools-next]$ cat /tmp/build/perf-tools-next/FEATURE-DUMP 
feature-backtrace=1
feature-dwarf=1
feature-dwarf_getlocations=1
feature-dwarf_getcfi=1
feature-eventfd=1
feature-fortify-source=1
feature-get_current_dir_name=1
feature-gettid=1
feature-glibc=1
feature-libbfd=1
feature-libbfd-buildid=1
feature-libcap=1
feature-libelf=1
feature-libelf-getphdrnum=1
feature-libelf-gelf_getnote=1
feature-libelf-getshdrstrndx=1
feature-libnuma=1
feature-numa_num_possible_cpus=1
feature-libperl=1
feature-libpython=1
feature-libslang=1
feature-libslang-include-subdir=1
feature-libtraceevent=1
feature-libtracefs=1
feature-libcrypto=1
feature-libunwind=1
feature-pthread-attr-setaffinity-np=1
feature-pthread-barrier=1
feature-reallocarray=1
feature-stackprotector-all=1
feature-timerfd=1
feature-libdw-dwarf-unwind=1
feature-zlib=1
feature-lzma=1
feature-get_cpuid=1
feature-bpf=1
feature-scandirat=1
feature-sched_getcpu=1
feature-sdt=1
feature-setns=1
feature-libaio=1
feature-libzstd=1
feature-disassembler-four-args=1
feature-disassembler-init-styled=1
feature-file-handle=1
⬢[acme@toolbox perf-tools-next]$

We can get the ones not supported so far in a followup patch, if/when we
agree it would be useful.

> +
> +OPTIONS
> +-------
> +--quiet::
> +        Do not print any messages or warnings
> +
> +        This can be used along with subcommands such as 'perf check feature'
> +        to hide unnecessary output in test scripts, eg.
> +        'perf check feature --quiet libtraceevent'
> diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> new file mode 100644
> index 000000000000..fe2d4468fd69
> --- /dev/null
> +++ b/tools/perf/builtin-check.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "builtin.h"
> +#include "color.h"
> +#include "util/debug.h"
> +#include "util/header.h"
> +#include <tools/config.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <subcmd/parse-options.h>
> +
> +static const char * const check_subcommands[] = { "feature", NULL };
> +static struct option check_options[] = {
> +	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
> +	OPT_END()
> +};
> +static struct option check_feature_options[] = { OPT_END() };
> +
> +static const char *check_usage[] = {
> +	"perf check [<subcommand>] [<options>]",
> +	NULL
> +};
> +static const char *check_feature_usage[] = {
> +	"perf check feature <feature_list>",
> +	NULL
> +};
> +
> +struct feature_status supported_features[] = {
> +	FEATURE_STATUS("aio", HAVE_AIO_SUPPORT),
> +	FEATURE_STATUS("bpf", HAVE_LIBBPF_SUPPORT),
> +	FEATURE_STATUS("bpf_skeletons", HAVE_BPF_SKEL),
> +	FEATURE_STATUS("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
> +	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
> +	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
> +	FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
> +	FEATURE_STATUS("get_cpuid", HAVE_AUXTRACE_SUPPORT),
> +	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
> +	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
> +	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
> +	FEATURE_STATUS("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
> +	FEATURE_STATUS("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
> +	FEATURE_STATUS("libelf", HAVE_LIBELF_SUPPORT),
> +	FEATURE_STATUS("libnuma", HAVE_LIBNUMA_SUPPORT),
> +	FEATURE_STATUS("libopencsd", HAVE_CSTRACE_SUPPORT),
> +	FEATURE_STATUS("libperl", HAVE_LIBPERL_SUPPORT),
> +	FEATURE_STATUS("libpfm4", HAVE_LIBPFM),
> +	FEATURE_STATUS("libpython", HAVE_LIBPYTHON_SUPPORT),
> +	FEATURE_STATUS("libslang", HAVE_SLANG_SUPPORT),
> +	FEATURE_STATUS("libtraceevent", HAVE_LIBTRACEEVENT),
> +	FEATURE_STATUS("libunwind", HAVE_LIBUNWIND_SUPPORT),
> +	FEATURE_STATUS("lzma", HAVE_LZMA_SUPPORT),
> +	FEATURE_STATUS("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
> +	FEATURE_STATUS("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
> +	FEATURE_STATUS("zlib", HAVE_ZLIB_SUPPORT),
> +	FEATURE_STATUS("zstd", HAVE_ZSTD_SUPPORT),
> +
> +	/* this should remain at end, to know the array end */
> +	FEATURE_STATUS(NULL, _)
> +};
> +
> +static void on_off_print(const char *status)
> +{
> +	printf("[ ");
> +
> +	if (!strcmp(status, "OFF"))
> +		color_fprintf(stdout, PERF_COLOR_RED, "%-3s", status);
> +	else
> +		color_fprintf(stdout, PERF_COLOR_GREEN, "%-3s", status);
> +
> +	printf(" ]");
> +}
> +
> +/* Helper function to print status of a feature along with name/macro */
> +static void status_print(const char *name, const char *macro,
> +			 const char *status)
> +{
> +	printf("%22s: ", name);
> +	on_off_print(status);
> +	printf("  # %s\n", macro);
> +}
> +
> +#define STATUS(feature)                                   \
> +do {                                                      \
> +	if (feature.is_builtin)                               \
> +		status_print(feature.name, feature.macro, "on");  \
> +	else                                                  \
> +		status_print(feature.name, feature.macro, "OFF"); \
> +} while (0)
> +
> +/**
> + * check whether "feature" is built-in with perf
> + *
> + * returns:
> + *    0: NOT built-in or Feature not known
> + *    1: Built-in
> + */
> +static int has_support(const char *feature)
> +{
> +	for (int i = 0; supported_features[i].name; ++i) {
> +		if ((strcasecmp(feature, supported_features[i].name) == 0) ||
> +		    (strcasecmp(feature, supported_features[i].macro) == 0)) {
> +			if (!quiet)
> +				STATUS(supported_features[i]);
> +			return supported_features[i].is_builtin;
> +		}
> +	}
> +
> +	if (!quiet)
> +		pr_err("Feature not known: '%s'\n", feature);
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * Usage: 'perf check feature <feature_list>'
> + *
> + * <feature_list> can be a single feature name/macro, or a comma-separated list
> + * of feature names/macros
> + * eg. argument can be "libtraceevent" or "libtraceevent,bpf" etc
> + *
> + * In case of a comma-separated list, feature_enabled will be 1, only if
> + * all features passed in the string are supported
> + *
> + * Note that argv will get modified
> + */
> +static int subcommand_feature(int argc, const char **argv)
> +{
> +	char *feature_list;
> +	char *feature_name;
> +	int feature_enabled;
> +
> +	argc = parse_options(argc, argv, check_feature_options,
> +			check_feature_usage, 0);
> +
> +	if (!argc)
> +		usage_with_options(check_feature_usage, check_feature_options);
> +
> +	if (argc > 1) {
> +		pr_err("Too many arguments passed to 'perf check feature'\n");
> +		return -1;
> +	}
> +
> +	feature_enabled = 1;
> +	/* feature_list is a non-const copy of 'argv[1]' */
> +	feature_list = strdup(argv[0]);
> +	if (!feature_list) {
> +		pr_err("ERROR: failed to allocate memory for feature list\n");
> +		return -1;
> +	}
> +
> +	feature_name = strtok(feature_list, ",");
> +
> +	while (feature_name) {
> +		feature_enabled &= has_support(feature_name);
> +		feature_name = strtok(NULL, ",");
> +	}
> +
> +	free(feature_list);
> +
> +	return !feature_enabled;
> +}
> +
> +int cmd_check(int argc, const char **argv)
> +{
> +	argc = parse_options_subcommand(argc, argv, check_options,
> +			check_subcommands, check_usage, 0);
> +
> +	if (!argc)
> +		usage_with_options(check_usage, check_options);
> +
> +	if (strcmp(argv[0], "feature") == 0)
> +		return subcommand_feature(argc, argv);
> +
> +	/* If no subcommand matched above, print usage help */
> +	usage_with_options(check_usage, check_options);
> +	return 0;
> +}
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index f4375deabfa3..0cdb11b9efec 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -2,6 +2,22 @@
>  #ifndef BUILTIN_H
>  #define BUILTIN_H
>  
> +#include <stddef.h>
> +#include <linux/compiler.h>
> +#include <tools/config.h>
> +
> +struct feature_status {
> +	const char *name;
> +	const char *macro;
> +	int is_builtin;
> +};
> +
> +#define FEATURE_STATUS(name_, macro_) { \
> +	.name = name_,                       \
> +	.macro = #macro_,                    \
> +	.is_builtin = IS_BUILTIN(macro_) }
> +
> +extern struct feature_status supported_features[];
>  struct cmdnames;
>  
>  void list_common_cmds_help(void);
> @@ -11,6 +27,7 @@ int cmd_annotate(int argc, const char **argv);
>  int cmd_bench(int argc, const char **argv);
>  int cmd_buildid_cache(int argc, const char **argv);
>  int cmd_buildid_list(int argc, const char **argv);
> +int cmd_check(int argc, const char **argv);
>  int cmd_config(int argc, const char **argv);
>  int cmd_c2c(int argc, const char **argv);
>  int cmd_diff(int argc, const char **argv);
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index bd3f80b5bb46..4def800f4089 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -52,6 +52,7 @@ static struct cmd_struct commands[] = {
>  	{ "archive",	NULL,	0 },
>  	{ "buildid-cache", cmd_buildid_cache, 0 },
>  	{ "buildid-list", cmd_buildid_list, 0 },
> +	{ "check",	cmd_check,	0 },
>  	{ "config",	cmd_config,	0 },
>  	{ "c2c",	cmd_c2c,	0 },
>  	{ "diff",	cmd_diff,	0 },
> -- 
> 2.45.2

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Arnaldo Carvalho de Melo]

On Fri, Jun 28, 2024 at 11:12:16AM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Jun 27, 2024 at 03:36:41PM +0530, Aditya Gupta wrote:
> > 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"
> > 
> > Instead of this, introduce a subcommand "perf check feature", with which
> > scripts can test for presence of a feature, such as:
> > 
> >     perf check feature HAVE_FEATURE
> > 
> > 'perf check feature' command is expected to have exit status of 0 if
> > feature is built-in, and 1 if it's not built-in or if feature is not known.
> > 
> > Multiple features can also be passed as a comma-separated list, in which
> > case the exit status will be 1 only if all of the passed features are
> > built-in. For example, with below command, it will have exit status of 0
> > only if both libtraceevent and bpf are enabled, else 1 in all other cases
> > 
> >     perf check feature libtraceevent,bpf
> > 
> > The arguments are case-insensitive.
> > An array 'supported_features' has also been introduced that can be used by
> > other commands like 'perf version --build-options', so that new features
> > can be added in one place, with the array

Now testing it with just this first patch applied:

⬢[acme@toolbox perf-tools-next]$ perf check feature bpf
                   bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
⬢[acme@toolbox perf-tools-next]$ echo $?
0
⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs

 Usage: perf check [<subcommand>] [<options>]

    -q, --quiet           do not show any warnings or messages

⬢[acme@toolbox perf-tools-next]$


I don't see a way to list what features can be tested against, would be
great to have.

Also it just says that the usage is wrong, no indication as to why, I
think this would be more informative:


⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
Unkown feature 'libtrafs', please use 'perf check feature --list' to see which ones are available.

⬢[acme@toolbox perf-tools-next]$

- Arnaldo

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Arnaldo Carvalho de Melo]

On Fri, Jun 28, 2024 at 11:15:57AM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Jun 28, 2024 at 11:12:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Thu, Jun 27, 2024 at 03:36:41PM +0530, Aditya Gupta wrote:
> > > 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"
> > > 
> > > Instead of this, introduce a subcommand "perf check feature", with which
> > > scripts can test for presence of a feature, such as:
> > > 
> > >     perf check feature HAVE_FEATURE
> > > 
> > > 'perf check feature' command is expected to have exit status of 0 if
> > > feature is built-in, and 1 if it's not built-in or if feature is not known.
> > > 
> > > Multiple features can also be passed as a comma-separated list, in which
> > > case the exit status will be 1 only if all of the passed features are
> > > built-in. For example, with below command, it will have exit status of 0
> > > only if both libtraceevent and bpf are enabled, else 1 in all other cases
> > > 
> > >     perf check feature libtraceevent,bpf
> > > 
> > > The arguments are case-insensitive.
> > > An array 'supported_features' has also been introduced that can be used by
> > > other commands like 'perf version --build-options', so that new features
> > > can be added in one place, with the array
> 
> Now testing it with just this first patch applied:
> 
> ⬢[acme@toolbox perf-tools-next]$ perf check feature bpf
>                    bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
> ⬢[acme@toolbox perf-tools-next]$ echo $?
> 0
> ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> 
>  Usage: perf check [<subcommand>] [<options>]
> 
>     -q, --quiet           do not show any warnings or messages
> 
> ⬢[acme@toolbox perf-tools-next]$
> 
> 
> I don't see a way to list what features can be tested against, would be
> great to have.
> 
> Also it just says that the usage is wrong, no indication as to why, I
> think this would be more informative:
> 
> 
> ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> Unkown feature 'libtrafs', please use 'perf check feature --list' to see which ones are available.
> 
> ⬢[acme@toolbox perf-tools-next]$

Ah, to clarify, these comments are for the v12 series even being replies
to the v11 thread, I'll move to the v12 e-mail thread. Using b4 I got
the latest version, v12:

Cover: ./v12_20240628_adityag_introduce_perf_check_subcommand.cover
 Link: https://lore.kernel.org/r/20240628064236.1123851-1-adityag@linux.ibm.com
 Base: applies clean to current tree
       git checkout -b v12_20240628_adityag_linux_ibm_com HEAD
       git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
⬢[acme@toolbox perf-tools-next]$        git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
Applying: perf check: introduce check subcommand
Applying: perf version: update --build-options to use 'supported_features' array
Applying: perf tests task_analyzer: use perf check for libtraceevent support
Applying: tools/perf/tests: Update probe_vfs_getname.sh script to use perf check feature
⬢[acme@toolbox perf-tools-next]$

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Arnaldo Carvalho de Melo]

On Fri, Jun 28, 2024 at 11:20:35AM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Jun 28, 2024 at 11:15:57AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Fri, Jun 28, 2024 at 11:12:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Thu, Jun 27, 2024 at 03:36:41PM +0530, Aditya Gupta wrote:
> > > > 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"
> > > > 
> > > > Instead of this, introduce a subcommand "perf check feature", with which
> > > > scripts can test for presence of a feature, such as:
> > > > 
> > > >     perf check feature HAVE_FEATURE
> > > > 
> > > > 'perf check feature' command is expected to have exit status of 0 if
> > > > feature is built-in, and 1 if it's not built-in or if feature is not known.
> > > > 
> > > > Multiple features can also be passed as a comma-separated list, in which
> > > > case the exit status will be 1 only if all of the passed features are
> > > > built-in. For example, with below command, it will have exit status of 0
> > > > only if both libtraceevent and bpf are enabled, else 1 in all other cases
> > > > 
> > > >     perf check feature libtraceevent,bpf
> > > > 
> > > > The arguments are case-insensitive.
> > > > An array 'supported_features' has also been introduced that can be used by
> > > > other commands like 'perf version --build-options', so that new features
> > > > can be added in one place, with the array
> > 
> > Now testing it with just this first patch applied:
> > 
> > ⬢[acme@toolbox perf-tools-next]$ perf check feature bpf
> >                    bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
> > ⬢[acme@toolbox perf-tools-next]$ echo $?
> > 0
> > ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> > 
> >  Usage: perf check [<subcommand>] [<options>]
> > 
> >     -q, --quiet           do not show any warnings or messages
> > 
> > ⬢[acme@toolbox perf-tools-next]$
> > 
> > 
> > I don't see a way to list what features can be tested against, would be
> > great to have.
> > 
> > Also it just says that the usage is wrong, no indication as to why, I
> > think this would be more informative:
> > 
> > 
> > ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> > Unkown feature 'libtrafs', please use 'perf check feature --list' to see which ones are available.

Or:

⬢[acme@toolbox perf-tools-next]$ perf version --build-options
perf version 6.10.rc1.g25cd7047ff7b
                 dwarf: [ on  ]  # HAVE_DWARF_SUPPORT
    dwarf_getlocations: [ on  ]  # HAVE_DWARF_GETLOCATIONS_SUPPORT
         syscall_table: [ on  ]  # HAVE_SYSCALL_TABLE_SUPPORT
                libbfd: [ OFF ]  # HAVE_LIBBFD_SUPPORT
            debuginfod: [ on  ]  # HAVE_DEBUGINFOD_SUPPORT
                libelf: [ on  ]  # HAVE_LIBELF_SUPPORT
               libnuma: [ on  ]  # HAVE_LIBNUMA_SUPPORT
numa_num_possible_cpus: [ on  ]  # HAVE_LIBNUMA_SUPPORT
               libperl: [ on  ]  # HAVE_LIBPERL_SUPPORT
             libpython: [ on  ]  # HAVE_LIBPYTHON_SUPPORT
              libslang: [ on  ]  # HAVE_SLANG_SUPPORT
             libcrypto: [ on  ]  # HAVE_LIBCRYPTO_SUPPORT
             libunwind: [ on  ]  # HAVE_LIBUNWIND_SUPPORT
    libdw-dwarf-unwind: [ on  ]  # HAVE_DWARF_SUPPORT
           libcapstone: [ on  ]  # HAVE_LIBCAPSTONE_SUPPORT
                  zlib: [ on  ]  # HAVE_ZLIB_SUPPORT
                  lzma: [ on  ]  # HAVE_LZMA_SUPPORT
             get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
                   bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
                   aio: [ on  ]  # HAVE_AIO_SUPPORT
                  zstd: [ on  ]  # HAVE_ZSTD_SUPPORT
               libpfm4: [ on  ]  # HAVE_LIBPFM
         libtraceevent: [ on  ]  # HAVE_LIBTRACEEVENT
         bpf_skeletons: [ on  ]  # HAVE_BPF_SKEL
  dwarf-unwind-support: [ on  ]  # HAVE_DWARF_UNWIND_SUPPORT
            libopencsd: [ on  ]  # HAVE_CSTRACE_SUPPORT
⬢[acme@toolbox perf-tools-next]$

I.e.:

Unkown feature 'libtrafs', please use 'perf version --build-options' to see which ones are available.

And while looking at it:

             get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT

This looks wrong, no? Or at least confusing, have to check the source
code...

Also:

               libnuma: [ on  ]  # HAVE_LIBNUMA_SUPPORT
numa_num_possible_cpus: [ on  ]  # HAVE_LIBNUMA_SUPPORT

- Arnaldo

> > ⬢[acme@toolbox perf-tools-next]$
> 
> Ah, to clarify, these comments are for the v12 series even being replies
> to the v11 thread, I'll move to the v12 e-mail thread. Using b4 I got
> the latest version, v12:
> 
> Cover: ./v12_20240628_adityag_introduce_perf_check_subcommand.cover
>  Link: https://lore.kernel.org/r/20240628064236.1123851-1-adityag@linux.ibm.com
>  Base: applies clean to current tree
>        git checkout -b v12_20240628_adityag_linux_ibm_com HEAD
>        git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
> ⬢[acme@toolbox perf-tools-next]$        git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
> Applying: perf check: introduce check subcommand
> Applying: perf version: update --build-options to use 'supported_features' array
> Applying: perf tests task_analyzer: use perf check for libtraceevent support
> Applying: tools/perf/tests: Update probe_vfs_getname.sh script to use perf check feature
> ⬢[acme@toolbox perf-tools-next]$

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Namhyung Kim]

On Fri, Jun 28, 2024 at 11:24:53AM -0300, Arnaldo Carvalho de Melo wrote:

> And while looking at it:
> 
>              get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
> 
> This looks wrong, no? Or at least confusing, have to check the source
> code...

We have this in Makefile.config

ifndef NO_AUXTRACE
  ifeq ($(SRCARCH),x86)
    ifeq ($(feature-get_cpuid), 0)
      $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
      NO_AUXTRACE := 1
    endif
  endif

Thanks,
Namhyung

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Arnaldo Carvalho de Melo]

On Fri, Jun 28, 2024 at 11:28:14AM -0700, Namhyung Kim wrote:
> On Fri, Jun 28, 2024 at 11:24:53AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> > And while looking at it:
> > 
> >              get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
> > 
> > This looks wrong, no? Or at least confusing, have to check the source
> > code...
> 
> We have this in Makefile.config
> 
> ifndef NO_AUXTRACE
>   ifeq ($(SRCARCH),x86)
>     ifeq ($(feature-get_cpuid), 0)
>       $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
>       NO_AUXTRACE := 1
>     endif
>   endif

The complete sequence is:

ifndef NO_AUXTRACE
  ifeq ($(SRCARCH),x86)
    ifeq ($(feature-get_cpuid), 0)
      $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
      NO_AUXTRACE := 1
    endif
  endif
  ifndef NO_AUXTRACE
    $(call detected,CONFIG_AUXTRACE)
    CFLAGS += -DHAVE_AUXTRACE_SUPPORT
    ifeq ($(feature-reallocarray), 0)
      CFLAGS += -DCOMPAT_NEED_REALLOCARRAY
    endif
  endif
endif

The most descriptive would be to HAVE_GET_CPUID_SUPPORT and have it used
in the source code.

That or have:

diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
index 44ffde6f8dbe51f3..ae4a686ff4f265be 100644
--- a/tools/perf/builtin-check.c
+++ b/tools/perf/builtin-check.c
@@ -33,7 +33,7 @@ struct feature_status supported_features[] = {
 	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
 	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
 	FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
-	FEATURE_STATUS("get_cpuid", HAVE_AUXTRACE_SUPPORT),
+	FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
 	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
 	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
 	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),

That:

        FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),

Should also really be:

        FEATURE_STATUS("dwarf-unwind", HAVE_DWARF_UNWIND_SUPPORT),

For consistency, the get_cpuid/auxtrace also for consistency, I think.

- Arnaldo

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Aditya Gupta]

Hello Arnaldo,

Thanks for the many reviews you shared.

On 24/06/28 11:12AM, Arnaldo Carvalho de Melo wrote:
> Some minor patch format comments, the commiter could fix those up while
> applying, but I don't think its a strict req for merging the patch
> series:
> 
>   Subject: Re: [PATCH v11 1/4] perf check: introduce check subcommand
> 
> Please capitalize the first letter in the summary sentence:
> 
>   Subject: Re: [PATCH v11 1/4] perf check: Introduce check subcommand
> 
> And when talking about some special name, quote it:
> 
>   Subject: Re: [PATCH v11 1/4] perf check: Introduce 'check' subcommand
> 

Sure, will update in v13.

> > <...snip...>
> >
> > +++ b/tools/perf/Documentation/perf-check.txt
> > @@ -0,0 +1,78 @@
> > +perf-check(1)
> > +===============
> > +
> > +NAME
> > +----
> > +perf-check - check features in perf
> 
> 
> perf-check - check if features are present

Will do

> 
> > +
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'perf check' [<options>]
> > +'perf check' {feature <feature_list>} [<options>]
> > +
> > +DESCRIPTION
> > +-----------
> > +With no options given, the 'perf check' just prints the perf version
>                                         subcommand

Sure, also since the statement is not true anymore ('perf check' does
not print the perf version now, only the command usage), how does this
sound ?

    With no subcommands given, 'perf check' command just prints the command
    usage on the standard output.

> > +on the standard output.
> > +
> > +If the subcommand 'feature' is used, then status of feature is printed
> > +on the standard output (unless '-q' is also passed), ie. whether it is
> > +compiled-in/built-in or not.
> > +Also, 'perf check feature' returns with exit status 0 if the feature
> > +is built-in, otherwise returns with exit status 1.
> > +
> > +SUBCOMMANDS
> > +-----------
> > +
> > +feature::
> > +
> > +        Print whether feature(s) is compiled-in or not, and also returns with an
> > +        exit status of 0, if passed feature(s) are compiled-in, else 1.
> > +
> > +        It expects a feature list as an argument. There can be a single feature
> > +        name/macro, or multiple features can also be passed as a comma-separated
> > +        list, in which case the exit status will be 0 only if all of the passed
> > +        features are compiled-in.
> > +
> > +        The feature names/macros are case-insensitive.
> > +
> > +        Example Usage:
> > +                perf check feature libtraceevent
> > +                perf check feature HAVE_LIBTRACEEVENT
> > +                perf check feature libtraceevent,bpf
> > +
> > +        Supported feature names/macro:
> > +                aio                     /  HAVE_AIO_SUPPORT
> > +                bpf                     /  HAVE_LIBBPF_SUPPORT
> > +                bpf_skeletons           /  HAVE_BPF_SKEL
> > +                debuginfod              /  HAVE_DEBUGINFOD_SUPPORT
> > +                dwarf                   /  HAVE_DWARF_SUPPORT
> > +                dwarf_getlocations      /  HAVE_DWARF_GETLOCATIONS_SUPPORT
> > +                get_cpuid               /  HAVE_AUXTRACE_SUPPORT
> > +                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
> > +                libaudit                /  HAVE_LIBAUDIT_SUPPORT
> > +                libbfd                  /  HAVE_LIBBFD_SUPPORT
> > +                libelf                  /  HAVE_LIBELF_SUPPORT
> > +                libcrypto               /  HAVE_LIBCRYPTO_SUPPORT
> > +                libdw-dwarf-unwind      /  HAVE_DWARF_SUPPORT
> > +                libnuma                 /  HAVE_LIBNUMA_SUPPORT
> > +                libperl                 /  HAVE_LIBPERL_SUPPORT
> > +                libpfm4                 /  HAVE_LIBPFM
> > +                libpython               /  HAVE_LIBPYTHON_SUPPORT
> > +                libslang                /  HAVE_SLANG_SUPPORT
> > +                libtraceevent           /  HAVE_LIBTRACEEVENT
> > +                libunwind               /  HAVE_LIBUNWIND_SUPPORT
> > +                lzma                    /  HAVE_LZMA_SUPPORT
> > +                syscall_table           /  HAVE_SYSCALL_TABLE_SUPPORT
> > +                zlib                    /  HAVE_ZLIB_SUPPORT
> > +                zstd                    /  HAVE_ZSTD_SUPPORT
> 
> There are actually more features that are detected and that probably
> would be great to support:
> 
> ⬢[acme@toolbox perf-tools-next]$ cat /tmp/build/perf-tools-next/FEATURE-DUMP 
> feature-backtrace=1
> feature-dwarf=1
> feature-dwarf_getlocations=1
> feature-dwarf_getcfi=1
> feature-eventfd=1
> feature-fortify-source=1
> feature-get_current_dir_name=1
> feature-gettid=1
> feature-glibc=1
> feature-libbfd=1
> feature-libbfd-buildid=1
> feature-libcap=1
> feature-libelf=1
> feature-libelf-getphdrnum=1
> feature-libelf-gelf_getnote=1
> feature-libelf-getshdrstrndx=1
> feature-libnuma=1
> feature-numa_num_possible_cpus=1
> feature-libperl=1
> feature-libpython=1
> feature-libslang=1
> feature-libslang-include-subdir=1
> feature-libtraceevent=1
> feature-libtracefs=1
> feature-libcrypto=1
> feature-libunwind=1
> feature-pthread-attr-setaffinity-np=1
> feature-pthread-barrier=1
> feature-reallocarray=1
> feature-stackprotector-all=1
> feature-timerfd=1
> feature-libdw-dwarf-unwind=1
> feature-zlib=1
> feature-lzma=1
> feature-get_cpuid=1
> feature-bpf=1
> feature-scandirat=1
> feature-sched_getcpu=1
> feature-sdt=1
> feature-setns=1
> feature-libaio=1
> feature-libzstd=1
> feature-disassembler-four-args=1
> feature-disassembler-init-styled=1
> feature-file-handle=1
> ⬢[acme@toolbox perf-tools-next]$
> 
> We can get the ones not supported so far in a followup patch, if/when we
> agree it would be useful.

Yes, would be nice if the feature list covers most feature checks.
I will look into it and try to improve by v13, or will post a follow up
patch later to add these !

Thanks,
- Aditya Gupta

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Aditya Gupta]

Hello Arnaldo,

On 24/06/28 11:15AM, Arnaldo Carvalho de Melo wrote:
> On Fri, Jun 28, 2024 at 11:12:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Thu, Jun 27, 2024 at 03:36:41PM +0530, Aditya Gupta wrote:
> > > 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"
> > > 
> > > Instead of this, introduce a subcommand "perf check feature", with which
> > > scripts can test for presence of a feature, such as:
> > > 
> > >     perf check feature HAVE_FEATURE
> > > 
> > > 'perf check feature' command is expected to have exit status of 0 if
> > > feature is built-in, and 1 if it's not built-in or if feature is not known.
> > > 
> > > Multiple features can also be passed as a comma-separated list, in which
> > > case the exit status will be 1 only if all of the passed features are
> > > built-in. For example, with below command, it will have exit status of 0
> > > only if both libtraceevent and bpf are enabled, else 1 in all other cases
> > > 
> > >     perf check feature libtraceevent,bpf
> > > 
> > > The arguments are case-insensitive.
> > > An array 'supported_features' has also been introduced that can be used by
> > > other commands like 'perf version --build-options', so that new features
> > > can be added in one place, with the array
> 
> Now testing it with just this first patch applied:
> 
> ⬢[acme@toolbox perf-tools-next]$ perf check feature bpf
>                    bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
> ⬢[acme@toolbox perf-tools-next]$ echo $?
> 0
> ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> 
>  Usage: perf check [<subcommand>] [<options>]
> 
>     -q, --quiet           do not show any warnings or messages
> 
> ⬢[acme@toolbox perf-tools-next]$
> 
> 
> I don't see a way to list what features can be tested against, would be
> great to have.

The feature list is actually same as 'perf version --build-options' as
you pointed out.

Should I have 'perf check feature --list', or 'perf version
--build-options' ?

If these build 'options' are all features only, does it make sense
replacing 'perf version --build-options' with 'perf check feature --list',
where the output will be exactly same ?

> 
> Also it just says that the usage is wrong, no indication as to why, I
> think this would be more informative:
> 
> 
> ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> Unkown feature 'libtrafs', please use 'perf check feature --list' to see which ones are available.

Ah, should use 'perf check feature bpf,libtrafs', then it should print
'Feature not known':

    $ ./perf check feature asdsaf
    Feature not known: 'asdsaf'

	$ perf git:(014587a4af6f) ✗ ./perf check feature bpf,libtrafs
                   bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
    Feature not known: 'libtrafs'

The suggestion to mention how to list feature is nice.
Will add 'please use 'perf check feature --list' to see which ones are available'.

Will also print 'Unknown subcommand: bpf,libtrafs' to mention what went
wrong, instead of just usage.

Thanks,
Aditya Gupta

> 
> ⬢[acme@toolbox perf-tools-next]$
> 
> - Arnaldo

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Aditya Gupta]

> Ah, to clarify, these comments are for the v12 series even being replies
> to the v11 thread, I'll move to the v12 e-mail thread. Using b4 I got
> the latest version, v12:

Sure, understood.

Thanks,
- Aditya Gupta

> 
> Cover: ./v12_20240628_adityag_introduce_perf_check_subcommand.cover
>  Link: https://lore.kernel.org/r/20240628064236.1123851-1-adityag@linux.ibm.com
>  Base: applies clean to current tree
>        git checkout -b v12_20240628_adityag_linux_ibm_com HEAD
>        git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
> ⬢[acme@toolbox perf-tools-next]$        git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
> Applying: perf check: introduce check subcommand
> Applying: perf version: update --build-options to use 'supported_features' array
> Applying: perf tests task_analyzer: use perf check for libtraceevent support
> Applying: tools/perf/tests: Update probe_vfs_getname.sh script to use perf check feature
> ⬢[acme@toolbox perf-tools-next]$

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Aditya Gupta]

Hello Arnaldo,

> > > ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> > > 
> > >  Usage: perf check [<subcommand>] [<options>]
> > > 
> > >     -q, --quiet           do not show any warnings or messages
> > > 
> > > ⬢[acme@toolbox perf-tools-next]$
> > > 
> > > 
> > > I don't see a way to list what features can be tested against, would be
> > > great to have.
> > > 
> > > Also it just says that the usage is wrong, no indication as to why, I
> > > think this would be more informative:
> > > 
> > > 
> > > ⬢[acme@toolbox perf-tools-next]$ perf check bpf,libtrafs
> > > Unkown feature 'libtrafs', please use 'perf check feature --list' to see which ones are available.
> 
> Or:
> 
> ⬢[acme@toolbox perf-tools-next]$ perf version --build-options

Sure, or we can continue discussion in the original thread where you
mentioned 'perf check feature --list'.

> perf version 6.10.rc1.g25cd7047ff7b
>                  dwarf: [ on  ]  # HAVE_DWARF_SUPPORT
>     dwarf_getlocations: [ on  ]  # HAVE_DWARF_GETLOCATIONS_SUPPORT
>          syscall_table: [ on  ]  # HAVE_SYSCALL_TABLE_SUPPORT
>                 libbfd: [ OFF ]  # HAVE_LIBBFD_SUPPORT
>             debuginfod: [ on  ]  # HAVE_DEBUGINFOD_SUPPORT
>                 libelf: [ on  ]  # HAVE_LIBELF_SUPPORT
>                libnuma: [ on  ]  # HAVE_LIBNUMA_SUPPORT
> numa_num_possible_cpus: [ on  ]  # HAVE_LIBNUMA_SUPPORT
>                libperl: [ on  ]  # HAVE_LIBPERL_SUPPORT
>              libpython: [ on  ]  # HAVE_LIBPYTHON_SUPPORT
>               libslang: [ on  ]  # HAVE_SLANG_SUPPORT
>              libcrypto: [ on  ]  # HAVE_LIBCRYPTO_SUPPORT
>              libunwind: [ on  ]  # HAVE_LIBUNWIND_SUPPORT
>     libdw-dwarf-unwind: [ on  ]  # HAVE_DWARF_SUPPORT
>            libcapstone: [ on  ]  # HAVE_LIBCAPSTONE_SUPPORT
>                   zlib: [ on  ]  # HAVE_ZLIB_SUPPORT
>                   lzma: [ on  ]  # HAVE_LZMA_SUPPORT
>              get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
>                    bpf: [ on  ]  # HAVE_LIBBPF_SUPPORT
>                    aio: [ on  ]  # HAVE_AIO_SUPPORT
>                   zstd: [ on  ]  # HAVE_ZSTD_SUPPORT
>                libpfm4: [ on  ]  # HAVE_LIBPFM
>          libtraceevent: [ on  ]  # HAVE_LIBTRACEEVENT
>          bpf_skeletons: [ on  ]  # HAVE_BPF_SKEL
>   dwarf-unwind-support: [ on  ]  # HAVE_DWARF_UNWIND_SUPPORT
>             libopencsd: [ on  ]  # HAVE_CSTRACE_SUPPORT
> ⬢[acme@toolbox perf-tools-next]$
> 
> I.e.:
> 
> Unkown feature 'libtrafs', please use 'perf version --build-options' to see which ones are available.
> 
> And while looking at it:
> 
>              get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
> 
> This looks wrong, no? Or at least confusing, have to check the source
> code...

Yes it is confusing. I don't have any knowledge on that macro though.
Will try to understand from git history.

> 
> Also:
> 
>                libnuma: [ on  ]  # HAVE_LIBNUMA_SUPPORT
> numa_num_possible_cpus: [ on  ]  # HAVE_LIBNUMA_SUPPORT

Interesting, found this code in Makefile.config:

    ifndef NO_LIBNUMA
      ifeq ($(feature-libnuma), 0)
        $(warning No numa.h found, disables 'perf bench numa mem' benchmark, please install numactl-devel/libnuma-devel/libnuma-dev)
        NO_LIBNUMA := 1
      else
        ifeq ($(feature-numa_num_possible_cpus), 0)
          $(warning Old numa library found, disables 'perf bench numa mem' benchmark, please install numactl-devel/libnuma-devel/libnuma-dev >= 2.0.8)
          NO_LIBNUMA := 1
        else
          CFLAGS += -DHAVE_LIBNUMA_SUPPORT
          EXTLIBS += -lnuma
          $(call detected,CONFIG_NUMA)
        endif
      endif
    endif

So, both 'numa_num_possible_cpus' and 'libnuma' define the same macro in
CFLAG: HAVE_LIBNUMA_SUPPORT

But then, both are not needed actually in, since either feature will
always have same status as the other feature.

Should I keep it ?

Thanks,
Aditya Gupta

> 
> - Arnaldo
> 
> > > ⬢[acme@toolbox perf-tools-next]$
> > 
> > Ah, to clarify, these comments are for the v12 series even being replies
> > to the v11 thread, I'll move to the v12 e-mail thread. Using b4 I got
> > the latest version, v12:
> > 
> > Cover: ./v12_20240628_adityag_introduce_perf_check_subcommand.cover
> >  Link: https://lore.kernel.org/r/20240628064236.1123851-1-adityag@linux.ibm.com
> >  Base: applies clean to current tree
> >        git checkout -b v12_20240628_adityag_linux_ibm_com HEAD
> >        git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
> > ⬢[acme@toolbox perf-tools-next]$        git am ./v12_20240628_adityag_introduce_perf_check_subcommand.mbx
> > Applying: perf check: introduce check subcommand
> > Applying: perf version: update --build-options to use 'supported_features' array
> > Applying: perf tests task_analyzer: use perf check for libtraceevent support
> > Applying: tools/perf/tests: Update probe_vfs_getname.sh script to use perf check feature
> > ⬢[acme@toolbox perf-tools-next]$

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Aditya Gupta]

Hi Arnaldo,


On 29/06/24 00:58, Arnaldo Carvalho de Melo wrote:
> On Fri, Jun 28, 2024 at 11:28:14AM -0700, Namhyung Kim wrote:
>> On Fri, Jun 28, 2024 at 11:24:53AM -0300, Arnaldo Carvalho de Melo wrote:
>>
>>> And while looking at it:
>>>
>>>               get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
>>>
>>> This looks wrong, no? Or at least confusing, have to check the source
>>> code...
>> We have this in Makefile.config
>>
>> ifndef NO_AUXTRACE
>>    ifeq ($(SRCARCH),x86)
>>      ifeq ($(feature-get_cpuid), 0)
>>        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
>>        NO_AUXTRACE := 1
>>      endif
>>    endif
> The complete sequence is:
>
> ifndef NO_AUXTRACE
>    ifeq ($(SRCARCH),x86)
>      ifeq ($(feature-get_cpuid), 0)
>        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
>        NO_AUXTRACE := 1
>      endif
>    endif
>    ifndef NO_AUXTRACE
>      $(call detected,CONFIG_AUXTRACE)
>      CFLAGS += -DHAVE_AUXTRACE_SUPPORT
>      ifeq ($(feature-reallocarray), 0)
>        CFLAGS += -DCOMPAT_NEED_REALLOCARRAY
>      endif
>    endif
> endif
>
> The most descriptive would be to HAVE_GET_CPUID_SUPPORT and have it used
> in the source code.
>
> That or have:
>
> diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> index 44ffde6f8dbe51f3..ae4a686ff4f265be 100644
> --- a/tools/perf/builtin-check.c
> +++ b/tools/perf/builtin-check.c
> @@ -33,7 +33,7 @@ struct feature_status supported_features[] = {
>   	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
>   	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
>   	FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
> -	FEATURE_STATUS("get_cpuid", HAVE_AUXTRACE_SUPPORT),
> +	FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
>   	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
>   	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
>   	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),


Looks better. Went through all instances of 'HAVE_AUXTRACE_SUPPORT', 
it's mostly been used to conditionally define perf_*_auxtrace functions.

No one seems to depend on the feature 'name' 'get_cpuid'.

Any comments ?

>
> That:
>
>          FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
>
> Should also really be:
>
>          FEATURE_STATUS("dwarf-unwind", HAVE_DWARF_UNWIND_SUPPORT),

Will do it in v13.


Thanks,

Aditya Gupta

> For consistency, the get_cpuid/auxtrace also for consistency, I think.
>
> - Arnaldo

Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Arnaldo Carvalho de Melo]

On Sun, Jun 30, 2024 at 07:38:44PM +0530, Aditya Gupta wrote:
> Hi Arnaldo,
> 
> 
> On 29/06/24 00:58, Arnaldo Carvalho de Melo wrote:
> > On Fri, Jun 28, 2024 at 11:28:14AM -0700, Namhyung Kim wrote:
> > > On Fri, Jun 28, 2024 at 11:24:53AM -0300, Arnaldo Carvalho de Melo wrote:
> > > 
> > > > And while looking at it:
> > > > 
> > > >               get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
> > > > 
> > > > This looks wrong, no? Or at least confusing, have to check the source
> > > > code...
> > > We have this in Makefile.config
> > > 
> > > ifndef NO_AUXTRACE
> > >    ifeq ($(SRCARCH),x86)
> > >      ifeq ($(feature-get_cpuid), 0)
> > >        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
> > >        NO_AUXTRACE := 1
> > >      endif
> > >    endif
> > The complete sequence is:
> > 
> > ifndef NO_AUXTRACE
> >    ifeq ($(SRCARCH),x86)
> >      ifeq ($(feature-get_cpuid), 0)
> >        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
> >        NO_AUXTRACE := 1
> >      endif
> >    endif
> >    ifndef NO_AUXTRACE
> >      $(call detected,CONFIG_AUXTRACE)
> >      CFLAGS += -DHAVE_AUXTRACE_SUPPORT
> >      ifeq ($(feature-reallocarray), 0)
> >        CFLAGS += -DCOMPAT_NEED_REALLOCARRAY
> >      endif
> >    endif
> > endif
> > 
> > The most descriptive would be to HAVE_GET_CPUID_SUPPORT and have it used
> > in the source code.
> > 
> > That or have:
> > 
> > diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> > index 44ffde6f8dbe51f3..ae4a686ff4f265be 100644
> > --- a/tools/perf/builtin-check.c
> > +++ b/tools/perf/builtin-check.c
> > @@ -33,7 +33,7 @@ struct feature_status supported_features[] = {
> >   	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
> >   	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
> >   	FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
> > -	FEATURE_STATUS("get_cpuid", HAVE_AUXTRACE_SUPPORT),
> > +	FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
> >   	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
> >   	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
> >   	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),
> 
> 
> Looks better. Went through all instances of 'HAVE_AUXTRACE_SUPPORT', it's
> mostly been used to conditionally define perf_*_auxtrace functions.
> 
> No one seems to depend on the feature 'name' 'get_cpuid'.
> 
> Any comments ?

So I think its better to go with HAVE_AUXTRACE_SUPPORT to cover this
feature, i.e. whatever is needed to have auxtrace support gets checked
and if all is in place, HAVE_AUXTRACE_SUPPORT is set up.

This in fact is what is being done, its just when reporting using
supported_features[] that we end up using the string "get_cpuid", right?

So it makes sense to use:

FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
 
> > That:
> > 
> >          FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
> > 
> > Should also really be:
> > 
> >          FEATURE_STATUS("dwarf-unwind", HAVE_DWARF_UNWIND_SUPPORT),
> 
> Will do it in v13.

Thanks, put that in a separate patch, as it is only barely related to
the other patches, we only are correcting an inconsistency we found
while writing/reviewing your patch kit.

Thanks,

- Arnaldo
 
> 
> Thanks,
> 
> Aditya Gupta
> 
> > For consistency, the get_cpuid/auxtrace also for consistency, I think.
> > 
> > - Arnaldo