Message-ID: <1887105478.12300.1600202496058.JavaMail.zimbra@efficios.com>
Date:   Tue, 15 Sep 2020 16:41:36 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Peter Oskolkov <posk@...gle.com>
Cc:     paulmck <paulmck@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Boqun Feng <boqun.feng@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Paul Turner <pjt@...gle.com>,
        Chris Kennelly <ckennelly@...gle.com>,
        Peter Oskolkov <posk@...k.io>, Shuah Khan <shuah@...nel.org>,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 2/2 v7] rseq/selftests: test
 MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ

[ Adding selftests maintainer and mailing list in CC. You should add them to the CC list
  of the selftests patches for the next round. ]

----- On Sep 15, 2020, at 2:55 PM, Peter Oskolkov posk@...gle.com wrote:

> Based on Google-internal RSEQ work done by
> Paul Turner and Andrew Hunter.
> 
> This patch adds a selftest for MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ.
> The test quite often fails without the previous patch in this patchset,
> but consistently passes with it.

Did you consider moving this test into tools/testing/selftests/rseq/param_test.c
instead, and update the script "run_param_test.sh" accordingly ? You would leverage for
free all the work I have done to insert configurable delay loops into the critical
sections, which will very likely increase the likelihood of failure tremendously.

Reproducible frequent and fast failure is really something we want to aim for here
when a bug is hiding.

> 
> v3: added rseq_offset_deref_addv() to x86_64 to make the test
>    more explicit; on other architectures I kept using existing
>    rseq_cmpeqv_cmpeqv_storev() as I have no easy way to test
>    there.  Added a comment explaining why the test works this way.
> v4: skipped the test if rseq_offset_deref_addv() is not present
>    (that is, on all architectures other than x86_64).
> 
> Signed-off-by: Peter Oskolkov <posk@...gle.com>
> ---
> .../selftests/rseq/basic_percpu_ops_test.c    | 187 ++++++++++++++++++
> tools/testing/selftests/rseq/rseq-x86.h       |  57 ++++++
> 2 files changed, 244 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..e6e10ba4b9ed 100644
> --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
> @@ -3,16 +3,24 @@
> #include <assert.h>
> #include <pthread.h>
> #include <sched.h>
> +#include <stdatomic.h>

That would be the first time stdatomic.h is included in the kernel selftests.
Do we want this dependency ?

> #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]))
> 
> +/* The local <linux/membarrier.h> may not contain the commands below. */
> +#define MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ	(1<<7)
> +#define MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ	(1<<8)
> +#define MEMBARRIER_CMD_FLAG_CPU		(1<<0)
> +

The usual way to build and run tests AFAIK is to do "make headers_install"
first, and then build the tests against those headers. Therefore, when
building the tests, the additional membarrier commands and flags should always
be there. Please remove those duplicated preprocessor defines.

> struct percpu_lock_entry {
> 	intptr_t v;
> } __attribute__((aligned(128)));
> @@ -289,6 +297,183 @@ 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();
> +	}
> +
> +	/* Wait for initialization. */
> +	while (!atomic_load(&args->percpu_list_ptr)) {}
> +
> +	for (i = 0; i < iters; ++i) {
> +		int ret;
> +
> +		do {
> +			int cpu = rseq_cpu_start();
> +
> +			ret = rseq_offset_deref_addv(&args->percpu_list_ptr,
> +							128 * cpu, 1, cpu);

That 128 happens to be related to:

struct percpu_list_entry {
        struct percpu_list_node *head;
} __attribute__((aligned(128)));

struct percpu_list {
        struct percpu_list_entry c[CPU_SETSIZE];
};

Please don't hardcode numbers like that. Instead:

+			ret = rseq_offset_deref_addv(&args->percpu_list_ptr,
+							sizeof(struct percpu_list_entry) * cpu, 1, cpu);

> +		} while (rseq_unlikely(ret));
> +	}
> +
> +	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, int cpu_id)
> +{
> +	return syscall(__NR_membarrier, cmd, flags, cpu_id);
> +}
> +
> +/*
> + * 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_EXPEDITED_RSEQ,
> +			       MEMBARRIER_CMD_FLAG_CPU, cpu_a);

missing error check.

> +		/*
> +		 * Cpu A should now only modify list_b, so we values

we -> the

> +		 * 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_EXPEDITED_RSEQ,
> +			       MEMBARRIER_CMD_FLAG_CPU, cpu_b);

missing error check.

> +		/* 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)
> +{
> +#ifndef RSEQ_ARCH_HAS_OFFSET_DEREF_ADDV

Please lift the preprocessor conditional outside of the function, e.g.:

#ifdef RSEQ_ARCH_HAS_OFFSET_DEREF_ADDV
void test_membarrier(void)
{
   [... code goes here...]
}
#else /* RSEQ_ARCH_HAS_OFFSET_DEREF_ADDV /*
void test_membarrier(void)
{
}
#endif /* RSEQ_ARCH_HAS_OFFSET_DEREF_ADDV */


> +	fprintf(stderr, "rseq_offset_deref_addv is not implemented on this
> architecture. "
> +			"Skipping membarrier test.\n");
> +	return;
> +#else
> +	struct test_membarrier_thread_args thread_args;
> +	pthread_t worker_threads[CPU_SETSIZE];
> +	pthread_t manager_thread;
> +	int i;
> +
> +	sys_membarrier(MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ, 0, 0);

Missing error handling.

> +
> +	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);

Arguably the existing tests do not check pthread_* errors, but I guess it would not
hurt to do so.

> +#endif
> +}
> +
> int main(int argc, char **argv)
> {
> 	if (rseq_register_current_thread()) {
> @@ -300,6 +485,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));
> diff --git a/tools/testing/selftests/rseq/rseq-x86.h
> b/tools/testing/selftests/rseq/rseq-x86.h
> index b2da6004fe30..640411518e46 100644
> --- a/tools/testing/selftests/rseq/rseq-x86.h
> +++ b/tools/testing/selftests/rseq/rseq-x86.h
> @@ -279,6 +279,63 @@ int rseq_addv(intptr_t *v, intptr_t count, int cpu)
> #endif
> }
> 
> +#define RSEQ_ARCH_HAS_OFFSET_DEREF_ADDV
> +
> +/*
> + *   pval = *(ptr+off)
> + *  *pval += inc;
> + */

Addition to rseq-x86.h should be split into its own commit.

Thanks,

Mathieu

> +static inline __attribute__((always_inline))
> +int rseq_offset_deref_addv(intptr_t *ptr, off_t off, intptr_t inc, int cpu)
> +{
> +	RSEQ_INJECT_C(9)
> +
> +	__asm__ __volatile__ goto (
> +		RSEQ_ASM_DEFINE_TABLE(3, 1f, 2f, 4f) /* start, commit, abort */
> +#ifdef RSEQ_COMPARE_TWICE
> +		RSEQ_ASM_DEFINE_EXIT_POINT(1f, %l[error1])
> +#endif
> +		/* Start rseq by storing table entry pointer into rseq_cs. */
> +		RSEQ_ASM_STORE_RSEQ_CS(1, 3b, RSEQ_CS_OFFSET(%[rseq_abi]))
> +		RSEQ_ASM_CMP_CPU_ID(cpu_id, RSEQ_CPU_ID_OFFSET(%[rseq_abi]), 4f)
> +		RSEQ_INJECT_ASM(3)
> +#ifdef RSEQ_COMPARE_TWICE
> +		RSEQ_ASM_CMP_CPU_ID(cpu_id, RSEQ_CPU_ID_OFFSET(%[rseq_abi]), %l[error1])
> +#endif
> +		/* get p+v */
> +		"movq %[ptr], %%rbx\n\t"
> +		"addq %[off], %%rbx\n\t"
> +		/* get pv */
> +		"movq (%%rbx), %%rcx\n\t"
> +		/* *pv += inc */
> +		"addq %[inc], (%%rcx)\n\t"
> +		"2:\n\t"
> +		RSEQ_INJECT_ASM(4)
> +		RSEQ_ASM_DEFINE_ABORT(4, "", abort)
> +		: /* gcc asm goto does not allow outputs */
> +		: [cpu_id]		"r" (cpu),
> +		  [rseq_abi]		"r" (&__rseq_abi),
> +		  /* final store input */
> +		  [ptr]			"m" (*ptr),
> +		  [off]			"er" (off),
> +		  [inc]			"er" (inc)
> +		: "memory", "cc", "rax", "rbx", "rcx"
> +		  RSEQ_INJECT_CLOBBER
> +		: abort
> +#ifdef RSEQ_COMPARE_TWICE
> +		  , error1
> +#endif
> +	);
> +	return 0;
> +abort:
> +	RSEQ_INJECT_FAILED
> +	return -1;
> +#ifdef RSEQ_COMPARE_TWICE
> +error1:
> +	rseq_bug("cpu_id comparison failed");
> +#endif
> +}
> +
> static inline __attribute__((always_inline))
> int rseq_cmpeqv_trystorev_storev(intptr_t *v, intptr_t expect,
> 				 intptr_t *v2, intptr_t newv2,
> --
> 2.28.0.618.gf4bc123cb7-goog

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists