Re: [PATCH 1/2 v6] rseq/membarrier: add MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Sep 15, 2020, at 2:12 AM, Peter Oskolkov posk@posk.io 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