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

[Mathieu Desnoyers]

----- On Aug 11, 2020, at 5:08 PM, Peter Oskolkov posk@xxxxxxx wrote:

> On Mon, Aug 10, 2020 at 11:27 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>>
>> On Mon, Aug 10, 2020 at 05:09:58PM -0700, Peter Oskolkov wrote:
>> > @@ -27,6 +35,12 @@
>> >
>> >  static void ipi_mb(void *info)
>> >  {
>>
>> The #ifdef wants to behere, otherwise you'll get a compile warning for
>> !RSEQ builds.
> 
> Ack. Will do in the next version - for now waiting for the rest to be
> worked out.
> 
> [...]
> 
>>
>> Mathieu did mention a few other points that I didn't see addressed:
>>
>>  - he didn't like abusing the @flags syscall argument for a CPUid;
> 
> @flags is not used now; maybe just rename it to something more
> generic? @param? Or @options? Or maybe more specific, like @cpu_id?

"flags" is there to allow extensibility without requiring to add new
membarrier commands for every change. Even though it is not used now,
I don't think re-purposing it is a good idea. What is wrong with just
adding an additional "cpu" parameter to the system call ?

A "flags" parameter is very common for system calls. I don't see why
we should change its name, especially given it is already exposed and
documented as "flags" in man pages.

> 
>>  - he wondered if we should support SYNC_CORE + RSEQ.
> 
> It seems to me that CMD_PRIVATE_EXPEDITED_RSEQ is basically
> CMD_PRIVATE_EXPEDITED_SYNC_CORE with the extra "restart RSEQ CSs"
> behavior. Am I missing something?

No. The "sync-core" is about doing context synchronization for JIts, and
is not implemented on all architectures today. RSEQ however is available
on a wider range of architectures.

> If not, what is the point of
> complicating the code as suggested below? Maybe just renaming
> CMD_PRIVATE_EXPEDITED_RSEQ to CMD_PRIVATE_EXPEDITED_SYNC_CORE_RSEQ
> will do?

We basically have the following feature matrix:

- private / global
- expedited / non-expedited
- sync-core / non-sync-core
- rseq-fence / non-rseq-fence

For a total of about 16 combinations in total if we want to support them
all.

We can continue to add separate commands for new combinations, but if we
want to allow them to be combined, using flags rather than adding extra
commands would have the advantage of keeping the number of commands
manageable.

However, if there is no actual use-case for combining a membarrier sync-core
and a membarrier rseq-fence, then it limits the number of commands and maybe
then it's acceptable to add the rseq-fence as a separate membarrier command.

I prefer to have this discussion now rather than once we get to the point of
having 40 membarrier commands for all possible combinations.

Thanks,

Mathieu

> 
>>
>>
>> Not sure we can easily change the syscall at this point, but the latter
>> point could be addressed with something like this.
>>
>> ---
>> Index: linux-2.6/kernel/sched/membarrier.c
>> ===================================================================
>> --- linux-2.6.orig/kernel/sched/membarrier.c
>> +++ linux-2.6/kernel/sched/membarrier.c
>> @@ -374,8 +374,26 @@ static int membarrier_register_private_e
>>   */
>>  SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
>>  {
>> +       int cflags = 0, int cpuid = -1;
>> +
>>         if (unlikely(flags) && cmd != MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ)
>>                 return -EINVAL;
>> +
>> +       if (cmd & (MEMBARRIER_CMD_PRIVATE_EXPEDITED |
>> +                  MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE |
>> +                  MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ)) {
>> +
>> +               if (cmd & MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ)
>> +                       cflags |= MEMBARRIER_FLAG_RSEQ;
>> +
>> +               if (cmd & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
>> +                       cflags |= MEMBARRIER_FLAG_SYNC_CORE;
>> +                       cpuid = flags;
>> +               }
>> +
>> +               cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED;
>> +       }
>> +
>>         switch (cmd) {
>>         case MEMBARRIER_CMD_QUERY:
>>         {
>> @@ -396,18 +414,16 @@ SYSCALL_DEFINE2(membarrier, int, cmd, in
>>                 return membarrier_global_expedited();
>>         case MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED:
>>                 return membarrier_register_global_expedited();
>> -       case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
>> -               return membarrier_private_expedited(0, -1);
>>         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,
>> -1);
>>         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,
>> flags);
>>         case MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ:
>>                 return membarrier_register_private_expedited(MEMBARRIER_FLAG_RSEQ);
>> +
>> +       case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
>> +               return membarrier_private_expedited(cflags, cpuid);
>> +
>>         default:
>>                 return -EINVAL;
>>         }

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com