[PATCH 1/2] perf record: Fix sample cgroup & namespace tracking

[PATCH 1/2] perf record: Fix sample cgroup & namespace tracking

[Namhyung Kim]

The recent change in perf_tool constification broke the cgroup and/or
namespace tracking by resetting tool fields.  It should set the values
after perf_tool__init().

Fixes: cecb1cf154b30 ("perf record: Use perf_tool__init()")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-record.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 39367709fd99..adbaf80b398c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2374,13 +2374,8 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	signal(SIGTERM, sig_handler);
 	signal(SIGSEGV, sigsegv_handler);
 
-	if (rec->opts.record_namespaces)
-		tool->namespace_events = true;
-
 	if (rec->opts.record_cgroup) {
-#ifdef HAVE_FILE_HANDLE
-		tool->cgroup_events = true;
-#else
+#ifndef HAVE_FILE_HANDLE
 		pr_err("cgroup tracking is not supported\n");
 		return -1;
 #endif
@@ -2406,6 +2401,8 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	tool->mmap2		= build_id__process_mmap2;
 	tool->itrace_start	= process_timestamp_boundary;
 	tool->aux		= process_timestamp_boundary;
+	tool->namespace_events	= rec->opts.record_namespaces;
+	tool->cgroup_events	= rec->opts.record_cgroup;
 	session = perf_session__new(data, tool);
 	if (IS_ERR(session)) {
 		pr_err("Perf session creation failed.\n");
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 2/2] perf test: Add cgroup sampling test

[Namhyung Kim]

Add it to the record.sh shell test to verify if it tracks cgroup
information correctly.  It records with --all-cgroups option can check
if it has PERF_RECORD_CGROUP and the names are not "unknown".

  $ sudo ./perf test -vv 95
   95: perf record tests:
  --- start ---
  test child forked, pid 2871922
   169c90-169cd0 g test_loop
  perf does have symbol 'test_loop'
  Basic --per-thread mode test
  Basic --per-thread mode test [Success]
  Register capture test
  Register capture test [Success]
  Basic --system-wide mode test
  Basic --system-wide mode test [Success]
  Basic target workload test
  Basic target workload test [Success]
  Branch counter test
  branch counter feature not supported on all core PMUs (/sys/bus/event_source/devices/cpu) [Skipped]
  Cgroup sampling test
  Cgroup sampling test [Success]
  ---- end(0) ----
   95: perf record tests                                               : Ok

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/record.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 36883b03169f..048078ee2eca 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -206,6 +206,28 @@ test_branch_counter() {
   echo "Branch counter test [Success]"
 }
 
+test_cgroup() {
+  echo "Cgroup sampling test"
+  if ! perf record -aB --synth=cgroup --all-cgroups -o "${perfdata}" ${testprog} 2> /dev/null
+  then
+    echo "Cgroup sampling [Skipped not supported]"
+    return
+  fi
+  if ! perf report -i "${perfdata}" -D | grep -q "CGROUP"
+  then
+    echo "Cgroup sampling [Failed missing output]"
+    err=1
+    return
+  fi
+  if ! perf script -i "${perfdata}" -F cgroup | grep -q -v "unknown"
+  then
+    echo "Cgroup sampling [Failed cannot resolve cgroup names]"
+    err=1
+    return
+  fi
+  echo "Cgroup sampling test [Success]"
+}
+
 # raise the limit of file descriptors to minimum
 if [[ $default_fd_limit -lt $min_fd_limit ]]; then
        ulimit -Sn $min_fd_limit
@@ -216,6 +238,7 @@ test_register_capture
 test_system_wide
 test_workload
 test_branch_counter
+test_cgroup
 
 # restore the default value
 ulimit -Sn $default_fd_limit
-- 
2.46.0.184.g6999bdac58-goog

Re: [PATCH 1/2] perf record: Fix sample cgroup & namespace tracking

[Ian Rogers]

On Sun, Aug 18, 2024 at 2:29 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The recent change in perf_tool constification broke the cgroup and/or
> namespace tracking by resetting tool fields.  It should set the values
> after perf_tool__init().
>
> Fixes: cecb1cf154b30 ("perf record: Use perf_tool__init()")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-record.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 39367709fd99..adbaf80b398c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2374,13 +2374,8 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>         signal(SIGTERM, sig_handler);
>         signal(SIGSEGV, sigsegv_handler);
>
> -       if (rec->opts.record_namespaces)
> -               tool->namespace_events = true;
> -
>         if (rec->opts.record_cgroup) {
> -#ifdef HAVE_FILE_HANDLE
> -               tool->cgroup_events = true;
> -#else
> +#ifndef HAVE_FILE_HANDLE
>                 pr_err("cgroup tracking is not supported\n");
>                 return -1;
>  #endif
> @@ -2406,6 +2401,8 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>         tool->mmap2             = build_id__process_mmap2;
>         tool->itrace_start      = process_timestamp_boundary;
>         tool->aux               = process_timestamp_boundary;
> +       tool->namespace_events  = rec->opts.record_namespaces;
> +       tool->cgroup_events     = rec->opts.record_cgroup;
>         session = perf_session__new(data, tool);
>         if (IS_ERR(session)) {
>                 pr_err("Perf session creation failed.\n");
> --
> 2.46.0.184.g6999bdac58-goog
>

Re: [PATCH 2/2] perf test: Add cgroup sampling test

[Ian Rogers]

On Sun, Aug 18, 2024 at 2:29 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Add it to the record.sh shell test to verify if it tracks cgroup
> information correctly.  It records with --all-cgroups option can check
> if it has PERF_RECORD_CGROUP and the names are not "unknown".
>
>   $ sudo ./perf test -vv 95
>    95: perf record tests:
>   --- start ---
>   test child forked, pid 2871922
>    169c90-169cd0 g test_loop
>   perf does have symbol 'test_loop'
>   Basic --per-thread mode test
>   Basic --per-thread mode test [Success]
>   Register capture test
>   Register capture test [Success]
>   Basic --system-wide mode test
>   Basic --system-wide mode test [Success]
>   Basic target workload test
>   Basic target workload test [Success]
>   Branch counter test
>   branch counter feature not supported on all core PMUs (/sys/bus/event_source/devices/cpu) [Skipped]
>   Cgroup sampling test
>   Cgroup sampling test [Success]
>   ---- end(0) ----
>    95: perf record tests                                               : Ok
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/tests/shell/record.sh | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> index 36883b03169f..048078ee2eca 100755
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
> @@ -206,6 +206,28 @@ test_branch_counter() {
>    echo "Branch counter test [Success]"
>  }
>
> +test_cgroup() {
> +  echo "Cgroup sampling test"
> +  if ! perf record -aB --synth=cgroup --all-cgroups -o "${perfdata}" ${testprog} 2> /dev/null
> +  then
> +    echo "Cgroup sampling [Skipped not supported]"
> +    return
> +  fi
> +  if ! perf report -i "${perfdata}" -D | grep -q "CGROUP"
> +  then
> +    echo "Cgroup sampling [Failed missing output]"
> +    err=1
> +    return
> +  fi
> +  if ! perf script -i "${perfdata}" -F cgroup | grep -q -v "unknown"
> +  then
> +    echo "Cgroup sampling [Failed cannot resolve cgroup names]"
> +    err=1
> +    return
> +  fi
> +  echo "Cgroup sampling test [Success]"
> +}
> +
>  # raise the limit of file descriptors to minimum
>  if [[ $default_fd_limit -lt $min_fd_limit ]]; then
>         ulimit -Sn $min_fd_limit
> @@ -216,6 +238,7 @@ test_register_capture
>  test_system_wide
>  test_workload
>  test_branch_counter
> +test_cgroup
>
>  # restore the default value
>  ulimit -Sn $default_fd_limit
> --
> 2.46.0.184.g6999bdac58-goog
>

Re: [PATCH 1/2] perf record: Fix sample cgroup & namespace tracking

[Arnaldo Carvalho de Melo]

On Mon, Aug 19, 2024 at 08:20:26AM -0700, Ian Rogers wrote:
> On Sun, Aug 18, 2024 at 2:29 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The recent change in perf_tool constification broke the cgroup and/or
> > namespace tracking by resetting tool fields.  It should set the values
> > after perf_tool__init().
> >
> > Fixes: cecb1cf154b30 ("perf record: Use perf_tool__init()")
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks, applied to perf-tools-next,

- Arnaldo