[PATCH v6 0/4] Introduce perf check subcommand

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v6 0/4] Introduce perf check subcommand
@ 2023-10-21 15:05 Aditya Gupta
  2023-10-21 15:05 ` [PATCH v6 1/4] perf check: introduce " Aditya Gupta
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-10-21 15:05 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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"

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, such as:

    perf check --feature HAVE_LIBTRACEEVENT

    or

    perf check --feature libtraceevent

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

Git tree
========

Git tree with this patch series applied for testing:
https://github.com/adi-g15-ibm/linux/tree/perf-check-feature-v6

Changelog
=========
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       |  59 +++++++++
 tools/perf/builtin-check.c                    | 122 ++++++++++++++++++
 tools/perf/builtin-version.c                  |  39 ++----
 tools/perf/builtin.h                          |  18 +++
 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, 221 insertions(+), 37 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-check.txt
 create mode 100644 tools/perf/builtin-check.c

-- 
2.41.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v6 1/4] perf check: introduce check subcommand
  2023-10-21 15:05 [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
@ 2023-10-21 15:05 ` Aditya Gupta
  2023-11-10 14:27   ` Arnaldo Carvalho de Melo
  2023-10-21 15:05 ` [PATCH v6 2/4] perf version: update --build-options to use 'supported_features' array Aditya Gupta
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Aditya Gupta @ 2023-10-21 15:05 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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.

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

Acked-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/Build                        |   1 +
 tools/perf/Documentation/perf-check.txt |  60 ++++++++++++
 tools/perf/builtin-check.c              | 123 ++++++++++++++++++++++++
 tools/perf/builtin.h                    |  18 ++++
 tools/perf/perf.c                       |   1 +
 5 files changed, 203 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 aa7623622834..a55a797c1b5f 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..b7f2902928e7
--- /dev/null
+++ b/tools/perf/Documentation/perf-check.txt
@@ -0,0 +1,60 @@
+perf-check(1)
+===============
+
+NAME
+----
+perf-check - check features in perf
+
+SYNOPSIS
+--------
+'perf check' [<options>]
+
+DESCRIPTION
+-----------
+With no options given, the 'perf check' just prints the perf version
+on the standard output.
+
+If the option '--feature' is given, 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.
+
+OPTIONS
+-------
+-q::
+		Do not print any messages or warnings
+
+--feature::
+        Print whether a feature is compiled-in or not. A feature name/macro is
+        required to be passed after this flag
+
+        Example Usage:
+                perf check --feature libtraceevent
+                perf check --feature HAVE_LIBTRACEEVENT
+
+        Supported feature names/macro:
+                dwarf      /  HAVE_DWARF_SUPPORT
+                dwarf_getlocations  /  HAVE_DWARF_GETLOCATIONS_SUPPORT
+                libaudit   /  HAVE_LIBAUDIT_SUPPORT
+                syscall_table       /  HAVE_SYSCALL_TABLE_SUPPORT
+                libbfd     /  HAVE_LIBBFD_SUPPORT
+                debuginfod /  HAVE_DEBUGINFOD_SUPPORT
+                libelf     /  HAVE_LIBELF_SUPPORT
+                libnuma    /  HAVE_LIBNUMA_SUPPORT
+                numa_num_possible_cpus  /  HAVE_LIBNUMA_SUPPORT
+                libperl    /  HAVE_LIBPERL_SUPPORT
+                libpython  /  HAVE_LIBPYTHON_SUPPORT
+                libslang   /  HAVE_SLANG_SUPPORT
+                libcrypto  /  HAVE_LIBCRYPTO_SUPPORT
+                libunwind  /  HAVE_LIBUNWIND_SUPPORT
+                libdw-dwarf-unwind  /  HAVE_DWARF_SUPPORT
+                zlib       /  HAVE_ZLIB_SUPPORT
+                lzma       /  HAVE_LZMA_SUPPORT
+                get_cpuid  /  HAVE_AUXTRACE_SUPPORT
+                bpf        /  HAVE_LIBBPF_SUPPORT
+                aio        /  HAVE_AIO_SUPPORT
+                zstd       /  HAVE_ZSTD_SUPPORT
+                libpfm4    /  HAVE_LIBPFM
+                libtraceevent      /  HAVE_LIBTRACEEVENT
+                bpf_skeletons      /  HAVE_BPF_SKEL
diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
new file mode 100644
index 000000000000..1183983e4352
--- /dev/null
+++ b/tools/perf/builtin-check.c
@@ -0,0 +1,123 @@
+// 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>
+
+struct check {
+	const char *feature;
+};
+
+static struct check check;
+
+static struct option check_options[] = {
+	OPT_STRING(0, "feature", &check.feature, NULL, "check if a feature is built in"),
+	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
+	OPT_END(),
+};
+
+static const char * const check_usage[] = {
+	"perf check [<options>]",
+	NULL
+};
+
+struct feature_support supported_features[] = {
+	FEATURE_SUPPORT("dwarf", HAVE_DWARF_SUPPORT),
+	FEATURE_SUPPORT("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
+	FEATURE_SUPPORT("libaudit", HAVE_LIBAUDIT_SUPPORT),
+	FEATURE_SUPPORT("syscall_table", HAVE_SYSCALL_TABLE_SUPPORT),
+	FEATURE_SUPPORT("libbfd", HAVE_LIBBFD_SUPPORT),
+	FEATURE_SUPPORT("debuginfod", HAVE_DEBUGINFOD_SUPPORT),
+	FEATURE_SUPPORT("libelf", HAVE_LIBELF_SUPPORT),
+	FEATURE_SUPPORT("libnuma", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_SUPPORT("numa_num_possible_cpus", HAVE_LIBNUMA_SUPPORT),
+	FEATURE_SUPPORT("libperl", HAVE_LIBPERL_SUPPORT),
+	FEATURE_SUPPORT("libpython", HAVE_LIBPYTHON_SUPPORT),
+	FEATURE_SUPPORT("libslang", HAVE_SLANG_SUPPORT),
+	FEATURE_SUPPORT("libcrypto", HAVE_LIBCRYPTO_SUPPORT),
+	FEATURE_SUPPORT("libunwind", HAVE_LIBUNWIND_SUPPORT),
+	FEATURE_SUPPORT("libdw-dwarf-unwind", HAVE_DWARF_SUPPORT),
+	FEATURE_SUPPORT("zlib", HAVE_ZLIB_SUPPORT),
+	FEATURE_SUPPORT("lzma", HAVE_LZMA_SUPPORT),
+	FEATURE_SUPPORT("get_cpuid", HAVE_AUXTRACE_SUPPORT),
+	FEATURE_SUPPORT("bpf", HAVE_LIBBPF_SUPPORT),
+	FEATURE_SUPPORT("aio", HAVE_AIO_SUPPORT),
+	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
+	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
+	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
+	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
+
+	/* this should remain at end, to know the array end */
+	FEATURE_SUPPORT(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(" ]");
+}
+
+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 ((strcmp(feature, supported_features[i].name) == 0) ||
+		    (strcmp(feature, supported_features[i].macro) == 0)) {
+			if (!quiet)
+				STATUS(supported_features[i]);
+			return supported_features[i].is_builtin;
+		}
+	}
+
+	if (!quiet)
+		color_fprintf(stdout, PERF_COLOR_RED, "Feature not known: %s", feature);
+
+	return 0;
+}
+
+int cmd_check(int argc, const char **argv)
+{
+	argc = parse_options(argc, argv, check_options, check_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (!quiet)
+		printf("perf check %s\n", perf_version_string);
+
+	if (check.feature)
+		return !has_support(check.feature);
+
+	return 0;
+}
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index f2ab5bae2150..9f895981bc9e 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -2,6 +2,23 @@
 #ifndef BUILTIN_H
 #define BUILTIN_H
 
+#include <stddef.h>
+#include <linux/compiler.h>
+#include <tools/config.h>
+
+struct feature_support {
+	const char *name;
+	const char *macro;
+	int is_builtin;
+};
+
+#define FEATURE_SUPPORT(name_, macro_) { \
+	.name = name_,                       \
+	.macro = #macro_,                    \
+	.is_builtin = IS_BUILTIN(macro_) }
+
+extern struct feature_support supported_features[];
+
 void list_common_cmds_help(void);
 const char *help_unknown_cmd(const char *cmd);
 
@@ -9,6 +26,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 d3fc8090413c..6514f4121c49 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -50,6 +50,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.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] perf check: introduce check subcommand
  2023-10-21 15:05 ` [PATCH v6 1/4] perf check: introduce " Aditya Gupta
@ 2023-11-10 14:27   ` Arnaldo Carvalho de Melo
  2023-11-10 14:31     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-11-10 14:27 UTC (permalink / raw)
  To: Aditya Gupta
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

Em Sat, Oct 21, 2023 at 08:35:23PM +0530, Aditya Gupta escreveu:
> 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.
> 
> 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
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>

Right after applying this first patch:

[acme@quaco perf-tools-next]$ m
make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
  BUILD:   Doing 'make -j8' parallel build
Warning: Kernel ABI header differences:
  diff -u tools/include/uapi/linux/kvm.h include/uapi/linux/kvm.h
  diff -u tools/include/uapi/linux/mount.h include/uapi/linux/mount.h
  diff -u tools/include/uapi/linux/perf_event.h include/uapi/linux/perf_event.h
  diff -u tools/arch/x86/include/asm/disabled-features.h arch/x86/include/asm/disabled-features.h
  diff -u tools/arch/x86/include/asm/cpufeatures.h arch/x86/include/asm/cpufeatures.h
  diff -u tools/arch/x86/include/uapi/asm/prctl.h arch/x86/include/uapi/asm/prctl.h
  diff -u tools/arch/arm64/include/uapi/asm/perf_regs.h arch/arm64/include/uapi/asm/perf_regs.h
  diff -u tools/arch/s390/include/uapi/asm/kvm.h arch/s390/include/uapi/asm/kvm.h
Makefile.config:1145: No openjdk development package found, please install JDK package, e.g. openjdk-8-jdk, java-1.8.0-openjdk-devel

  INSTALL libsubcmd_headers
  INSTALL libperf_headers
  INSTALL libsymbol_headers
  INSTALL libapi_headers
  INSTALL libbpf_headers
  CC      /tmp/build/perf-tools-next/builtin-check.o
  CC      /tmp/build/perf-tools-next/builtin-buildid-list.o
  CC      /tmp/build/perf-tools-next/builtin-buildid-cache.o
  CC      /tmp/build/perf-tools-next/builtin-kallsyms.o
  CC      /tmp/build/perf-tools-next/builtin-list.o
  CC      /tmp/build/perf-tools-next/builtin-record.o
  CC      /tmp/build/perf-tools-next/builtin-report.o
  CC      /tmp/build/perf-tools-next/builtin-stat.o
builtin-check.c:53:56: error: expected ‘}’ before ‘;’ token
   53 |         FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
      |                                                        ^
builtin-check.c:29:47: note: to match this ‘{’
   29 | struct feature_support supported_features[] = {
      |                                               ^
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/builtin-check.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [Makefile.perf:669: /tmp/build/perf-tools-next/perf-in.o] Error 2
make[1]: *** [Makefile.perf:242: sub-make] Error 2
make: *** [Makefile:113: install-bin] Error 2
make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'

 Performance counter stats for 'make -k EXTRA_CFLAGS=-fsanitize=address BUILD_BPF_SKEL=1 CORESIGHT=1 O=/tmp/build/perf-tools-next -C tools/perf install-bin':

    26,249,850,723      cycles:u
    34,380,184,715      instructions:u                   #    1.31  insn per cycle

       4.021721145 seconds time elapsed

       7.780017000 seconds user
       1.793663000 seconds sys


[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.002 MB perf.data (9 samples) ]

 Performance counter stats for 'sleep 0.01':

       0.012377637 seconds time elapsed

       0.000000000 seconds user
       0.002203000 seconds sys


[acme@quaco perf-tools-next]$

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] perf check: introduce check subcommand
  2023-11-10 14:27   ` Arnaldo Carvalho de Melo
@ 2023-11-10 14:31     ` Arnaldo Carvalho de Melo
  2023-11-10 14:38       ` Arnaldo Carvalho de Melo
  2023-11-16  6:52       ` Aditya Gupta
  0 siblings, 2 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-11-10 14:31 UTC (permalink / raw)
  To: Aditya Gupta
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

Em Fri, Nov 10, 2023 at 11:27:42AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sat, Oct 21, 2023 at 08:35:23PM +0530, Aditya Gupta escreveu:
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
 
> Right after applying this first patch:
 
> [acme@quaco perf-tools-next]$ m
> make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
> builtin-check.c:53:56: error: expected ‘}’ before ‘;’ token
>    53 |         FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
>       |                                                        ^
> builtin-check.c:29:47: note: to match this ‘{’
>    29 | struct feature_support supported_features[] = {
>       |                                               ^
> make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/builtin-check.o] Error 1
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [Makefile.perf:669: /tmp/build/perf-tools-next/perf-in.o] Error 2
> make[1]: *** [Makefile.perf:242: sub-make] Error 2
> make: *** [Makefile:113: install-bin] Error 2
> make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'

It was a simple error, please be more careful next time.

I'm testing the rest of the patchset now.

- Arnaldo

diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
index 1183983e4352798d..1502804780507b5d 100644
--- a/tools/perf/builtin-check.c
+++ b/tools/perf/builtin-check.c
@@ -50,7 +50,7 @@ struct feature_support supported_features[] = {
 	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
 	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
 	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
-	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
+	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL),
 
 	/* this should remain at end, to know the array end */
 	FEATURE_SUPPORT(NULL, _)

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] perf check: introduce check subcommand
  2023-11-10 14:31     ` Arnaldo Carvalho de Melo
@ 2023-11-10 14:38       ` Arnaldo Carvalho de Melo
  2023-11-10 22:46         ` Namhyung Kim
  2023-11-16  7:06         ` Aditya Gupta
  2023-11-16  6:52       ` Aditya Gupta
  1 sibling, 2 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-11-10 14:38 UTC (permalink / raw)
  To: Aditya Gupta
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

Em Fri, Nov 10, 2023 at 11:31:57AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Nov 10, 2023 at 11:27:42AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Sat, Oct 21, 2023 at 08:35:23PM +0530, Aditya Gupta escreveu:
> > > Acked-by: Namhyung Kim <namhyung@kernel.org>
> > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
  
> > Right after applying this first patch:
  
> > [acme@quaco perf-tools-next]$ m
> > make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
> > builtin-check.c:53:56: error: expected ‘}’ before ‘;’ token
> >    53 |         FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> >       |                                                        ^
> > builtin-check.c:29:47: note: to match this ‘{’
> >    29 | struct feature_support supported_features[] = {
> >       |                                               ^
> > make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/builtin-check.o] Error 1
> > make[3]: *** Waiting for unfinished jobs....
> > make[2]: *** [Makefile.perf:669: /tmp/build/perf-tools-next/perf-in.o] Error 2
> > make[1]: *** [Makefile.perf:242: sub-make] Error 2
> > make: *** [Makefile:113: install-bin] Error 2
> > make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
 
> It was a simple error, please be more careful next time.
 
> I'm testing the rest of the patchset now.

Please resubmit when you address the problem fixed with this patch and:

Mussing newline, why print the version again? What is that "perf check "
prefix for?

[acme@quaco perf-tools-next]$ perf check --feature traceevent
perf check 6.6.rc1.g78fa196349bc
Feature not known: traceevent[acme@quaco perf-tools-next]$

Why should we require "--feature"? The following format is descriptive
enough:

[acme@quaco perf-tools-next]$
[acme@quaco perf-tools-next]$ perf check traceevent
perf check 6.6.rc1.g78fa196349bc
[acme@quaco perf-tools-next]$

So I had to go back and use:

[acme@quaco perf-tools-next]$ perf -vv
perf version 6.6.rc1.g78fa196349bc
                 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
                  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
[acme@quaco perf-tools-next]$

To see (some) of the features, please add a:

# perf check --list-features

So that we can get that features array printed.

Some other feature requests:

[acme@quaco perf-tools-next]$ perf check --feature libtraceevent,libbpf_support
perf check 6.6.rc1.g78fa196349bc
Feature not known: libtraceevent,libbpf_support[acme@quaco perf-tools-next]$

This should return true if both are available.

Also, shouldn't be --quiet be the default since this is being designed
for use in scripts?

- Arnaldo
 
> - Arnaldo
> 
> diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> index 1183983e4352798d..1502804780507b5d 100644
> --- a/tools/perf/builtin-check.c
> +++ b/tools/perf/builtin-check.c
> @@ -50,7 +50,7 @@ struct feature_support supported_features[] = {
>  	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
>  	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
>  	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
> -	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> +	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL),
>  
>  	/* this should remain at end, to know the array end */
>  	FEATURE_SUPPORT(NULL, _)

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] perf check: introduce check subcommand
  2023-11-10 14:38       ` Arnaldo Carvalho de Melo
@ 2023-11-10 22:46         ` Namhyung Kim
  2023-11-16  7:12           ` Aditya Gupta
  2023-11-16  7:06         ` Aditya Gupta
  1 sibling, 1 reply; 13+ messages in thread
From: Namhyung Kim @ 2023-11-10 22:46 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Aditya Gupta, jolsa, irogers, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

On Fri, Nov 10, 2023 at 6:38 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Nov 10, 2023 at 11:31:57AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Nov 10, 2023 at 11:27:42AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Sat, Oct 21, 2023 at 08:35:23PM +0530, Aditya Gupta escreveu:
> > > > Acked-by: Namhyung Kim <namhyung@kernel.org>
> > > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
>
> > > Right after applying this first patch:
>
> > > [acme@quaco perf-tools-next]$ m
> > > make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
> > > builtin-check.c:53:56: error: expected ‘}’ before ‘;’ token
> > >    53 |         FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> > >       |                                                        ^
> > > builtin-check.c:29:47: note: to match this ‘{’
> > >    29 | struct feature_support supported_features[] = {
> > >       |                                               ^
> > > make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/builtin-check.o] Error 1
> > > make[3]: *** Waiting for unfinished jobs....
> > > make[2]: *** [Makefile.perf:669: /tmp/build/perf-tools-next/perf-in.o] Error 2
> > > make[1]: *** [Makefile.perf:242: sub-make] Error 2
> > > make: *** [Makefile:113: install-bin] Error 2
> > > make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
>
> > It was a simple error, please be more careful next time.
>
> > I'm testing the rest of the patchset now.
>
> Please resubmit when you address the problem fixed with this patch and:
>
> Mussing newline, why print the version again? What is that "perf check "
> prefix for?
>
> [acme@quaco perf-tools-next]$ perf check --feature traceevent
> perf check 6.6.rc1.g78fa196349bc
> Feature not known: traceevent[acme@quaco perf-tools-next]$
>
> Why should we require "--feature"? The following format is descriptive
> enough:

I thought we might want to extend the functionality of the check
command later.  Maybe we can check how many perf counters
are supported or which sysctl settings are affected and so on.

Maybe we need to make '--feature' option to a sub-command
like `perf check feature xxx`.  So it cannot be mixed with others
later.

>
> [acme@quaco perf-tools-next]$
> [acme@quaco perf-tools-next]$ perf check traceevent
> perf check 6.6.rc1.g78fa196349bc
> [acme@quaco perf-tools-next]$
>
> So I had to go back and use:
>
> [acme@quaco perf-tools-next]$ perf -vv
> perf version 6.6.rc1.g78fa196349bc
>                  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
>                   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
> [acme@quaco perf-tools-next]$
>
> To see (some) of the features, please add a:
>
> # perf check --list-features
>
> So that we can get that features array printed.
>
> Some other feature requests:
>
> [acme@quaco perf-tools-next]$ perf check --feature libtraceevent,libbpf_support
> perf check 6.6.rc1.g78fa196349bc
> Feature not known: libtraceevent,libbpf_support[acme@quaco perf-tools-next]$
>
> This should return true if both are available.

Sounds good.

>
> Also, shouldn't be --quiet be the default since this is being designed
> for use in scripts?

I don't know.. users might want to see the result instead
of checking the return value.

     libtraceevent: [ on  ]
    libbpf_support: [ OFF ]

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] perf check: introduce check subcommand
  2023-11-10 22:46         ` Namhyung Kim
@ 2023-11-16  7:12           ` Aditya Gupta
  0 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16  7:12 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, jolsa, irogers, linux-perf-users, maddy,
	atrajeev, kjain, disgoel

On Fri, Nov 10, 2023 at 02:46:46PM -0800, Namhyung Kim wrote:
> On Fri, Nov 10, 2023 at 6:38 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Fri, Nov 10, 2023 at 11:31:57AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Fri, Nov 10, 2023 at 11:27:42AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Sat, Oct 21, 2023 at 08:35:23PM +0530, Aditya Gupta escreveu:
> > > > > Acked-by: Namhyung Kim <namhyung@kernel.org>
> > > > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> >
> > > > Right after applying this first patch:
> >
> > > > [acme@quaco perf-tools-next]$ m
> > > > make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
> > > > builtin-check.c:53:56: error: expected ‘}’ before ‘;’ token
> > > >    53 |         FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> > > >       |                                                        ^
> > > > builtin-check.c:29:47: note: to match this ‘{’
> > > >    29 | struct feature_support supported_features[] = {
> > > >       |                                               ^
> > > > make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/builtin-check.o] Error 1
> > > > make[3]: *** Waiting for unfinished jobs....
> > > > make[2]: *** [Makefile.perf:669: /tmp/build/perf-tools-next/perf-in.o] Error 2
> > > > make[1]: *** [Makefile.perf:242: sub-make] Error 2
> > > > make: *** [Makefile:113: install-bin] Error 2
> > > > make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
> >
> > > It was a simple error, please be more careful next time.
> >
> > > I'm testing the rest of the patchset now.
> >
> > Please resubmit when you address the problem fixed with this patch and:
> >
> > Mussing newline, why print the version again? What is that "perf check "
> > prefix for?
> >
> > [acme@quaco perf-tools-next]$ perf check --feature traceevent
> > perf check 6.6.rc1.g78fa196349bc
> > Feature not known: traceevent[acme@quaco perf-tools-next]$
> >
> > Why should we require "--feature"? The following format is descriptive
> > enough:
> 
> I thought we might want to extend the functionality of the check
> command later.  Maybe we can check how many perf counters
> are supported or which sysctl settings are affected and so on.
> 
> Maybe we need to make '--feature' option to a sub-command
> like `perf check feature xxx`.  So it cannot be mixed with others
> later.

True, 'perf check' is intended to be extended.
We can make it 'perf check feature libtraceevent', but any extra flags etc to
'perf check feature' can be equally verbose of what was intended to be a replacement
for 'perf version --build-options | grep "libtraceevent.*on"'

> 
> >
> > [acme@quaco perf-tools-next]$
> > [acme@quaco perf-tools-next]$ perf check traceevent
> > perf check 6.6.rc1.g78fa196349bc
> > [acme@quaco perf-tools-next]$
> >
> > So I had to go back and use:
> >
> > [acme@quaco perf-tools-next]$ perf -vv
> > perf version 6.6.rc1.g78fa196349bc
> >                  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
> >                   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
> > [acme@quaco perf-tools-next]$
> >
> > To see (some) of the features, please add a:
> >
> > # perf check --list-features
> >
> > So that we can get that features array printed.
> >
> > Some other feature requests:
> >
> > [acme@quaco perf-tools-next]$ perf check --feature libtraceevent,libbpf_support
> > perf check 6.6.rc1.g78fa196349bc
> > Feature not known: libtraceevent,libbpf_support[acme@quaco perf-tools-next]$
> >
> > This should return true if both are available.
> 
> Sounds good.

Sure, I will do it next version.

> 
> >
> > Also, shouldn't be --quiet be the default since this is being designed
> > for use in scripts?
> 
> I don't know.. users might want to see the result instead
> of checking the return value.
> 
>      libtraceevent: [ on  ]
>     libbpf_support: [ OFF ]
> 

Thanks Namhyung for answering the questions while I was not active.

Thanks,
- Aditya

> Thanks,
> Namhyung


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] perf check: introduce check subcommand
  2023-11-10 14:38       ` Arnaldo Carvalho de Melo
  2023-11-10 22:46         ` Namhyung Kim
@ 2023-11-16  7:06         ` Aditya Gupta
  1 sibling, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16  7:06 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

On Fri, Nov 10, 2023 at 11:38:43AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 10, 2023 at 11:31:57AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Nov 10, 2023 at 11:27:42AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Sat, Oct 21, 2023 at 08:35:23PM +0530, Aditya Gupta escreveu:
> > > > Acked-by: Namhyung Kim <namhyung@kernel.org>
> > > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
>   
> > > Right after applying this first patch:
>   
> > > [acme@quaco perf-tools-next]$ m
> > > make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
> > > builtin-check.c:53:56: error: expected ‘}’ before ‘;’ token
> > >    53 |         FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> > >       |                                                        ^
> > > builtin-check.c:29:47: note: to match this ‘{’
> > >    29 | struct feature_support supported_features[] = {
> > >       |                                               ^
> > > make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/builtin-check.o] Error 1
> > > make[3]: *** Waiting for unfinished jobs....
> > > make[2]: *** [Makefile.perf:669: /tmp/build/perf-tools-next/perf-in.o] Error 2
> > > make[1]: *** [Makefile.perf:242: sub-make] Error 2
> > > make: *** [Makefile:113: install-bin] Error 2
> > > make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
>  
> > It was a simple error, please be more careful next time.
>  
> > I'm testing the rest of the patchset now.
> 
> Please resubmit when you address the problem fixed with this patch and:
> 
> Mussing newline, why print the version again? What is that "perf check "
> prefix for?

Sure, I will send a V7 with the problems fixed.

> 
> [acme@quaco perf-tools-next]$ perf check --feature traceevent
> perf check 6.6.rc1.g78fa196349bc
> Feature not known: traceevent[acme@quaco perf-tools-next]$
> 
> Why should we require "--feature"? The following format is descriptive
> enough:
> 
> [acme@quaco perf-tools-next]$
> [acme@quaco perf-tools-next]$ perf check traceevent
> perf check 6.6.rc1.g78fa196349bc
> [acme@quaco perf-tools-next]$

This was done, so that the proposed 'perf check' command could be extended
in future for other usage also, and not just for checking if a feature is
available or not

In initial versions of this patch series, it was implemented also
'perf build --has libtraceevent', though with discussions, we arrived at a new
sucommand 'perf check', and checking if a feature is enabled is done just
with a flag 'perf check --feature', so that 'perf check' can be extended in
future.

> 
> So I had to go back and use:
> 
> [acme@quaco perf-tools-next]$ perf -vv
> perf version 6.6.rc1.g78fa196349bc
>                  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
>                   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
> [acme@quaco perf-tools-next]$
> 
> To see (some) of the features, please add a:
> 
> # perf check --list-features
> 
> So that we can get that features array printed.

This is already there in 'perf version --build-options' command. And that is the
command which was being used by scripts to check if a feature is enabled or not.

> 
> Some other feature requests:
> 
> [acme@quaco perf-tools-next]$ perf check --feature libtraceevent,libbpf_support
> perf check 6.6.rc1.g78fa196349bc
> Feature not known: libtraceevent,libbpf_support[acme@quaco perf-tools-next]$
> 
> This should return true if both are available.

This can be done. Sure.

> 
> Also, shouldn't be --quiet be the default since this is being designed
> for use in scripts?
> 

It will mostly be used by scripts only, but still if used as a command by any
user, it's intended that they also see whether feature was enabled or not,
instead of relying on the exit code.

Thanks for your reviews, and fixing the mistake I made.

Thanks,
- Aditya Gupta

> - Arnaldo
>  
> > - Arnaldo
> > 
> > diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> > index 1183983e4352798d..1502804780507b5d 100644
> > --- a/tools/perf/builtin-check.c
> > +++ b/tools/perf/builtin-check.c
> > @@ -50,7 +50,7 @@ struct feature_support supported_features[] = {
> >  	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
> >  	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
> >  	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
> > -	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> > +	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL),
> >  
> >  	/* this should remain at end, to know the array end */
> >  	FEATURE_SUPPORT(NULL, _)
> 
> -- 
> 
> - Arnaldo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 1/4] perf check: introduce check subcommand
  2023-11-10 14:31     ` Arnaldo Carvalho de Melo
  2023-11-10 14:38       ` Arnaldo Carvalho de Melo
@ 2023-11-16  6:52       ` Aditya Gupta
  1 sibling, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-16  6:52 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: jolsa, irogers, namhyung, linux-perf-users, maddy, atrajeev,
	kjain, disgoel

Extremely sorry for the delayed reply, I was on leave. Around this time, we have
a major festival in India, so was on a long leave.

On Fri, Nov 10, 2023 at 11:31:56AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 10, 2023 at 11:27:42AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Sat, Oct 21, 2023 at 08:35:23PM +0530, Aditya Gupta escreveu:
> > > Acked-by: Namhyung Kim <namhyung@kernel.org>
> > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
>  
> > Right after applying this first patch:
>  
> > [acme@quaco perf-tools-next]$ m
> > make: Entering directory '/home/acme/git/perf-tools-next/tools/perf'
> > builtin-check.c:53:56: error: expected ‘}’ before ‘;’ token
> >    53 |         FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> >       |                                                        ^
> > builtin-check.c:29:47: note: to match this ‘{’
> >    29 | struct feature_support supported_features[] = {
> >       |                                               ^
> > make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/builtin-check.o] Error 1
> > make[3]: *** Waiting for unfinished jobs....
> > make[2]: *** [Makefile.perf:669: /tmp/build/perf-tools-next/perf-in.o] Error 2
> > make[1]: *** [Makefile.perf:242: sub-make] Error 2
> > make: *** [Makefile:113: install-bin] Error 2
> > make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
> 
> It was a simple error, please be more careful next time.
> 
> I'm testing the rest of the patchset now.

Thank you. I will be more careful next time.

> 
> - Arnaldo
> 
> diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> index 1183983e4352798d..1502804780507b5d 100644
> --- a/tools/perf/builtin-check.c
> +++ b/tools/perf/builtin-check.c
> @@ -50,7 +50,7 @@ struct feature_support supported_features[] = {
>  	FEATURE_SUPPORT("zstd", HAVE_ZSTD_SUPPORT),
>  	FEATURE_SUPPORT("libpfm4", HAVE_LIBPFM),
>  	FEATURE_SUPPORT("libtraceevent", HAVE_LIBTRACEEVENT),
> -	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL);
> +	FEATURE_SUPPORT("bpf_skeletons", HAVE_BPF_SKEL),
>  
>  	/* this should remain at end, to know the array end */
>  	FEATURE_SUPPORT(NULL, _)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v6 2/4] perf version: update --build-options to use 'supported_features' array
  2023-10-21 15:05 [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
  2023-10-21 15:05 ` [PATCH v6 1/4] perf check: introduce " Aditya Gupta
@ 2023-10-21 15:05 ` Aditya Gupta
  2023-10-21 15:05 ` [PATCH v6 3/4] perf tests task_analyzer: use perf check for libtraceevent support Aditya Gupta
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-10-21 15:05 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 tools/perf/builtin-version.c | 40 ++++++++----------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
index ac20c2b9bbc2..e149d96c6dc5 100644
--- a/tools/perf/builtin-version.c
+++ b/tools/perf/builtin-version.c
@@ -46,42 +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_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);
+	for (int i = 0; supported_features[i].name; ++i)
+		STATUS(supported_features[i]);
 }
 
 int cmd_version(int argc, const char **argv)
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v6 3/4] perf tests task_analyzer: use perf check for libtraceevent support
  2023-10-21 15:05 [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
  2023-10-21 15:05 ` [PATCH v6 1/4] perf check: introduce " Aditya Gupta
  2023-10-21 15:05 ` [PATCH v6 2/4] perf version: update --build-options to use 'supported_features' array Aditya Gupta
@ 2023-10-21 15:05 ` Aditya Gupta
  2023-10-21 15:05 ` [PATCH v6 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature Aditya Gupta
  2023-11-07  3:42 ` [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
  4 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-10-21 15:05 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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>
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..f4db68edb2e3 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 -q --feature libtraceevent && return 0
+	return 2
 }
 
 prepare_perf_data() {
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v6 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature
  2023-10-21 15:05 [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
                   ` (2 preceding siblings ...)
  2023-10-21 15:05 ` [PATCH v6 3/4] perf tests task_analyzer: use perf check for libtraceevent support Aditya Gupta
@ 2023-10-21 15:05 ` Aditya Gupta
  2023-11-07  3:42 ` [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
  4 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-10-21 15:05 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

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..368a55c02134 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 -q --feature 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 eebeea6bdc76..758da2a23e26 100755
--- a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
+++ b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
@@ -60,7 +60,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.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v6 0/4] Introduce perf check subcommand
  2023-10-21 15:05 [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
                   ` (3 preceding siblings ...)
  2023-10-21 15:05 ` [PATCH v6 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature Aditya Gupta
@ 2023-11-07  3:42 ` Aditya Gupta
  4 siblings, 0 replies; 13+ messages in thread
From: Aditya Gupta @ 2023-11-07  3:42 UTC (permalink / raw)
  To: acme, jolsa, irogers, namhyung
  Cc: linux-perf-users, maddy, atrajeev, kjain, disgoel

Hello,
Just a ping. Any comments on this series ?

Thanks,
Aditya Gupta

On Sat, Oct 21, 2023 at 08:35:22PM +0530, 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"
> 
> 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, such as:
> 
>     perf check --feature HAVE_LIBTRACEEVENT
> 
>     or
> 
>     perf check --feature libtraceevent
> 
> 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
> 
> Git tree
> ========
> 
> Git tree with this patch series applied for testing:
> https://github.com/adi-g15-ibm/linux/tree/perf-check-feature-v6
> 
> Changelog
> =========
> 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       |  59 +++++++++
>  tools/perf/builtin-check.c                    | 122 ++++++++++++++++++
>  tools/perf/builtin-version.c                  |  39 ++----
>  tools/perf/builtin.h                          |  18 +++
>  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, 221 insertions(+), 37 deletions(-)
>  create mode 100644 tools/perf/Documentation/perf-check.txt
>  create mode 100644 tools/perf/builtin-check.c
> 
> -- 
> 2.41.0
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-11-16  7:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-21 15:05 [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta
2023-10-21 15:05 ` [PATCH v6 1/4] perf check: introduce " Aditya Gupta
2023-11-10 14:27   ` Arnaldo Carvalho de Melo
2023-11-10 14:31     ` Arnaldo Carvalho de Melo
2023-11-10 14:38       ` Arnaldo Carvalho de Melo
2023-11-10 22:46         ` Namhyung Kim
2023-11-16  7:12           ` Aditya Gupta
2023-11-16  7:06         ` Aditya Gupta
2023-11-16  6:52       ` Aditya Gupta
2023-10-21 15:05 ` [PATCH v6 2/4] perf version: update --build-options to use 'supported_features' array Aditya Gupta
2023-10-21 15:05 ` [PATCH v6 3/4] perf tests task_analyzer: use perf check for libtraceevent support Aditya Gupta
2023-10-21 15:05 ` [PATCH v6 4/4] tools/perf/tests: Update probe_vfs_getname.sh script to use perf check --feature Aditya Gupta
2023-11-07  3:42 ` [PATCH v6 0/4] Introduce perf check subcommand Aditya Gupta

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.