[PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Michal Clapinski]

This change provides a method to query previously issued registrations.
It's needed for CRIU (checkpoint/restore in userspace). Before this
change we had to issue private membarrier commands during checkpoint -
if they succeeded, they must have been registered. Unfortunately global
membarrier succeeds even on unregistered processes, so there was no way to
tell if MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED had been issued or not.

CRIU is run after the process has been frozen with ptrace, so we don't
have to worry too much about the result of running this command in parallel
with registration commands.

Michal Clapinski (2):
  sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
  selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS

 include/uapi/linux/membarrier.h               |  4 ++
 kernel/sched/membarrier.c                     | 39 ++++++++++++++++++-
 .../membarrier/membarrier_test_impl.h         | 33 ++++++++++++++++
 .../membarrier/membarrier_test_multi_thread.c |  2 +-
 .../membarrier_test_single_thread.c           |  6 ++-
 5 files changed, 81 insertions(+), 3 deletions(-)

-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Michal Clapinski]

Provide a method to query previously issued registrations.

Signed-off-by: Michal Clapinski <mclapinski@google.com>
---
 include/uapi/linux/membarrier.h |  4 ++++
 kernel/sched/membarrier.c       | 39 ++++++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
index 737605897f36..5f3ad6d5be6f 100644
--- a/include/uapi/linux/membarrier.h
+++ b/include/uapi/linux/membarrier.h
@@ -137,6 +137,9 @@
  * @MEMBARRIER_CMD_SHARED:
  *                          Alias to MEMBARRIER_CMD_GLOBAL. Provided for
  *                          header backward compatibility.
+ * @MEMBARRIER_CMD_GET_REGISTRATIONS:
+ *                          Returns a bitmask of previously issued
+ *                          registration commands.
  *
  * Command to be passed to the membarrier system call. The commands need to
  * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
@@ -153,6 +156,7 @@ enum membarrier_cmd {
 	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE	= (1 << 6),
 	MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ			= (1 << 7),
 	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ		= (1 << 8),
+	MEMBARRIER_CMD_GET_REGISTRATIONS			= (1 << 9),
 
 	/* Alias for header backward compatibility. */
 	MEMBARRIER_CMD_SHARED			= MEMBARRIER_CMD_GLOBAL,
diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
index 0c5be7ebb1dc..2ad881d07752 100644
--- a/kernel/sched/membarrier.c
+++ b/kernel/sched/membarrier.c
@@ -159,7 +159,8 @@
 	| MEMBARRIER_CMD_PRIVATE_EXPEDITED				\
 	| MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED			\
 	| MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK		\
-	| MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
+	| MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK			\
+	| MEMBARRIER_CMD_GET_REGISTRATIONS)
 
 static void ipi_mb(void *info)
 {
@@ -540,6 +541,40 @@ static int membarrier_register_private_expedited(int flags)
 	return 0;
 }
 
+static int membarrier_get_registrations(void)
+{
+	struct task_struct *p = current;
+	struct mm_struct *mm = p->mm;
+	int registrations_mask = 0, membarrier_state, i;
+	static const int states[] = {
+		MEMBARRIER_STATE_GLOBAL_EXPEDITED |
+			MEMBARRIER_STATE_GLOBAL_EXPEDITED_READY,
+		MEMBARRIER_STATE_PRIVATE_EXPEDITED |
+			MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY,
+		MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE |
+			MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY,
+		MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ |
+			MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY
+	};
+	static const int registration_cmds[] = {
+		MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
+		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
+		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
+		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
+	};
+	BUILD_BUG_ON(ARRAY_SIZE(states) != ARRAY_SIZE(registration_cmds));
+
+	membarrier_state = atomic_read(&mm->membarrier_state);
+	for (i = 0; i < ARRAY_SIZE(states); ++i) {
+		if (membarrier_state & states[i]) {
+			registrations_mask |= registration_cmds[i];
+			membarrier_state &= ~states[i];
+		}
+	}
+	WARN_ON_ONCE(membarrier_state != 0);
+	return registrations_mask;
+}
+
 /**
  * sys_membarrier - issue memory barriers on a set of threads
  * @cmd:    Takes command values defined in enum membarrier_cmd.
@@ -623,6 +658,8 @@ SYSCALL_DEFINE3(membarrier, int, cmd, unsigned int, flags, int, cpu_id)
 		return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ, cpu_id);
 	case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
 		return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
+	case MEMBARRIER_CMD_GET_REGISTRATIONS:
+		return membarrier_get_registrations();
 	default:
 		return -EINVAL;
 	}
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH 2/2] selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS

[Michal Clapinski]

Keep track of previously issued registrations and compare the result
with MEMBARRIER_CMD_GET_REGISTRATIONS return value.

Signed-off-by: Michal Clapinski <mclapinski@google.com>
---
 .../membarrier/membarrier_test_impl.h         | 33 +++++++++++++++++++
 .../membarrier/membarrier_test_multi_thread.c |  2 +-
 .../membarrier_test_single_thread.c           |  6 +++-
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/membarrier/membarrier_test_impl.h b/tools/testing/selftests/membarrier/membarrier_test_impl.h
index 186be69f0a59..af89855adb7b 100644
--- a/tools/testing/selftests/membarrier/membarrier_test_impl.h
+++ b/tools/testing/selftests/membarrier/membarrier_test_impl.h
@@ -9,11 +9,38 @@
 
 #include "../kselftest.h"
 
+static int registrations;
+
 static int sys_membarrier(int cmd, int flags)
 {
 	return syscall(__NR_membarrier, cmd, flags);
 }
 
+static int test_membarrier_get_registrations(int cmd)
+{
+	int ret, flags = 0;
+	const char *test_name =
+		"sys membarrier MEMBARRIER_CMD_GET_REGISTRATIONS";
+
+	registrations |= cmd;
+
+	ret = sys_membarrier(MEMBARRIER_CMD_GET_REGISTRATIONS, 0);
+	if (ret < 0) {
+		ksft_exit_fail_msg(
+			"%s test: flags = %d, errno = %d\n",
+			test_name, flags, errno);
+	} else if (ret != registrations) {
+		ksft_exit_fail_msg(
+			"%s test: flags = %d, ret = %d, registrations = %d\n",
+			test_name, flags, ret, registrations);
+	}
+	ksft_test_result_pass(
+		"%s test: flags = %d, ret = %d, registrations = %d\n",
+		test_name, flags, ret, registrations);
+
+	return 0;
+}
+
 static int test_membarrier_cmd_fail(void)
 {
 	int cmd = -1, flags = 0;
@@ -113,6 +140,8 @@ static int test_membarrier_register_private_expedited_success(void)
 	ksft_test_result_pass(
 		"%s test: flags = %d\n",
 		test_name, flags);
+
+	test_membarrier_get_registrations(cmd);
 	return 0;
 }
 
@@ -170,6 +199,8 @@ static int test_membarrier_register_private_expedited_sync_core_success(void)
 	ksft_test_result_pass(
 		"%s test: flags = %d\n",
 		test_name, flags);
+
+	test_membarrier_get_registrations(cmd);
 	return 0;
 }
 
@@ -204,6 +235,8 @@ static int test_membarrier_register_global_expedited_success(void)
 	ksft_test_result_pass(
 		"%s test: flags = %d\n",
 		test_name, flags);
+
+	test_membarrier_get_registrations(cmd);
 	return 0;
 }
 
diff --git a/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c b/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c
index ac5613e5b0eb..a9cc17facfb3 100644
--- a/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c
+++ b/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c
@@ -62,7 +62,7 @@ static int test_mt_membarrier(void)
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(13);
+	ksft_set_plan(16);
 
 	test_membarrier_query();
 
diff --git a/tools/testing/selftests/membarrier/membarrier_test_single_thread.c b/tools/testing/selftests/membarrier/membarrier_test_single_thread.c
index c1c963902854..4cdc8b1d124c 100644
--- a/tools/testing/selftests/membarrier/membarrier_test_single_thread.c
+++ b/tools/testing/selftests/membarrier/membarrier_test_single_thread.c
@@ -12,7 +12,9 @@
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(13);
+	ksft_set_plan(18);
+
+	test_membarrier_get_registrations(/*cmd=*/0);
 
 	test_membarrier_query();
 
@@ -20,5 +22,7 @@ int main(int argc, char **argv)
 
 	test_membarrier_success();
 
+	test_membarrier_get_registrations(/*cmd=*/0);
+
 	return ksft_exit_pass();
 }
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Mathieu Desnoyers]

On 2022-12-07 11:43, Michal Clapinski wrote:
> Provide a method to query previously issued registrations.
> 
> Signed-off-by: Michal Clapinski <mclapinski@google.com>
> ---
>   include/uapi/linux/membarrier.h |  4 ++++
>   kernel/sched/membarrier.c       | 39 ++++++++++++++++++++++++++++++++-
>   2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
> index 737605897f36..5f3ad6d5be6f 100644
> --- a/include/uapi/linux/membarrier.h
> +++ b/include/uapi/linux/membarrier.h
> @@ -137,6 +137,9 @@
>    * @MEMBARRIER_CMD_SHARED:
>    *                          Alias to MEMBARRIER_CMD_GLOBAL. Provided for
>    *                          header backward compatibility.
> + * @MEMBARRIER_CMD_GET_REGISTRATIONS:
> + *                          Returns a bitmask of previously issued
> + *                          registration commands.
>    *
>    * Command to be passed to the membarrier system call. The commands need to
>    * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
> @@ -153,6 +156,7 @@ enum membarrier_cmd {
>   	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE	= (1 << 6),
>   	MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ			= (1 << 7),
>   	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ		= (1 << 8),
> +	MEMBARRIER_CMD_GET_REGISTRATIONS			= (1 << 9),
>   
>   	/* Alias for header backward compatibility. */
>   	MEMBARRIER_CMD_SHARED			= MEMBARRIER_CMD_GLOBAL,
> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> index 0c5be7ebb1dc..2ad881d07752 100644
> --- a/kernel/sched/membarrier.c
> +++ b/kernel/sched/membarrier.c
> @@ -159,7 +159,8 @@
>   	| MEMBARRIER_CMD_PRIVATE_EXPEDITED				\
>   	| MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED			\
>   	| MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK		\
> -	| MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
> +	| MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK			\
> +	| MEMBARRIER_CMD_GET_REGISTRATIONS)
>   
>   static void ipi_mb(void *info)
>   {
> @@ -540,6 +541,40 @@ static int membarrier_register_private_expedited(int flags)
>   	return 0;
>   }
>   
> +static int membarrier_get_registrations(void)
> +{
> +	struct task_struct *p = current;
> +	struct mm_struct *mm = p->mm;
> +	int registrations_mask = 0, membarrier_state, i;
> +	static const int states[] = {
> +		MEMBARRIER_STATE_GLOBAL_EXPEDITED |
> +			MEMBARRIER_STATE_GLOBAL_EXPEDITED_READY,

What is the purpose of checking for the _READY state flag as well here ?

> +		MEMBARRIER_STATE_PRIVATE_EXPEDITED |
> +			MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY,
> +		MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE |
> +			MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY,
> +		MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ |
> +			MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY
> +	};
> +	static const int registration_cmds[] = {
> +		MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
> +		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
> +		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
> +		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
> +	};
> +	BUILD_BUG_ON(ARRAY_SIZE(states) != ARRAY_SIZE(registration_cmds));
> +
> +	membarrier_state = atomic_read(&mm->membarrier_state);
> +	for (i = 0; i < ARRAY_SIZE(states); ++i) {
> +		if (membarrier_state & states[i]) {

The mask will match if either of the flags to match are set. Is that 
your intent ?

> +			registrations_mask |= registration_cmds[i];
> +			membarrier_state &= ~states[i];

So I understand that those _READY flags are there purely for making sure 
we clear the membarrier_state for validation that they have all been 
checked with the following WARN_ON_ONCE(). Am I on the right track ?

> +		}
> +	}
> +	WARN_ON_ONCE(membarrier_state != 0);

Thanks,

Mathieu

> +	return registrations_mask;
> +}
> +
>   /**
>    * sys_membarrier - issue memory barriers on a set of threads
>    * @cmd:    Takes command values defined in enum membarrier_cmd.
> @@ -623,6 +658,8 @@ SYSCALL_DEFINE3(membarrier, int, cmd, unsigned int, flags, int, cpu_id)
>   		return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ, cpu_id);
>   	case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
>   		return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
> +	case MEMBARRIER_CMD_GET_REGISTRATIONS:
> +		return membarrier_get_registrations();
>   	default:
>   		return -EINVAL;
>   	}

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

Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Michał Cłapiński]

On Wed, Dec 7, 2022 at 6:07 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2022-12-07 11:43, Michal Clapinski wrote:
> > Provide a method to query previously issued registrations.
> >
> > Signed-off-by: Michal Clapinski <mclapinski@google.com>
> > ---
> >   include/uapi/linux/membarrier.h |  4 ++++
> >   kernel/sched/membarrier.c       | 39 ++++++++++++++++++++++++++++++++-
> >   2 files changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
> > index 737605897f36..5f3ad6d5be6f 100644
> > --- a/include/uapi/linux/membarrier.h
> > +++ b/include/uapi/linux/membarrier.h
> > @@ -137,6 +137,9 @@
> >    * @MEMBARRIER_CMD_SHARED:
> >    *                          Alias to MEMBARRIER_CMD_GLOBAL. Provided for
> >    *                          header backward compatibility.
> > + * @MEMBARRIER_CMD_GET_REGISTRATIONS:
> > + *                          Returns a bitmask of previously issued
> > + *                          registration commands.
> >    *
> >    * Command to be passed to the membarrier system call. The commands need to
> >    * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
> > @@ -153,6 +156,7 @@ enum membarrier_cmd {
> >       MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE     = (1 << 6),
> >       MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ                   = (1 << 7),
> >       MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ          = (1 << 8),
> > +     MEMBARRIER_CMD_GET_REGISTRATIONS                        = (1 << 9),

Btw. I could do this as a flag to MEMBARRIER_CMD_QUERY instead of a
separate command. Would that be preferable?


> >
> >       /* Alias for header backward compatibility. */
> >       MEMBARRIER_CMD_SHARED                   = MEMBARRIER_CMD_GLOBAL,
> > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> > index 0c5be7ebb1dc..2ad881d07752 100644
> > --- a/kernel/sched/membarrier.c
> > +++ b/kernel/sched/membarrier.c
> > @@ -159,7 +159,8 @@
> >       | MEMBARRIER_CMD_PRIVATE_EXPEDITED                              \
> >       | MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED                     \
> >       | MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK                \
> > -     | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
> > +     | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK                     \
> > +     | MEMBARRIER_CMD_GET_REGISTRATIONS)
> >
> >   static void ipi_mb(void *info)
> >   {
> > @@ -540,6 +541,40 @@ static int membarrier_register_private_expedited(int flags)
> >       return 0;
> >   }
> >
> > +static int membarrier_get_registrations(void)
> > +{
> > +     struct task_struct *p = current;
> > +     struct mm_struct *mm = p->mm;
> > +     int registrations_mask = 0, membarrier_state, i;
> > +     static const int states[] = {
> > +             MEMBARRIER_STATE_GLOBAL_EXPEDITED |
> > +                     MEMBARRIER_STATE_GLOBAL_EXPEDITED_READY,
>
> What is the purpose of checking for the _READY state flag as well here ?

Answered below.


>
>
> > +             MEMBARRIER_STATE_PRIVATE_EXPEDITED |
> > +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY,
> > +             MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE |
> > +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY,
> > +             MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ |
> > +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY
> > +     };
> > +     static const int registration_cmds[] = {
> > +             MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
> > +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
> > +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
> > +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
> > +     };
> > +     BUILD_BUG_ON(ARRAY_SIZE(states) != ARRAY_SIZE(registration_cmds));
> > +
> > +     membarrier_state = atomic_read(&mm->membarrier_state);
> > +     for (i = 0; i < ARRAY_SIZE(states); ++i) {
> > +             if (membarrier_state & states[i]) {
>
> The mask will match if either of the flags to match are set. Is that
> your intent ?

Kind of, it was just the easiest to write. As explained in the cover
letter, I don't really care much about the result of this while the
process is running. And when the process is frozen, either state and
state_ready are set or none of them.


>
>
> > +                     registrations_mask |= registration_cmds[i];
> > +                     membarrier_state &= ~states[i];
>
> So I understand that those _READY flags are there purely for making sure
> we clear the membarrier_state for validation that they have all been
> checked with the following WARN_ON_ONCE(). Am I on the right track ?

Yes, exactly. It wastes time but I'm worried about people adding new
states and not updating this function. A suggestion on how to do this
better (especially at compile time) would be greatly appreciated.


>
> > +             }
> > +     }
> > +     WARN_ON_ONCE(membarrier_state != 0);
>
> Thanks,
>
> Mathieu
>
> > +     return registrations_mask;
> > +}
> > +
> >   /**
> >    * sys_membarrier - issue memory barriers on a set of threads
> >    * @cmd:    Takes command values defined in enum membarrier_cmd.
> > @@ -623,6 +658,8 @@ SYSCALL_DEFINE3(membarrier, int, cmd, unsigned int, flags, int, cpu_id)
> >               return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ, cpu_id);
> >       case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
> >               return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
> > +     case MEMBARRIER_CMD_GET_REGISTRATIONS:
> > +             return membarrier_get_registrations();
> >       default:
> >               return -EINVAL;
> >       }
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com
>

Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Michał Cłapiński]

On Wed, Dec 7, 2022 at 7:04 PM Michał Cłapiński <mclapinski@google.com> wrote:
>
> On Wed, Dec 7, 2022 at 6:07 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> >
> > On 2022-12-07 11:43, Michal Clapinski wrote:
> > > Provide a method to query previously issued registrations.
> > >
> > > Signed-off-by: Michal Clapinski <mclapinski@google.com>
> > > ---
> > >   include/uapi/linux/membarrier.h |  4 ++++
> > >   kernel/sched/membarrier.c       | 39 ++++++++++++++++++++++++++++++++-
> > >   2 files changed, 42 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
> > > index 737605897f36..5f3ad6d5be6f 100644
> > > --- a/include/uapi/linux/membarrier.h
> > > +++ b/include/uapi/linux/membarrier.h
> > > @@ -137,6 +137,9 @@
> > >    * @MEMBARRIER_CMD_SHARED:
> > >    *                          Alias to MEMBARRIER_CMD_GLOBAL. Provided for
> > >    *                          header backward compatibility.
> > > + * @MEMBARRIER_CMD_GET_REGISTRATIONS:
> > > + *                          Returns a bitmask of previously issued
> > > + *                          registration commands.
> > >    *
> > >    * Command to be passed to the membarrier system call. The commands need to
> > >    * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
> > > @@ -153,6 +156,7 @@ enum membarrier_cmd {
> > >       MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE     = (1 << 6),
> > >       MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ                   = (1 << 7),
> > >       MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ          = (1 << 8),
> > > +     MEMBARRIER_CMD_GET_REGISTRATIONS                        = (1 << 9),
>
> Btw. I could do this as a flag to MEMBARRIER_CMD_QUERY instead of a
> separate command. Would that be preferable?
>
>
> > >
> > >       /* Alias for header backward compatibility. */
> > >       MEMBARRIER_CMD_SHARED                   = MEMBARRIER_CMD_GLOBAL,
> > > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> > > index 0c5be7ebb1dc..2ad881d07752 100644
> > > --- a/kernel/sched/membarrier.c
> > > +++ b/kernel/sched/membarrier.c
> > > @@ -159,7 +159,8 @@
> > >       | MEMBARRIER_CMD_PRIVATE_EXPEDITED                              \
> > >       | MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED                     \
> > >       | MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK                \
> > > -     | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
> > > +     | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK                     \
> > > +     | MEMBARRIER_CMD_GET_REGISTRATIONS)
> > >
> > >   static void ipi_mb(void *info)
> > >   {
> > > @@ -540,6 +541,40 @@ static int membarrier_register_private_expedited(int flags)
> > >       return 0;
> > >   }
> > >
> > > +static int membarrier_get_registrations(void)
> > > +{
> > > +     struct task_struct *p = current;
> > > +     struct mm_struct *mm = p->mm;
> > > +     int registrations_mask = 0, membarrier_state, i;
> > > +     static const int states[] = {
> > > +             MEMBARRIER_STATE_GLOBAL_EXPEDITED |
> > > +                     MEMBARRIER_STATE_GLOBAL_EXPEDITED_READY,
> >
> > What is the purpose of checking for the _READY state flag as well here ?
>
> Answered below.
>
>
> >
> >
> > > +             MEMBARRIER_STATE_PRIVATE_EXPEDITED |
> > > +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY,
> > > +             MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE |
> > > +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY,
> > > +             MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ |
> > > +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY
> > > +     };
> > > +     static const int registration_cmds[] = {
> > > +             MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
> > > +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
> > > +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
> > > +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
> > > +     };
> > > +     BUILD_BUG_ON(ARRAY_SIZE(states) != ARRAY_SIZE(registration_cmds));
> > > +
> > > +     membarrier_state = atomic_read(&mm->membarrier_state);
> > > +     for (i = 0; i < ARRAY_SIZE(states); ++i) {
> > > +             if (membarrier_state & states[i]) {
> >
> > The mask will match if either of the flags to match are set. Is that
> > your intent ?
>
> Kind of, it was just the easiest to write. As explained in the cover
> letter, I don't really care much about the result of this while the
> process is running. And when the process is frozen, either state and
> state_ready are set or none of them.
>
>
> >
> >
> > > +                     registrations_mask |= registration_cmds[i];
> > > +                     membarrier_state &= ~states[i];
> >
> > So I understand that those _READY flags are there purely for making sure
> > we clear the membarrier_state for validation that they have all been
> > checked with the following WARN_ON_ONCE(). Am I on the right track ?
>
> Yes, exactly. It wastes time but I'm worried about people adding new
> states and not updating this function. A suggestion on how to do this
> better (especially at compile time) would be greatly appreciated.
>
>
> >
> > > +             }
> > > +     }
> > > +     WARN_ON_ONCE(membarrier_state != 0);
> >
> > Thanks,
> >
> > Mathieu
> >
> > > +     return registrations_mask;
> > > +}
> > > +
> > >   /**
> > >    * sys_membarrier - issue memory barriers on a set of threads
> > >    * @cmd:    Takes command values defined in enum membarrier_cmd.
> > > @@ -623,6 +658,8 @@ SYSCALL_DEFINE3(membarrier, int, cmd, unsigned int, flags, int, cpu_id)
> > >               return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ, cpu_id);
> > >       case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
> > >               return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
> > > +     case MEMBARRIER_CMD_GET_REGISTRATIONS:
> > > +             return membarrier_get_registrations();
> > >       default:
> > >               return -EINVAL;
> > >       }
> >
> > --
> > Mathieu Desnoyers
> > EfficiOS Inc.
> > https://www.efficios.com
> >

Hi Mathieu,
is there anything more you need from my side?

Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Mathieu Desnoyers]

On 2022-12-20 12:51, Michał Cłapiński wrote:
> On Wed, Dec 7, 2022 at 7:04 PM Michał Cłapiński <mclapinski@google.com> wrote:
>>
>> On Wed, Dec 7, 2022 at 6:07 PM Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> On 2022-12-07 11:43, Michal Clapinski wrote:
>>>> Provide a method to query previously issued registrations.
>>>>
>>>> Signed-off-by: Michal Clapinski <mclapinski@google.com>
>>>> ---
>>>>    include/uapi/linux/membarrier.h |  4 ++++
>>>>    kernel/sched/membarrier.c       | 39 ++++++++++++++++++++++++++++++++-
>>>>    2 files changed, 42 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
>>>> index 737605897f36..5f3ad6d5be6f 100644
>>>> --- a/include/uapi/linux/membarrier.h
>>>> +++ b/include/uapi/linux/membarrier.h
>>>> @@ -137,6 +137,9 @@
>>>>     * @MEMBARRIER_CMD_SHARED:
>>>>     *                          Alias to MEMBARRIER_CMD_GLOBAL. Provided for
>>>>     *                          header backward compatibility.
>>>> + * @MEMBARRIER_CMD_GET_REGISTRATIONS:
>>>> + *                          Returns a bitmask of previously issued
>>>> + *                          registration commands.
>>>>     *
>>>>     * Command to be passed to the membarrier system call. The commands need to
>>>>     * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
>>>> @@ -153,6 +156,7 @@ enum membarrier_cmd {
>>>>        MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE     = (1 << 6),
>>>>        MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ                   = (1 << 7),
>>>>        MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ          = (1 << 8),
>>>> +     MEMBARRIER_CMD_GET_REGISTRATIONS                        = (1 << 9),
>>
>> Btw. I could do this as a flag to MEMBARRIER_CMD_QUERY instead of a
>> separate command. Would that be preferable?

I do not think that would be better, no. We can keep it with 
GET_REGISTRATIONS.

>>
>>
>>>>
>>>>        /* Alias for header backward compatibility. */
>>>>        MEMBARRIER_CMD_SHARED                   = MEMBARRIER_CMD_GLOBAL,
>>>> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
>>>> index 0c5be7ebb1dc..2ad881d07752 100644
>>>> --- a/kernel/sched/membarrier.c
>>>> +++ b/kernel/sched/membarrier.c
>>>> @@ -159,7 +159,8 @@
>>>>        | MEMBARRIER_CMD_PRIVATE_EXPEDITED                              \
>>>>        | MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED                     \
>>>>        | MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK                \
>>>> -     | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
>>>> +     | MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK                     \
>>>> +     | MEMBARRIER_CMD_GET_REGISTRATIONS)
>>>>
>>>>    static void ipi_mb(void *info)
>>>>    {
>>>> @@ -540,6 +541,40 @@ static int membarrier_register_private_expedited(int flags)
>>>>        return 0;
>>>>    }
>>>>
>>>> +static int membarrier_get_registrations(void)
>>>> +{
>>>> +     struct task_struct *p = current;
>>>> +     struct mm_struct *mm = p->mm;
>>>> +     int registrations_mask = 0, membarrier_state, i;
>>>> +     static const int states[] = {
>>>> +             MEMBARRIER_STATE_GLOBAL_EXPEDITED |
>>>> +                     MEMBARRIER_STATE_GLOBAL_EXPEDITED_READY,
>>>
>>> What is the purpose of checking for the _READY state flag as well here ?
>>
>> Answered below.
>>
>>
>>>
>>>
>>>> +             MEMBARRIER_STATE_PRIVATE_EXPEDITED |
>>>> +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY,
>>>> +             MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE |
>>>> +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY,
>>>> +             MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ |
>>>> +                     MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY
>>>> +     };
>>>> +     static const int registration_cmds[] = {
>>>> +             MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
>>>> +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
>>>> +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
>>>> +             MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
>>>> +     };
>>>> +     BUILD_BUG_ON(ARRAY_SIZE(states) != ARRAY_SIZE(registration_cmds));
>>>> +
>>>> +     membarrier_state = atomic_read(&mm->membarrier_state);
>>>> +     for (i = 0; i < ARRAY_SIZE(states); ++i) {
>>>> +             if (membarrier_state & states[i]) {
>>>
>>> The mask will match if either of the flags to match are set. Is that
>>> your intent ?
>>
>> Kind of, it was just the easiest to write. As explained in the cover
>> letter, I don't really care much about the result of this while the
>> process is running. And when the process is frozen, either state and
>> state_ready are set or none of them.

OK

>>
>>
>>>
>>>
>>>> +                     registrations_mask |= registration_cmds[i];
>>>> +                     membarrier_state &= ~states[i];
>>>
>>> So I understand that those _READY flags are there purely for making sure
>>> we clear the membarrier_state for validation that they have all been
>>> checked with the following WARN_ON_ONCE(). Am I on the right track ?
>>
>> Yes, exactly. It wastes time but I'm worried about people adding new
>> states and not updating this function. A suggestion on how to do this
>> better (especially at compile time) would be greatly appreciated.

Although it's not a fast-path, so let's keep it this way for now.

>>
>>
>>>
>>>> +             }
>>>> +     }
>>>> +     WARN_ON_ONCE(membarrier_state != 0);
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>>> +     return registrations_mask;
>>>> +}
>>>> +
>>>>    /**
>>>>     * sys_membarrier - issue memory barriers on a set of threads
>>>>     * @cmd:    Takes command values defined in enum membarrier_cmd.
>>>> @@ -623,6 +658,8 @@ SYSCALL_DEFINE3(membarrier, int, cmd, unsigned int, flags, int, cpu_id)
>>>>                return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ, cpu_id);
>>>>        case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
>>>>                return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
>>>> +     case MEMBARRIER_CMD_GET_REGISTRATIONS:
>>>> +             return membarrier_get_registrations();
>>>>        default:
>>>>                return -EINVAL;
>>>>        }
>>>
>>> --
>>> Mathieu Desnoyers
>>> EfficiOS Inc.
>>> https://www.efficios.com
>>>
> 
> Hi Mathieu,
> is there anything more you need from my side?

No, I think those patches are ok.

Thanks,

Mathieu





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

Re: [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Mathieu Desnoyers]

On 2022-12-07 11:43, Michal Clapinski wrote:
> This change provides a method to query previously issued registrations.
> It's needed for CRIU (checkpoint/restore in userspace). Before this
> change we had to issue private membarrier commands during checkpoint -
> if they succeeded, they must have been registered. Unfortunately global
> membarrier succeeds even on unregistered processes, so there was no way to
> tell if MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED had been issued or not.
> 
> CRIU is run after the process has been frozen with ptrace, so we don't
> have to worry too much about the result of running this command in parallel
> with registration commands.

Peter, Paul, I'm OK with the proposed changes. Should we route this 
through sched/core from the tip tree ?

For both patches:

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

Thanks,

Mathieu

> 
> Michal Clapinski (2):
>    sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
>    selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS
> 
>   include/uapi/linux/membarrier.h               |  4 ++
>   kernel/sched/membarrier.c                     | 39 ++++++++++++++++++-
>   .../membarrier/membarrier_test_impl.h         | 33 ++++++++++++++++
>   .../membarrier/membarrier_test_multi_thread.c |  2 +-
>   .../membarrier_test_single_thread.c           |  6 ++-
>   5 files changed, 81 insertions(+), 3 deletions(-)
> 

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

Re: [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[Paul E. McKenney]

On Thu, Dec 22, 2022 at 10:28:28AM -0500, Mathieu Desnoyers wrote:
> On 2022-12-07 11:43, Michal Clapinski wrote:
> > This change provides a method to query previously issued registrations.
> > It's needed for CRIU (checkpoint/restore in userspace). Before this
> > change we had to issue private membarrier commands during checkpoint -
> > if they succeeded, they must have been registered. Unfortunately global
> > membarrier succeeds even on unregistered processes, so there was no way to
> > tell if MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED had been issued or not.
> > 
> > CRIU is run after the process has been frozen with ptrace, so we don't
> > have to worry too much about the result of running this command in parallel
> > with registration commands.
> 
> Peter, Paul, I'm OK with the proposed changes. Should we route this through
> sched/core from the tip tree ?
> 
> For both patches:
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Also for both patches:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> Thanks,
> 
> Mathieu
> 
> > 
> > Michal Clapinski (2):
> >    sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
> >    selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS
> > 
> >   include/uapi/linux/membarrier.h               |  4 ++
> >   kernel/sched/membarrier.c                     | 39 ++++++++++++++++++-
> >   .../membarrier/membarrier_test_impl.h         | 33 ++++++++++++++++
> >   .../membarrier/membarrier_test_multi_thread.c |  2 +-
> >   .../membarrier_test_single_thread.c           |  6 ++-
> >   5 files changed, 81 insertions(+), 3 deletions(-)
> > 
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com
> 

[tip: sched/core] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

[tip-bot2 for Michal Clapinski]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     544a4f2ecd45f9d6ed78d207583f39130ad40349
Gitweb:        https://git.kernel.org/tip/544a4f2ecd45f9d6ed78d207583f39130ad40349
Author:        Michal Clapinski <mclapinski@google.com>
AuthorDate:    Wed, 07 Dec 2022 17:43:37 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sat, 07 Jan 2023 11:29:29 +01:00

sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS

Provide a method to query previously issued registrations.

Signed-off-by: Michal Clapinski <mclapinski@google.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
Link: https://lore.kernel.org/r/20221207164338.1535591-2-mclapinski@google.com
---
 include/uapi/linux/membarrier.h |  4 +++-
 kernel/sched/membarrier.c       | 39 +++++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
index 7376058..5f3ad6d 100644
--- a/include/uapi/linux/membarrier.h
+++ b/include/uapi/linux/membarrier.h
@@ -137,6 +137,9 @@
  * @MEMBARRIER_CMD_SHARED:
  *                          Alias to MEMBARRIER_CMD_GLOBAL. Provided for
  *                          header backward compatibility.
+ * @MEMBARRIER_CMD_GET_REGISTRATIONS:
+ *                          Returns a bitmask of previously issued
+ *                          registration commands.
  *
  * Command to be passed to the membarrier system call. The commands need to
  * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
@@ -153,6 +156,7 @@ enum membarrier_cmd {
 	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE	= (1 << 6),
 	MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ			= (1 << 7),
 	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ		= (1 << 8),
+	MEMBARRIER_CMD_GET_REGISTRATIONS			= (1 << 9),
 
 	/* Alias for header backward compatibility. */
 	MEMBARRIER_CMD_SHARED			= MEMBARRIER_CMD_GLOBAL,
diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
index 0c5be7e..2ad881d 100644
--- a/kernel/sched/membarrier.c
+++ b/kernel/sched/membarrier.c
@@ -159,7 +159,8 @@
 	| MEMBARRIER_CMD_PRIVATE_EXPEDITED				\
 	| MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED			\
 	| MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK		\
-	| MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
+	| MEMBARRIER_PRIVATE_EXPEDITED_RSEQ_BITMASK			\
+	| MEMBARRIER_CMD_GET_REGISTRATIONS)
 
 static void ipi_mb(void *info)
 {
@@ -540,6 +541,40 @@ static int membarrier_register_private_expedited(int flags)
 	return 0;
 }
 
+static int membarrier_get_registrations(void)
+{
+	struct task_struct *p = current;
+	struct mm_struct *mm = p->mm;
+	int registrations_mask = 0, membarrier_state, i;
+	static const int states[] = {
+		MEMBARRIER_STATE_GLOBAL_EXPEDITED |
+			MEMBARRIER_STATE_GLOBAL_EXPEDITED_READY,
+		MEMBARRIER_STATE_PRIVATE_EXPEDITED |
+			MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY,
+		MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE |
+			MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY,
+		MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ |
+			MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY
+	};
+	static const int registration_cmds[] = {
+		MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
+		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
+		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
+		MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
+	};
+	BUILD_BUG_ON(ARRAY_SIZE(states) != ARRAY_SIZE(registration_cmds));
+
+	membarrier_state = atomic_read(&mm->membarrier_state);
+	for (i = 0; i < ARRAY_SIZE(states); ++i) {
+		if (membarrier_state & states[i]) {
+			registrations_mask |= registration_cmds[i];
+			membarrier_state &= ~states[i];
+		}
+	}
+	WARN_ON_ONCE(membarrier_state != 0);
+	return registrations_mask;
+}
+
 /**
  * sys_membarrier - issue memory barriers on a set of threads
  * @cmd:    Takes command values defined in enum membarrier_cmd.
@@ -623,6 +658,8 @@ SYSCALL_DEFINE3(membarrier, int, cmd, unsigned int, flags, int, cpu_id)
 		return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ, cpu_id);
 	case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
 		return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
+	case MEMBARRIER_CMD_GET_REGISTRATIONS:
+		return membarrier_get_registrations();
 	default:
 		return -EINVAL;
 	}

[tip: sched/core] selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS

[tip-bot2 for Michal Clapinski]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     d74f87f37672e71457bfcc14eca5eeb1d61b6438
Gitweb:        https://git.kernel.org/tip/d74f87f37672e71457bfcc14eca5eeb1d61b6438
Author:        Michal Clapinski <mclapinski@google.com>
AuthorDate:    Wed, 07 Dec 2022 17:43:38 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sat, 07 Jan 2023 11:29:29 +01:00

selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS

Keep track of previously issued registrations and compare the result
with MEMBARRIER_CMD_GET_REGISTRATIONS return value.

Signed-off-by: Michal Clapinski <mclapinski@google.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
Link: https://lore.kernel.org/r/20221207164338.1535591-3-mclapinski@google.com
---
 tools/testing/selftests/membarrier/membarrier_test_impl.h          | 33 +++++++++++++++++++++++++++++++++
 tools/testing/selftests/membarrier/membarrier_test_multi_thread.c  |  2 +-
 tools/testing/selftests/membarrier/membarrier_test_single_thread.c |  6 +++++-
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/membarrier/membarrier_test_impl.h b/tools/testing/selftests/membarrier/membarrier_test_impl.h
index 186be69..af89855 100644
--- a/tools/testing/selftests/membarrier/membarrier_test_impl.h
+++ b/tools/testing/selftests/membarrier/membarrier_test_impl.h
@@ -9,11 +9,38 @@
 
 #include "../kselftest.h"
 
+static int registrations;
+
 static int sys_membarrier(int cmd, int flags)
 {
 	return syscall(__NR_membarrier, cmd, flags);
 }
 
+static int test_membarrier_get_registrations(int cmd)
+{
+	int ret, flags = 0;
+	const char *test_name =
+		"sys membarrier MEMBARRIER_CMD_GET_REGISTRATIONS";
+
+	registrations |= cmd;
+
+	ret = sys_membarrier(MEMBARRIER_CMD_GET_REGISTRATIONS, 0);
+	if (ret < 0) {
+		ksft_exit_fail_msg(
+			"%s test: flags = %d, errno = %d\n",
+			test_name, flags, errno);
+	} else if (ret != registrations) {
+		ksft_exit_fail_msg(
+			"%s test: flags = %d, ret = %d, registrations = %d\n",
+			test_name, flags, ret, registrations);
+	}
+	ksft_test_result_pass(
+		"%s test: flags = %d, ret = %d, registrations = %d\n",
+		test_name, flags, ret, registrations);
+
+	return 0;
+}
+
 static int test_membarrier_cmd_fail(void)
 {
 	int cmd = -1, flags = 0;
@@ -113,6 +140,8 @@ static int test_membarrier_register_private_expedited_success(void)
 	ksft_test_result_pass(
 		"%s test: flags = %d\n",
 		test_name, flags);
+
+	test_membarrier_get_registrations(cmd);
 	return 0;
 }
 
@@ -170,6 +199,8 @@ static int test_membarrier_register_private_expedited_sync_core_success(void)
 	ksft_test_result_pass(
 		"%s test: flags = %d\n",
 		test_name, flags);
+
+	test_membarrier_get_registrations(cmd);
 	return 0;
 }
 
@@ -204,6 +235,8 @@ static int test_membarrier_register_global_expedited_success(void)
 	ksft_test_result_pass(
 		"%s test: flags = %d\n",
 		test_name, flags);
+
+	test_membarrier_get_registrations(cmd);
 	return 0;
 }
 
diff --git a/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c b/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c
index ac5613e..a9cc17f 100644
--- a/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c
+++ b/tools/testing/selftests/membarrier/membarrier_test_multi_thread.c
@@ -62,7 +62,7 @@ static int test_mt_membarrier(void)
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(13);
+	ksft_set_plan(16);
 
 	test_membarrier_query();
 
diff --git a/tools/testing/selftests/membarrier/membarrier_test_single_thread.c b/tools/testing/selftests/membarrier/membarrier_test_single_thread.c
index c1c9639..4cdc8b1 100644
--- a/tools/testing/selftests/membarrier/membarrier_test_single_thread.c
+++ b/tools/testing/selftests/membarrier/membarrier_test_single_thread.c
@@ -12,7 +12,9 @@
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(13);
+	ksft_set_plan(18);
+
+	test_membarrier_get_registrations(/*cmd=*/0);
 
 	test_membarrier_query();
 
@@ -20,5 +22,7 @@ int main(int argc, char **argv)
 
 	test_membarrier_success();
 
+	test_membarrier_get_registrations(/*cmd=*/0);
+
 	return ksft_exit_pass();
 }