Re: [PATCH 1/2 v6] rseq/membarrier: add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
From: Mathieu Desnoyers
Date: Tue Sep 15 2020 - 12:26:40 EST
----- On Sep 15, 2020, at 2:12 AM, Peter Oskolkov posk@xxxxxxx wrote:
> Any comments here? Should I change anything?
See below,
>> diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
>> index 5891d7614c8c..98c2b0e7c0d8 100644
>> --- a/include/uapi/linux/membarrier.h
>> +++ b/include/uapi/linux/membarrier.h
>> @@ -114,6 +114,29 @@
>> * If this command is not implemented by an
>> * architecture, -EINVAL is returned.
>> * Returns 0 on success.
>> + * @MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ:
>> + * In addition to provide memory ordering
>> + * guarantees described in
>> + * MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
^ is the above still true ? I see from the code that the rseq membarrier
only issues rseq_preempt, not any (documented) explicit core serialization nor
memory barrier.
>> + * ensure the caller thread, upon return from
>> + * system call, that all its running thread
>> + * siblings have any currently running rseq
>> + * critical sections restarted if @flags
>> + * parameter is 0; if @flags parameter is
>> + * MEMBARRIER_CMD_FLAG_CPU,
>> + * then this operation is performed only
>> + * on CPU indicated by @cpu_id. If this command is
>> + * not implemented by an architecture, -EINVAL
>> + * is returned. A process needs to register its
>> + * intent to use the private expedited rseq
>> + * command prior to using it, otherwise
>> + * this command returns -EPERM.
>> + * @MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
>> + * Register the process intent to use
>> + * MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ.
>> + * If this command is not implemented by an
>> + * architecture, -EINVAL is returned.
>> + * Returns 0 on success.
>> * @MEMBARRIER_CMD_SHARED:
>> * Alias to MEMBARRIER_CMD_GLOBAL. Provided for
>> * header backward compatibility.
>> @@ -131,9 +154,15 @@ enum membarrier_cmd {
>> MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED = (1 << 4),
>> MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 5),
>> MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE = (1 << 6),
>> + MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ = (1 << 7),
>> + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ = (1 << 8),
>>
>> /* Alias for header backward compatibility. */
>> MEMBARRIER_CMD_SHARED = MEMBARRIER_CMD_GLOBAL,
>> };
>>
>> +enum membarrier_cmd_flag {
>> + MEMBARRIER_CMD_FLAG_CPU = (1 << 0),
>> +};
>> +
>> #endif /* _UAPI_LINUX_MEMBARRIER_H */
>> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
>> index 168479a7d61b..e32e9476ccf3 100644
>> --- a/kernel/sched/membarrier.c
>> +++ b/kernel/sched/membarrier.c
>> @@ -18,6 +18,14 @@
>> #define MEMBARRIER_PRIVATE_EXPEDITED_SYNC_CORE_BITMASK 0
>> #endif
>>
>> +#ifdef CONFIG_RSEQ
>> +#define MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ_BITMASK \
>> + (MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ \
>> + | MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ_BITMASK)
>> +#else
>> +#define MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ_BITMASK 0
>> +#endif
>> +
>> #define MEMBARRIER_CMD_BITMASK \
>> (MEMBARRIER_CMD_GLOBAL | MEMBARRIER_CMD_GLOBAL_EXPEDITED \
>> | MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED \
>> @@ -30,6 +38,13 @@ static void ipi_mb(void *info)
>> smp_mb(); /* IPIs should be serializing but paranoid. */
>> }
>>
>> +#ifdef CONFIG_RSEQ
>> +static void ipi_rseq(void *info)
>> +{
>> + rseq_preempt(current);
>> +}
>> +#endif
>> +
>> static void ipi_sync_rq_state(void *info)
>> {
>> struct mm_struct *mm = (struct mm_struct *) info;
>> @@ -129,19 +144,29 @@ static int membarrier_global_expedited(void)
>> return 0;
>> }
>>
>> -static int membarrier_private_expedited(int flags)
>> +static int membarrier_private_expedited(int flags, int cpu_id)
>> {
>> - int cpu;
>> cpumask_var_t tmpmask;
>> struct mm_struct *mm = current->mm;
>> + smp_call_func_t ipi_func = ipi_mb;
>>
>> - if (flags & MEMBARRIER_FLAG_SYNC_CORE) {
>> + if (flags == MEMBARRIER_FLAG_SYNC_CORE) {
>> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE))
>> return -EINVAL;
>> if (!(atomic_read(&mm->membarrier_state) &
>> MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY))
>> return -EPERM;
>> + } else if (flags == MEMBARRIER_FLAG_RSEQ) {
>> +#ifdef CONFIG_RSEQ
>> + if (!(atomic_read(&mm->membarrier_state) &
>> + MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY))
>> + return -EPERM;
>> + ipi_func = ipi_rseq;
>> +#else
>> + return -EINVAL;
>> +#endif
I'm allergic to mixing code logic and preprocessor logic. Anything against
the following changes ?
(above)
static void ipi_rseq(void *info)
{
rseq_preempt(current);
}
^ without the #ifdef CONFIG_RSEQ (rseq_preempt is already defined as a no-op when
CONFIG_RSEQ=n)
and within membarrier_private_expedited:
if (!IS_ENABLED(CONFIG_RSEQ))
return -EINVAL;
if (!(atomic_read(&mm->membarrier_state) &
MEMBARRIER_STATE_PRIVATE_EXPEDITED_RSEQ_READY))
return -EPERM;
ipi_func = ipi_rseq;
[...]
>> @@ -310,8 +368,15 @@ static int membarrier_register_private_expedited(int flags)
>>
>> /**
>> * sys_membarrier - issue memory barriers on a set of threads
>> - * @cmd: Takes command values defined in enum membarrier_cmd.
>> - * @flags: Currently needs to be 0. For future extensions.
>> + * @cmd: Takes command values defined in enum membarrier_cmd.
>> + * @flags: Currently needs to be 0 for all commands other than
>> + * MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ: in the latter
>> + * case it can be MEMBARRIER_CMD_FLAG_CPU, indicating that @cpu_id
>> + * contains the CPU on which to interrupt (= restart)
>> + * the RSEQ critical section.
>> + * @cpu_id: if @flags == MEMBARRIER_CMD_FLAG_CPU, indicates the cpu on which
>> + * RSEQ CS should be interrupted (@cmd must be
>> + * MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ).
>> *
>> * If this system call is not implemented, -ENOSYS is returned. If the
>> * command specified does not exist, not available on the running
>> @@ -337,10 +402,21 @@ static int membarrier_register_private_expedited(int
>> flags)
>> * smp_mb() X O O
>> * sys_membarrier() O O O
>> */
>> -SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
>> +SYSCALL_DEFINE3(membarrier, int, cmd, int, flags, int, cpu_id)
Now that we have the first use of "flags", it would be a good time to change
"int flags" to "unsigned int flags", which is a preferred way to express system
call flags parameters.
Thanks,
Mathieu
>> {
>> - if (unlikely(flags))
>> - return -EINVAL;
>> + switch (cmd) {
>> + case MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ:
>> + if (unlikely(flags && flags != MEMBARRIER_CMD_FLAG_CPU))
>> + return -EINVAL;
>> + break;
>> + default:
>> + if (unlikely(flags))
>> + return -EINVAL;
>> + }
>> +
>> + if (!(flags & MEMBARRIER_CMD_FLAG_CPU))
>> + cpu_id = -1;
>> +
>> switch (cmd) {
>> case MEMBARRIER_CMD_QUERY:
>> {
>> @@ -362,13 +438,17 @@ SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
>> case MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED:
>> return membarrier_register_global_expedited();
>> case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
>> - return membarrier_private_expedited(0);
>> + return membarrier_private_expedited(0, cpu_id);
>> case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED:
>> return membarrier_register_private_expedited(0);
>> case MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE:
>> - return membarrier_private_expedited(MEMBARRIER_FLAG_SYNC_CORE);
>> + return membarrier_private_expedited(MEMBARRIER_FLAG_SYNC_CORE,
>> cpu_id);
>> case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE:
>> return membarrier_register_private_expedited(MEMBARRIER_FLAG_SYNC_CORE);
>> + case MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ:
>> + return membarrier_private_expedited(MEMBARRIER_FLAG_RSEQ,
>> cpu_id);
>> + case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
>> + return
>> membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
>> default:
>> return -EINVAL;
>> }
>> --
>> 2.28.0.402.g5ffc5be6b7-goog
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com