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(-)
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; }
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 forheader 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();
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 forheader 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
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 forheader 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?
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
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(); }
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(-)
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
linux-kselftest-mirror@lists.linaro.org
-
Mathieu Desnoyers -
Michal Clapinski -
Michał Cłapiński
-
Paul E. McKenney