[PATCH v1] perf tests: Fix data symbol test with LTO builds

[PATCH v1] perf tests: Fix data symbol test with LTO builds

[Ian Rogers]

With LTO builds, although regular builds could also see this as
all the code is in one file, the datasym workload can realize the
buf1.reserved data is never accessed. The compiler moves the
variable to bss and only keeps the data1 and data2 parts as
separate variables. This causes the symbol check to fail in the
test. Make the variable volatile to disable the more aggressive
optimization. Rename the variable to make which buf1 in perf is
being referred to.

Before:
```
$ perf test -vv "data symbol"
126: Test data symbol:
--- start ---
test child forked, pid 299808
perf does not have symbol 'buf1'
perf is missing symbols - skipping test
---- end(-2) ----
126: Test data symbol                                                : Skip
$ nm perf|grep buf1
0000000000a5fa40 b buf1.0
0000000000a5fa48 b buf1.1
```

After:
```
$ nm perf|grep buf1
0000000000a53a00 d buf1
$ perf test -vv "data symbol"126: Test data symbol:
--- start ---
test child forked, pid 302166
 a53a00-a53a39 l buf1
perf does have symbol 'buf1'
Recording workload...
Waiting for "perf record has started" message
OK
Cleaning up files...
---- end(0) ----
126: Test data symbol                                                : Ok
```

Fixes: 3dfc01fe9d12 ("perf test: Add 'datasym' test workload")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/test_data_symbol.sh | 17 +++++++++--------
 tools/perf/tests/workloads/datasym.c       | 11 ++++++-----
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/tools/perf/tests/shell/test_data_symbol.sh b/tools/perf/tests/shell/test_data_symbol.sh
index c86da0235059..7da606db97cb 100755
--- a/tools/perf/tests/shell/test_data_symbol.sh
+++ b/tools/perf/tests/shell/test_data_symbol.sh
@@ -18,7 +18,7 @@ skip_if_no_mem_event() {
 
 skip_if_no_mem_event || exit 2
 
-skip_test_missing_symbol buf1
+skip_test_missing_symbol workload_datasym_buf1
 
 TEST_PROGRAM="perf test -w datasym"
 PERF_DATA=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
@@ -26,18 +26,19 @@ ERR_FILE=$(mktemp /tmp/__perf_test.stderr.XXXXX)
 
 check_result() {
 	# The memory report format is as below:
-	#    99.92%  ...  [.] buf1+0x38
+	#    99.92%  ...  [.] workload_datasym_buf1+0x38
 	result=$(perf mem report -i ${PERF_DATA} -s symbol_daddr -q 2>&1 |
-		 awk '/buf1/ { print $4 }')
+		 awk '/workload_datasym_buf1/ { print $4 }')
 
-	# Testing is failed if has no any sample for "buf1"
+	# Testing is failed if has no any sample for "workload_datasym_buf1"
 	[ -z "$result" ] && return 1
 
 	while IFS= read -r line; do
-		# The "data1" and "data2" fields in structure "buf1" have
-		# offset "0x0" and "0x38", returns failure if detect any
-		# other offset value.
-		if [ "$line" != "buf1+0x0" ] && [ "$line" != "buf1+0x38" ]; then
+		# The "data1" and "data2" fields in structure
+		# "workload_datasym_buf1" have offset "0x0" and "0x38", returns
+		# failure if detect any other offset value.
+		if [ "$line" != "workload_datasym_buf1+0x0" ] && \
+		   [ "$line" != "workload_datasym_buf1+0x38" ]; then
 			return 1
 		fi
 	done <<< "$result"
diff --git a/tools/perf/tests/workloads/datasym.c b/tools/perf/tests/workloads/datasym.c
index 8e08fc75a973..5074b3439835 100644
--- a/tools/perf/tests/workloads/datasym.c
+++ b/tools/perf/tests/workloads/datasym.c
@@ -7,7 +7,8 @@ typedef struct _buf {
 	char data2;
 } buf __attribute__((aligned(64)));
 
-static buf buf1 = {
+/* volatile to try to avoid the compiler seeing reserved as unused. */
+static volatile buf workload_datasym_buf1 = {
 	/* to have this in the data section */
 	.reserved[0] = 1,
 };
@@ -15,8 +16,8 @@ static buf buf1 = {
 static int datasym(int argc __maybe_unused, const char **argv __maybe_unused)
 {
 	for (;;) {
-		buf1.data1++;
-		if (buf1.data1 == 123) {
+		workload_datasym_buf1.data1++;
+		if (workload_datasym_buf1.data1 == 123) {
 			/*
 			 * Add some 'noise' in the loop to work around errata
 			 * 1694299 on Arm N1.
@@ -30,9 +31,9 @@ static int datasym(int argc __maybe_unused, const char **argv __maybe_unused)
 			 * longer a continuous repeating pattern that interacts
 			 * badly with the bias.
 			 */
-			buf1.data1++;
+			workload_datasym_buf1.data1++;
 		}
-		buf1.data2 += buf1.data1;
+		workload_datasym_buf1.data2 += workload_datasym_buf1.data1;
 	}
 	return 0;
 }
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v1] perf tests: Fix data symbol test with LTO builds

[Ian Rogers]

On Wed, Feb 26, 2025 at 3:01 PM Ian Rogers <irogers@google.com> wrote:
>
> With LTO builds, although regular builds could also see this as
> all the code is in one file, the datasym workload can realize the
> buf1.reserved data is never accessed. The compiler moves the
> variable to bss and only keeps the data1 and data2 parts as
> separate variables. This causes the symbol check to fail in the
> test. Make the variable volatile to disable the more aggressive
> optimization. Rename the variable to make which buf1 in perf is
> being referred to.
>
> Before:
> ```
> $ perf test -vv "data symbol"
> 126: Test data symbol:
> --- start ---
> test child forked, pid 299808
> perf does not have symbol 'buf1'
> perf is missing symbols - skipping test
> ---- end(-2) ----
> 126: Test data symbol                                                : Skip
> $ nm perf|grep buf1
> 0000000000a5fa40 b buf1.0
> 0000000000a5fa48 b buf1.1
> ```
>
> After:
> ```
> $ nm perf|grep buf1
> 0000000000a53a00 d buf1
> $ perf test -vv "data symbol"126: Test data symbol:
> --- start ---
> test child forked, pid 302166
>  a53a00-a53a39 l buf1
> perf does have symbol 'buf1'
> Recording workload...
> Waiting for "perf record has started" message
> OK
> Cleaning up files...
> ---- end(0) ----
> 126: Test data symbol                                                : Ok
> ```
>
> Fixes: 3dfc01fe9d12 ("perf test: Add 'datasym' test workload")
> Signed-off-by: Ian Rogers <irogers@google.com>

Ah, I see we're trying to force -O0 and -fno-inline in the Makefile
(git.kernel.org is giving 403s):
https://github.com/torvalds/linux/blob/master/tools/perf/tests/workloads/Build#L11
Which LTO later undoes. I'm not seeing LTO breakages for brstack.c and
the shell test "Check branch stack sampling". I think LTO is able to
optimize this case as it is a variable/struct being optimized, so the
"-O0" and "-fno-inline" mustn't be being made to apply. Not a wholly
satisfactory reason to add the volatile, but I'm short on
alternatives.

Thanks,
Ian



> ---
>  tools/perf/tests/shell/test_data_symbol.sh | 17 +++++++++--------
>  tools/perf/tests/workloads/datasym.c       | 11 ++++++-----
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/tests/shell/test_data_symbol.sh b/tools/perf/tests/shell/test_data_symbol.sh
> index c86da0235059..7da606db97cb 100755
> --- a/tools/perf/tests/shell/test_data_symbol.sh
> +++ b/tools/perf/tests/shell/test_data_symbol.sh
> @@ -18,7 +18,7 @@ skip_if_no_mem_event() {
>
>  skip_if_no_mem_event || exit 2
>
> -skip_test_missing_symbol buf1
> +skip_test_missing_symbol workload_datasym_buf1
>
>  TEST_PROGRAM="perf test -w datasym"
>  PERF_DATA=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> @@ -26,18 +26,19 @@ ERR_FILE=$(mktemp /tmp/__perf_test.stderr.XXXXX)
>
>  check_result() {
>         # The memory report format is as below:
> -       #    99.92%  ...  [.] buf1+0x38
> +       #    99.92%  ...  [.] workload_datasym_buf1+0x38
>         result=$(perf mem report -i ${PERF_DATA} -s symbol_daddr -q 2>&1 |
> -                awk '/buf1/ { print $4 }')
> +                awk '/workload_datasym_buf1/ { print $4 }')
>
> -       # Testing is failed if has no any sample for "buf1"
> +       # Testing is failed if has no any sample for "workload_datasym_buf1"
>         [ -z "$result" ] && return 1
>
>         while IFS= read -r line; do
> -               # The "data1" and "data2" fields in structure "buf1" have
> -               # offset "0x0" and "0x38", returns failure if detect any
> -               # other offset value.
> -               if [ "$line" != "buf1+0x0" ] && [ "$line" != "buf1+0x38" ]; then
> +               # The "data1" and "data2" fields in structure
> +               # "workload_datasym_buf1" have offset "0x0" and "0x38", returns
> +               # failure if detect any other offset value.
> +               if [ "$line" != "workload_datasym_buf1+0x0" ] && \
> +                  [ "$line" != "workload_datasym_buf1+0x38" ]; then
>                         return 1
>                 fi
>         done <<< "$result"
> diff --git a/tools/perf/tests/workloads/datasym.c b/tools/perf/tests/workloads/datasym.c
> index 8e08fc75a973..5074b3439835 100644
> --- a/tools/perf/tests/workloads/datasym.c
> +++ b/tools/perf/tests/workloads/datasym.c
> @@ -7,7 +7,8 @@ typedef struct _buf {
>         char data2;
>  } buf __attribute__((aligned(64)));
>
> -static buf buf1 = {
> +/* volatile to try to avoid the compiler seeing reserved as unused. */
> +static volatile buf workload_datasym_buf1 = {
>         /* to have this in the data section */
>         .reserved[0] = 1,
>  };
> @@ -15,8 +16,8 @@ static buf buf1 = {
>  static int datasym(int argc __maybe_unused, const char **argv __maybe_unused)
>  {
>         for (;;) {
> -               buf1.data1++;
> -               if (buf1.data1 == 123) {
> +               workload_datasym_buf1.data1++;
> +               if (workload_datasym_buf1.data1 == 123) {
>                         /*
>                          * Add some 'noise' in the loop to work around errata
>                          * 1694299 on Arm N1.
> @@ -30,9 +31,9 @@ static int datasym(int argc __maybe_unused, const char **argv __maybe_unused)
>                          * longer a continuous repeating pattern that interacts
>                          * badly with the bias.
>                          */
> -                       buf1.data1++;
> +                       workload_datasym_buf1.data1++;
>                 }
> -               buf1.data2 += buf1.data1;
> +               workload_datasym_buf1.data2 += workload_datasym_buf1.data1;
>         }
>         return 0;
>  }
> --
> 2.48.1.711.g2feabab25a-goog
>

Re: [PATCH v1] perf tests: Fix data symbol test with LTO builds

[James Clark]

On 27/02/2025 5:42 am, Ian Rogers wrote:
> On Wed, Feb 26, 2025 at 3:01 PM Ian Rogers <irogers@google.com> wrote:
>>
>> With LTO builds, although regular builds could also see this as
>> all the code is in one file, the datasym workload can realize the
>> buf1.reserved data is never accessed. The compiler moves the
>> variable to bss and only keeps the data1 and data2 parts as
>> separate variables. This causes the symbol check to fail in the
>> test. Make the variable volatile to disable the more aggressive
>> optimization. Rename the variable to make which buf1 in perf is
>> being referred to.
>>
>> Before:
>> ```
>> $ perf test -vv "data symbol"
>> 126: Test data symbol:
>> --- start ---
>> test child forked, pid 299808
>> perf does not have symbol 'buf1'
>> perf is missing symbols - skipping test
>> ---- end(-2) ----
>> 126: Test data symbol                                                : Skip
>> $ nm perf|grep buf1
>> 0000000000a5fa40 b buf1.0
>> 0000000000a5fa48 b buf1.1
>> ```
>>
>> After:
>> ```
>> $ nm perf|grep buf1
>> 0000000000a53a00 d buf1
>> $ perf test -vv "data symbol"126: Test data symbol:
>> --- start ---
>> test child forked, pid 302166
>>   a53a00-a53a39 l buf1
>> perf does have symbol 'buf1'
>> Recording workload...
>> Waiting for "perf record has started" message
>> OK
>> Cleaning up files...
>> ---- end(0) ----
>> 126: Test data symbol                                                : Ok
>> ```
>>
>> Fixes: 3dfc01fe9d12 ("perf test: Add 'datasym' test workload")
>> Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Ah, I see we're trying to force -O0 and -fno-inline in the Makefile
> (git.kernel.org is giving 403s):
> https://github.com/torvalds/linux/blob/master/tools/perf/tests/workloads/Build#L11
> Which LTO later undoes. I'm not seeing LTO breakages for brstack.c and
> the shell test "Check branch stack sampling". I think LTO is able to
> optimize this case as it is a variable/struct being optimized, so the
> "-O0" and "-fno-inline" mustn't be being made to apply. Not a wholly
> satisfactory reason to add the volatile, but I'm short on
> alternatives.
> 
> Thanks,
> Ian
> 
> 
> 

Adding -fno-lto to that file works for me (llvm-15):

   CFLAGS_datasym.o          = -fno-lto -g -O0 -fno-inline ...

In fact it looks like we should add this to everywhere we're already 
doing -g and -O0. If lto is just another form of optimisation it should 
be disabled as well.

Re: [PATCH v1] perf tests: Fix data symbol test with LTO builds

[Namhyung Kim]

Hello,

On Thu, Feb 27, 2025 at 10:35:30AM +0000, James Clark wrote:
> 
> 
> On 27/02/2025 5:42 am, Ian Rogers wrote:
> > On Wed, Feb 26, 2025 at 3:01 PM Ian Rogers <irogers@google.com> wrote:
> > > 
> > > With LTO builds, although regular builds could also see this as
> > > all the code is in one file, the datasym workload can realize the
> > > buf1.reserved data is never accessed. The compiler moves the
> > > variable to bss and only keeps the data1 and data2 parts as
> > > separate variables. This causes the symbol check to fail in the
> > > test. Make the variable volatile to disable the more aggressive
> > > optimization. Rename the variable to make which buf1 in perf is
> > > being referred to.
> > > 
> > > Before:
> > > ```
> > > $ perf test -vv "data symbol"
> > > 126: Test data symbol:
> > > --- start ---
> > > test child forked, pid 299808
> > > perf does not have symbol 'buf1'
> > > perf is missing symbols - skipping test
> > > ---- end(-2) ----
> > > 126: Test data symbol                                                : Skip
> > > $ nm perf|grep buf1
> > > 0000000000a5fa40 b buf1.0
> > > 0000000000a5fa48 b buf1.1
> > > ```
> > > 
> > > After:
> > > ```
> > > $ nm perf|grep buf1
> > > 0000000000a53a00 d buf1
> > > $ perf test -vv "data symbol"126: Test data symbol:
> > > --- start ---
> > > test child forked, pid 302166
> > >   a53a00-a53a39 l buf1
> > > perf does have symbol 'buf1'
> > > Recording workload...
> > > Waiting for "perf record has started" message
> > > OK
> > > Cleaning up files...
> > > ---- end(0) ----
> > > 126: Test data symbol                                                : Ok
> > > ```
> > > 
> > > Fixes: 3dfc01fe9d12 ("perf test: Add 'datasym' test workload")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > 
> > Ah, I see we're trying to force -O0 and -fno-inline in the Makefile
> > (git.kernel.org is giving 403s):
> > https://github.com/torvalds/linux/blob/master/tools/perf/tests/workloads/Build#L11
> > Which LTO later undoes. I'm not seeing LTO breakages for brstack.c and
> > the shell test "Check branch stack sampling". I think LTO is able to
> > optimize this case as it is a variable/struct being optimized, so the
> > "-O0" and "-fno-inline" mustn't be being made to apply. Not a wholly
> > satisfactory reason to add the volatile, but I'm short on
> > alternatives.
> > 
> > Thanks,
> > Ian
> > 
> > 
> > 
> 
> Adding -fno-lto to that file works for me (llvm-15):
> 
>   CFLAGS_datasym.o          = -fno-lto -g -O0 -fno-inline ...
> 
> In fact it looks like we should add this to everywhere we're already doing
> -g and -O0. If lto is just another form of optimisation it should be
> disabled as well.
 
Make sense.  But it turned out that disabling LTO caused some subtle
troubles in my work environment unfortunately. :(

Adding 'volatile' seems to be a simple and reasonable alternative and
I agree with the renaming.

Thanks,
Namhyung

Re: [PATCH v1] perf tests: Fix data symbol test with LTO builds

[Namhyung Kim]

On Wed, 26 Feb 2025 15:01:09 -0800, Ian Rogers wrote:
> With LTO builds, although regular builds could also see this as
> all the code is in one file, the datasym workload can realize the
> buf1.reserved data is never accessed. The compiler moves the
> variable to bss and only keeps the data1 and data2 parts as
> separate variables. This causes the symbol check to fail in the
> test. Make the variable volatile to disable the more aggressive
> optimization. Rename the variable to make which buf1 in perf is
> being referred to.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung