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 <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
Note: Previous RFC https://lore.kernel.org/r/[email protected]
---
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> Note: Previous RFC https://lore.kernel.org/r/[email protected]
> ---
> 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 <[email protected]>
>
[1]: https://lore.kernel.org/all/[email protected]/
Thanks
Justin
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 <[email protected]> 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 <[email protected]>
>> Signed-off-by: Justin Stitt <[email protected]>
Reviewed-by: Mathieu Desnoyers <[email protected]>
Peter, should this go through tip ?
Shuah, should to go through selftests ?
Thanks,
Mathieu
>> ---
>> Note: Previous RFC https://lore.kernel.org/r/[email protected]
>> ---
>> 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 <[email protected]>
>>
>
> [1]: https://lore.kernel.org/all/[email protected]/
>
> Thanks
> Justin
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
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 <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Justin Stitt <[email protected]>
>
> Reviewed-by: Mathieu Desnoyers <[email protected]>
>
> 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
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 <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Justin Stitt <[email protected]>
>>
>> Reviewed-by: Mathieu Desnoyers <[email protected]>
>>
>> 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