2023-05-05 10:14:12

by Sandipan Das

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

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 <[email protected]>
Cc: [email protected] # 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


2023-05-05 10:24:47

by Sandipan Das

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

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 <[email protected]>
---
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

2023-05-06 00:00:06

by Namhyung Kim

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

On Fri, May 5, 2023 at 3:03 AM Sandipan Das <[email protected]> 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 <[email protected]>

Acked-by: Namhyung Kim <[email protected]>

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
>

2023-05-06 00:09:15

by Namhyung Kim

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

Hello,

On Fri, May 5, 2023 at 3:03 AM Sandipan Das <[email protected]> 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 <[email protected]>
> Cc: [email protected] # v6.2+

Acked-by: Namhyung Kim <[email protected]>

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
>

2023-05-10 17:46:38

by Arnaldo Carvalho de Melo

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

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 <[email protected]> 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 <[email protected]>
> > Cc: [email protected] # v6.2+
>
> Acked-by: Namhyung Kim <[email protected]>

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

- Arnaldo