* [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 16:43 [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS Michal Clapinski
@ 2022-12-07 16:43 ` Michal Clapinski
2022-12-07 17:07 ` Mathieu Desnoyers
2023-01-07 10:52 ` [tip: sched/core] " tip-bot2 for Michal Clapinski
2022-12-07 16:43 ` [PATCH 2/2] selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS Michal Clapinski
2022-12-22 15:28 ` [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS Mathieu Desnoyers
2 siblings, 2 replies; 11+ messages in thread
From: Michal Clapinski @ 2022-12-07 16:43 UTC (permalink / raw)
To: Mathieu Desnoyers, Paul E. McKenney
Cc: Ingo Molnar, Peter Zijlstra, Andrei Vagin, Shuah Khan,
linux-kernel, linux-kselftest, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 16:43 ` [PATCH 1/2] sched/membarrier: " Michal Clapinski
@ 2022-12-07 17:07 ` Mathieu Desnoyers
2022-12-07 18:04 ` Michał Cłapiński
2023-01-07 10:52 ` [tip: sched/core] " tip-bot2 for Michal Clapinski
1 sibling, 1 reply; 11+ messages in thread
From: Mathieu Desnoyers @ 2022-12-07 17:07 UTC (permalink / raw)
To: Michal Clapinski, Paul E. McKenney
Cc: Ingo Molnar, Peter Zijlstra, Andrei Vagin, Shuah Khan,
linux-kernel, linux-kselftest
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 17:07 ` Mathieu Desnoyers
@ 2022-12-07 18:04 ` Michał Cłapiński
2022-12-20 17:51 ` Michał Cłapiński
0 siblings, 1 reply; 11+ messages in thread
From: Michał Cłapiński @ 2022-12-07 18:04 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E. McKenney, Ingo Molnar, Peter Zijlstra, Andrei Vagin,
Shuah Khan, linux-kernel, linux-kselftest
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 18:04 ` Michał Cłapiński
@ 2022-12-20 17:51 ` Michał Cłapiński
2022-12-22 15:23 ` Mathieu Desnoyers
0 siblings, 1 reply; 11+ messages in thread
From: Michał Cłapiński @ 2022-12-20 17:51 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E. McKenney, Ingo Molnar, Peter Zijlstra, Andrei Vagin,
Shuah Khan, linux-kernel, linux-kselftest
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?
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-20 17:51 ` Michał Cłapiński
@ 2022-12-22 15:23 ` Mathieu Desnoyers
0 siblings, 0 replies; 11+ messages in thread
From: Mathieu Desnoyers @ 2022-12-22 15:23 UTC (permalink / raw)
To: Michał Cłapiński
Cc: Paul E. McKenney, Ingo Molnar, Peter Zijlstra, Andrei Vagin,
Shuah Khan, linux-kernel, linux-kselftest
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip: sched/core] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 16:43 ` [PATCH 1/2] sched/membarrier: " Michal Clapinski
2022-12-07 17:07 ` Mathieu Desnoyers
@ 2023-01-07 10:52 ` tip-bot2 for Michal Clapinski
1 sibling, 0 replies; 11+ messages in thread
From: tip-bot2 for Michal Clapinski @ 2023-01-07 10:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Michal Clapinski, Ingo Molnar, Mathieu Desnoyers,
Paul E. McKenney, x86, linux-kernel
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;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 16:43 [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS Michal Clapinski
2022-12-07 16:43 ` [PATCH 1/2] sched/membarrier: " Michal Clapinski
@ 2022-12-07 16:43 ` Michal Clapinski
2023-01-07 10:52 ` [tip: sched/core] " tip-bot2 for Michal Clapinski
2022-12-22 15:28 ` [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS Mathieu Desnoyers
2 siblings, 1 reply; 11+ messages in thread
From: Michal Clapinski @ 2022-12-07 16:43 UTC (permalink / raw)
To: Mathieu Desnoyers, Paul E. McKenney
Cc: Ingo Molnar, Peter Zijlstra, Andrei Vagin, Shuah Khan,
linux-kernel, linux-kselftest, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [tip: sched/core] selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 16:43 ` [PATCH 2/2] selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS Michal Clapinski
@ 2023-01-07 10:52 ` tip-bot2 for Michal Clapinski
0 siblings, 0 replies; 11+ messages in thread
From: tip-bot2 for Michal Clapinski @ 2023-01-07 10:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Michal Clapinski, Ingo Molnar, Mathieu Desnoyers,
Paul E. McKenney, x86, linux-kernel
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();
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-07 16:43 [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS Michal Clapinski
2022-12-07 16:43 ` [PATCH 1/2] sched/membarrier: " Michal Clapinski
2022-12-07 16:43 ` [PATCH 2/2] selftests/membarrier: Test MEMBARRIER_CMD_GET_REGISTRATIONS Michal Clapinski
@ 2022-12-22 15:28 ` Mathieu Desnoyers
2022-12-23 1:05 ` Paul E. McKenney
2 siblings, 1 reply; 11+ messages in thread
From: Mathieu Desnoyers @ 2022-12-22 15:28 UTC (permalink / raw)
To: Michal Clapinski, Paul E. McKenney, Peter Zijlstra
Cc: Ingo Molnar, Andrei Vagin, Shuah Khan, linux-kernel, linux-kselftest
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS
2022-12-22 15:28 ` [PATCH 0/2] sched/membarrier, selftests: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS Mathieu Desnoyers
@ 2022-12-23 1:05 ` Paul E. McKenney
0 siblings, 0 replies; 11+ messages in thread
From: Paul E. McKenney @ 2022-12-23 1:05 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Michal Clapinski, Peter Zijlstra, Ingo Molnar, Andrei Vagin,
Shuah Khan, linux-kernel, linux-kselftest
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread