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

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

[Mathieu Desnoyers]

----- On Aug 12, 2020, at 2:48 PM, Peter Oskolkov posk@posk.io wrote:

> On Wed, Aug 12, 2020 at 11:30 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> 
> [...]
> 
>> "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 ?
> 
> Can we do that? I thought adding an additional parameter means adding
> another syscall (ABI => parameter types/count cannot change?)

I was under the impression that adding parameters to a system call
for new flags (or commands) was not an issue. One example is the
clone system call which expects the ctid argument if the
CLONE_CHILD_CLEARTID flag is set. But maybe it was OK at some earlier
point in time, but it's not OK anymore ? (CCing linux-arch to ask for
advice)

> 
>> 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.
>>
> 
> [...]
> 
>> 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.
> 
> All commands are currently distinct bits, but are treated as separate commands.

Indeed! I forgot about that. It was done so we can return a mask of supported
commands with the MEMBARRIER_CMD_QUERY for feature discoverability. Those were
never meant to be OR'd though, because then it's hard for user-space to discover
what are the allowed command combinations.

> One way of doing what you suggest is to allow some commands to be bitwise-ORed.
> 
> So, for example, the user could call
> 
> membarrier(CMD_PRIVATE_EXPEDITED_SYNC_CORE | CMD_PRIVATE_EXPEDITED_RSEQ, cpu_id)
> 
> Is this what you have in mind?

Not really. This would not take care of the fact that we would end up multiplying
the number of commands as we allow combinations. E.g. if we ever want to have RSEQ
work in private and global, and in non-expedited and expedited, we end up needing:

- CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
- CMD_PRIVATE_EXPEDITED_RSEQ
- CMD_PRIVATE_RSEQ
- CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ
- CMD_GLOBAL_EXPEDITED_RSEQ
- CMD_GLOBAL_RSEQ

The only thing we would save by OR'ing it with the SYNC_CORE command is the additional
list:

- CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ_SYNC_CORE
- CMD_PRIVATE_EXPEDITED_RSEQ_SYNC_CORE
- CMD_PRIVATE_RSEQ_SYNC_CORE
- CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ_SYNC_CORE
- CMD_GLOBAL_EXPEDITED_RSEQ_SYNC_CORE
- CMD_GLOBAL_RSEQ_SYNC_CORE

But unless we receive feedback that doing a membarrier with RSEQ+sync_core all in
one go is a significant use-case, I am tempted to leave out that scenario for now.
If we go for new commands, this means we could add (for private-expedited-rseq):

- MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ,
- MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ,

I do however have use-cases for using RSEQ across shared memory (between
processes). Not currently for a rseq-fence, but for rseq acting as per-cpu
atomic operations. If I ever end up needing rseq-fence across shared memory,
that would result in the following new commands:

- MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ,
- MEMBARRIER_CMD_GLOBAL_EXPEDITED_RSEQ,

The remaining open question is whether it would be OK to define a new
membarrier flag=MEMBARRIER_FLAG_CPU, which would expect an additional
@cpu parameter.

Thanks,

Mathieu

> 
> [...]
> 
> Thanks,
> Peter

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

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

[Peter Oskolkov]

On Wed, Aug 12, 2020 at 12:44 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
[...]
>
> > One way of doing what you suggest is to allow some commands to be bitwise-ORed.
> >
> > So, for example, the user could call
> >
> > membarrier(CMD_PRIVATE_EXPEDITED_SYNC_CORE | CMD_PRIVATE_EXPEDITED_RSEQ, cpu_id)
> >
> > Is this what you have in mind?
>
> Not really. This would not take care of the fact that we would end up multiplying
> the number of commands as we allow combinations. E.g. if we ever want to have RSEQ
> work in private and global, and in non-expedited and expedited, we end up needing:
>
> - CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
> - CMD_PRIVATE_EXPEDITED_RSEQ
> - CMD_PRIVATE_RSEQ
> - CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ
> - CMD_GLOBAL_EXPEDITED_RSEQ
> - CMD_GLOBAL_RSEQ
>
> The only thing we would save by OR'ing it with the SYNC_CORE command is the additional
> list:
>
> - CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ_SYNC_CORE
> - CMD_PRIVATE_EXPEDITED_RSEQ_SYNC_CORE
> - CMD_PRIVATE_RSEQ_SYNC_CORE
> - CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ_SYNC_CORE
> - CMD_GLOBAL_EXPEDITED_RSEQ_SYNC_CORE
> - CMD_GLOBAL_RSEQ_SYNC_CORE
>
> But unless we receive feedback that doing a membarrier with RSEQ+sync_core all in
> one go is a significant use-case, I am tempted to leave out that scenario for now.
> If we go for new commands, this means we could add (for private-expedited-rseq):
>
> - MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ,
> - MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ,
>
> I do however have use-cases for using RSEQ across shared memory (between
> processes). Not currently for a rseq-fence, but for rseq acting as per-cpu
> atomic operations. If I ever end up needing rseq-fence across shared memory,
> that would result in the following new commands:
>
> - MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ,
> - MEMBARRIER_CMD_GLOBAL_EXPEDITED_RSEQ,
>
> The remaining open question is whether it would be OK to define a new
> membarrier flag=MEMBARRIER_FLAG_CPU, which would expect an additional
> @cpu parameter.

Hi  Mathieu,

I do not think there is any reason to wait for additional feedback, so I believe
we should finalize the API/ABI.

I see two issues to resolve:
1: how to combine commands:
  - you do not like adding new commands that are combinations of existing ones;
  - you do not like ORing.
=> I'm not sure what other options we have here?

2: should @flags be repurposed for cpu_id, or MEMBARRIER_FLAG_CPU
   added with a new syscall parameter.
=> I'm still not sure a new parameter can be cleanly added, but I can try
   it in the next patchset if you prefer it this way.

Please let me know your thoughts.

Thanks,
Peter

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

[Mathieu Desnoyers]

----- On Aug 20, 2020, at 1:42 PM, Peter Oskolkov posk@posk.io wrote:

> On Wed, Aug 12, 2020 at 12:44 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
> [...]
>>
>> > One way of doing what you suggest is to allow some commands to be bitwise-ORed.
>> >
>> > So, for example, the user could call
>> >
>> > membarrier(CMD_PRIVATE_EXPEDITED_SYNC_CORE | CMD_PRIVATE_EXPEDITED_RSEQ, cpu_id)
>> >
>> > Is this what you have in mind?
>>
>> Not really. This would not take care of the fact that we would end up
>> multiplying
>> the number of commands as we allow combinations. E.g. if we ever want to have
>> RSEQ
>> work in private and global, and in non-expedited and expedited, we end up
>> needing:
>>
>> - CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
>> - CMD_PRIVATE_EXPEDITED_RSEQ
>> - CMD_PRIVATE_RSEQ
>> - CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ
>> - CMD_GLOBAL_EXPEDITED_RSEQ
>> - CMD_GLOBAL_RSEQ
>>
>> The only thing we would save by OR'ing it with the SYNC_CORE command is the
>> additional
>> list:
>>
>> - CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ_SYNC_CORE
>> - CMD_PRIVATE_EXPEDITED_RSEQ_SYNC_CORE
>> - CMD_PRIVATE_RSEQ_SYNC_CORE
>> - CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ_SYNC_CORE
>> - CMD_GLOBAL_EXPEDITED_RSEQ_SYNC_CORE
>> - CMD_GLOBAL_RSEQ_SYNC_CORE
>>
>> But unless we receive feedback that doing a membarrier with RSEQ+sync_core all
>> in
>> one go is a significant use-case, I am tempted to leave out that scenario for
>> now.
>> If we go for new commands, this means we could add (for private-expedited-rseq):
>>
>> - MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ,
>> - MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ,
>>
>> I do however have use-cases for using RSEQ across shared memory (between
>> processes). Not currently for a rseq-fence, but for rseq acting as per-cpu
>> atomic operations. If I ever end up needing rseq-fence across shared memory,
>> that would result in the following new commands:
>>
>> - MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED_RSEQ,
>> - MEMBARRIER_CMD_GLOBAL_EXPEDITED_RSEQ,
>>
>> The remaining open question is whether it would be OK to define a new
>> membarrier flag=MEMBARRIER_FLAG_CPU, which would expect an additional
>> @cpu parameter.
> 
> Hi  Mathieu,
> 
> I do not think there is any reason to wait for additional feedback, so I believe
> we should finalize the API/ABI.
> 
> I see two issues to resolve:
> 1: how to combine commands:
>  - you do not like adding new commands that are combinations of existing ones;
>  - you do not like ORing.
> => I'm not sure what other options we have here?

Concretely speaking, let's just add a new membarrier command for the use-case
at hand. All other ways of doing things we have discussed are tricky to expose
in a way that is discoverable by user-space through the QUERY command. (using
a flag, or OR'ing many commands together)

> 
> 2: should @flags be repurposed for cpu_id, or MEMBARRIER_FLAG_CPU
>   added with a new syscall parameter.
> => I'm still not sure a new parameter can be cleanly added, but I can try
>   it in the next patchset if you prefer it this way.

Yes please, it's easy to implement and we'll quickly see if anyone yells. If
it turns out to be a bad idea, you can always blame me. ;-)

In summary:

- We add 2 new membarrier commands:
  - MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
  - MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ

- We reserve a membarrier flag:

enum membarrier_flag {
  MEMBARRIER_FLAG_CPU = (1 << 0),
}

So for CMD_PRIVATE_EXPEDITED_RSEQ, if flags & MEMBARRIER_FLAG_CPU is true,
then we expect the additional "int cpu" parameter (3rd parameter). Else the cpu
parameter is unused.

Are you OK with this approach ?

Thanks,

Mathieu

> 
> Please let me know your thoughts.
> 
> Thanks,
> Peter

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

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

[Peter Oskolkov]

On Tue, Aug 25, 2020 at 9:58 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
[...]
>
> Concretely speaking, let's just add a new membarrier command for the use-case
> at hand. All other ways of doing things we have discussed are tricky to expose
> in a way that is discoverable by user-space through the QUERY command. (using
> a flag, or OR'ing many commands together)
>
> >
> > 2: should @flags be repurposed for cpu_id, or MEMBARRIER_FLAG_CPU
> >   added with a new syscall parameter.
> > => I'm still not sure a new parameter can be cleanly added, but I can try
> >   it in the next patchset if you prefer it this way.
>
> Yes please, it's easy to implement and we'll quickly see if anyone yells. If
> it turns out to be a bad idea, you can always blame me. ;-)
>
> In summary:
>
> - We add 2 new membarrier commands:
>   - MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ
>   - MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
>
> - We reserve a membarrier flag:
>
> enum membarrier_flag {
>   MEMBARRIER_FLAG_CPU = (1 << 0),
> }
>
> So for CMD_PRIVATE_EXPEDITED_RSEQ, if flags & MEMBARRIER_FLAG_CPU is true,
> then we expect the additional "int cpu" parameter (3rd parameter). Else the cpu
> parameter is unused.
>
> Are you OK with this approach ?

Yes, thanks for looking into this. I'll send a v4 later this week.

Thanks,
Peter

[...]