diff mbox series

selftests/rseq: fix kselftest Clang build warnings

Message ID 20230912-kselftest-param_test-c-v1-1-80a6cffc7374@google.com
State Accepted
Commit 078a2ead544c56fbe6a3bed8cc90b5151a328b81
Headers show
Series selftests/rseq: fix kselftest Clang build warnings | expand

Commit Message

Justin Stitt Sept. 12, 2023, 9:03 p.m. UTC 
When building with Clang, I am getting many warnings from the selftests/rseq tree.

Here's one such example from rseq tree:
|  param_test.c:1234:10: error: address argument to atomic operation must be a pointer to _Atomic type ('intptr_t *' (aka 'long *') invalid)
|   1234 |         while (!atomic_load(&args->percpu_list_ptr)) {}
|        |                 ^           ~~~~~~~~~~~~~~~~~~~~~~
|  /usr/local/google/home/justinstitt/repos/tc-build/build/llvm/final/lib/clang/18/include/stdatomic.h:140:29: note: expanded from macro 'atomic_load'
|    140 | #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST)
|        |                             ^                 ~~~~~~

Use compiler builtins `__atomic_load_n()` and `__atomic_store_n()` with
accompanying __ATOMIC_ACQUIRE and __ATOMIC_RELEASE, respectively. This
will fix the warnings because the compiler builtins do not expect their
arguments to have _Atomic type. This should also make TSAN happier.

Link: https://github.com/ClangBuiltLinux/linux/issues/1698
Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: Previous RFC https://lore.kernel.org/r/20230908-kselftest-param_test-c-v1-1-e35bd9052d61@google.com
---
 tools/testing/selftests/rseq/param_test.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)


---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230908-kselftest-param_test-c-1763b62e762f

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Justin Stitt Sept. 26, 2023, 7:20 a.m. UTC | #1 
Ping.

Looking to get this patch and [1] slated for 6.7 which fixes some
kselftest builds on older kernels.

On Wed, Sep 13, 2023 at 6:03 AM Justin Stitt <justinstitt@google.com> wrote:
>
> When building with Clang, I am getting many warnings from the selftests/rseq tree.
>
> Here's one such example from rseq tree:
> |  param_test.c:1234:10: error: address argument to atomic operation must be a pointer to _Atomic type ('intptr_t *' (aka 'long *') invalid)
> |   1234 |         while (!atomic_load(&args->percpu_list_ptr)) {}
> |        |                 ^           ~~~~~~~~~~~~~~~~~~~~~~
> |  /usr/local/google/home/justinstitt/repos/tc-build/build/llvm/final/lib/clang/18/include/stdatomic.h:140:29: note: expanded from macro 'atomic_load'
> |    140 | #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST)
> |        |                             ^                 ~~~~~~
>
> Use compiler builtins `__atomic_load_n()` and `__atomic_store_n()` with
> accompanying __ATOMIC_ACQUIRE and __ATOMIC_RELEASE, respectively. This
> will fix the warnings because the compiler builtins do not expect their
> arguments to have _Atomic type. This should also make TSAN happier.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1698
> Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Note: Previous RFC https://lore.kernel.org/r/20230908-kselftest-param_test-c-v1-1-e35bd9052d61@google.com
> ---
>  tools/testing/selftests/rseq/param_test.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c
> index bf951a490bb4..20403d58345c 100644
> --- a/tools/testing/selftests/rseq/param_test.c
> +++ b/tools/testing/selftests/rseq/param_test.c
> @@ -1231,7 +1231,7 @@ void *test_membarrier_worker_thread(void *arg)
>         }
>
>         /* Wait for initialization. */
> -       while (!atomic_load(&args->percpu_list_ptr)) {}
> +       while (!__atomic_load_n(&args->percpu_list_ptr, __ATOMIC_ACQUIRE)) {}
>
>         for (i = 0; i < iters; ++i) {
>                 int ret;
> @@ -1299,22 +1299,22 @@ void *test_membarrier_manager_thread(void *arg)
>         test_membarrier_init_percpu_list(&list_a);
>         test_membarrier_init_percpu_list(&list_b);
>
> -       atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> +       __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
>
> -       while (!atomic_load(&args->stop)) {
> +       while (!__atomic_load_n(&args->stop, __ATOMIC_ACQUIRE)) {
>                 /* list_a is "active". */
>                 cpu_a = rand() % CPU_SETSIZE;
>                 /*
>                  * As list_b is "inactive", we should never see changes
>                  * to list_b.
>                  */
> -               if (expect_b != atomic_load(&list_b.c[cpu_b].head->data)) {
> +               if (expect_b != __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE)) {
>                         fprintf(stderr, "Membarrier test failed\n");
>                         abort();
>                 }
>
>                 /* Make list_b "active". */
> -               atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
> +               __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_b, __ATOMIC_RELEASE);
>                 if (rseq_membarrier_expedited(cpu_a) &&
>                                 errno != ENXIO /* missing CPU */) {
>                         perror("sys_membarrier");
> @@ -1324,27 +1324,27 @@ void *test_membarrier_manager_thread(void *arg)
>                  * Cpu A should now only modify list_b, so the values
>                  * in list_a should be stable.
>                  */
> -               expect_a = atomic_load(&list_a.c[cpu_a].head->data);
> +               expect_a = __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE);
>
>                 cpu_b = rand() % CPU_SETSIZE;
>                 /*
>                  * As list_a is "inactive", we should never see changes
>                  * to list_a.
>                  */
> -               if (expect_a != atomic_load(&list_a.c[cpu_a].head->data)) {
> +               if (expect_a != __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE)) {
>                         fprintf(stderr, "Membarrier test failed\n");
>                         abort();
>                 }
>
>                 /* Make list_a "active". */
> -               atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> +               __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
>                 if (rseq_membarrier_expedited(cpu_b) &&
>                                 errno != ENXIO /* missing CPU*/) {
>                         perror("sys_membarrier");
>                         abort();
>                 }
>                 /* Remember a value from list_b. */
> -               expect_b = atomic_load(&list_b.c[cpu_b].head->data);
> +               expect_b = __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE);
>         }
>
>         test_membarrier_free_percpu_list(&list_a);
> @@ -1401,7 +1401,7 @@ void test_membarrier(void)
>                 }
>         }
>
> -       atomic_store(&thread_args.stop, 1);
> +       __atomic_store_n(&thread_args.stop, 1, __ATOMIC_RELEASE);
>         ret = pthread_join(manager_thread, NULL);
>         if (ret) {
>                 errno = ret;
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230908-kselftest-param_test-c-1763b62e762f
>
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
>

[1]: https://lore.kernel.org/all/20230908-kselftest-09-08-v2-0-0def978a4c1b@google.com/

Thanks
Justin
Mathieu Desnoyers Sept. 26, 2023, 7:02 p.m. UTC | #2 
On 9/26/23 08:20, Justin Stitt wrote:
> Ping.
> 
> Looking to get this patch and [1] slated for 6.7 which fixes some
> kselftest builds on older kernels.
> 
> On Wed, Sep 13, 2023 at 6:03 AM Justin Stitt <justinstitt@google.com> wrote:
>>
>> When building with Clang, I am getting many warnings from the selftests/rseq tree.
>>
>> Here's one such example from rseq tree:
>> |  param_test.c:1234:10: error: address argument to atomic operation must be a pointer to _Atomic type ('intptr_t *' (aka 'long *') invalid)
>> |   1234 |         while (!atomic_load(&args->percpu_list_ptr)) {}
>> |        |                 ^           ~~~~~~~~~~~~~~~~~~~~~~
>> |  /usr/local/google/home/justinstitt/repos/tc-build/build/llvm/final/lib/clang/18/include/stdatomic.h:140:29: note: expanded from macro 'atomic_load'
>> |    140 | #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST)
>> |        |                             ^                 ~~~~~~
>>
>> Use compiler builtins `__atomic_load_n()` and `__atomic_store_n()` with
>> accompanying __ATOMIC_ACQUIRE and __ATOMIC_RELEASE, respectively. This
>> will fix the warnings because the compiler builtins do not expect their
>> arguments to have _Atomic type. This should also make TSAN happier.
>>
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1698
>> Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
>> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Signed-off-by: Justin Stitt <justinstitt@google.com>

Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Peter, should this go through tip ?

Shuah, should to go through selftests ?

Thanks,

Mathieu

>> ---
>> Note: Previous RFC https://lore.kernel.org/r/20230908-kselftest-param_test-c-v1-1-e35bd9052d61@google.com
>> ---
>>   tools/testing/selftests/rseq/param_test.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c
>> index bf951a490bb4..20403d58345c 100644
>> --- a/tools/testing/selftests/rseq/param_test.c
>> +++ b/tools/testing/selftests/rseq/param_test.c
>> @@ -1231,7 +1231,7 @@ void *test_membarrier_worker_thread(void *arg)
>>          }
>>
>>          /* Wait for initialization. */
>> -       while (!atomic_load(&args->percpu_list_ptr)) {}
>> +       while (!__atomic_load_n(&args->percpu_list_ptr, __ATOMIC_ACQUIRE)) {}
>>
>>          for (i = 0; i < iters; ++i) {
>>                  int ret;
>> @@ -1299,22 +1299,22 @@ void *test_membarrier_manager_thread(void *arg)
>>          test_membarrier_init_percpu_list(&list_a);
>>          test_membarrier_init_percpu_list(&list_b);
>>
>> -       atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
>> +       __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
>>
>> -       while (!atomic_load(&args->stop)) {
>> +       while (!__atomic_load_n(&args->stop, __ATOMIC_ACQUIRE)) {
>>                  /* list_a is "active". */
>>                  cpu_a = rand() % CPU_SETSIZE;
>>                  /*
>>                   * As list_b is "inactive", we should never see changes
>>                   * to list_b.
>>                   */
>> -               if (expect_b != atomic_load(&list_b.c[cpu_b].head->data)) {
>> +               if (expect_b != __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE)) {
>>                          fprintf(stderr, "Membarrier test failed\n");
>>                          abort();
>>                  }
>>
>>                  /* Make list_b "active". */
>> -               atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
>> +               __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_b, __ATOMIC_RELEASE);
>>                  if (rseq_membarrier_expedited(cpu_a) &&
>>                                  errno != ENXIO /* missing CPU */) {
>>                          perror("sys_membarrier");
>> @@ -1324,27 +1324,27 @@ void *test_membarrier_manager_thread(void *arg)
>>                   * Cpu A should now only modify list_b, so the values
>>                   * in list_a should be stable.
>>                   */
>> -               expect_a = atomic_load(&list_a.c[cpu_a].head->data);
>> +               expect_a = __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE);
>>
>>                  cpu_b = rand() % CPU_SETSIZE;
>>                  /*
>>                   * As list_a is "inactive", we should never see changes
>>                   * to list_a.
>>                   */
>> -               if (expect_a != atomic_load(&list_a.c[cpu_a].head->data)) {
>> +               if (expect_a != __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE)) {
>>                          fprintf(stderr, "Membarrier test failed\n");
>>                          abort();
>>                  }
>>
>>                  /* Make list_a "active". */
>> -               atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
>> +               __atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
>>                  if (rseq_membarrier_expedited(cpu_b) &&
>>                                  errno != ENXIO /* missing CPU*/) {
>>                          perror("sys_membarrier");
>>                          abort();
>>                  }
>>                  /* Remember a value from list_b. */
>> -               expect_b = atomic_load(&list_b.c[cpu_b].head->data);
>> +               expect_b = __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE);
>>          }
>>
>>          test_membarrier_free_percpu_list(&list_a);
>> @@ -1401,7 +1401,7 @@ void test_membarrier(void)
>>                  }
>>          }
>>
>> -       atomic_store(&thread_args.stop, 1);
>> +       __atomic_store_n(&thread_args.stop, 1, __ATOMIC_RELEASE);
>>          ret = pthread_join(manager_thread, NULL);
>>          if (ret) {
>>                  errno = ret;
>>
>> ---
>> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
>> change-id: 20230908-kselftest-param_test-c-1763b62e762f
>>
>> Best regards,
>> --
>> Justin Stitt <justinstitt@google.com>
>>
> 
> [1]: https://lore.kernel.org/all/20230908-kselftest-09-08-v2-0-0def978a4c1b@google.com/
> 
> Thanks
> Justin
Shuah Khan Sept. 26, 2023, 8:39 p.m. UTC | #3 
On 9/26/23 13:02, Mathieu Desnoyers wrote:
> On 9/26/23 08:20, Justin Stitt wrote:
>> Ping.
>>
>> Looking to get this patch and [1] slated for 6.7 which fixes some
>> kselftest builds on older kernels.
>>
>> On Wed, Sep 13, 2023 at 6:03 AM Justin Stitt <justinstitt@google.com> wrote:
>>>
>>> When building with Clang, I am getting many warnings from the selftests/rseq tree.
>>>
>>> Here's one such example from rseq tree:
>>> |  param_test.c:1234:10: error: address argument to atomic operation must be a pointer to _Atomic type ('intptr_t *' (aka 'long *') invalid)
>>> |   1234 |         while (!atomic_load(&args->percpu_list_ptr)) {}
>>> |        |                 ^           ~~~~~~~~~~~~~~~~~~~~~~
>>> |  /usr/local/google/home/justinstitt/repos/tc-build/build/llvm/final/lib/clang/18/include/stdatomic.h:140:29: note: expanded from macro 'atomic_load'
>>> |    140 | #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST)
>>> |        |                             ^                 ~~~~~~
>>>
>>> Use compiler builtins `__atomic_load_n()` and `__atomic_store_n()` with
>>> accompanying __ATOMIC_ACQUIRE and __ATOMIC_RELEASE, respectively. This
>>> will fix the warnings because the compiler builtins do not expect their
>>> arguments to have _Atomic type. This should also make TSAN happier.
>>>
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1698
>>> Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
>>> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>> Signed-off-by: Justin Stitt <justinstitt@google.com>
> 
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> Peter, should this go through tip ?
> 
> Shuah, should to go through selftests ?
> 

I can take this through selftests and apply it - hoping there
are no conflicts with what's in Peter's tree.

thanks,
-- Shuah
Shuah Khan Sept. 27, 2023, 1:26 a.m. UTC | #4 
On 9/26/23 14:39, Shuah Khan wrote:
> On 9/26/23 13:02, Mathieu Desnoyers wrote:
>> On 9/26/23 08:20, Justin Stitt wrote:
>>> Ping.
>>>
>>> Looking to get this patch and [1] slated for 6.7 which fixes some
>>> kselftest builds on older kernels.
>>>
>>> On Wed, Sep 13, 2023 at 6:03 AM Justin Stitt <justinstitt@google.com> wrote:
>>>>
>>>> When building with Clang, I am getting many warnings from the selftests/rseq tree.
>>>>
>>>> Here's one such example from rseq tree:
>>>> |  param_test.c:1234:10: error: address argument to atomic operation must be a pointer to _Atomic type ('intptr_t *' (aka 'long *') invalid)
>>>> |   1234 |         while (!atomic_load(&args->percpu_list_ptr)) {}
>>>> |        |                 ^           ~~~~~~~~~~~~~~~~~~~~~~
>>>> |  /usr/local/google/home/justinstitt/repos/tc-build/build/llvm/final/lib/clang/18/include/stdatomic.h:140:29: note: expanded from macro 'atomic_load'
>>>> |    140 | #define atomic_load(object) __c11_atomic_load(object, __ATOMIC_SEQ_CST)
>>>> |        |                             ^                 ~~~~~~
>>>>
>>>> Use compiler builtins `__atomic_load_n()` and `__atomic_store_n()` with
>>>> accompanying __ATOMIC_ACQUIRE and __ATOMIC_RELEASE, respectively. This
>>>> will fix the warnings because the compiler builtins do not expect their
>>>> arguments to have _Atomic type. This should also make TSAN happier.
>>>>
>>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1698
>>>> Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
>>>> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>>> Signed-off-by: Justin Stitt <justinstitt@google.com>
>>
>> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>
>> Peter, should this go through tip ?
>>
>> Shuah, should to go through selftests ?
>>
> 
> I can take this through selftests and apply it - hoping there
> are no conflicts with what's in Peter's tree.
> 

Applied to linux-kselftest next for Linux 6.7-rc1

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c
index bf951a490bb4..20403d58345c 100644
--- a/tools/testing/selftests/rseq/param_test.c
+++ b/tools/testing/selftests/rseq/param_test.c
@@ -1231,7 +1231,7 @@  void *test_membarrier_worker_thread(void *arg)
 	}
 
 	/* Wait for initialization. */
-	while (!atomic_load(&args->percpu_list_ptr)) {}
+	while (!__atomic_load_n(&args->percpu_list_ptr, __ATOMIC_ACQUIRE)) {}
 
 	for (i = 0; i < iters; ++i) {
 		int ret;
@@ -1299,22 +1299,22 @@  void *test_membarrier_manager_thread(void *arg)
 	test_membarrier_init_percpu_list(&list_a);
 	test_membarrier_init_percpu_list(&list_b);
 
-	atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
+	__atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
 
-	while (!atomic_load(&args->stop)) {
+	while (!__atomic_load_n(&args->stop, __ATOMIC_ACQUIRE)) {
 		/* list_a is "active". */
 		cpu_a = rand() % CPU_SETSIZE;
 		/*
 		 * As list_b is "inactive", we should never see changes
 		 * to list_b.
 		 */
-		if (expect_b != atomic_load(&list_b.c[cpu_b].head->data)) {
+		if (expect_b != __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE)) {
 			fprintf(stderr, "Membarrier test failed\n");
 			abort();
 		}
 
 		/* Make list_b "active". */
-		atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
+		__atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_b, __ATOMIC_RELEASE);
 		if (rseq_membarrier_expedited(cpu_a) &&
 				errno != ENXIO /* missing CPU */) {
 			perror("sys_membarrier");
@@ -1324,27 +1324,27 @@  void *test_membarrier_manager_thread(void *arg)
 		 * Cpu A should now only modify list_b, so the values
 		 * in list_a should be stable.
 		 */
-		expect_a = atomic_load(&list_a.c[cpu_a].head->data);
+		expect_a = __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE);
 
 		cpu_b = rand() % CPU_SETSIZE;
 		/*
 		 * As list_a is "inactive", we should never see changes
 		 * to list_a.
 		 */
-		if (expect_a != atomic_load(&list_a.c[cpu_a].head->data)) {
+		if (expect_a != __atomic_load_n(&list_a.c[cpu_a].head->data, __ATOMIC_ACQUIRE)) {
 			fprintf(stderr, "Membarrier test failed\n");
 			abort();
 		}
 
 		/* Make list_a "active". */
-		atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
+		__atomic_store_n(&args->percpu_list_ptr, (intptr_t)&list_a, __ATOMIC_RELEASE);
 		if (rseq_membarrier_expedited(cpu_b) &&
 				errno != ENXIO /* missing CPU*/) {
 			perror("sys_membarrier");
 			abort();
 		}
 		/* Remember a value from list_b. */
-		expect_b = atomic_load(&list_b.c[cpu_b].head->data);
+		expect_b = __atomic_load_n(&list_b.c[cpu_b].head->data, __ATOMIC_ACQUIRE);
 	}
 
 	test_membarrier_free_percpu_list(&list_a);
@@ -1401,7 +1401,7 @@  void test_membarrier(void)
 		}
 	}
 
-	atomic_store(&thread_args.stop, 1);
+	__atomic_store_n(&thread_args.stop, 1, __ATOMIC_RELEASE);
 	ret = pthread_join(manager_thread, NULL);
 	if (ret) {
 		errno = ret;