| Message ID | 20221207164338.1535591-2-mclapinski@google.com |
|---|---|
| State | Accepted |
| Commit | 544a4f2ecd45f9d6ed78d207583f39130ad40349 |
| Headers | show |
| Series | [1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS | expand |
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?
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
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; }
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(-)