Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 7 Aug 2020 08:27:05 +0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Peter Oskolkov <posk@google.com>
Cc:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org, Paul Turner <pjt@google.com>,
        Chris Kennelly <ckennelly@google.com>,
        Peter Oskolkov <posk@posk.io>
Subject: Re: [PATCH 2/2 v2] rseq/selftests: test
 MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU
Message-ID: <20200807002705.GA889@tardis>
References: <20200806170544.382140-1-posk@google.com>
 <20200806170544.382140-2-posk@google.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="HlL+5n6rz5pIUxbD"
Content-Disposition: inline
In-Reply-To: <20200806170544.382140-2-posk@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--HlL+5n6rz5pIUxbD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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.
>=20
> Signed-off-by: Peter Oskolkov <posk@google.com>
> ---
>  .../selftests/rseq/basic_percpu_ops_test.c    | 181 ++++++++++++++++++
>  1 file changed, 181 insertions(+)
>=20
> 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>
> =20
>  #include "rseq.h"
> =20
>  #define ARRAY_SIZE(arr)	(sizeof(arr) / sizeof((arr)[0]))
> =20
> +#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 =3D=3D expected_sum);
>  }
> =20
> +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 =3D
> +		(struct test_membarrier_thread_args *)arg;
> +	const int iters =3D 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 =3D 0; i < iters; ++i) {
> +		while (true) {
> +			int cpu, ret;
> +			struct percpu_list *list_ptr =3D (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 =3D atomic_load(&args->percpu_list_ptr); // read list_b
=09
	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 =3D 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 =3D rseq_cpu_start();
> +			struct percpu_list_node *node =3D list_ptr->c[cpu].head;
> +			const intptr_t prev =3D node->data;
> +
> +			ret =3D 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 =3D 0; i < CPU_SETSIZE; i++) {
> +		struct percpu_list_node *node;
> +
> +		node =3D malloc(sizeof(*node));
> +		assert(node);
> +		node->data =3D 0;
> +		node->next =3D NULL;
> +		list->c[i].head =3D node;
> +	}
> +}
> +
> +void test_membarrier_free_percpu_list(struct percpu_list *list)
> +{
> +	int i;
> +
> +	for (i =3D 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 =3D
> +		(struct test_membarrier_thread_args *)arg;
> +	struct percpu_list list_a, list_b;
> +	intptr_t expect_a =3D 0, expect_b =3D 0;
> +	int cpu_a =3D 0, cpu_b =3D 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 =3D rand() % CPU_SETSIZE;
> +		/*
> +		 * As list_b is "inactive", we should never see changes
> +		 * to list_b.
> +		 */
> +		if (expect_b !=3D 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 =3D atomic_load(&list_a.c[cpu_a].head->data);
> +
> +		cpu_b =3D rand() % CPU_SETSIZE;
> +		/*
> +		 * As list_a is "inactive", we should never see changes
> +		 * to list_a.
> +		 */
> +		if (expect_a !=3D 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 =3D 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 =3D 0;
> +	thread_args.percpu_list_ptr =3D 0;
> +	pthread_create(&manager_thread, NULL,
> +		       test_membarrier_manager_thread, &thread_args);
> +
> +	for (i =3D 0; i < CPU_SETSIZE; i++)
> +		pthread_create(&worker_threads[i], NULL,
> +		       test_membarrier_worker_thread, &thread_args);
> +
> +	for (i =3D 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));
> --=20
> 2.28.0.163.g6104cc2f0b6-goog
>=20

--HlL+5n6rz5pIUxbD
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAl8sn9YACgkQSXnow7UH
+rjiHQf/fJpwAY8SZvR+JcVF/c8SVaeCyYI9dfblAYjO3L/olqZ9jlr7SfDUthHs
gqrNNWpLxSbStIdogWCsnm+cMcipISOyKAQABjYuCD6EL4EE0fVElt4STpu7w7Mm
emsvZamuEiOyqrQY6WjU51q/O771UAfo2nqPLWGVB6dPZMALTtUxWYqTGrj6IMjL
uvp7BNdVvvnqkdSvfdQDER6t//c3mvjT35si1o66FZQyOTsZujgB/j+GE/qh4vef
BrOYWYVFbHi9JA2kTE+o7Hi5yVYrUFnIkCdrKVZnIvIMR7TFsshzz9iQMGKWQMa0
JbHzRewB2nPNIeBnQcc+MGLvDoIjuw==
=hwwt
-----END PGP SIGNATURE-----

--HlL+5n6rz5pIUxbD--