[PATCH 1/2] perf script: Skip aggregation for stat events

[PATCH 1/2] perf script: Skip aggregation for stat events

[Sandipan Das]

The script command does not support aggregation modes by itself although
that can be achieved using post-processing scripts. Because of this, it
does not allocate memory for aggregated event values.

Upon running perf stat record, the aggregation mode is set in the perf
data file. If the mode is AGGR_GLOBAL, the aggregated event values are
accessed and this leads to a segmentation fault since these were never
allocated to begin with. Set the mode to AGGR_NONE explicitly to avoid
this.

E.g.

  $ perf stat record -e cycles true
  $ perf script

Before:
  Segmentation fault (core dumped)

After:
  CPU   THREAD             VAL             ENA             RUN            TIME EVENT
   -1   231919          162831          362069          362069          935289 cycles:u

Fixes: 8b76a3188b85 ("perf stat: Remove unused perf_counts.aggr field")
Signed-off-by: Sandipan Das <sandipan.das@amd.com>
Cc: stable@vger.kernel.org # v6.2+
---
 tools/perf/builtin-script.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 006f522d0e7f..c57be48d65bb 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3647,6 +3647,13 @@ static int process_stat_config_event(struct perf_session *session __maybe_unused
 				     union perf_event *event)
 {
 	perf_event__read_stat_config(&stat_config, &event->stat_config);
+
+	/*
+	 * Aggregation modes are not used since post-processing scripts are
+	 * supposed to take care of such requirements
+	 */
+	stat_config.aggr_mode = AGGR_NONE;
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH 2/2] perf test: Add stat test for record and script

[Sandipan Das]

When using the global aggregation mode, running perf script after perf
stat record can result in a segmentation fault as seen with commit
8b76a3188b85 ("perf stat: Remove unused perf_counts.aggr field"). Add a
basic test to the existing suite of stat-related tests for checking if
that workflow runs without erroring out.

Signed-off-by: Sandipan Das <sandipan.das@amd.com>
---
 tools/perf/tests/shell/stat.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
index 2c1d3f704995..b154fbb15d54 100755
--- a/tools/perf/tests/shell/stat.sh
+++ b/tools/perf/tests/shell/stat.sh
@@ -28,6 +28,18 @@ test_stat_record_report() {
   echo "stat record and report test [Success]"
 }
 
+test_stat_record_script() {
+  echo "stat record and script test"
+  if ! perf stat record -o - true | perf script -i - 2>&1 | \
+    grep -E -q "CPU[[:space:]]+THREAD[[:space:]]+VAL[[:space:]]+ENA[[:space:]]+RUN[[:space:]]+TIME[[:space:]]+EVENT"
+  then
+    echo "stat record and script test [Failed]"
+    err=1
+    return
+  fi
+  echo "stat record and script test [Success]"
+}
+
 test_stat_repeat_weak_groups() {
   echo "stat repeat weak groups test"
   if ! perf stat -e '{cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles}' \
@@ -93,6 +105,7 @@ test_topdown_weak_groups() {
 
 test_default_stat
 test_stat_record_report
+test_stat_record_script
 test_stat_repeat_weak_groups
 test_topdown_groups
 test_topdown_weak_groups
-- 
2.34.1

Re: [PATCH 2/2] perf test: Add stat test for record and script

[Namhyung Kim]

On Fri, May 5, 2023 at 3:03 AM Sandipan Das <sandipan.das@amd.com> wrote:
>
> When using the global aggregation mode, running perf script after perf
> stat record can result in a segmentation fault as seen with commit
> 8b76a3188b85 ("perf stat: Remove unused perf_counts.aggr field"). Add a
> basic test to the existing suite of stat-related tests for checking if
> that workflow runs without erroring out.
>
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/tests/shell/stat.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
> index 2c1d3f704995..b154fbb15d54 100755
> --- a/tools/perf/tests/shell/stat.sh
> +++ b/tools/perf/tests/shell/stat.sh
> @@ -28,6 +28,18 @@ test_stat_record_report() {
>    echo "stat record and report test [Success]"
>  }
>
> +test_stat_record_script() {
> +  echo "stat record and script test"
> +  if ! perf stat record -o - true | perf script -i - 2>&1 | \
> +    grep -E -q "CPU[[:space:]]+THREAD[[:space:]]+VAL[[:space:]]+ENA[[:space:]]+RUN[[:space:]]+TIME[[:space:]]+EVENT"
> +  then
> +    echo "stat record and script test [Failed]"
> +    err=1
> +    return
> +  fi
> +  echo "stat record and script test [Success]"
> +}
> +
>  test_stat_repeat_weak_groups() {
>    echo "stat repeat weak groups test"
>    if ! perf stat -e '{cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles,cycles}' \
> @@ -93,6 +105,7 @@ test_topdown_weak_groups() {
>
>  test_default_stat
>  test_stat_record_report
> +test_stat_record_script
>  test_stat_repeat_weak_groups
>  test_topdown_groups
>  test_topdown_weak_groups
> --
> 2.34.1
>

Re: [PATCH 1/2] perf script: Skip aggregation for stat events

[Namhyung Kim]

Hello,

On Fri, May 5, 2023 at 3:03 AM Sandipan Das <sandipan.das@amd.com> wrote:
>
> The script command does not support aggregation modes by itself although
> that can be achieved using post-processing scripts. Because of this, it
> does not allocate memory for aggregated event values.
>
> Upon running perf stat record, the aggregation mode is set in the perf
> data file. If the mode is AGGR_GLOBAL, the aggregated event values are
> accessed and this leads to a segmentation fault since these were never
> allocated to begin with. Set the mode to AGGR_NONE explicitly to avoid
> this.
>
> E.g.
>
>   $ perf stat record -e cycles true
>   $ perf script
>
> Before:
>   Segmentation fault (core dumped)
>
> After:
>   CPU   THREAD             VAL             ENA             RUN            TIME EVENT
>    -1   231919          162831          362069          362069          935289 cycles:u
>
> Fixes: 8b76a3188b85 ("perf stat: Remove unused perf_counts.aggr field")
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> Cc: stable@vger.kernel.org # v6.2+

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/builtin-script.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 006f522d0e7f..c57be48d65bb 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3647,6 +3647,13 @@ static int process_stat_config_event(struct perf_session *session __maybe_unused
>                                      union perf_event *event)
>  {
>         perf_event__read_stat_config(&stat_config, &event->stat_config);
> +
> +       /*
> +        * Aggregation modes are not used since post-processing scripts are
> +        * supposed to take care of such requirements
> +        */
> +       stat_config.aggr_mode = AGGR_NONE;
> +
>         return 0;
>  }
>
> --
> 2.34.1
>

Re: [PATCH 1/2] perf script: Skip aggregation for stat events

[Arnaldo Carvalho de Melo]

Em Fri, May 05, 2023 at 04:43:20PM -0700, Namhyung Kim escreveu:
> Hello,
> 
> On Fri, May 5, 2023 at 3:03 AM Sandipan Das <sandipan.das@amd.com> wrote:
> >
> > The script command does not support aggregation modes by itself although
> > that can be achieved using post-processing scripts. Because of this, it
> > does not allocate memory for aggregated event values.
> >
> > Upon running perf stat record, the aggregation mode is set in the perf
> > data file. If the mode is AGGR_GLOBAL, the aggregated event values are
> > accessed and this leads to a segmentation fault since these were never
> > allocated to begin with. Set the mode to AGGR_NONE explicitly to avoid
> > this.
> >
> > E.g.
> >
> >   $ perf stat record -e cycles true
> >   $ perf script
> >
> > Before:
> >   Segmentation fault (core dumped)
> >
> > After:
> >   CPU   THREAD             VAL             ENA             RUN            TIME EVENT
> >    -1   231919          162831          362069          362069          935289 cycles:u
> >
> > Fixes: 8b76a3188b85 ("perf stat: Remove unused perf_counts.aggr field")
> > Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> > Cc: stable@vger.kernel.org # v6.2+
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks, applied both to perf-tools, for v6.4.

- Arnaldo