Based on Google-internal RSEQ work done by
Paul Turner and Andrew Hunter.
This patch adds a selftest for MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU.
The test quite often fails without the previous patch in this patchset,
but consistently passes with it.
Signed-off-by: Peter Oskolkov <[email protected]>
---
.../selftests/rseq/basic_percpu_ops_test.c | 181 ++++++++++++++++++
1 file changed, 181 insertions(+)
diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
index eb3f6db36d36..147c80deac19 100644
--- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
+++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
@@ -3,16 +3,21 @@
#include <assert.h>
#include <pthread.h>
#include <sched.h>
+#include <stdatomic.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stddef.h>
+#include <syscall.h>
+#include <unistd.h>
#include "rseq.h"
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+#define MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU (1<<7)
+
struct percpu_lock_entry {
intptr_t v;
} __attribute__((aligned(128)));
@@ -289,6 +294,180 @@ void test_percpu_list(void)
assert(sum == expected_sum);
}
+struct test_membarrier_thread_args {
+ int stop;
+ intptr_t percpu_list_ptr;
+};
+
+/* Worker threads modify data in their "active" percpu lists. */
+void *test_membarrier_worker_thread(void *arg)
+{
+ struct test_membarrier_thread_args *args =
+ (struct test_membarrier_thread_args *)arg;
+ const int iters = 10 * 1000 * 1000;
+ int i;
+
+ if (rseq_register_current_thread()) {
+ fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
+ errno, strerror(errno));
+ abort();
+ }
+
+ for (i = 0; i < iters; ++i) {
+ while (true) {
+ int cpu, ret;
+ struct percpu_list *list_ptr = (struct percpu_list *)
+ atomic_load(&args->percpu_list_ptr);
+
+ if (!list_ptr)
+ continue; /* Not yet initialized. */
+
+ cpu = rseq_cpu_start();
+ struct percpu_list_node *node = list_ptr->c[cpu].head;
+ const intptr_t prev = node->data;
+
+ ret = rseq_cmpeqv_cmpeqv_storev(&node->data, prev,
+ &args->percpu_list_ptr,
+ (intptr_t)list_ptr, prev + 1, cpu);
+ if (!ret)
+ break; /* Success. */
+ }
+ }
+
+ if (rseq_unregister_current_thread()) {
+ fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
+ errno, strerror(errno));
+ abort();
+ }
+ return NULL;
+}
+
+void test_membarrier_init_percpu_list(struct percpu_list *list)
+{
+ int i;
+
+ memset(list, 0, sizeof(*list));
+ for (i = 0; i < CPU_SETSIZE; i++) {
+ struct percpu_list_node *node;
+
+ node = malloc(sizeof(*node));
+ assert(node);
+ node->data = 0;
+ node->next = NULL;
+ list->c[i].head = node;
+ }
+}
+
+void test_membarrier_free_percpu_list(struct percpu_list *list)
+{
+ int i;
+
+ for (i = 0; i < CPU_SETSIZE; i++)
+ free(list->c[i].head);
+}
+
+static int sys_membarrier(int cmd, int flags)
+{
+ return syscall(__NR_membarrier, cmd, flags);
+}
+
+/*
+ * The manager thread swaps per-cpu lists that worker threads see,
+ * and validates that there are no unexpected modifications.
+ */
+void *test_membarrier_manager_thread(void *arg)
+{
+ struct test_membarrier_thread_args *args =
+ (struct test_membarrier_thread_args *)arg;
+ struct percpu_list list_a, list_b;
+ intptr_t expect_a = 0, expect_b = 0;
+ int cpu_a = 0, cpu_b = 0;
+
+ if (rseq_register_current_thread()) {
+ fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
+ errno, strerror(errno));
+ abort();
+ }
+
+ /* Init lists. */
+ test_membarrier_init_percpu_list(&list_a);
+ test_membarrier_init_percpu_list(&list_b);
+
+ atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
+
+ while (!atomic_load(&args->stop)) {
+ /* 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)) {
+ fprintf(stderr, "Membarrier test failed\n");
+ abort();
+ }
+
+ /* Make list_b "active". */
+ atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
+ sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_a);
+ /*
+ * Cpu A should now only modify list_b, so we values
+ * in list_a should be stable.
+ */
+ expect_a = atomic_load(&list_a.c[cpu_a].head->data);
+
+ 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)) {
+ fprintf(stderr, "Membarrier test failed\n");
+ abort();
+ }
+
+ /* Make list_a "active". */
+ atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
+ sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_b);
+ /* Remember a value from list_b. */
+ expect_b = atomic_load(&list_b.c[cpu_b].head->data);
+ }
+
+ test_membarrier_free_percpu_list(&list_a);
+ test_membarrier_free_percpu_list(&list_b);
+
+ if (rseq_unregister_current_thread()) {
+ fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
+ errno, strerror(errno));
+ abort();
+ }
+ return NULL;
+}
+
+/* Test MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU membarrier command. */
+void test_membarrier(void)
+{
+ struct test_membarrier_thread_args thread_args;
+ pthread_t worker_threads[CPU_SETSIZE];
+ pthread_t manager_thread;
+ int i;
+
+ thread_args.stop = 0;
+ thread_args.percpu_list_ptr = 0;
+ pthread_create(&manager_thread, NULL,
+ test_membarrier_manager_thread, &thread_args);
+
+ for (i = 0; i < CPU_SETSIZE; i++)
+ pthread_create(&worker_threads[i], NULL,
+ test_membarrier_worker_thread, &thread_args);
+
+ for (i = 0; i < CPU_SETSIZE; i++)
+ pthread_join(worker_threads[i], NULL);
+
+ atomic_store(&thread_args.stop, 1);
+ pthread_join(manager_thread, NULL);
+}
+
int main(int argc, char **argv)
{
if (rseq_register_current_thread()) {
@@ -300,6 +479,8 @@ int main(int argc, char **argv)
test_percpu_spinlock();
printf("percpu_list\n");
test_percpu_list();
+ printf("membarrier\n");
+ test_membarrier();
if (rseq_unregister_current_thread()) {
fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
errno, strerror(errno));
--
2.28.0.163.g6104cc2f0b6-goog
On Thu, Aug 06, 2020 at 10:05:44AM -0700, Peter Oskolkov wrote:
> Based on Google-internal RSEQ work done by
> Paul Turner and Andrew Hunter.
>
> This patch adds a selftest for MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU.
> The test quite often fails without the previous patch in this patchset,
> but consistently passes with it.
>
> Signed-off-by: Peter Oskolkov <[email protected]>
> ---
> .../selftests/rseq/basic_percpu_ops_test.c | 181 ++++++++++++++++++
> 1 file changed, 181 insertions(+)
>
> diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> index eb3f6db36d36..147c80deac19 100644
> --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> @@ -3,16 +3,21 @@
> #include <assert.h>
> #include <pthread.h>
> #include <sched.h>
> +#include <stdatomic.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <stddef.h>
> +#include <syscall.h>
> +#include <unistd.h>
>
> #include "rseq.h"
>
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>
> +#define MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU (1<<7)
> +
> struct percpu_lock_entry {
> intptr_t v;
> } __attribute__((aligned(128)));
> @@ -289,6 +294,180 @@ void test_percpu_list(void)
> assert(sum == expected_sum);
> }
>
> +struct test_membarrier_thread_args {
> + int stop;
> + intptr_t percpu_list_ptr;
> +};
> +
> +/* Worker threads modify data in their "active" percpu lists. */
> +void *test_membarrier_worker_thread(void *arg)
> +{
> + struct test_membarrier_thread_args *args =
> + (struct test_membarrier_thread_args *)arg;
> + const int iters = 10 * 1000 * 1000;
> + int i;
> +
> + if (rseq_register_current_thread()) {
> + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
> + errno, strerror(errno));
> + abort();
> + }
> +
> + for (i = 0; i < iters; ++i) {
> + while (true) {
> + int cpu, ret;
> + struct percpu_list *list_ptr = (struct percpu_list *)
> + atomic_load(&args->percpu_list_ptr);
> +
What if the manager thread update ->percpu_list_ptr and call
membarrier() here? I.e.
CPU0 CPU1
list_ptr = atomic_load(&args->percpu_list_ptr); // read list_b
atomic_store(&args->percpu_list_ptr, list_a);
sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, 1); // send ipi to restart rseq.cs on CPU1
<got IPI, but not in a rseq.cs, so nothing to do>
cpu = rseq_cpu_start(); // start a rseq.cs and accessing list_b!
The thing is, atomic_load() is an reference to ->percpu_list_ptr, which
is outside the rseq.cs, simply restarting rseq doesn't kill this
reference.
Am I missing something subtle?
Regards,
Boqun
> + if (!list_ptr)
> + continue; /* Not yet initialized. */
> +
> + cpu = rseq_cpu_start();
> + struct percpu_list_node *node = list_ptr->c[cpu].head;
> + const intptr_t prev = node->data;
> +
> + ret = rseq_cmpeqv_cmpeqv_storev(&node->data, prev,
> + &args->percpu_list_ptr,
> + (intptr_t)list_ptr, prev + 1, cpu);
> + if (!ret)
> + break; /* Success. */
> + }
> + }
> +
> + if (rseq_unregister_current_thread()) {
> + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
> + errno, strerror(errno));
> + abort();
> + }
> + return NULL;
> +}
> +
> +void test_membarrier_init_percpu_list(struct percpu_list *list)
> +{
> + int i;
> +
> + memset(list, 0, sizeof(*list));
> + for (i = 0; i < CPU_SETSIZE; i++) {
> + struct percpu_list_node *node;
> +
> + node = malloc(sizeof(*node));
> + assert(node);
> + node->data = 0;
> + node->next = NULL;
> + list->c[i].head = node;
> + }
> +}
> +
> +void test_membarrier_free_percpu_list(struct percpu_list *list)
> +{
> + int i;
> +
> + for (i = 0; i < CPU_SETSIZE; i++)
> + free(list->c[i].head);
> +}
> +
> +static int sys_membarrier(int cmd, int flags)
> +{
> + return syscall(__NR_membarrier, cmd, flags);
> +}
> +
> +/*
> + * The manager thread swaps per-cpu lists that worker threads see,
> + * and validates that there are no unexpected modifications.
> + */
> +void *test_membarrier_manager_thread(void *arg)
> +{
> + struct test_membarrier_thread_args *args =
> + (struct test_membarrier_thread_args *)arg;
> + struct percpu_list list_a, list_b;
> + intptr_t expect_a = 0, expect_b = 0;
> + int cpu_a = 0, cpu_b = 0;
> +
> + if (rseq_register_current_thread()) {
> + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
> + errno, strerror(errno));
> + abort();
> + }
> +
> + /* Init lists. */
> + test_membarrier_init_percpu_list(&list_a);
> + test_membarrier_init_percpu_list(&list_b);
> +
> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> +
> + while (!atomic_load(&args->stop)) {
> + /* 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)) {
> + fprintf(stderr, "Membarrier test failed\n");
> + abort();
> + }
> +
> + /* Make list_b "active". */
> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
> + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_a);
> + /*
> + * Cpu A should now only modify list_b, so we values
> + * in list_a should be stable.
> + */
> + expect_a = atomic_load(&list_a.c[cpu_a].head->data);
> +
> + 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)) {
> + fprintf(stderr, "Membarrier test failed\n");
> + abort();
> + }
> +
> + /* Make list_a "active". */
> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_b);
> + /* Remember a value from list_b. */
> + expect_b = atomic_load(&list_b.c[cpu_b].head->data);
> + }
> +
> + test_membarrier_free_percpu_list(&list_a);
> + test_membarrier_free_percpu_list(&list_b);
> +
> + if (rseq_unregister_current_thread()) {
> + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
> + errno, strerror(errno));
> + abort();
> + }
> + return NULL;
> +}
> +
> +/* Test MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU membarrier command. */
> +void test_membarrier(void)
> +{
> + struct test_membarrier_thread_args thread_args;
> + pthread_t worker_threads[CPU_SETSIZE];
> + pthread_t manager_thread;
> + int i;
> +
> + thread_args.stop = 0;
> + thread_args.percpu_list_ptr = 0;
> + pthread_create(&manager_thread, NULL,
> + test_membarrier_manager_thread, &thread_args);
> +
> + for (i = 0; i < CPU_SETSIZE; i++)
> + pthread_create(&worker_threads[i], NULL,
> + test_membarrier_worker_thread, &thread_args);
> +
> + for (i = 0; i < CPU_SETSIZE; i++)
> + pthread_join(worker_threads[i], NULL);
> +
> + atomic_store(&thread_args.stop, 1);
> + pthread_join(manager_thread, NULL);
> +}
> +
> int main(int argc, char **argv)
> {
> if (rseq_register_current_thread()) {
> @@ -300,6 +479,8 @@ int main(int argc, char **argv)
> test_percpu_spinlock();
> printf("percpu_list\n");
> test_percpu_list();
> + printf("membarrier\n");
> + test_membarrier();
> if (rseq_unregister_current_thread()) {
> fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
> errno, strerror(errno));
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
----- On Aug 6, 2020, at 8:27 PM, Boqun Feng [email protected] wrote:
> On Thu, Aug 06, 2020 at 10:05:44AM -0700, Peter Oskolkov wrote:
>> Based on Google-internal RSEQ work done by
>> Paul Turner and Andrew Hunter.
>>
>> This patch adds a selftest for MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU.
>> The test quite often fails without the previous patch in this patchset,
>> but consistently passes with it.
>>
>> Signed-off-by: Peter Oskolkov <[email protected]>
>> ---
>> .../selftests/rseq/basic_percpu_ops_test.c | 181 ++++++++++++++++++
>> 1 file changed, 181 insertions(+)
>>
>> diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> index eb3f6db36d36..147c80deac19 100644
>> --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> @@ -3,16 +3,21 @@
>> #include <assert.h>
>> #include <pthread.h>
>> #include <sched.h>
>> +#include <stdatomic.h>
>> #include <stdint.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> #include <stddef.h>
>> +#include <syscall.h>
>> +#include <unistd.h>
>>
>> #include "rseq.h"
>>
>> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>>
>> +#define MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU (1<<7)
>> +
>> struct percpu_lock_entry {
>> intptr_t v;
>> } __attribute__((aligned(128)));
>> @@ -289,6 +294,180 @@ void test_percpu_list(void)
>> assert(sum == expected_sum);
>> }
>>
>> +struct test_membarrier_thread_args {
>> + int stop;
>> + intptr_t percpu_list_ptr;
>> +};
>> +
>> +/* Worker threads modify data in their "active" percpu lists. */
>> +void *test_membarrier_worker_thread(void *arg)
>> +{
>> + struct test_membarrier_thread_args *args =
>> + (struct test_membarrier_thread_args *)arg;
>> + const int iters = 10 * 1000 * 1000;
>> + int i;
>> +
>> + if (rseq_register_current_thread()) {
>> + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> +
>> + for (i = 0; i < iters; ++i) {
>> + while (true) {
>> + int cpu, ret;
>> + struct percpu_list *list_ptr = (struct percpu_list *)
>> + atomic_load(&args->percpu_list_ptr);
>> +
>
> What if the manager thread update ->percpu_list_ptr and call
> membarrier() here? I.e.
>
> CPU0 CPU1
> list_ptr = atomic_load(&args->percpu_list_ptr); // read list_b
>
> atomic_store(&args->percpu_list_ptr, list_a);
> sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, 1); // send ipi to
> restart rseq.cs on CPU1
>
> <got IPI, but not in a rseq.cs, so nothing to do>
> cpu = rseq_cpu_start(); // start a rseq.cs and accessing list_b!
>
> The thing is, atomic_load() is an reference to ->percpu_list_ptr, which
> is outside the rseq.cs, simply restarting rseq doesn't kill this
> reference.
>
> Am I missing something subtle?
I'm with you on this, something looks fishy. It would be good to use
delay-inducing testing methods like rseq parametrized selftests to
increase the odds of hitting this race more reliably.
Thanks,
Mathieu
>
> Regards,
> Boqun
>
>> + if (!list_ptr)
>> + continue; /* Not yet initialized. */
>> +
>> + cpu = rseq_cpu_start();
>> + struct percpu_list_node *node = list_ptr->c[cpu].head;
>> + const intptr_t prev = node->data;
>> +
>> + ret = rseq_cmpeqv_cmpeqv_storev(&node->data, prev,
>> + &args->percpu_list_ptr,
>> + (intptr_t)list_ptr, prev + 1, cpu);
>> + if (!ret)
>> + break; /* Success. */
>> + }
>> + }
>> +
>> + if (rseq_unregister_current_thread()) {
>> + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d):
>> %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> + return NULL;
>> +}
>> +
>> +void test_membarrier_init_percpu_list(struct percpu_list *list)
>> +{
>> + int i;
>> +
>> + memset(list, 0, sizeof(*list));
>> + for (i = 0; i < CPU_SETSIZE; i++) {
>> + struct percpu_list_node *node;
>> +
>> + node = malloc(sizeof(*node));
>> + assert(node);
>> + node->data = 0;
>> + node->next = NULL;
>> + list->c[i].head = node;
>> + }
>> +}
>> +
>> +void test_membarrier_free_percpu_list(struct percpu_list *list)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < CPU_SETSIZE; i++)
>> + free(list->c[i].head);
>> +}
>> +
>> +static int sys_membarrier(int cmd, int flags)
>> +{
>> + return syscall(__NR_membarrier, cmd, flags);
>> +}
>> +
>> +/*
>> + * The manager thread swaps per-cpu lists that worker threads see,
>> + * and validates that there are no unexpected modifications.
>> + */
>> +void *test_membarrier_manager_thread(void *arg)
>> +{
>> + struct test_membarrier_thread_args *args =
>> + (struct test_membarrier_thread_args *)arg;
>> + struct percpu_list list_a, list_b;
>> + intptr_t expect_a = 0, expect_b = 0;
>> + int cpu_a = 0, cpu_b = 0;
>> +
>> + if (rseq_register_current_thread()) {
>> + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> +
>> + /* Init lists. */
>> + test_membarrier_init_percpu_list(&list_a);
>> + test_membarrier_init_percpu_list(&list_b);
>> +
>> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
>> +
>> + while (!atomic_load(&args->stop)) {
>> + /* 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)) {
>> + fprintf(stderr, "Membarrier test failed\n");
>> + abort();
>> + }
>> +
>> + /* Make list_b "active". */
>> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
>> + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_a);
>> + /*
>> + * Cpu A should now only modify list_b, so we values
>> + * in list_a should be stable.
>> + */
>> + expect_a = atomic_load(&list_a.c[cpu_a].head->data);
>> +
>> + 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)) {
>> + fprintf(stderr, "Membarrier test failed\n");
>> + abort();
>> + }
>> +
>> + /* Make list_a "active". */
>> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
>> + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_b);
>> + /* Remember a value from list_b. */
>> + expect_b = atomic_load(&list_b.c[cpu_b].head->data);
>> + }
>> +
>> + test_membarrier_free_percpu_list(&list_a);
>> + test_membarrier_free_percpu_list(&list_b);
>> +
>> + if (rseq_unregister_current_thread()) {
>> + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d):
>> %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> + return NULL;
>> +}
>> +
>> +/* Test MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU membarrier command. */
>> +void test_membarrier(void)
>> +{
>> + struct test_membarrier_thread_args thread_args;
>> + pthread_t worker_threads[CPU_SETSIZE];
>> + pthread_t manager_thread;
>> + int i;
>> +
>> + thread_args.stop = 0;
>> + thread_args.percpu_list_ptr = 0;
>> + pthread_create(&manager_thread, NULL,
>> + test_membarrier_manager_thread, &thread_args);
>> +
>> + for (i = 0; i < CPU_SETSIZE; i++)
>> + pthread_create(&worker_threads[i], NULL,
>> + test_membarrier_worker_thread, &thread_args);
>> +
>> + for (i = 0; i < CPU_SETSIZE; i++)
>> + pthread_join(worker_threads[i], NULL);
>> +
>> + atomic_store(&thread_args.stop, 1);
>> + pthread_join(manager_thread, NULL);
>> +}
>> +
>> int main(int argc, char **argv)
>> {
>> if (rseq_register_current_thread()) {
>> @@ -300,6 +479,8 @@ int main(int argc, char **argv)
>> test_percpu_spinlock();
>> printf("percpu_list\n");
>> test_percpu_list();
>> + printf("membarrier\n");
>> + test_membarrier();
>> if (rseq_unregister_current_thread()) {
>> fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
>> errno, strerror(errno));
>> --
>> 2.28.0.163.g6104cc2f0b6-goog
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Thu, Aug 6, 2020 at 5:27 PM Boqun Feng <[email protected]> wrote:
>
> On Thu, Aug 06, 2020 at 10:05:44AM -0700, Peter Oskolkov wrote:
> > Based on Google-internal RSEQ work done by
> > Paul Turner and Andrew Hunter.
> >
> > This patch adds a selftest for MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU.
> > The test quite often fails without the previous patch in this patchset,
> > but consistently passes with it.
> >
> > Signed-off-by: Peter Oskolkov <[email protected]>
> > ---
> > .../selftests/rseq/basic_percpu_ops_test.c | 181 ++++++++++++++++++
> > 1 file changed, 181 insertions(+)
> >
> > diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> > index eb3f6db36d36..147c80deac19 100644
> > --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> > +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> > @@ -3,16 +3,21 @@
> > #include <assert.h>
> > #include <pthread.h>
> > #include <sched.h>
> > +#include <stdatomic.h>
> > #include <stdint.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > #include <stddef.h>
> > +#include <syscall.h>
> > +#include <unistd.h>
> >
> > #include "rseq.h"
> >
> > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> >
> > +#define MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU (1<<7)
> > +
> > struct percpu_lock_entry {
> > intptr_t v;
> > } __attribute__((aligned(128)));
> > @@ -289,6 +294,180 @@ void test_percpu_list(void)
> > assert(sum == expected_sum);
> > }
> >
> > +struct test_membarrier_thread_args {
> > + int stop;
> > + intptr_t percpu_list_ptr;
> > +};
> > +
> > +/* Worker threads modify data in their "active" percpu lists. */
> > +void *test_membarrier_worker_thread(void *arg)
> > +{
> > + struct test_membarrier_thread_args *args =
> > + (struct test_membarrier_thread_args *)arg;
> > + const int iters = 10 * 1000 * 1000;
> > + int i;
> > +
> > + if (rseq_register_current_thread()) {
> > + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
> > + errno, strerror(errno));
> > + abort();
> > + }
> > +
> > + for (i = 0; i < iters; ++i) {
> > + while (true) {
> > + int cpu, ret;
> > + struct percpu_list *list_ptr = (struct percpu_list *)
> > + atomic_load(&args->percpu_list_ptr);
> > +
>
> What if the manager thread update ->percpu_list_ptr and call
> membarrier() here? I.e.
>
> CPU0 CPU1
> list_ptr = atomic_load(&args->percpu_list_ptr); // read list_b
>
> atomic_store(&args->percpu_list_ptr, list_a);
> sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, 1); // send ipi to restart rseq.cs on CPU1
>
> <got IPI, but not in a rseq.cs, so nothing to do>
> cpu = rseq_cpu_start(); // start a rseq.cs and accessing list_b!
>
> The thing is, atomic_load() is an reference to ->percpu_list_ptr, which
> is outside the rseq.cs, simply restarting rseq doesn't kill this
> reference.
>
> Am I missing something subtle?
rseq_cmpeqv_cmpeqv_store is used below to make sure the reference is
the one that should be used; if it is no longer "active", the
iteration is restarted.
>
> Regards,
> Boqun
>
> > + if (!list_ptr)
> > + continue; /* Not yet initialized. */
> > +
> > + cpu = rseq_cpu_start();
> > + struct percpu_list_node *node = list_ptr->c[cpu].head;
> > + const intptr_t prev = node->data;
> > +
> > + ret = rseq_cmpeqv_cmpeqv_storev(&node->data, prev,
> > + &args->percpu_list_ptr,
> > + (intptr_t)list_ptr, prev + 1, cpu);
> > + if (!ret)
> > + break; /* Success. */
> > + }
> > + }
> > +
> > + if (rseq_unregister_current_thread()) {
> > + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
> > + errno, strerror(errno));
> > + abort();
> > + }
> > + return NULL;
> > +}
> > +
> > +void test_membarrier_init_percpu_list(struct percpu_list *list)
> > +{
> > + int i;
> > +
> > + memset(list, 0, sizeof(*list));
> > + for (i = 0; i < CPU_SETSIZE; i++) {
> > + struct percpu_list_node *node;
> > +
> > + node = malloc(sizeof(*node));
> > + assert(node);
> > + node->data = 0;
> > + node->next = NULL;
> > + list->c[i].head = node;
> > + }
> > +}
> > +
> > +void test_membarrier_free_percpu_list(struct percpu_list *list)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < CPU_SETSIZE; i++)
> > + free(list->c[i].head);
> > +}
> > +
> > +static int sys_membarrier(int cmd, int flags)
> > +{
> > + return syscall(__NR_membarrier, cmd, flags);
> > +}
> > +
> > +/*
> > + * The manager thread swaps per-cpu lists that worker threads see,
> > + * and validates that there are no unexpected modifications.
> > + */
> > +void *test_membarrier_manager_thread(void *arg)
> > +{
> > + struct test_membarrier_thread_args *args =
> > + (struct test_membarrier_thread_args *)arg;
> > + struct percpu_list list_a, list_b;
> > + intptr_t expect_a = 0, expect_b = 0;
> > + int cpu_a = 0, cpu_b = 0;
> > +
> > + if (rseq_register_current_thread()) {
> > + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
> > + errno, strerror(errno));
> > + abort();
> > + }
> > +
> > + /* Init lists. */
> > + test_membarrier_init_percpu_list(&list_a);
> > + test_membarrier_init_percpu_list(&list_b);
> > +
> > + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> > +
> > + while (!atomic_load(&args->stop)) {
> > + /* 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)) {
> > + fprintf(stderr, "Membarrier test failed\n");
> > + abort();
> > + }
> > +
> > + /* Make list_b "active". */
> > + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
> > + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_a);
> > + /*
> > + * Cpu A should now only modify list_b, so we values
> > + * in list_a should be stable.
> > + */
> > + expect_a = atomic_load(&list_a.c[cpu_a].head->data);
> > +
> > + 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)) {
> > + fprintf(stderr, "Membarrier test failed\n");
> > + abort();
> > + }
> > +
> > + /* Make list_a "active". */
> > + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
> > + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_b);
> > + /* Remember a value from list_b. */
> > + expect_b = atomic_load(&list_b.c[cpu_b].head->data);
> > + }
> > +
> > + test_membarrier_free_percpu_list(&list_a);
> > + test_membarrier_free_percpu_list(&list_b);
> > +
> > + if (rseq_unregister_current_thread()) {
> > + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
> > + errno, strerror(errno));
> > + abort();
> > + }
> > + return NULL;
> > +}
> > +
> > +/* Test MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU membarrier command. */
> > +void test_membarrier(void)
> > +{
> > + struct test_membarrier_thread_args thread_args;
> > + pthread_t worker_threads[CPU_SETSIZE];
> > + pthread_t manager_thread;
> > + int i;
> > +
> > + thread_args.stop = 0;
> > + thread_args.percpu_list_ptr = 0;
> > + pthread_create(&manager_thread, NULL,
> > + test_membarrier_manager_thread, &thread_args);
> > +
> > + for (i = 0; i < CPU_SETSIZE; i++)
> > + pthread_create(&worker_threads[i], NULL,
> > + test_membarrier_worker_thread, &thread_args);
> > +
> > + for (i = 0; i < CPU_SETSIZE; i++)
> > + pthread_join(worker_threads[i], NULL);
> > +
> > + atomic_store(&thread_args.stop, 1);
> > + pthread_join(manager_thread, NULL);
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > if (rseq_register_current_thread()) {
> > @@ -300,6 +479,8 @@ int main(int argc, char **argv)
> > test_percpu_spinlock();
> > printf("percpu_list\n");
> > test_percpu_list();
> > + printf("membarrier\n");
> > + test_membarrier();
> > if (rseq_unregister_current_thread()) {
> > fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
> > errno, strerror(errno));
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
----- On Aug 7, 2020, at 1:55 PM, Peter Oskolkov [email protected] wrote:
> On Thu, Aug 6, 2020 at 5:27 PM Boqun Feng <[email protected]> wrote:
[...]
>> What if the manager thread update ->percpu_list_ptr and call
>> membarrier() here? I.e.
>>
>> CPU0 CPU1
>> list_ptr = atomic_load(&args->percpu_list_ptr); // read list_b
>>
>> atomic_store(&args->percpu_list_ptr, list_a);
>> sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, 1); // send ipi to
>> restart rseq.cs on CPU1
>>
>> <got IPI, but not in a rseq.cs, so nothing to do>
>> cpu = rseq_cpu_start(); // start a rseq.cs and accessing list_b!
>>
>> The thing is, atomic_load() is an reference to ->percpu_list_ptr, which
>> is outside the rseq.cs, simply restarting rseq doesn't kill this
>> reference.
>>
>> Am I missing something subtle?
>
> rseq_cmpeqv_cmpeqv_store is used below to make sure the reference is
> the one that should be used; if it is no longer "active", the
> iteration is restarted.
I suspect it "works" because the manager thread does not free and
repurpose the memory where list_a is allocated, nor does it store to
its list head (which would corrupt the pointer dereferenced by CPU 1
in the scenario above). This shares similarities with type-safe memory
allocation (see SLAB_TYPESAFE_BY_RCU).
Even though it is not documented as such (or otherwise) in the example code,
I feel this example looks like it guarantees that the manager thread "owns"
list_a after the rseq-fence, when in fact it can still be read by the rseq
critical sections.
AFAIU moving the atomic_load(&args->percpu_list_ptr) into the critical section
should entirely solve this and guarantee exclusive access to the old list
after the manager's rseq-fence. I wonder why this simpler approach is not
favored ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Fri, Aug 7, 2020 at 11:25 AM Mathieu Desnoyers
<[email protected]> wrote:
>
> ----- On Aug 7, 2020, at 1:55 PM, Peter Oskolkov [email protected] wrote:
>
> > On Thu, Aug 6, 2020 at 5:27 PM Boqun Feng <[email protected]> wrote:
> [...]
> >> What if the manager thread update ->percpu_list_ptr and call
> >> membarrier() here? I.e.
> >>
> >> CPU0 CPU1
> >> list_ptr = atomic_load(&args->percpu_list_ptr); // read list_b
> >>
> >> atomic_store(&args->percpu_list_ptr, list_a);
> >> sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, 1); // send ipi to
> >> restart rseq.cs on CPU1
> >>
> >> <got IPI, but not in a rseq.cs, so nothing to do>
> >> cpu = rseq_cpu_start(); // start a rseq.cs and accessing list_b!
> >>
> >> The thing is, atomic_load() is an reference to ->percpu_list_ptr, which
> >> is outside the rseq.cs, simply restarting rseq doesn't kill this
> >> reference.
> >>
> >> Am I missing something subtle?
> >
> > rseq_cmpeqv_cmpeqv_store is used below to make sure the reference is
> > the one that should be used; if it is no longer "active", the
> > iteration is restarted.
>
> I suspect it "works" because the manager thread does not free and
> repurpose the memory where list_a is allocated, nor does it store to
> its list head (which would corrupt the pointer dereferenced by CPU 1
> in the scenario above). This shares similarities with type-safe memory
> allocation (see SLAB_TYPESAFE_BY_RCU).
>
> Even though it is not documented as such (or otherwise) in the example code,
> I feel this example looks like it guarantees that the manager thread "owns"
> list_a after the rseq-fence, when in fact it can still be read by the rseq
> critical sections.
>
> AFAIU moving the atomic_load(&args->percpu_list_ptr) into the critical section
> should entirely solve this and guarantee exclusive access to the old list
> after the manager's rseq-fence. I wonder why this simpler approach is not
> favored ?
I think the test code mimics our actual production code, where the concerns
you outlined are not particularly relevant. I'll see if the test can
be simplified
in v3 along the lines you suggested.
Thanks,
Peter
>
> Thanks,
>
> Mathieu
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
----- On Aug 7, 2020, at 2:47 PM, Peter Oskolkov [email protected] wrote:
> On Fri, Aug 7, 2020 at 11:25 AM Mathieu Desnoyers
> <[email protected]> wrote:
>>
>> ----- On Aug 7, 2020, at 1:55 PM, Peter Oskolkov [email protected] wrote:
>>
>> > On Thu, Aug 6, 2020 at 5:27 PM Boqun Feng <[email protected]> wrote:
>> [...]
>> >> What if the manager thread update ->percpu_list_ptr and call
>> >> membarrier() here? I.e.
>> >>
>> >> CPU0 CPU1
>> >> list_ptr = atomic_load(&args->percpu_list_ptr); // read list_b
>> >>
>> >> atomic_store(&args->percpu_list_ptr, list_a);
>> >> sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, 1); // send ipi to
>> >> restart rseq.cs on CPU1
>> >>
>> >> <got IPI, but not in a rseq.cs, so nothing to do>
>> >> cpu = rseq_cpu_start(); // start a rseq.cs and accessing list_b!
>> >>
>> >> The thing is, atomic_load() is an reference to ->percpu_list_ptr, which
>> >> is outside the rseq.cs, simply restarting rseq doesn't kill this
>> >> reference.
>> >>
>> >> Am I missing something subtle?
>> >
>> > rseq_cmpeqv_cmpeqv_store is used below to make sure the reference is
>> > the one that should be used; if it is no longer "active", the
>> > iteration is restarted.
>>
>> I suspect it "works" because the manager thread does not free and
>> repurpose the memory where list_a is allocated, nor does it store to
>> its list head (which would corrupt the pointer dereferenced by CPU 1
>> in the scenario above). This shares similarities with type-safe memory
>> allocation (see SLAB_TYPESAFE_BY_RCU).
>>
>> Even though it is not documented as such (or otherwise) in the example code,
>> I feel this example looks like it guarantees that the manager thread "owns"
>> list_a after the rseq-fence, when in fact it can still be read by the rseq
>> critical sections.
>>
>> AFAIU moving the atomic_load(&args->percpu_list_ptr) into the critical section
>> should entirely solve this and guarantee exclusive access to the old list
>> after the manager's rseq-fence. I wonder why this simpler approach is not
>> favored ?
>
> I think the test code mimics our actual production code, where the concerns
> you outlined are not particularly relevant. I'll see if the test can
> be simplified
> in v3 along the lines you suggested.
In order to implement that, you'll need to extend the rseq per-arch
macros. Here is one I did for x86 (but not all other arch) which dereferences
a pointer, adds an offset that the resulting address, and loads the contents
of that memory location, all within a rseq critical section. See
https://git.kernel.org/pub/scm/libs/librseq/librseq.git/tree/include/rseq/rseq-x86.h#n1292
int rseq_deref_loadoffp(intptr_t *p, off_t voffp, intptr_t *load, int cpu)
I did that following a discussion with Paul Turner about the requirements for the
rseq fence.
For the use-case you have in this example, you will probably want to create a new
int rseq_deref_offset_addv(intptr_t *p, off_t voffp, intptr_t count, int cpu)
Which dereferences the list pointer and adds an offset within the critical section,
and then increments the value at that memory location as a commit.
offsetof() is very useful to generate the voffp argument.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com