Messages in this thread | | | Date | Thu, 22 Dec 2022 10:23:17 -0500 | Subject | Re: [PATCH 1/2] sched/membarrier: Introduce MEMBARRIER_CMD_GET_REGISTRATIONS | From | 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
| |