Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448160 > unrolled thread

[RFC PATCH v7 0/7] Restartable sequences system call

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v7 0/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-21 23:20 +0200
    [RFC PATCH v7 2/7] tracing: instrument restartable sequences Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-21 23:20 +0200
    [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-21 23:20 +0200
      Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-07-26 01:10 +0200
        Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-26 05:10 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-03 14:30 +0200
            Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-03 18:50 +0200
              Re: [RFC PATCH v7 1/7] Restartable sequences system call Christoph Lameter <cl@linux.com> - 2016-08-03 20:40 +0200
                Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-04 07:10 +0200
              Re: [RFC PATCH v7 1/7] Restartable sequences system call Boqun Feng <boqun.feng@gmail.com> - 2016-08-04 06:30 +0200
                Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-04 07:20 +0200
                  Re: [RFC PATCH v7 1/7] Restartable sequences system call Boqun Feng <boqun.feng@gmail.com> - 2016-08-09 18:20 +0200
                    Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 20:20 +0200
                    Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-10 21:10 +0200
                    Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 22:50 +0200
                  Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-10 21:40 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Christoph Lameter <cl@linux.com> - 2016-08-03 20:40 +0200
            Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 22:10 +0200
              Re: [RFC PATCH v7 1/7] Restartable sequences system call Christoph Lameter <cl@linux.com> - 2016-08-10 22:40 +0200
      Re: [RFC PATCH v7 1/7] Restartable sequences system call Boqun Feng <boqun.feng@gmail.com> - 2016-07-27 17:10 +0200
        [RFC 4/4] Restartable sequences: Add self-tests for PPC Boqun Feng <boqun.feng@gmail.com> - 2016-07-27 17:10 +0200
          Re: [RFC 4/4] Restartable sequences: Add self-tests for PPC Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-28 05:10 +0200
            Re: [RFC 4/4] Restartable sequences: Add self-tests for PPC Boqun Feng <boqun.feng@gmail.com> - 2016-07-28 06:50 +0200
              [RFC v2] Restartable sequences: Add self-tests for PPC Boqun Feng <boqun.feng@gmail.com> - 2016-07-28 09:40 +0200
                Re: [RFC v2] Restartable sequences: Add self-tests for PPC Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-28 16:10 +0200
              Re: [RFC 4/4] Restartable sequences: Add self-tests for PPC Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-28 15:50 +0200
        [RFC 1/4] rseq/param_test: Convert test_data_entry::count to intptr_t Boqun Feng <boqun.feng@gmail.com> - 2016-07-27 17:10 +0200
          [RFC 3/4] Restartable sequences: Wire up powerpc system call Boqun Feng <boqun.feng@gmail.com> - 2016-07-27 17:10 +0200
            Re: [RFC 3/4] Restartable sequences: Wire up powerpc system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-28 05:20 +0200
          Re: [RFC 1/4] rseq/param_test: Convert test_data_entry::count to  intptr_t Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-28 05:10 +0200
        Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-28 05:20 +0200
      Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-03 16:00 +0200
        Re: [RFC PATCH v7 1/7] Restartable sequences system call "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-03 17:00 +0200
        Re: [RFC PATCH v7 1/7] Restartable sequences system call Boqun Feng <boqun.feng@gmail.com> - 2016-08-03 17:50 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-07 17:40 +0200
            Re: [RFC PATCH v7 1/7] Restartable sequences system call Boqun Feng <boqun.feng@gmail.com> - 2016-08-08 01:40 +0200
              Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-09 15:30 +0200
        Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-09 22:10 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-09 23:40 +0200
            Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 00:50 +0200
              Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 21:00 +0200
                Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 21:00 +0200
                Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-10 21:20 +0200
              Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-10 22:10 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 20:20 +0200
            Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 20:30 +0200
            Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-10 21:20 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 21:10 +0200
            Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-10 21:20 +0200
              Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 22:10 +0200
                Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-10 22:20 +0200
                  Re: [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-10 23:10 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Andy Lutomirski <luto@amacapital.net> - 2016-08-10 21:20 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-10 22:10 +0200
          Re: [RFC PATCH v7 1/7] Restartable sequences system call Peter Zijlstra <peterz@infradead.org> - 2016-08-10 22:10 +0200
    [RFC PATCH v7 6/7] Restartable sequences: wire up x86 32/64 system call Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-21 23:20 +0200
    Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-24 05:10 +0200
      Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Dave Watson <davejwatson@fb.com> - 2016-07-24 20:10 +0200
        Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-25 18:50 +0200
    Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-07-25 20:20 +0200

Page 3 of 3 — ← Prev page 1 2 [3]

#1459548 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 3:50 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Tue, Aug 09, 2016 at 10:41:47PM +0000, Mathieu Desnoyers wrote:
>> #ifdef __LP64__
>> 
>> static bool rseq_update_cpu_id_event_counter(struct task_struct *t)
>> {
>>         union rseq_cpu_event u;
>> 
>>         u.e.cpu_id = raw_smp_processor_id();
>>         u.e.event_counter = ++t->rseq_event_counter;
>>         if (__put_user(u.v, &t->rseq->u.v))
>>                 return false;
>>         trace_rseq_inc(t->rseq_event_counter);
>>         return true;
>> }
>> 
>> #else /* #ifdef __LP64__ */
>> 
>> static bool rseq_update_cpu_id_event_counter(struct task_struct *t)
>> {
>>         if (__put_user(raw_smp_processor_id(), &t->rseq->u.e.cpu_id))
>>                 return false;
>>         if (__put_user(++t->rseq_event_counter, &t->rseq->u.e.event_counter))
>>                 return false;
>>         trace_rseq_inc(t->rseq_event_counter);
>>         return true;
>> }
>> 
>> #endif /* #else #ifdef __LP64__ */
> 
> I don't think you need to guard it (and CONFIG_64BIT is the 'right'
> kernel symbol for that), 32bit should have u64 __put_user() only
> implemented as 2 u32 stores.

OK, I can then simplify the implementation to:

[...]
 * On 64-bit architectures, both cpu_id and event_counter can be updated
 * with a single 64-bit store. On 32-bit architectures, __put_user() is
 * expected to perform two 32-bit single-copy stores to guarantee
 * single-copy atomicity semantics for other threads.
 */
static bool rseq_update_cpu_id_event_counter(struct task_struct *t)
{
        union rseq_cpu_event u;

        u.e.cpu_id = raw_smp_processor_id();
        u.e.event_counter = ++t->rseq_event_counter;
        if (__put_user(u.v, &t->rseq->u.v))
                return false;
        trace_rseq_inc(t->rseq_event_counter);
        return true;
}

Thanks!

Mathieu

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

[toc] | [prev] | [next] | [standalone]

#1459563 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 9:33 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Wed, Aug 10, 2016 at 01:26:04PM +0000, Mathieu Desnoyers wrote:
> 
>> static bool rseq_update_cpu_id_event_counter(struct task_struct *t)
>> {
>>         union rseq_cpu_event u;
>> 
>>         u.e.cpu_id = raw_smp_processor_id();
>>         u.e.event_counter = ++t->rseq_event_counter;
>>         if (__put_user(u.v, &t->rseq->u.v))
>>                 return false;
>>         trace_rseq_inc(t->rseq_event_counter);
> 
> I had not previously noticed the trace_* muck, but I would suggest
> passing in t and leaving it up to the tracepoint implementation to pick
> out the value.

OK, fixed.

> 
> Also, since this not only increments (it also updates the cpu number)
> the naming is 'wrong'.

I'll rename the event to "rseq_update" then, and have two fields:
cpu_id and event_counter.

Thanks,

Mathieu

> 
>>         return true;
> > }

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

[toc] | [prev] | [next] | [standalone]

#1459646 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Wed, Aug 10, 2016 at 01:26:04PM +0000, Mathieu Desnoyers wrote:

> static bool rseq_update_cpu_id_event_counter(struct task_struct *t)
> {
>         union rseq_cpu_event u;
> 
>         u.e.cpu_id = raw_smp_processor_id();
>         u.e.event_counter = ++t->rseq_event_counter;
>         if (__put_user(u.v, &t->rseq->u.v))
>                 return false;
>         trace_rseq_inc(t->rseq_event_counter);

I had not previously noticed the trace_* muck, but I would suggest
passing in t and leaving it up to the tracepoint implementation to pick
out the value.

Also, since this not only increments (it also updates the cpu number)
the naming is 'wrong'.

>         return true;
> }

[toc] | [prev] | [next] | [standalone]

#1459793 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Tue, Aug 09, 2016 at 10:41:47PM +0000, Mathieu Desnoyers wrote:
> #ifdef __LP64__
> 
> static bool rseq_update_cpu_id_event_counter(struct task_struct *t)
> {
>         union rseq_cpu_event u;
> 
>         u.e.cpu_id = raw_smp_processor_id();
>         u.e.event_counter = ++t->rseq_event_counter;
>         if (__put_user(u.v, &t->rseq->u.v))
>                 return false;
>         trace_rseq_inc(t->rseq_event_counter);
>         return true;
> }
> 
> #else /* #ifdef __LP64__ */
> 
> static bool rseq_update_cpu_id_event_counter(struct task_struct *t)
> {
>         if (__put_user(raw_smp_processor_id(), &t->rseq->u.e.cpu_id))
>                 return false;
>         if (__put_user(++t->rseq_event_counter, &t->rseq->u.e.event_counter))
>                 return false;
>         trace_rseq_inc(t->rseq_event_counter);
>         return true;
> }
> 
> #endif /* #else #ifdef __LP64__ */

I don't think you need to guard it (and CONFIG_64BIT is the 'right'
kernel symbol for that), 32bit should have u64 __put_user() only
implemented as 2 u32 stores.

[toc] | [prev] | [next] | [standalone]

#1459384 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 4:43 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Tue, Aug 09, 2016 at 08:06:40PM +0000, Mathieu Desnoyers wrote:

<snip>

>> > Also, I think it would be good to have a comment explaining why this is
>> > split in two structures? Don't you rely on the address dependency?
>> 
>> The comment above the rseq_cs fields needs clarification, how about:
>> 
>>         /*
>>          * Restartable sequences rseq_cs field.
>>          * Contains NULL when no critical section is active for the
>>          * current thread, or holds a pointer to the currently active
>>          * struct rseq_cs.
>>          * Updated by user-space at the beginning and end of assembly
>>          * instruction sequence block, and by the kernel when it
>>          * restarts an assembly instruction sequence block. Read by the
>>          * kernel with single-copy atomicity semantics. Aligned on
>>          * 64-bit.
>>          */
>> 
>> This really explains that rseq_cs field of struct rseq holds a pointer
>> to the current struct rseq_cs (or NULL), which makes it obvious why this
>> needs to be two different structures.
> 
> I think I'm still missing things as its not obvious to me at all :/
> 
> We could equally well have chosen a single structure and picked the
> post_commit_ip field to trigger things from, no?
> 
> The only down side seems to be that we must then impose ordering (but UP
> ordering, so that's cheap) between writing the abort_ip and
> post_commit_ip.
> 
> That is; something like so:
> 
> struct rseq {
>	union rseq_event_cpu u;
> 
>	u64 abort_ip;
>	u64 post_commit_ip;
> };
> 
> Where userspace must do:
> 
>	r->abort_ip = $abort_ip;
>	barrier();
>	WRITE_ONCE(r->post_commit_ip, $post_commit_ip);
>	barrier();
> 
> Which is not much different from what Paul did, except he kept the
> abort_ip in a register (which must be loaded before setting the
> commit_ip).
> 
> And the kernel checks post_commit_ip, if 0, nothing happens, otherwise
> we check instruction_pointer and do magic.
> 
> Then after the commit, we clear post_commit_ip again; just like we now
> clear the rseq_cs pointer.
> 
> AFAICT this is an equally valid approach. So why split and put that
> indirection in?

Now I understand from which angle you are looking at it.

The reason for this indirection is to speed up the user-space rseq_finish()
fast path:

With Paul Turner's approach, we needed to clobber a register, issue
instructions to move abort_ip to that register, and store the post_commit_ip
to the TLS.

With your approach here, you need 2 stores, ordered with compiler-barriers:
storing abort_ip to TLS, and then post_commit_ip to TLS.

The approach I propose (indirection) only requires a single store to the TLS:
we store the address of the currently active struct rseq_cs descriptor. The
kernel can then fetch the content of that descriptor (start_ip, post_commit_ip,
abort_ip) when/if it preempts/deliver a signal over that critical section.

On architectures like arm32, it makes a very significant difference
performance-wise to simply remove useless register movement or stores.

So I add an indirection in the kernel slow path (upon return to user-space after
preempting a rseq asm sequence, or upon signal delivery over a rseq asm sequence),
to speed up the user-space fast path.

By using the indirection approach, we also get the "start_ip" pointer for free,
which can be used to let the kernel know the exact range of the restartable
sequence, and means we can implement the abort handler in pure C, even if it
is placed at addresses before the restartable block by the compiler. This saves
us a jump on the fast path (otherwise required to skip over the abort code).
Doing the same with Paul's approach and yours would require to clobber yet
another register or add one more store for the start_ip.

> 
>> Combined with other recent feedback, this becomes:
>> 
>>  *   The abort_ip address needs to be lesser than start_ip, or
> 
> Isn't it "less than" ?

Indeed, I had to look this one up. "lesser" is an adjective, and here
I should use "to be less than", but below the use the "be at addresses
lesser than" would appear to be OK.

> 
>>  *   greater-or-equal the post_commit_ip. Step [4] and the failure
>>  *   code step [F1] need to be at addresses lesser than start_ip, or
>>  *   greater-or-equal the post_commit_ip.
>> 
> 

<snip>

Thanks!

Mathieu

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

[toc] | [prev] | [next] | [standalone]

#1459416 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 10:28 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Wed, Aug 10, 2016 at 01:57:05PM +0000, Mathieu Desnoyers wrote:
> 
>> > We could equally well have chosen a single structure and picked the
>> > post_commit_ip field to trigger things from, no?
>> > 
>> > The only down side seems to be that we must then impose ordering (but UP
>> > ordering, so that's cheap) between writing the abort_ip and
>> > post_commit_ip.
>> > 
>> > That is; something like so:
>> > 
>> > struct rseq {
>> >	union rseq_event_cpu u;
>> > 
>> >	u64 abort_ip;
>> >	u64 post_commit_ip;
>> > };
>> > 
>> > Where userspace must do:
>> > 
>> >	r->abort_ip = $abort_ip;
>> >	barrier();
>> >	WRITE_ONCE(r->post_commit_ip, $post_commit_ip);
>> >	barrier();
>> > 
>> > Which is not much different from what Paul did, except he kept the
>> > abort_ip in a register (which must be loaded before setting the
>> > commit_ip).
>> > 
>> > And the kernel checks post_commit_ip, if 0, nothing happens, otherwise
>> > we check instruction_pointer and do magic.
>> > 
>> > Then after the commit, we clear post_commit_ip again; just like we now
>> > clear the rseq_cs pointer.
>> > 
>> > AFAICT this is an equally valid approach. So why split and put that
>> > indirection in?
>> 
>> Now I understand from which angle you are looking at it.
>> 
>> The reason for this indirection is to speed up the user-space rseq_finish()
>> fast path:
>> 
>> With Paul Turner's approach, we needed to clobber a register, issue
>> instructions to move abort_ip to that register, and store the post_commit_ip
>> to the TLS.
>> 
>> With your approach here, you need 2 stores, ordered with compiler-barriers:
>> storing abort_ip to TLS, and then post_commit_ip to TLS.
>> 
>> The approach I propose (indirection) only requires a single store to the TLS:
>> we store the address of the currently active struct rseq_cs descriptor. The
>> kernel can then fetch the content of that descriptor (start_ip, post_commit_ip,
>> abort_ip) when/if it preempts/deliver a signal over that critical section.
>> 
>> On architectures like arm32, it makes a very significant difference
>> performance-wise to simply remove useless register movement or stores.
>> 
>> So I add an indirection in the kernel slow path (upon return to user-space after
>> preempting a rseq asm sequence, or upon signal delivery over a rseq asm
>> sequence),
>> to speed up the user-space fast path.
>> 
>> By using the indirection approach, we also get the "start_ip" pointer for free,
>> which can be used to let the kernel know the exact range of the restartable
>> sequence, and means we can implement the abort handler in pure C, even if it
>> is placed at addresses before the restartable block by the compiler. This saves
>> us a jump on the fast path (otherwise required to skip over the abort code).
>> Doing the same with Paul's approach and yours would require to clobber yet
>> another register or add one more store for the start_ip.
> 
> Ah, because the {start,abort,commit} tuple is link time constants? Which
> means we can have this in .data and not on the stack, avoiding the
> stores entirely.

Yes, this is exactly what we do in the selftests rseq.h for x86 and ppc. For
ARM32, we put this in the code (we jump over it), so we can calculate the
address pointing to the descriptor using the ip-relative "adr" instruction,
which is faster than loading an arbitrary address constant.

> 
> Because the moment we put the thing on the stack, we need to do those
> stores anyway.

Since those are link-time constants, we don't need to store them, ever.

Thanks,

Mathieu

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

[toc] | [prev] | [next] | [standalone]

#1459635 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Wed, Aug 10, 2016 at 01:57:05PM +0000, Mathieu Desnoyers wrote:

> > We could equally well have chosen a single structure and picked the
> > post_commit_ip field to trigger things from, no?
> > 
> > The only down side seems to be that we must then impose ordering (but UP
> > ordering, so that's cheap) between writing the abort_ip and
> > post_commit_ip.
> > 
> > That is; something like so:
> > 
> > struct rseq {
> >	union rseq_event_cpu u;
> > 
> >	u64 abort_ip;
> >	u64 post_commit_ip;
> > };
> > 
> > Where userspace must do:
> > 
> >	r->abort_ip = $abort_ip;
> >	barrier();
> >	WRITE_ONCE(r->post_commit_ip, $post_commit_ip);
> >	barrier();
> > 
> > Which is not much different from what Paul did, except he kept the
> > abort_ip in a register (which must be loaded before setting the
> > commit_ip).
> > 
> > And the kernel checks post_commit_ip, if 0, nothing happens, otherwise
> > we check instruction_pointer and do magic.
> > 
> > Then after the commit, we clear post_commit_ip again; just like we now
> > clear the rseq_cs pointer.
> > 
> > AFAICT this is an equally valid approach. So why split and put that
> > indirection in?
> 
> Now I understand from which angle you are looking at it.
> 
> The reason for this indirection is to speed up the user-space rseq_finish()
> fast path:
> 
> With Paul Turner's approach, we needed to clobber a register, issue
> instructions to move abort_ip to that register, and store the post_commit_ip
> to the TLS.
> 
> With your approach here, you need 2 stores, ordered with compiler-barriers:
> storing abort_ip to TLS, and then post_commit_ip to TLS.
> 
> The approach I propose (indirection) only requires a single store to the TLS:
> we store the address of the currently active struct rseq_cs descriptor. The
> kernel can then fetch the content of that descriptor (start_ip, post_commit_ip,
> abort_ip) when/if it preempts/deliver a signal over that critical section.
> 
> On architectures like arm32, it makes a very significant difference
> performance-wise to simply remove useless register movement or stores.
> 
> So I add an indirection in the kernel slow path (upon return to user-space after
> preempting a rseq asm sequence, or upon signal delivery over a rseq asm sequence),
> to speed up the user-space fast path.
> 
> By using the indirection approach, we also get the "start_ip" pointer for free,
> which can be used to let the kernel know the exact range of the restartable
> sequence, and means we can implement the abort handler in pure C, even if it
> is placed at addresses before the restartable block by the compiler. This saves
> us a jump on the fast path (otherwise required to skip over the abort code).
> Doing the same with Paul's approach and yours would require to clobber yet
> another register or add one more store for the start_ip.

Ah, because the {start,abort,commit} tuple is link time constants? Which
means we can have this in .data and not on the stack, avoiding the
stores entirely.

Because the moment we put the thing on the stack, we need to do those
stores anyway.

[toc] | [prev] | [next] | [standalone]

#1459600 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 4:10 AM, Andy Lutomirski luto@amacapital.net wrote:

> On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:

<snip>

>> Actually, we want copy_from_user() there. This executes upon
>> resume to user-space, so we can take a page fault is needed, so
>> no "inatomic" needed. I therefore suggest:
> 
> Running the code below via exit_to_usermode_loop...
> 
>>
>> static bool rseq_get_rseq_cs(struct task_struct *t,
>>                 void __user **start_ip,
>>                 void __user **post_commit_ip,
>>                 void __user **abort_ip)
>> {
>>         unsigned long ptr;
>>         struct rseq_cs __user *urseq_cs;
>>         struct rseq_cs rseq_cs;
>>
>>         if (__get_user(ptr, &t->rseq->rseq_cs))
>>                 return false;
>>         if (!ptr)
>>                 return true;
>> #ifdef CONFIG_COMPAT
>>         if (in_compat_syscall()) {
>>                 urseq_cs = compat_ptr((compat_uptr_t)ptr);
>>                 if (copy_from_user(&rseq_cs, urseq_cs, sizeof(*rseq_cs)))
>>                         return false;
>>                 *start_ip = compat_ptr((compat_uptr_t)rseq_cs.start_ip);
>>                 *post_commit_ip = compat_ptr((compat_uptr_t)rseq_cs.post_commit_ip);
>>                 *abort_ip = compat_ptr((compat_uptr_t)rseq_cs.abort_ip);
>>                 return true;
>>         }
>> #endif
> 
> ...means that in_compat_syscall() is nonsense.  (It *works* there, but
> I can't imagine that it does anything that is actually sensible for
> this use.)

Agreed that we are not per-se in a system call here. It works for
in_ia32_syscall(), but it may not work for in_x32_syscall().

Then should we test for this ?

if (!is_64bit_mm(current->mm))

This is currently x86-specific. Is this how we are expected to test
the user-space pointer size in the current mm in arch-agnostic code ?
If so, we should implement is_64bit_mm() on all other architectures.

> 
> Can't you just define the ABI so that no compat junk is needed?
> (Also, CRIU will thank you for doing that.)

We are dealing with user-space pointers here, so AFAIU we need to
be aware of their size, which involves compat code. Am I missing
something ?

> 
> 
>>>> +SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags)
>>>> +{
>>>> +    if (unlikely(flags))
>>>> +            return -EINVAL;
>>>
>>> (add whitespace)
>>
>> fixed.
>>
>>>
>>>> +    if (!rseq) {
>>>> +            if (!current->rseq)
>>>> +                    return -ENOENT;
>>>> +            return 0;
>>>> +    }
> 
> This looks entirely wrong.  Setting rseq to NULL fails if it's already
> NULL but silently does nothing if rseq is already set?  Surely it
> should always succeed and it should actually do something if rseq is
> set.

From the proposed rseq(2) manpage:

"A NULL rseq value can be used to check whether rseq is registered
for the current thread."

The implementation does just that: it returns -1, errno=ENOENT if no
rseq is currently registered, or 0 if rseq is currently registered.

Thanks,

Mathieu


> 
> 
> --
> Andy Lutomirski
> AMA Capital Management, LLC

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

[toc] | [prev] | [next] | [standalone]

#1459619 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Wed, Aug 10, 2016 at 12:04 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> ----- On Aug 10, 2016, at 4:10 AM, Andy Lutomirski luto@amacapital.net wrote:
>
>> On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>
> <snip>
>
>>> Actually, we want copy_from_user() there. This executes upon
>>> resume to user-space, so we can take a page fault is needed, so
>>> no "inatomic" needed. I therefore suggest:
>>
>> Running the code below via exit_to_usermode_loop...
>>
>>>
>>> static bool rseq_get_rseq_cs(struct task_struct *t,
>>>                 void __user **start_ip,
>>>                 void __user **post_commit_ip,
>>>                 void __user **abort_ip)
>>> {
>>>         unsigned long ptr;
>>>         struct rseq_cs __user *urseq_cs;
>>>         struct rseq_cs rseq_cs;
>>>
>>>         if (__get_user(ptr, &t->rseq->rseq_cs))
>>>                 return false;
>>>         if (!ptr)
>>>                 return true;
>>> #ifdef CONFIG_COMPAT
>>>         if (in_compat_syscall()) {
>>>                 urseq_cs = compat_ptr((compat_uptr_t)ptr);
>>>                 if (copy_from_user(&rseq_cs, urseq_cs, sizeof(*rseq_cs)))
>>>                         return false;
>>>                 *start_ip = compat_ptr((compat_uptr_t)rseq_cs.start_ip);
>>>                 *post_commit_ip = compat_ptr((compat_uptr_t)rseq_cs.post_commit_ip);
>>>                 *abort_ip = compat_ptr((compat_uptr_t)rseq_cs.abort_ip);
>>>                 return true;
>>>         }
>>> #endif
>>
>> ...means that in_compat_syscall() is nonsense.  (It *works* there, but
>> I can't imagine that it does anything that is actually sensible for
>> this use.)
>
> Agreed that we are not per-se in a system call here. It works for
> in_ia32_syscall(), but it may not work for in_x32_syscall().
>
> Then should we test for this ?
>
> if (!is_64bit_mm(current->mm))
>
> This is currently x86-specific. Is this how we are expected to test
> the user-space pointer size in the current mm in arch-agnostic code ?
> If so, we should implement is_64bit_mm() on all other architectures.

There is no universal concept of the user-space pointer size on x86
because x86 code can change it via long jumps.

What are you actually trying to do?  I would guess that
user_64bit_mode(regs) is the right thing here, because the rseq data
structure is describing the currently executing code.

>
>>
>> Can't you just define the ABI so that no compat junk is needed?
>> (Also, CRIU will thank you for doing that.)
>
> We are dealing with user-space pointers here, so AFAIU we need to
> be aware of their size, which involves compat code. Am I missing
> something ?

u64 is a perfectly valid, if odd, userspace pointer on all
architecures that I know of, and it's certainly a valid userspace
pointer on x86 32-bit userspace (the high bits will just all be zero).
Can you just use u64?

If this would be a performance problem on ARM, then maybe that's a
reason to use compat helpers.

>
>>
>>
>>>>> +SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags)
>>>>> +{
>>>>> +    if (unlikely(flags))
>>>>> +            return -EINVAL;
>>>>
>>>> (add whitespace)
>>>
>>> fixed.
>>>
>>>>
>>>>> +    if (!rseq) {
>>>>> +            if (!current->rseq)
>>>>> +                    return -ENOENT;
>>>>> +            return 0;
>>>>> +    }
>>
>> This looks entirely wrong.  Setting rseq to NULL fails if it's already
>> NULL but silently does nothing if rseq is already set?  Surely it
>> should always succeed and it should actually do something if rseq is
>> set.
>
> From the proposed rseq(2) manpage:
>
> "A NULL rseq value can be used to check whether rseq is registered
> for the current thread."
>
> The implementation does just that: it returns -1, errno=ENOENT if no
> rseq is currently registered, or 0 if rseq is currently registered.

I think that's problematic.  Why can't you unregister an existing
rseq?  If you can't, how is a thread supposed to clean up after
itself?

--Andy

[toc] | [prev] | [next] | [standalone]

#1459783 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 3:16 PM, Andy Lutomirski luto@amacapital.net wrote:

> On Wed, Aug 10, 2016 at 12:04 PM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> ----- On Aug 10, 2016, at 4:10 AM, Andy Lutomirski luto@amacapital.net wrote:
>>
>>> On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers
>>> <mathieu.desnoyers@efficios.com> wrote:
>>
>> <snip>
>>
>>>> Actually, we want copy_from_user() there. This executes upon
>>>> resume to user-space, so we can take a page fault is needed, so
>>>> no "inatomic" needed. I therefore suggest:
>>>
>>> Running the code below via exit_to_usermode_loop...
>>>
>>>>
>>>> static bool rseq_get_rseq_cs(struct task_struct *t,
>>>>                 void __user **start_ip,
>>>>                 void __user **post_commit_ip,
>>>>                 void __user **abort_ip)
>>>> {
>>>>         unsigned long ptr;
>>>>         struct rseq_cs __user *urseq_cs;
>>>>         struct rseq_cs rseq_cs;
>>>>
>>>>         if (__get_user(ptr, &t->rseq->rseq_cs))
>>>>                 return false;
>>>>         if (!ptr)
>>>>                 return true;
>>>> #ifdef CONFIG_COMPAT
>>>>         if (in_compat_syscall()) {
>>>>                 urseq_cs = compat_ptr((compat_uptr_t)ptr);
>>>>                 if (copy_from_user(&rseq_cs, urseq_cs, sizeof(*rseq_cs)))
>>>>                         return false;
>>>>                 *start_ip = compat_ptr((compat_uptr_t)rseq_cs.start_ip);
>>>>                 *post_commit_ip = compat_ptr((compat_uptr_t)rseq_cs.post_commit_ip);
>>>>                 *abort_ip = compat_ptr((compat_uptr_t)rseq_cs.abort_ip);
>>>>                 return true;
>>>>         }
>>>> #endif
>>>
>>> ...means that in_compat_syscall() is nonsense.  (It *works* there, but
>>> I can't imagine that it does anything that is actually sensible for
>>> this use.)
>>
>> Agreed that we are not per-se in a system call here. It works for
>> in_ia32_syscall(), but it may not work for in_x32_syscall().
>>
>> Then should we test for this ?
>>
>> if (!is_64bit_mm(current->mm))
>>
>> This is currently x86-specific. Is this how we are expected to test
>> the user-space pointer size in the current mm in arch-agnostic code ?
>> If so, we should implement is_64bit_mm() on all other architectures.
> 
> There is no universal concept of the user-space pointer size on x86
> because x86 code can change it via long jumps.
> 
> What are you actually trying to do?  I would guess that
> user_64bit_mode(regs) is the right thing here, because the rseq data
> structure is describing the currently executing code.

Yes, that's correct, we care about the pointer size of currently executing
code. On x86 user_64bit_mode(regs) would appear to be the right thing to do.

> 
>>
>>>
>>> Can't you just define the ABI so that no compat junk is needed?
>>> (Also, CRIU will thank you for doing that.)
>>
>> We are dealing with user-space pointers here, so AFAIU we need to
>> be aware of their size, which involves compat code. Am I missing
>> something ?
> 
> u64 is a perfectly valid, if odd, userspace pointer on all
> architecures that I know of, and it's certainly a valid userspace
> pointer on x86 32-bit userspace (the high bits will just all be zero).
> Can you just use u64?

My concern is about a 32-bit user-space putting garbage rather than zeroes
(on purpose) to fool the kernel on those upper 32 bits. Doing

  compat_ptr((compat_uptr_t)rseq_cs.start_ip)

effectively ends up clearing the upper 32 bits.

But since we only use those pointer values for comparisons, perhaps we
just don't care if a 32-bit userspace app try to shoot itself in
the foot by passing garbage upper 32 bits ?

> 
> If this would be a performance problem on ARM, then maybe that's a
> reason to use compat helpers.

We already use 64-bit values for the pointers, even on 32-bit. Normally
userspace just puts zeroes in the top bits. It's mostly a question of
clearing the top 32 bits or not when loading them in the kernel. If we
don't need to, then I can remove the compat code entirely, and we don't
care about user_64bit_mode() anymore, as you initially recommended.
Does it make sense ?

> 
>>
>>>
>>>
>>>>>> +SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags)
>>>>>> +{
>>>>>> +    if (unlikely(flags))
>>>>>> +            return -EINVAL;
>>>>>
>>>>> (add whitespace)
>>>>
>>>> fixed.
>>>>
>>>>>
>>>>>> +    if (!rseq) {
>>>>>> +            if (!current->rseq)
>>>>>> +                    return -ENOENT;
>>>>>> +            return 0;
>>>>>> +    }
>>>
>>> This looks entirely wrong.  Setting rseq to NULL fails if it's already
>>> NULL but silently does nothing if rseq is already set?  Surely it
>>> should always succeed and it should actually do something if rseq is
>>> set.
>>
>> From the proposed rseq(2) manpage:
>>
>> "A NULL rseq value can be used to check whether rseq is registered
>> for the current thread."
>>
>> The implementation does just that: it returns -1, errno=ENOENT if no
>> rseq is currently registered, or 0 if rseq is currently registered.
> 
> I think that's problematic.  Why can't you unregister an existing
> rseq?  If you can't, how is a thread supposed to clean up after
> itself?
> 

Unregistering an existing thread rseq would require that we keep reference
counting, in case multiple libs and/or the app are using rseq. I am
trying to keep things as simple as needed.

If I understand your concern, the problematic scenario would be at
thread exit (this is my current approximate understanding of glibc
handling of library TLS variable reclaim at thread exit):

thread exits in userspace:
- glibc frees its rseq TLS memory area (in case the TLS is in a library),
- thread preempted before really exiting,
- kernel reads/writes to freed TLS memory.
  - corruption may occur (e.g. memory re-allocated by another thread already)

Am I getting it right ?

Thanks,

Mathieu

> --Andy

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

[toc] | [prev] | [next] | [standalone]

#1459800 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Wed, Aug 10, 2016 at 1:06 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> ----- On Aug 10, 2016, at 3:16 PM, Andy Lutomirski luto@amacapital.net wrote:
>
>> On Wed, Aug 10, 2016 at 12:04 PM, Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>> ----- On Aug 10, 2016, at 4:10 AM, Andy Lutomirski luto@amacapital.net wrote:
>>>
>>>> On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers
>>>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> <snip>
>>>
>>>>> Actually, we want copy_from_user() there. This executes upon
>>>>> resume to user-space, so we can take a page fault is needed, so
>>>>> no "inatomic" needed. I therefore suggest:
>>>>
>>>> Running the code below via exit_to_usermode_loop...
>>>>
>>>>>
>>>>> static bool rseq_get_rseq_cs(struct task_struct *t,
>>>>>                 void __user **start_ip,
>>>>>                 void __user **post_commit_ip,
>>>>>                 void __user **abort_ip)
>>>>> {
>>>>>         unsigned long ptr;
>>>>>         struct rseq_cs __user *urseq_cs;
>>>>>         struct rseq_cs rseq_cs;
>>>>>
>>>>>         if (__get_user(ptr, &t->rseq->rseq_cs))
>>>>>                 return false;
>>>>>         if (!ptr)
>>>>>                 return true;
>>>>> #ifdef CONFIG_COMPAT
>>>>>         if (in_compat_syscall()) {
>>>>>                 urseq_cs = compat_ptr((compat_uptr_t)ptr);
>>>>>                 if (copy_from_user(&rseq_cs, urseq_cs, sizeof(*rseq_cs)))
>>>>>                         return false;
>>>>>                 *start_ip = compat_ptr((compat_uptr_t)rseq_cs.start_ip);
>>>>>                 *post_commit_ip = compat_ptr((compat_uptr_t)rseq_cs.post_commit_ip);
>>>>>                 *abort_ip = compat_ptr((compat_uptr_t)rseq_cs.abort_ip);
>>>>>                 return true;
>>>>>         }
>>>>> #endif
>>>>
>>>> ...means that in_compat_syscall() is nonsense.  (It *works* there, but
>>>> I can't imagine that it does anything that is actually sensible for
>>>> this use.)
>>>
>>> Agreed that we are not per-se in a system call here. It works for
>>> in_ia32_syscall(), but it may not work for in_x32_syscall().
>>>
>>> Then should we test for this ?
>>>
>>> if (!is_64bit_mm(current->mm))
>>>
>>> This is currently x86-specific. Is this how we are expected to test
>>> the user-space pointer size in the current mm in arch-agnostic code ?
>>> If so, we should implement is_64bit_mm() on all other architectures.
>>
>> There is no universal concept of the user-space pointer size on x86
>> because x86 code can change it via long jumps.
>>
>> What are you actually trying to do?  I would guess that
>> user_64bit_mode(regs) is the right thing here, because the rseq data
>> structure is describing the currently executing code.
>
> Yes, that's correct, we care about the pointer size of currently executing
> code. On x86 user_64bit_mode(regs) would appear to be the right thing to do.
>
>>
>>>
>>>>
>>>> Can't you just define the ABI so that no compat junk is needed?
>>>> (Also, CRIU will thank you for doing that.)
>>>
>>> We are dealing with user-space pointers here, so AFAIU we need to
>>> be aware of their size, which involves compat code. Am I missing
>>> something ?
>>
>> u64 is a perfectly valid, if odd, userspace pointer on all
>> architecures that I know of, and it's certainly a valid userspace
>> pointer on x86 32-bit userspace (the high bits will just all be zero).
>> Can you just use u64?
>
> My concern is about a 32-bit user-space putting garbage rather than zeroes
> (on purpose) to fool the kernel on those upper 32 bits. Doing
>
>   compat_ptr((compat_uptr_t)rseq_cs.start_ip)
>
> effectively ends up clearing the upper 32 bits.
>
> But since we only use those pointer values for comparisons, perhaps we
> just don't care if a 32-bit userspace app try to shoot itself in
> the foot by passing garbage upper 32 bits ?
>

How is garbage in the high bits any different than garbage in any
other bits in there?

>
>> If this would be a performance problem on ARM, then maybe that's a
>> reason to use compat helpers.
>
> We already use 64-bit values for the pointers, even on 32-bit. Normally
> userspace just puts zeroes in the top bits. It's mostly a question of
> clearing the top 32 bits or not when loading them in the kernel. If we
> don't need to, then I can remove the compat code entirely, and we don't
> care about user_64bit_mode() anymore, as you initially recommended.
> Does it make sense ?

Yes, I think so.  I'd suggest just honoring all the bits.

>
>>
>>>
>>>>
>>>>
>>>>>>> +SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags)
>>>>>>> +{
>>>>>>> +    if (unlikely(flags))
>>>>>>> +            return -EINVAL;
>>>>>>
>>>>>> (add whitespace)
>>>>>
>>>>> fixed.
>>>>>
>>>>>>
>>>>>>> +    if (!rseq) {
>>>>>>> +            if (!current->rseq)
>>>>>>> +                    return -ENOENT;
>>>>>>> +            return 0;
>>>>>>> +    }
>>>>
>>>> This looks entirely wrong.  Setting rseq to NULL fails if it's already
>>>> NULL but silently does nothing if rseq is already set?  Surely it
>>>> should always succeed and it should actually do something if rseq is
>>>> set.
>>>
>>> From the proposed rseq(2) manpage:
>>>
>>> "A NULL rseq value can be used to check whether rseq is registered
>>> for the current thread."
>>>
>>> The implementation does just that: it returns -1, errno=ENOENT if no
>>> rseq is currently registered, or 0 if rseq is currently registered.
>>
>> I think that's problematic.  Why can't you unregister an existing
>> rseq?  If you can't, how is a thread supposed to clean up after
>> itself?
>>
>
> Unregistering an existing thread rseq would require that we keep reference
> counting, in case multiple libs and/or the app are using rseq. I am
> trying to keep things as simple as needed.
>
> If I understand your concern, the problematic scenario would be at
> thread exit (this is my current approximate understanding of glibc
> handling of library TLS variable reclaim at thread exit):
>
> thread exits in userspace:
> - glibc frees its rseq TLS memory area (in case the TLS is in a library),
> - thread preempted before really exiting,
> - kernel reads/writes to freed TLS memory.
>   - corruption may occur (e.g. memory re-allocated by another thread already)
>
> Am I getting it right ?

Yes.

[toc] | [prev] | [next] | [standalone]

#1459939 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

----- On Aug 10, 2016, at 4:09 PM, Andy Lutomirski luto@amacapital.net wrote:

> On Wed, Aug 10, 2016 at 1:06 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

<snip>

>>> u64 is a perfectly valid, if odd, userspace pointer on all
>>> architecures that I know of, and it's certainly a valid userspace
>>> pointer on x86 32-bit userspace (the high bits will just all be zero).
>>> Can you just use u64?
>>
>> My concern is about a 32-bit user-space putting garbage rather than zeroes
>> (on purpose) to fool the kernel on those upper 32 bits. Doing
>>
>>   compat_ptr((compat_uptr_t)rseq_cs.start_ip)
>>
>> effectively ends up clearing the upper 32 bits.
>>
>> But since we only use those pointer values for comparisons, perhaps we
>> just don't care if a 32-bit userspace app try to shoot itself in
>> the foot by passing garbage upper 32 bits ?
>>
> 
> How is garbage in the high bits any different than garbage in any
> other bits in there?

It's not :)

> 
>>
>>> If this would be a performance problem on ARM, then maybe that's a
>>> reason to use compat helpers.
>>
>> We already use 64-bit values for the pointers, even on 32-bit. Normally
>> userspace just puts zeroes in the top bits. It's mostly a question of
>> clearing the top 32 bits or not when loading them in the kernel. If we
>> don't need to, then I can remove the compat code entirely, and we don't
>> care about user_64bit_mode() anymore, as you initially recommended.
>> Does it make sense ?
> 
> Yes, I think so.  I'd suggest just honoring all the bits.

OK, will do !

> 
>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>>> +SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags)
>>>>>>>> +{
>>>>>>>> +    if (unlikely(flags))
>>>>>>>> +            return -EINVAL;
>>>>>>>
>>>>>>> (add whitespace)
>>>>>>
>>>>>> fixed.
>>>>>>
>>>>>>>
>>>>>>>> +    if (!rseq) {
>>>>>>>> +            if (!current->rseq)
>>>>>>>> +                    return -ENOENT;
>>>>>>>> +            return 0;
>>>>>>>> +    }
>>>>>
>>>>> This looks entirely wrong.  Setting rseq to NULL fails if it's already
>>>>> NULL but silently does nothing if rseq is already set?  Surely it
>>>>> should always succeed and it should actually do something if rseq is
>>>>> set.
>>>>
>>>> From the proposed rseq(2) manpage:
>>>>
>>>> "A NULL rseq value can be used to check whether rseq is registered
>>>> for the current thread."
>>>>
>>>> The implementation does just that: it returns -1, errno=ENOENT if no
>>>> rseq is currently registered, or 0 if rseq is currently registered.
>>>
>>> I think that's problematic.  Why can't you unregister an existing
>>> rseq?  If you can't, how is a thread supposed to clean up after
>>> itself?
>>>
>>
>> Unregistering an existing thread rseq would require that we keep reference
>> counting, in case multiple libs and/or the app are using rseq. I am
>> trying to keep things as simple as needed.
>>
>> If I understand your concern, the problematic scenario would be at
>> thread exit (this is my current approximate understanding of glibc
>> handling of library TLS variable reclaim at thread exit):
>>
>> thread exits in userspace:
>> - glibc frees its rseq TLS memory area (in case the TLS is in a library),
>> - thread preempted before really exiting,
>> - kernel reads/writes to freed TLS memory.
>>   - corruption may occur (e.g. memory re-allocated by another thread already)
>>
>> Am I getting it right ?
> 
> Yes.

Hrm, then we should:

- add a rseq_refcount field to the task struct,
- increment this refcount whenever rseq receives a registration, after
  ensuring that we are registering the same address as was previously
  requested by preceding registrations for the thread (except if the
  refcount was 0),
- When rseq receives a NULL address, decrement refcount. Set address to
  NULL when it reaches 0.

Doing the refcounting in kernel-space rather than user-space allows us to
keep both registration/unregistration and refcount atomic, which simplify
things if we plan to use rseq from signal handlers.

With current glibc, a library that would lazily register and use rseq
without knowledge of the application would then have to use pthread_key_create()
to set a destr_function to run at thread exit, which would take care of
unregistration.

We could add a RSEQ_FORCE_UNREGISTER flag to rseq flags to allow future
glibc versions to force unregistering rseq before freeing its TLS memory,
just in case a userspace library omits to unregister itself.

Thoughts ?

Thanks,

Mathieu


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

[toc] | [prev] | [next] | [standalone]

#1459632 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Tue, Aug 9, 2016 at 1:06 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> ----- On Aug 3, 2016, at 9:19 AM, Peter Zijlstra peterz@infradead.org wrote:
>

>>
>>> +#endif
>>>  /* CPU-specific state of this task */
>>>      struct thread_struct thread;
>>>  /*
>>> @@ -3387,4 +3392,67 @@ void cpufreq_add_update_util_hook(int cpu, struct
>>> update_util_data *data,
>>>  void cpufreq_remove_update_util_hook(int cpu);
>>>  #endif /* CONFIG_CPU_FREQ */
>>>
>>> +#ifdef CONFIG_RSEQ
>>> +static inline void rseq_set_notify_resume(struct task_struct *t)
>>> +{
>>> +    if (t->rseq)
>>> +            set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
>>> +}
>>
>> Maybe I missed it, but why do we want to hook into NOTIFY_RESUME and not
>> have our own TIF flag?
>
> The short answer is that used the same approach as Paul Turner's patchset. ;)
>
> Through a deeper look into this, the only times we set the flag is when
> preempting and delivering a signal to a thread that has registered to
> rseq.
>
> Upon return to user-space with the flag set, the performance difference
> between having our own flag and hopping into the NOTIFY_RESUME bandwagon
> is that we can skip the various tests in exit_to_usermode_loop()
> with our own flag, at the expense of crowding the thread flags even
> nearer to filling up 32 bits, which will at some point require extra
> tests on the fast-path.

I don't think we're anywhere near running out.  Several of those flags
can probably go away pretty easily, too.

>
> Thinking about it, one benchmark I have not done so far is to modify
> hackbench so it registers its threads with the rseq system call. We
> can then figure out whether reserving a flag for rseq is justified or
> not.
>
> Comparing 10 runs of hackbench registering its sender/receiver threads
> with unmodified hackbench: (hackbench -l 100000)
>
>     Configuration: 2 sockets * 8-core Intel(R) Xeon(R) CPU E5-2630 v3 @
>     2.40GHz (directly on hardware, hyperthreading disabled in BIOS, energy
>     saving disabled in BIOS, turboboost disabled in BIOS, cpuidle.off=1
>     kernel parameter), with a Linux v4.7 defconfig+localyesconfig,
>     restartable sequences series applied.
>
>                                     Avg. Time (s)    Std.dev. (s)
> Unmodified Hackbench                    40.5            0.1
> Rseq-Registered Hackbench Threads       40.4            0.1
>
> So initial results seems to indicate that adding the notify_resume
> handling upon preemption does not have noticeable effects on
> performance, so I don't consider it worthwhile to try optimizing
> it by reserving its own thread flag. Or perhaps am I missing something
> important here ?
>

I don't think so.  One benefit of using do_notify_resume would be less
arch code.

> Actually, we want copy_from_user() there. This executes upon
> resume to user-space, so we can take a page fault is needed, so
> no "inatomic" needed. I therefore suggest:

Running the code below via exit_to_usermode_loop...

>
> static bool rseq_get_rseq_cs(struct task_struct *t,
>                 void __user **start_ip,
>                 void __user **post_commit_ip,
>                 void __user **abort_ip)
> {
>         unsigned long ptr;
>         struct rseq_cs __user *urseq_cs;
>         struct rseq_cs rseq_cs;
>
>         if (__get_user(ptr, &t->rseq->rseq_cs))
>                 return false;
>         if (!ptr)
>                 return true;
> #ifdef CONFIG_COMPAT
>         if (in_compat_syscall()) {
>                 urseq_cs = compat_ptr((compat_uptr_t)ptr);
>                 if (copy_from_user(&rseq_cs, urseq_cs, sizeof(*rseq_cs)))
>                         return false;
>                 *start_ip = compat_ptr((compat_uptr_t)rseq_cs.start_ip);
>                 *post_commit_ip = compat_ptr((compat_uptr_t)rseq_cs.post_commit_ip);
>                 *abort_ip = compat_ptr((compat_uptr_t)rseq_cs.abort_ip);
>                 return true;
>         }
> #endif

...means that in_compat_syscall() is nonsense.  (It *works* there, but
I can't imagine that it does anything that is actually sensible for
this use.)

Can't you just define the ABI so that no compat junk is needed?
(Also, CRIU will thank you for doing that.)


>>> +SYSCALL_DEFINE2(rseq, struct rseq __user *, rseq, int, flags)
>>> +{
>>> +    if (unlikely(flags))
>>> +            return -EINVAL;
>>
>> (add whitespace)
>
> fixed.
>
>>
>>> +    if (!rseq) {
>>> +            if (!current->rseq)
>>> +                    return -ENOENT;
>>> +            return 0;
>>> +    }

This looks entirely wrong.  Setting rseq to NULL fails if it's already
NULL but silently does nothing if rseq is already set?  Surely it
should always succeed and it should actually do something if rseq is
set.


-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1459770 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Tue, Aug 09, 2016 at 08:06:40PM +0000, Mathieu Desnoyers wrote:
> On Aug 3, 2016, at 9:19 AM, Peter Zijlstra peterz@infradead.org wrote:

> >> +++ b/kernel/sched/core.c
> >> @@ -2664,6 +2664,7 @@ prepare_task_switch(struct rq *rq, struct task_struct
> >> *prev,
> >>  {
> >>  	sched_info_switch(rq, prev, next);
> >>  	perf_event_task_sched_out(prev, next);
> >> +	rseq_sched_out(prev);
> > 
> > One thing I considered is doing something like:
> > 
> > static inline void rseq_sched_out(struct task_struct *t)
> > {
> >	unsigned long ptr;
> >	int err;
> > 
> >	if (!t->rseq)
> >		return;
> > 
> >	err = __get_user(ptr, &t->rseq->rseq_cs);
> >	if (err || ptr)
> >		set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> > }
> > 
> > That will optimistically try to read the rseq_cs pointer and, on success
> > and empty (the most likely case) avoid setting the TIF flag.
> > 
> > This will require an explicit migration hook to unconditionally set the
> > TIF flag such that we keep the cpu_id field correct of course.
> > 
> > And obviously we can do this later, as an optimization. Its just
> > something I figured might be worth it.
> 
> This won't work. The rseq mechanism proposed here is really the overlap
> of _two_ distinct restart mechanisms: a sequence counter for C code,
> and a ip-fixup-based mechanism for the assembly "finish" instruction
> sequence.
> 
> What you propose here only considers the fixup of the assembly instruction
> sequence, but not the C code that runs before. The C code between
> rseq_start() and rseq_finish() loads the current value of the sequence
> counter in rseq_start(), and then it gets compared with the new current
> value within the rseq_finish restartable sequence of instructions. So the
> sequence counter needs to be updated upon preemption/signal delivery that
> occurs on top of C code, even if not nesting over a sequence of
> restartable assembly instructions.

True; we could of course have the rseq_start() also set a !0 state
before reading the seq, but not sure that all is worth it.

[toc] | [prev] | [next] | [standalone]

#1459771 — Re: [RFC PATCH v7 1/7] Restartable sequences system call

On Tue, Aug 09, 2016 at 08:06:40PM +0000, Mathieu Desnoyers wrote:
> > Also, do we want a comment somewhere that explains why overflow isn't a
> > problem?
> 
> I can add a comment about rseq_increment_event_counter stating:
> 
>  * Overflow of the event counter is not a problem in practice. It
>  * increments at most once between each user-space thread instruction
>  * executed, so we would need a thread to execute 2^32 instructions or
>  * more between rseq_start() and rseq_finish(), while single-stepping,
>  * for this to be an issue.
> 
> Is it fine, or should we be more conservative and care about the overflow,
> extending the counter to a 64-bit value in the process ?

I think its good enough; and using u64 has the unfortunate side effect
of not being able to share the word with the cpu number.

My point was more to have this stuff clearly documented.

> > Maybe I missed it, but why do we want to hook into NOTIFY_RESUME and not
> > have our own TIF flag?
> 
> The short answer is that used the same approach as Paul Turner's patchset. ;)
> 
> Through a deeper look into this, the only times we set the flag is when
> preempting and delivering a signal to a thread that has registered to
> rseq.

 <snip>

> So initial results seems to indicate that adding the notify_resume
> handling upon preemption does not have noticeable effects on
> performance, so I don't consider it worthwhile to try optimizing
> it by reserving its own thread flag. Or perhaps am I missing something
> important here ?

Not sure; seems like we can leave it as is for the moment. Again my
point was to make sure we've thought about the decision, and per the
above you clearly have now ;-)

> > Also, I think it would be good to have a comment explaining why this is
> > split in two structures? Don't you rely on the address dependency?
> 
> The comment above the rseq_cs fields needs clarification, how about:
> 
>         /*
>          * Restartable sequences rseq_cs field.
>          * Contains NULL when no critical section is active for the
>          * current thread, or holds a pointer to the currently active
>          * struct rseq_cs.
>          * Updated by user-space at the beginning and end of assembly
>          * instruction sequence block, and by the kernel when it
>          * restarts an assembly instruction sequence block. Read by the
>          * kernel with single-copy atomicity semantics. Aligned on
>          * 64-bit.
>          */
> 
> This really explains that rseq_cs field of struct rseq holds a pointer
> to the current struct rseq_cs (or NULL), which makes it obvious why this
> needs to be two different structures.

I think I'm still missing things as its not obvious to me at all :/

We could equally well have chosen a single structure and picked the
post_commit_ip field to trigger things from, no?

The only down side seems to be that we must then impose ordering (but UP
ordering, so that's cheap) between writing the abort_ip and
post_commit_ip.

That is; something like so:

struct rseq {
	union rseq_event_cpu u;

	u64 abort_ip;
	u64 post_commit_ip;
};

Where userspace must do:

	r->abort_ip = $abort_ip;
	barrier();
	WRITE_ONCE(r->post_commit_ip, $post_commit_ip);
	barrier();

Which is not much different from what Paul did, except he kept the
abort_ip in a register (which must be loaded before setting the
commit_ip).

And the kernel checks post_commit_ip, if 0, nothing happens, otherwise
we check instruction_pointer and do magic.

Then after the commit, we clear post_commit_ip again; just like we now
clear the rseq_cs pointer.

AFAICT this is an equally valid approach. So why split and put that
indirection in?

> Combined with other recent feedback, this becomes:
> 
>  *   The abort_ip address needs to be lesser than start_ip, or

Isn't it "less than" ?

>  *   greater-or-equal the post_commit_ip. Step [4] and the failure
>  *   code step [F1] need to be at addresses lesser than start_ip, or
>  *   greater-or-equal the post_commit_ip.
> 


> >> +	if (current->rseq) {
> >> +		/*
> >> +		 * If rseq is already registered, check whether
> >> +		 * the provided address differs from the prior
> >> +		 * one.
> >> +		 */
> >> +		if (current->rseq != rseq)
> >> +			return -EBUSY;
> > 
> > Why explicitly allow resetting the same value?
> 
> The foreseen use is as follows: let's assume we have one or more
> user-space libraries, and possibly the application, each using rseq.
> They would each define a struct rseq TLS. They are expected to all
> give it the same name (e.g. __rseq_thread_state), and mark it as a
> weak symbol, so all uses of that symbol within the process address
> space will refer to the same address for a given thread.

Cute!

[toc] | [prev] | [next] | [standalone]

#1448164 — [RFC PATCH v7 6/7] Restartable sequences: wire up x86 32/64 system call

Wire up the rseq system call on x86 32/64.

This provides an ABI improving the speed of a user-space getcpu
operation on x86 by removing the need to perform a function call, "lsl"
instruction, or system call on the fast path, as well as improving the
speed of user-space operations on per-cpu data.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Paul Turner <pjt@google.com>
CC: Andrew Hunter <ahh@google.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
 arch/x86/entry/syscalls/syscall_32.tbl | 1 +
 arch/x86/entry/syscalls/syscall_64.tbl | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 4cddd17..15fb98c 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -386,3 +386,4 @@
 377	i386	copy_file_range		sys_copy_file_range
 378	i386	preadv2			sys_preadv2			compat_sys_preadv2
 379	i386	pwritev2		sys_pwritev2			compat_sys_pwritev2
+380	i386	rseq			sys_rseq
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 555263e..c7f3c7e 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -335,6 +335,7 @@
 326	common	copy_file_range		sys_copy_file_range
 327	64	preadv2			sys_preadv2
 328	64	pwritev2		sys_pwritev2
+329	common	rseq			sys_rseq
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1449039 — Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests

----- On Jul 23, 2016, at 5:26 PM, Dave Watson davejwatson@fb.com wrote:

> Hi Mathieu,

> > Implements two basic tests of RSEQ functionality, and one more
> > exhaustive parameterizable test.

> Thanks for beefing up the tests. I ran this set through our jemalloc
> tests using rseq, and everything looks good so far.

> +static inline __attribute__((always_inline))
> +bool rseq_finish(struct rseq_lock *rlock,
> + intptr_t *p, intptr_t to_write,
> + struct rseq_state start_value)
> +{
> + RSEQ_INJECT_C(9)
> +
> + if (unlikely(start_value.lock_state != RSEQ_LOCK_STATE_RESTART)) {
> + if (start_value.lock_state == RSEQ_LOCK_STATE_LOCK)
> + rseq_fallback_wait(rlock);
> + return false;
> + }
> +
> +#ifdef __x86_64__
> + /*
> + * The __rseq_table section can be used by debuggers to better
> + * handle single-stepping through the restartable critical
> + * sections.
> + */
> + __asm__ __volatile__ goto (
> + ".pushsection __rseq_table, \"aw\"\n\t"
> + ".balign 8\n\t"
> + "4:\n\t"
> + ".quad 1f, 2f, 3f\n\t"
> + ".popsection\n\t"

> Is there a reason we're also passing the start ip? It looks unused.
> I see the "for debuggers" comment, but it looks like all the debugger
> support is done in userspace.

> + "1:\n\t"
> + RSEQ_INJECT_ASM(1)
> + "movq $4b, (%[rseq_cs])\n\t"
> + RSEQ_INJECT_ASM(2)
> + "cmpl %[start_event_counter], %[current_event_counter]\n\t"
> + "jnz 3f\n\t"
> + RSEQ_INJECT_ASM(3)
> + "movq %[to_write], (%[target])\n\t"
> + "2:\n\t"
> + RSEQ_INJECT_ASM(4)
> + "movq $0, (%[rseq_cs])\n\t"
> + "jmp %l[succeed]\n\t"
> + "3: movq $0, (%[rseq_cs])\n\t"
> + : /* no outputs */
> + : [start_event_counter]"r"(start_value.event_counter),
> + [current_event_counter]"m"(start_value.rseqp->abi.u.e.event_counter),
> + [to_write]"r"(to_write),
> + [target]"r"(p),
> + [rseq_cs]"r"(&start_value.rseqp->abi.rseq_cs)
> + RSEQ_INJECT_INPUT
> + : "memory", "cc"
> + RSEQ_INJECT_CLOBBER
> + : succeed
> + );

> This ABI looks like it will work fine for our use case. I don't think it
> has been mentioned yet, but we may still need multiple asm blocks
> for differing numbers of writes. For example, an array-based freelist push:

> void push(void *obj) {
> if (index < maxlen) {
> freelist[index++] = obj;
> }
> }

> would be more efficiently implemented with a two-write rseq_finish:

> rseq_finish2(&freelist[index], obj, // first write
> &index, index + 1, // second write
> ...);

> where it is ok to abort between the two writes, but both need to happen
> on the same cpu.

(re-send without html formatting for the mailing lists)

Would pairing one rseq_start with two rseq_finish do the trick
there ?

Thanks,

Mathieu


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

[toc] | [prev] | [next] | [standalone]

#1449184 — Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests

>> +static inline __attribute__((always_inline))
>> +bool rseq_finish(struct rseq_lock *rlock,
>> + intptr_t *p, intptr_t to_write,
>> + struct rseq_state start_value)

>> This ABI looks like it will work fine for our use case. I don't think it
>> has been mentioned yet, but we may still need multiple asm blocks
>> for differing numbers of writes. For example, an array-based freelist push:

>> void push(void *obj) {
>> if (index < maxlen) {
>> freelist[index++] = obj;
>> }
>> }

>> would be more efficiently implemented with a two-write rseq_finish:

>> rseq_finish2(&freelist[index], obj, // first write
>> &index, index + 1, // second write
>> ...);

> Would pairing one rseq_start with two rseq_finish do the trick
> there ?

Yes, two rseq_finish works, as long as the extra rseq management overhead
is not substantial.  

[toc] | [prev] | [next] | [standalone]

#1449653 — Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests

----- On Jul 24, 2016, at 2:01 PM, Dave Watson davejwatson@fb.com wrote:

>>> +static inline __attribute__((always_inline))
>>> +bool rseq_finish(struct rseq_lock *rlock,
>>> + intptr_t *p, intptr_t to_write,
>>> + struct rseq_state start_value)
> 
>>> This ABI looks like it will work fine for our use case. I don't think it
>>> has been mentioned yet, but we may still need multiple asm blocks
>>> for differing numbers of writes. For example, an array-based freelist push:
> 
>>> void push(void *obj) {
>>> if (index < maxlen) {
>>> freelist[index++] = obj;
>>> }
>>> }
> 
>>> would be more efficiently implemented with a two-write rseq_finish:
> 
>>> rseq_finish2(&freelist[index], obj, // first write
>>> &index, index + 1, // second write
>>> ...);
> 
>> Would pairing one rseq_start with two rseq_finish do the trick
>> there ?
> 
> Yes, two rseq_finish works, as long as the extra rseq management overhead
> is not substantial.

The different is actually not negligible. On x86-64
Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz:
(counter increment benchmark (single-thread))

* Single store per increment:                                  3.6 ns
* Two rseq_finish() per increment:                             5.2 ns
* rseq_finish2() with two mov instructions per rseq_finish2(): 4.0 ns

And I expect the difference to be even larger on non-x86 architectures.

I'll try to figure out a way to do rseq_finish() and rseq_finish2()
without duplicating the code. Perhaps macros will be helpful there.

Thanks,

Mathieu

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

[toc] | [prev] | [next] | [standalone]

#1449712 — Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests

----- On Jul 23, 2016, at 5:26 PM, Dave Watson davejwatson@fb.com wrote:

[...]

> +static inline __attribute__((always_inline))
> +bool rseq_finish(struct rseq_lock *rlock,
> + intptr_t *p, intptr_t to_write,
> + struct rseq_state start_value)
> +{
> + RSEQ_INJECT_C(9)
> +
> + if (unlikely(start_value.lock_state != RSEQ_LOCK_STATE_RESTART)) {
> + if (start_value.lock_state == RSEQ_LOCK_STATE_LOCK)
> + rseq_fallback_wait(rlock);
> + return false;
> + }
> +
> +#ifdef __x86_64__
> + /*
> + * The __rseq_table section can be used by debuggers to better
> + * handle single-stepping through the restartable critical
> + * sections.
> + */
> + __asm__ __volatile__ goto (
> + ".pushsection __rseq_table, \"aw\"\n\t"
> + ".balign 8\n\t"
> + "4:\n\t"
> + ".quad 1f, 2f, 3f\n\t"
> + ".popsection\n\t"

> Is there a reason we're also passing the start ip? It looks unused.
> I see the "for debuggers" comment, but it looks like all the debugger
> support is done in userspace.

I notice I did not answer this question. This __rseq_table section is
populated by struct rseq_cs elements. This has two uses:

1) Interface with the kernel: only fields "post_commit_ip" and "abort_ip"
   are currently used by the kernel. This is a "critical section descriptor".
   User-space stores a pointer to this descriptor in the struct rseq
   "rseq_cs" field to tell the kernel that it needs to handle the
   rseq assembly block critical section, and it is set back to 0 when
   exiting the critical section.

2) Interface for debuggers: all three fields are used: "start_ip",
   "post_commit_ip", and "abort_ip". When a debugger single-steps
   through the rseq assembly block by placing breakpoints at the
   following instruction (I observed this behavior with gdb on arm32),
   it needs to be made aware that, when single-stepping instructions
   between "start_ip" (included) and "post_commit_ip" (excluded), it
   should also place a breakpoint at "abort_ip", or single-stepping
   would be fooled by an abort.

On 32-bit and 64-bit x86, we can combine the structures for (1) and (2)
and only keep one structure for both. The assembly fast-path can therefore
use the address within the __rseq_table section as pointer to descriptor.

On 32-bit ARM, as an optimization, we keep two copies of this structure:
one is in the __rseq_table section (for debuggers), and the other is
placed near the instruction pointer, so a cheaper ip-relative "adr"
instruction can be used to calculate the address of the descriptor.

If my understanding if correct, you suggest we do the following instead:

struct rseq_cs {
        RSEQ_FIELD_u32_u64(post_commit_ip);
        RSEQ_FIELD_u32_u64(abort_ip);
};

struct rseq_debug_cs {
        struct rseq_cs rseq_cs;
        RSEQ_FIELD_u32_u64(start_ip);
};

So we put struct rseq_debug_cs elements within the __rseq_table section,
and only expose the struct rseq_cs part to the kernel ABI.

Thinking a bit more about this, I think we should use the start_ip in the
kernel too. The idea here is that the end of critical sections (user-space fast
path) currently looks like this tangled mess (e.g. x86-64):

                "cmpl %[start_event_counter], %[current_event_counter]\n\t"
                "jnz 3f\n\t"                         <--- branch in case of failure
                "movq %[to_write], (%[target])\n\t"  <--- commit instruction
                "2:\n\t"
                "movq $0, (%[rseq_cs])\n\t"
                "jmp %l[succeed]\n\t"                <--- jump over the failure path
                "3: movq $0, (%[rseq_cs])\n\t"

Where we basically need to jump over the failure path at the end of the
successful fast path, all because the failure path needs to be placed
at addresses greater or equal to the post_commit_ip.

In the kernel, if rather than testing for:

if ((void __user *)instruction_pointer(regs) < post_commit_ip) {

we could test for both start_ip and post_commit_ip:

if ((void __user *)instruction_pointer(regs) < post_commit_ip
    && (void __user *)instruction_pointer(regs) >= start_ip) {

We could perform the failure path (storing NULL into the rseq_cs
field of struct rseq) in C rather than being required to do it in
assembly at addresses >= to post_commit_ip, all because the kernel
would test whether we are within the assembly block address range
using both the lower and upper bounds (start_ip and post_commit_ip).

The extra check with start_ip in the kernel is only done in a slow
path (in the notify resume handler), and only if the rseq_cs field
of struct rseq is non-NULL (when the kernel actually preempts or
delivers a signal over a rseq critical section), so it should not
matter at all in terms of kernel performance.

Removing this extra jump from the user-space fast-path might not have
much impact on x86, but I expect it to be more important on
architectures like ARM32, where the architecture is less forgiving
when fed sub-optimal assembly.

Thoughts ?

Thanks,

Mathieu

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

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web