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

Groups > linux.kernel > #1460866 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-12 01:30 +0200
    Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Boqun Feng <boqun.feng@gmail.com> - 2016-08-12 03:30 +0200
      Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-12 05:20 +0200
        Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-12 05:20 +0200
        Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Boqun Feng <boqun.feng@gmail.com> - 2016-08-12 07:40 +0200
          Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Boqun Feng <boqun.feng@gmail.com> - 2016-08-12 18:40 +0200
            Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-12 20:20 +0200
              Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Boqun Feng <boqun.feng@gmail.com> - 2016-08-13 03:30 +0200
                Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-14 17:10 +0200
                  Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Boqun Feng <boqun.feng@gmail.com> - 2016-08-15 03:00 +0200
                    Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-15 20:10 +0200
    Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-12 21:40 +0200
      Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Dave Watson <davejwatson@fb.com> - 2016-08-12 22:10 +0200
        Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-08-14 19:20 +0200

#1460866 — 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.

I've added a commit implementing rseq_finish2() in my rseq volatile
dev branch. You can fetch it at:

https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback

I also have a separate test and benchmark tree in addition to the
kernel selftests here:

https://github.com/compudj/rseq-test

I named the first write a "speculative" write, and the second write
the "final" write.

Would you like to extend the test cases to cover your intended use-case ?

Thanks,

Mathieu

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

[toc] | [next] | [standalone]

#1460888

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 11, 2016 at 11:26:30PM +0000, Mathieu Desnoyers wrote:
> ----- 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.
> 
> I've added a commit implementing rseq_finish2() in my rseq volatile
> dev branch. You can fetch it at:
> 
> https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback
> 
> I also have a separate test and benchmark tree in addition to the
> kernel selftests here:
> 
> https://github.com/compudj/rseq-test
> 
> I named the first write a "speculative" write, and the second write
> the "final" write.
> 

Maybe I miss something subtle, but if the first write is only a
"speculative" write, why can't we put it in the rseq critical section
rather than asm block? Like this:

	do_rseq(..., result, targetptr, newval
		{	
			newval = index;
			targetptr = &index;
			if (newval < maxlen)
				freelist[newval++] = obj;
			else
				result = false;
		}

No extra rseq_finish() is needed here, but maybe a little more
"speculative" writes?

> Would you like to extend the test cases to cover your intended use-case ?
> 

Dave, if you are going to write some test cases about your use-cases,
would you also try the away I mentioned above?


Besides, do we allow userspace programs do read-only access to the
memory objects modified by do_rseq(). If so, we have a problem when
there are two writes in a do_rseq()(either in the rseq critical section
or in the asm block), because in current implemetation, these two writes
are unordered, which makes the readers outside a do_rseq() could observe
the ordering of writes differently.

For rseq_finish2(), a simple solution would be making the "final" write
a RELEASE.

Regards,
Boqun

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

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

#1460927

----- On Aug 11, 2016, at 9:28 PM, Boqun Feng boqun.feng@gmail.com wrote:

> On Thu, Aug 11, 2016 at 11:26:30PM +0000, Mathieu Desnoyers wrote:
>> ----- 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.
>> 
>> I've added a commit implementing rseq_finish2() in my rseq volatile
>> dev branch. You can fetch it at:
>> 
>> https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback
>> 
>> I also have a separate test and benchmark tree in addition to the
>> kernel selftests here:
>> 
>> https://github.com/compudj/rseq-test
>> 
>> I named the first write a "speculative" write, and the second write
>> the "final" write.
>> 
> 
> Maybe I miss something subtle, but if the first write is only a
> "speculative" write, why can't we put it in the rseq critical section
> rather than asm block? Like this:
> 
>	do_rseq(..., result, targetptr, newval
>		{
>			newval = index;
>			targetptr = &index;
>			if (newval < maxlen)
>				freelist[newval++] = obj;
>			else
>				result = false;
>		}
> 
> No extra rseq_finish() is needed here, but maybe a little more
> "speculative" writes?

This won't work unfortunately. The speculative stores need to be
between the rseq_event_counter comparison instruction in the rseq_finish
asm sequence and the final store. The ip fixup is really needed for
correctness of speculative stores. The sequence number scheme only works
for loads.

Putting it in the C code between rseq_start and rseq_finish would lead
to races such as:

thread A                                thread B
rseq_start
<preempted>
                                        <sched in>
                                        rseq_start
                                        freelist[offset + 1] = obj
                                        rseq_finish
                                           offset++
                                        <preempted>
<sched in>
freelist[newval + 1] = obj  <--- corrupts the list content.

<snip>

> Besides, do we allow userspace programs do read-only access to the
> memory objects modified by do_rseq(). If so, we have a problem when
> there are two writes in a do_rseq()(either in the rseq critical section
> or in the asm block), because in current implemetation, these two writes
> are unordered, which makes the readers outside a do_rseq() could observe
> the ordering of writes differently.
> 
> For rseq_finish2(), a simple solution would be making the "final" write
> a RELEASE.

Indeed, we would need a release semantic for the final store here if this
is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
add a rseq_finish2_release. We should find a way to eliminate code duplication
there. I suspect we'll end up doing macros.

Thanks,

Mathieu

> 
> Regards,
> Boqun
> 
>> Thanks,
>> 
>> Mathieu
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
> > http://www.efficios.com

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

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

#1460930

----- On Aug 11, 2016, at 11:10 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Aug 11, 2016, at 9:28 PM, Boqun Feng boqun.feng@gmail.com wrote:
> 
>> On Thu, Aug 11, 2016 at 11:26:30PM +0000, Mathieu Desnoyers wrote:
>>> ----- 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.
>>> 
>>> I've added a commit implementing rseq_finish2() in my rseq volatile
>>> dev branch. You can fetch it at:
>>> 
>>> https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback
>>> 
>>> I also have a separate test and benchmark tree in addition to the
>>> kernel selftests here:
>>> 
>>> https://github.com/compudj/rseq-test
>>> 
>>> I named the first write a "speculative" write, and the second write
>>> the "final" write.
>>> 
>> 
>> Maybe I miss something subtle, but if the first write is only a
>> "speculative" write, why can't we put it in the rseq critical section
>> rather than asm block? Like this:
>> 
>>	do_rseq(..., result, targetptr, newval
>>		{
>>			newval = index;
>>			targetptr = &index;
>>			if (newval < maxlen)
>>				freelist[newval++] = obj;
>>			else
>>				result = false;
>>		}
>> 
>> No extra rseq_finish() is needed here, but maybe a little more
>> "speculative" writes?
> 
> This won't work unfortunately. The speculative stores need to be
> between the rseq_event_counter comparison instruction in the rseq_finish
> asm sequence and the final store. The ip fixup is really needed for
> correctness of speculative stores. The sequence number scheme only works
> for loads.
> 
> Putting it in the C code between rseq_start and rseq_finish would lead
> to races such as:
> 
> thread A                                thread B
> rseq_start
> <preempted>
>                                        <sched in>
>                                        rseq_start
>                                        freelist[offset + 1] = obj
>                                        rseq_finish
>                                           offset++
>                                        <preempted>
> <sched in>
> freelist[newval + 1] = obj  <--- corrupts the list content.
> 

Small clarification to the scenario:

thread A                                thread B
rseq_start
load offset into (register 1)
<preempted>
                                       <sched in>
                                       rseq_start
                                       freelist[offset + 1] = obj
                                       rseq_finish
                                          offset++
                                       <preempted>
<sched in>
freelist[(register 1) + 1] = obj  <--- corrupts the list content.

Thanks,

Mathieu


> <snip>
> 
>> Besides, do we allow userspace programs do read-only access to the
>> memory objects modified by do_rseq(). If so, we have a problem when
>> there are two writes in a do_rseq()(either in the rseq critical section
>> or in the asm block), because in current implemetation, these two writes
>> are unordered, which makes the readers outside a do_rseq() could observe
>> the ordering of writes differently.
>> 
>> For rseq_finish2(), a simple solution would be making the "final" write
>> a RELEASE.
> 
> Indeed, we would need a release semantic for the final store here if this
> is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
> add a rseq_finish2_release. We should find a way to eliminate code duplication
> there. I suspect we'll end up doing macros.
> 
> Thanks,
> 
> Mathieu
> 
>> 
>> Regards,
>> Boqun
>> 
>>> Thanks,
>>> 
>>> Mathieu
>>> 
>>> --
>>> Mathieu Desnoyers
>>> EfficiOS Inc.
>> > http://www.efficios.com
> 
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

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

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

#1460954

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 12, 2016 at 03:10:38AM +0000, Mathieu Desnoyers wrote:
> ----- On Aug 11, 2016, at 9:28 PM, Boqun Feng boqun.feng@gmail.com wrote:
> 
> > On Thu, Aug 11, 2016 at 11:26:30PM +0000, Mathieu Desnoyers wrote:
> >> ----- 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.
> >> 
> >> I've added a commit implementing rseq_finish2() in my rseq volatile
> >> dev branch. You can fetch it at:
> >> 
> >> https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback
> >> 
> >> I also have a separate test and benchmark tree in addition to the
> >> kernel selftests here:
> >> 
> >> https://github.com/compudj/rseq-test
> >> 
> >> I named the first write a "speculative" write, and the second write
> >> the "final" write.
> >> 
> > 
> > Maybe I miss something subtle, but if the first write is only a
> > "speculative" write, why can't we put it in the rseq critical section
> > rather than asm block? Like this:
> > 
> >	do_rseq(..., result, targetptr, newval
> >		{
> >			newval = index;
> >			targetptr = &index;
> >			if (newval < maxlen)
> >				freelist[newval++] = obj;
> >			else
> >				result = false;
> >		}
> > 
> > No extra rseq_finish() is needed here, but maybe a little more
> > "speculative" writes?
> 
> This won't work unfortunately. The speculative stores need to be
> between the rseq_event_counter comparison instruction in the rseq_finish
> asm sequence and the final store. The ip fixup is really needed for
> correctness of speculative stores. The sequence number scheme only works
> for loads.
> 
> Putting it in the C code between rseq_start and rseq_finish would lead
> to races such as:
> 
> thread A                                thread B
> rseq_start
> <preempted>
>                                         <sched in>
>                                         rseq_start
>                                         freelist[offset + 1] = obj
>                                         rseq_finish
>                                            offset++
>                                         <preempted>
> <sched in>
> freelist[newval + 1] = obj  <--- corrupts the list content.
> 

Ah, right!

We couldn't do any "global"(real global or percpu) update in the rseq
critical section(code between rseq_start and rseq_finish), because
without an ip fixup, we cannot abort the critical section immediately,
we have to compare the event_counter in rseq_finish, but that's too late
for speculates stores.

> <snip>
> 
> > Besides, do we allow userspace programs do read-only access to the
> > memory objects modified by do_rseq(). If so, we have a problem when
> > there are two writes in a do_rseq()(either in the rseq critical section
> > or in the asm block), because in current implemetation, these two writes
> > are unordered, which makes the readers outside a do_rseq() could observe
> > the ordering of writes differently.
> > 
> > For rseq_finish2(), a simple solution would be making the "final" write
> > a RELEASE.
> 
> Indeed, we would need a release semantic for the final store here if this
> is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
> add a rseq_finish2_release. We should find a way to eliminate code duplication

I'm in favor of a separate rseq_finish2_release().

> there. I suspect we'll end up doing macros.
> 

Me too. Lemme have a try ;-)

Regards,
Boqun

> Thanks,
> 
> Mathieu
> 
> > 
> > Regards,
> > Boqun
> > 
> >> Thanks,
> >> 
> >> Mathieu
> >> 
> >> --
> >> Mathieu Desnoyers
> >> EfficiOS Inc.
> > > http://www.efficios.com
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

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

#1461313

On Fri, Aug 12, 2016 at 01:30:15PM +0800, Boqun Feng wrote:
[snip]
> > > Besides, do we allow userspace programs do read-only access to the
> > > memory objects modified by do_rseq(). If so, we have a problem when
> > > there are two writes in a do_rseq()(either in the rseq critical section
> > > or in the asm block), because in current implemetation, these two writes
> > > are unordered, which makes the readers outside a do_rseq() could observe
> > > the ordering of writes differently.
> > > 
> > > For rseq_finish2(), a simple solution would be making the "final" write
> > > a RELEASE.
> > 
> > Indeed, we would need a release semantic for the final store here if this
> > is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
> > add a rseq_finish2_release. We should find a way to eliminate code duplication
> 
> I'm in favor of a separate rseq_finish2_release().
> 
> > there. I suspect we'll end up doing macros.
> > 
> 
> Me too. Lemme have a try ;-)
> 

How about this? Although a little messy, I separated the asm block into
several parts and implemented each part in a arch-diagnose way.

Compiled successfully on x86 and ppc64le, no more further tests.

Regards,
Boqun

-------------------->8
From 3a4c40ded1320b824af462d875f942913e5c46a3 Mon Sep 17 00:00:00 2001
From: Boqun Feng <boqun.feng@gmail.com>
Date: Sat, 13 Aug 2016 00:16:13 +0800
Subject: [PATCH] WIP1

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 tools/testing/selftests/rseq/rseq.h | 541 ++++++++++++++----------------------
 1 file changed, 205 insertions(+), 336 deletions(-)

diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
index e8614e76b377..7e13aab2ec8b 100644
--- a/tools/testing/selftests/rseq/rseq.h
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -304,6 +304,172 @@ struct rseq_state rseq_start(struct rseq_lock *rlock)
 	return result;
 }
 
+/*
+ * ASM code for building the rseq_cs table
+ */
+
+#if defined(__x86_64__) || defined(__PPC64__)
+# define RSEQ_CS_TABLE(table, start, post_commit, abort)	\
+	".balign 32\n\t"						\
+	table ":\n\t"						\
+	".quad " start "," post_commit "," abort ", 0x0\n\t"
+#elif defined(__ARMEL__)
+# define RSEQ_CS_TABLE(table, start, post_commit, abort)	\
+	".balign 32\n\t"						\
+	table ":\n\t"						\
+	".long " start ", 0x0," post_commit ", 0x0," abort ", 0x0, 0x0, 0x0\n\t"
+#elif defined(__PPC__) /* PPC32 */
+# define RSEQ_CS_TABLE(table, start_ip, post_commit_ip, abort_ip)	\
+	".balign 32\n\t"						\
+	table ":\n\t"							\
+	".long 0x0," start ", 0x0," post_commit ", 0x0," abort ", 0x0, 0x0\n\t"
+#else
+#endif
+
+/*
+ * ASM code for putting the rseq_cs table into a special section for debugging
+ */
+
+#define RSEQ_CS_TABLE_SECTION(table, start, post_commit, abort)		\
+	".pushsection __rseq_table, \"aw\"\n\t"				\
+	RSEQ_CS_TABLE(table, start, post_commit, abort)			\
+	".popsection\n\t"						\
+	start ":\n\t"
+
+
+/*
+ * ASM code to store the pointer of rseq_cs table into rseq structure, which
+ * indicates the start of rseq asm block
+ */
+#ifdef __x86_64__
+# define RSEQ_CS_STORE(cs_table, shadow_table, rseq_cs)			\
+	"movq $" cs_table ",(" rseq_cs ")\n\t"
+#elif defined(__i386__)
+# define RSEQ_CS_STORE(cs_table, shadow_table, rseq_cs)			\
+	"movl $" cs_table ",(" rseq_cs ")\n\t"
+#elif defined(__ARMEL__)
+# define RSEQ_CS_STORE(cs_table, shadow_table, rseq_cs)			\
+	"adr r0, " shadow_table "\n\t"					\
+	"str r0, [" rseq_cs "]\n\t"
+#elif defined(__PPC64__)
+# define RSEQ_CS_STORE(cs_table, shadow_table, rseq_cs)			\
+	"lis %%r17, (" cs_table ")@highest\n\t"			\
+	"ori %%r17, %%r17, (" cs_table ")@higher\n\t"			\
+	"rldicr %%r17, %%r17, 32, 31\n\t"				\
+	"oris %%r17, %%r17, (" cs_table ")@h\n\t"			\
+	"ori %%r17, %%r17, (" cs_table ")@l\n\t"			\
+	"std %%r17, 0(" rseq_cs ")\n\t"
+#elif defined(__PPC__)
+# define RSEQ_CS_STORE(cs_table, shadow_table, rseq_cs)			\
+	"lis %%r17, (" cs_table ")@ha\n\t"				\
+	"addi %%r17, %%r17, (" cs_table ")@l\n\t"			\
+	"stw %%r17, 0(" rseq_cs ")\n\t"
+#else
+# error unsupported target
+#endif
+
+/* ASM code to check whether the event_counter changed */
+#ifdef __x86_64__
+# define RSEQ_CHECK_COUNTER(start_counter, current_counter, abort_ip)	\
+	"cmpl " start_counter ", " current_counter "\n\t"		\
+	"jnz " abort_ip "\n\t"
+#elif defined(__i386__)
+# define RSEQ_CHECK_COUNTER(start_counter, current_counter, abort_ip)	\
+	"cmpl " start_counter ", " current_counter "\n\t"		\
+	"jnz " abort_ip "\n\t"
+#elif defined(__ARMEL__)
+# define RSEQ_CHECK_COUNTER(start_counter, current_counter, abort_ip)	\
+	"ldr r0, " current_counter "\n\t"				\
+	"cmp " start_counter ", r0\n\t"					\
+	"bne " abort_ip "\n\t"
+#elif defined(__PPC__)
+# define RSEQ_CHECK_COUNTER(start_counter, current_counter, abort_ip)	\
+	"lwz %%r17, " current_counter "\n\t"				\
+	"cmpw cr7, " start_counter ", %%r17\n\t"			\
+	"bne- cr7, " abort_ip "\n\t"
+#else
+# error unsupported target
+#endif
+
+/* ASM code to do a normal write in rseq block*/
+#ifdef __x86_64__
+# define RSEQ_WRITE(to_write, target_addr)				\
+	"movq " to_write ", (" target_addr ")\n\t"
+
+#elif defined(__i386__)
+# define RSEQ_WRITE(to_write, target_addr)				\
+	"movl " to_write ", (" target_addr ")\n\t"
+
+#elif defined(__ARMEL__)
+# define RSEQ_WRITE(to_write, target_addr)				\
+	"str " to_write ", [" target_addr "]\n\t"
+
+#elif defined(__PPC64__)
+# define RSEQ_WRITE(to_write, target_addr)				\
+	"std " to_write ", 0(" target_addr ")\n\t"
+
+#elif defined(__PPC__)
+# define RSEQ_WRITE(to_write, target_addr)				\
+	"stw " to_write ", 0(" target_addr ")\n\t"
+#else
+# error unsupported target
+#endif
+
+/* ASM code to do a commit(final) write */
+#define RSEQ_COMMIT_WRITE(to_write, target_addr, post_commit)		\
+	RSEQ_WRITE(to_write, target_addr)				\
+	post_commit ":\n\t"
+
+/*
+ * ASM code to zero the rseq_cs, which indicates the end of the rseq asm block
+ */
+#if defined(__x86_64__) || defined(__i386__)
+# define RSEQ_ZERO_CS(rseq_cs)						\
+	RSEQ_WRITE("$0", rseq_cs)
+
+#elif defined(__ARMEL__)
+# define RSEQ_ZERO_CS(rseq_cs)						\
+	"mov r0, #0\n\t"						\
+	RSEQ_WRITE("r0", rseq_cs)
+
+#elif defined(__PPC__)
+# define RSEQ_ZERO_CS(rseq_cs)						\
+	"li %%r17, 0\n\t"						\
+	RSEQ_WRITE("%%r17", rseq_cs)
+
+#else
+# error unsupported target
+#endif
+
+/* ARM use another table to set the rseq_cs */
+#if defined(__ARMEL__)
+# define RSEQ_CS_SHADOW_TABLE(table, start, post_commit, abort)	\
+	"b skip\n\t"						\
+	RSEQ_CS_TABLE(table, start, post_commit, abort)		\
+	"skip:\n\t"
+#else
+# define RSEQ_CS_SHADOW_TABLE(table, start, post_commit, abort)
+#endif
+
+#define RSEQ_VAR_REG(sym, expr)	[sym] "r" (expr)
+#define RSEQ_VAR_MEM(sym, expr)	[sym] "m" (expr)
+
+#ifdef __PPC__ /* PPC64 and PPC32 */
+# define RSEQ_ADDR_REG(sym, expr)	[sym] "b" (expr)
+#endif
+
+#ifndef RSEQ_ADDR_REG
+# define RSEQ_ADDR_REG(sym, expr)	RSEQ_VAR_REG(sym, expr)
+#endif
+
+#ifdef __PPC__
+# define RSEQ_REG_COBBLER	,"r17"
+#elif defined(__ARMEL__)
+# define RSEQ_REG_COBBLER	,"r0"
+#else
+# define RSEQ_REG_COBBLER	,"memory"
+#endif
+
 static inline __attribute__((always_inline))
 bool rseq_finish(struct rseq_lock *rlock,
 		intptr_t *p, intptr_t to_write,
@@ -322,174 +488,33 @@ bool rseq_finish(struct rseq_lock *rlock,
 	 * handle single-stepping through the restartable critical
 	 * sections.
 	 */
-
-#ifdef __x86_64__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".quad 1f, 2f, %l[failure], 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"movq $3b, (%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"cmpl %[start_event_counter], %[current_event_counter]\n\t"
-		"jnz %l[failure]\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"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"r"(p),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#elif defined(__i386__)
 	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".long 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
+		RSEQ_CS_TABLE_SECTION("cs_table%=", "start%=", "post_commit%=", "%l[failure]")
+		/* start */
 		RSEQ_INJECT_ASM(1)
-		"movl $3b, (%[rseq_cs])\n\t"
+		RSEQ_CS_STORE("cs_table%=", "shadow_table%=", "%[rseq_cs]")
 		RSEQ_INJECT_ASM(2)
-		"cmpl %[start_event_counter], %[current_event_counter]\n\t"
-		"jnz %l[failure]\n\t"
+		RSEQ_CHECK_COUNTER("%[start_event_counter]",
+				   "%[current_event_counter]",
+				   "%l[failure]")
 		RSEQ_INJECT_ASM(3)
-		"movl %[to_write], (%[target])\n\t"
-		"2:\n\t"
+		RSEQ_COMMIT_WRITE("%[to_write]", "%[target]", "post_commit%=")
+		/* post_commit */
 		RSEQ_INJECT_ASM(4)
-		"movl $0, (%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"r"(p),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
+		RSEQ_ZERO_CS("%[rseq_cs]")
+		RSEQ_CS_SHADOW_TABLE("shadow_table%=", "start%=", "post_commit%=", "%l[failure]")
+		:
+		: RSEQ_VAR_REG(start_event_counter, start_value.event_counter),
+		  RSEQ_VAR_MEM(current_event_counter, start_value.rseqp->u.e.event_counter),
+		  RSEQ_VAR_REG(to_write, to_write),
+		  RSEQ_ADDR_REG(target, p),
+		  RSEQ_ADDR_REG(rseq_cs, &start_value.rseqp->rseq_cs)
 		  RSEQ_INJECT_INPUT
 		: "memory", "cc"
+		  RSEQ_REG_COBBLER
 		  RSEQ_INJECT_CLOBBER
 		: failure
-	);
-#elif defined(__ARMEL__)
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		".word 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"adr r0, 3f\n\t"
-		"str r0, [%[rseq_cs]]\n\t"
-		RSEQ_INJECT_ASM(2)
-		"ldr r0, %[current_event_counter]\n\t"
-		"mov r1, #0\n\t"
-		"cmp %[start_event_counter], r0\n\t"
-		"bne %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"str %[to_write], [%[target]]\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(4)
-		"str r1, [%[rseq_cs]]\n\t"
-		"b 4f\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".word 1b, 0x0, 2b, 0x0, l[failure], 0x0, 0x0, 0x0\n\t"
-		"4:\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"r"(p),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r0", "r1", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#elif __PPC64__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".quad 1f, 2f, %l[failure], 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"lis %%r17, (3b)@highest\n\t"
-		"ori %%r17, %%r17, (3b)@higher\n\t"
-		"rldicr %%r17, %%r17, 32, 31\n\t"
-		"oris %%r17, %%r17, (3b)@h\n\t"
-		"ori %%r17, %%r17, (3b)@l\n\t"
-		"std %%r17, 0(%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"lwz %%r17, %[current_event_counter]\n\t"
-		"cmpw cr7, %[start_event_counter], %%r17\n\t"
-		"bne- cr7, %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"std %[to_write], 0(%[target])\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(4)
-		"li %%r17, 0\n\t"
-		"std %%r17, 0(%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"b"(p),
-		  [rseq_cs]"b"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r17", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#elif __PPC__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		/* 32-bit only supported on BE */
-		".long 0x0, 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"lis %%r17, (3b)@ha\n\t"
-		"addi %%r17, %%r17, (3b)@l\n\t"
-		"stw %%r17, 0(%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"lwz %%r17, %[current_event_counter]\n\t"
-		"cmpw cr7, %[start_event_counter], %%r17\n\t"
-		"bne- cr7, %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"stw %[to_write], 0(%[target])\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(4)
-		"li %%r17, 0\n\t"
-		"stw %%r17, 0(%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"b"(p),
-		  [rseq_cs]"b"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r17", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#else
-#error unsupported target
-#endif
+		);
 	return true;
 failure:
 	RSEQ_INJECT_FAILED
@@ -525,193 +550,37 @@ bool rseq_finish2(struct rseq_lock *rlock,
 	 * sections.
 	 */
 
-#ifdef __x86_64__
 	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".quad 1f, 2f, %l[failure], 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
+		RSEQ_CS_TABLE_SECTION("cs_table%=", "start%=", "post_commit%=", "%l[failure]")
+		/* start */
 		RSEQ_INJECT_ASM(1)
-		"movq $3b, (%[rseq_cs])\n\t"
+		RSEQ_CS_STORE("cs_table%=", "shadow_table%=", "%[rseq_cs]")
 		RSEQ_INJECT_ASM(2)
-		"cmpl %[start_event_counter], %[current_event_counter]\n\t"
-		"jnz %l[failure]\n\t"
+		RSEQ_CHECK_COUNTER("%[start_event_counter]",
+				   "%[current_event_counter]",
+				   "%l[failure]")
 		RSEQ_INJECT_ASM(3)
-		"movq %[to_write_spec], (%[target_spec])\n\t"
+		RSEQ_WRITE("%[to_write_spec]", "%[target_spec]")
 		RSEQ_INJECT_ASM(4)
-		"movq %[to_write_final], (%[target_final])\n\t"
-		"2:\n\t"
+		RSEQ_COMMIT_WRITE("%[to_write_final]", "%[target_final]", "post_commit%=")
+		/* post_commit */
 		RSEQ_INJECT_ASM(5)
-		"movq $0, (%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write_spec]"r"(to_write_spec),
-		  [target_spec]"r"(p_spec),
-		  [to_write_final]"r"(to_write_final),
-		  [target_final]"r"(p_final),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
+		RSEQ_ZERO_CS("%[rseq_cs]")
+		RSEQ_CS_SHADOW_TABLE("shadow_table%=", "start%=", "post_commit%=", "%l[failure]")
+		:
+		: RSEQ_VAR_REG(start_event_counter, start_value.event_counter),
+		  RSEQ_VAR_MEM(current_event_counter, start_value.rseqp->u.e.event_counter),
+		  RSEQ_VAR_REG(to_write_spec, to_write_spec),
+		  RSEQ_ADDR_REG(target_spec, p_spec),
+		  RSEQ_VAR_REG(to_write_final, to_write_final),
+		  RSEQ_ADDR_REG(target_final, p_final),
+		  RSEQ_ADDR_REG(rseq_cs, &start_value.rseqp->rseq_cs)
 		  RSEQ_INJECT_INPUT
 		: "memory", "cc"
+		  RSEQ_REG_COBBLER
 		  RSEQ_INJECT_CLOBBER
 		: failure
-	);
-#elif defined(__i386__)
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".long 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"movl $3b, (%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"cmpl %[start_event_counter], %[current_event_counter]\n\t"
-		"jnz %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"movl %[to_write_spec], (%[target_spec])\n\t"
-		RSEQ_INJECT_ASM(4)
-		"movl %[to_write_final], (%[target_final])\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(5)
-		"movl $0, (%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write_spec]"r"(to_write_spec),
-		  [target_spec]"r"(p_spec),
-		  [to_write_final]"r"(to_write_final),
-		  [target_final]"r"(p_final),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#elif defined(__ARMEL__)
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		".word 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"adr r0, 3f\n\t"
-		"str r0, [%[rseq_cs]]\n\t"
-		RSEQ_INJECT_ASM(2)
-		"ldr r0, %[current_event_counter]\n\t"
-		"mov r1, #0\n\t"
-		"cmp %[start_event_counter], r0\n\t"
-		"bne %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"str %[to_write_spec], [%[target_spec]]\n\t"
-		RSEQ_INJECT_ASM(4)
-		"str %[to_write_final], [%[target_final]]\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(5)
-		"str r1, [%[rseq_cs]]\n\t"
-		"b 4f\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".word 1b, 0x0, 2b, 0x0, l[failure], 0x0, 0x0, 0x0\n\t"
-		"4:\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write_spec]"r"(to_write_spec),
-		  [target_spec]"r"(p_spec),
-		  [to_write_final]"r"(to_write_final),
-		  [target_final]"r"(p_final),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r0", "r1", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#elif __PPC64__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".quad 1f, 2f, %l[failure], 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"lis %%r17, (3b)@highest\n\t"
-		"ori %%r17, %%r17, (3b)@higher\n\t"
-		"rldicr %%r17, %%r17, 32, 31\n\t"
-		"oris %%r17, %%r17, (3b)@h\n\t"
-		"ori %%r17, %%r17, (3b)@l\n\t"
-		"std %%r17, 0(%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"lwz %%r17, %[current_event_counter]\n\t"
-		"cmpw cr7, %[start_event_counter], %%r17\n\t"
-		"bne- cr7, %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"std %[to_write_spec], 0(%[target_spec])\n\t"
-		RSEQ_INJECT_ASM(4)
-		"std %[to_write_final], 0(%[target_final])\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(5)
-		"li %%r17, 0\n\t"
-		"std %%r17, 0(%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write_spec]"r"(to_write_spec),
-		  [target_spec]"b"(p_spec),
-		  [to_write_final]"r"(to_write_final),
-		  [target_final]"b"(p_final),
-		  [rseq_cs]"b"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r17", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#elif __PPC__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		/* 32-bit only supported on BE */
-		".long 0x0, 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"lis %%r17, (3b)@ha\n\t"
-		"addi %%r17, %%r17, (3b)@l\n\t"
-		"stw %%r17, 0(%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"lwz %%r17, %[current_event_counter]\n\t"
-		"cmpw cr7, %[start_event_counter], %%r17\n\t"
-		"bne- cr7, %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"stw %[to_write_spec], 0(%[target_spec])\n\t"
-		RSEQ_INJECT_ASM(4)
-		"stw %[to_write_final], 0(%[target_final])\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(5)
-		"li %%r17, 0\n\t"
-		"stw %%r17, 0(%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write_spec]"r"(to_write_spec),
-		  [target_spec]"b"(p_spec),
-		  [to_write_final]"r"(to_write_final),
-		  [target_final]"b"(p_final),
-		  [rseq_cs]"b"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r17", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
-#else
-#error unsupported target
-#endif
+		);
 	return true;
 failure:
 	RSEQ_INJECT_FAILED
-- 
2.9.0

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

#1461366

----- On Aug 12, 2016, at 12:35 PM, Boqun Feng boqun.feng@gmail.com wrote:

> On Fri, Aug 12, 2016 at 01:30:15PM +0800, Boqun Feng wrote:
> [snip]
>> > > Besides, do we allow userspace programs do read-only access to the
>> > > memory objects modified by do_rseq(). If so, we have a problem when
>> > > there are two writes in a do_rseq()(either in the rseq critical section
>> > > or in the asm block), because in current implemetation, these two writes
>> > > are unordered, which makes the readers outside a do_rseq() could observe
>> > > the ordering of writes differently.
>> > > 
>> > > For rseq_finish2(), a simple solution would be making the "final" write
>> > > a RELEASE.
>> > 
>> > Indeed, we would need a release semantic for the final store here if this
>> > is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
>> > add a rseq_finish2_release. We should find a way to eliminate code duplication
>> 
>> I'm in favor of a separate rseq_finish2_release().
>> 
>> > there. I suspect we'll end up doing macros.
>> > 
>> 
>> Me too. Lemme have a try ;-)
>> 
> 
> How about this? Although a little messy, I separated the asm block into
> several parts and implemented each part in a arch-diagnose way.

I find it rather hard to follow the per-arch assembly with this approach.
It might prove to be troublesome if we want to do arch-specific optimizations
in the future.

I've come up with the following macro approach instead, feedback welcome!


commit 4d27431d6aefaee617540ef04518962b0e4d14f4
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Thu Aug 11 19:11:27 2016 -0400

    rseq_finish2, rseq_finish2_release (WIP)

diff --git a/tools/testing/selftests/rseq/param_test.c b/tools/testing/selftests/rseq/param_test.c
index 5f88b6b..59efc98 100644
--- a/tools/testing/selftests/rseq/param_test.c
+++ b/tools/testing/selftests/rseq/param_test.c
@@ -41,7 +41,8 @@ static __thread unsigned int yield_mod_cnt, nr_retry;
 	, [loop_cnt_1]"m"(loop_cnt[1]) \
 	, [loop_cnt_2]"m"(loop_cnt[2]) \
 	, [loop_cnt_3]"m"(loop_cnt[3]) \
-	, [loop_cnt_4]"m"(loop_cnt[4])
+	, [loop_cnt_4]"m"(loop_cnt[4]) \
+	, [loop_cnt_5]"m"(loop_cnt[5])
 
 #if defined(__x86_64__) || defined(__i386__)
 
@@ -548,7 +549,7 @@ static void show_usage(int argc, char **argv)
 	printf("	[-2 loops] Number of loops for delay injection 2\n");
 	printf("	[-3 loops] Number of loops for delay injection 3\n");
 	printf("	[-4 loops] Number of loops for delay injection 4\n");
-	printf("	[-5 loops] Number of loops for delay injection 5 (-1 to enable -m)\n");
+	printf("	[-5 loops] Number of loops for delay injection 5\n");
 	printf("	[-6 loops] Number of loops for delay injection 6 (-1 to enable -m)\n");
 	printf("	[-7 loops] Number of loops for delay injection 7 (-1 to enable -m)\n");
 	printf("	[-8 loops] Number of loops for delay injection 8 (-1 to enable -m)\n");
diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
index 5853b17..6da993d 100644
--- a/tools/testing/selftests/rseq/rseq.h
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -269,7 +269,7 @@ struct rseq_state rseq_start(struct rseq_lock *rlock)
 		result.event_counter =
 			ACCESS_ONCE(result.rseqp->u.e.event_counter);
 		/* load event_counter before cpu_id. */
-		RSEQ_INJECT_C(5)
+		RSEQ_INJECT_C(6)
 		result.cpu_id = ACCESS_ONCE(result.rseqp->u.e.cpu_id);
 	}
 	/*
@@ -281,7 +281,7 @@ struct rseq_state rseq_start(struct rseq_lock *rlock)
 	 * preemption/signalling will cause them to restart, so they
 	 * don't interfere with the lock.
 	 */
-	RSEQ_INJECT_C(6)
+	RSEQ_INJECT_C(7)
 
 	if (!has_fast_acquire_release() && likely(rseq_has_sys_membarrier)) {
 		result.lock_state = ACCESS_ONCE(rlock->state);
@@ -304,192 +304,342 @@ struct rseq_state rseq_start(struct rseq_lock *rlock)
 	return result;
 }
 
-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)
+/*
+ * The __rseq_table section can be used by debuggers to better handle
+ * single-stepping through the restartable critical sections.
+ */
 
-	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.
-	 */
+#define RSEQ_FINISH_ASM(_target_final, _to_write_final, _start_value, \
+		_failure, extra_store, extra_input) \
+	__asm__ __volatile__ goto ( \
+		".pushsection __rseq_table, \"aw\"\n\t" \
+		".balign 32\n\t" \
+		"3:\n\t" \
+		".quad 1f, 2f, %l[failure], 0x0\n\t" \
+		".popsection\n\t" \
+		"1:\n\t" \
+		RSEQ_INJECT_ASM(1) \
+		"movq $3b, (%[rseq_cs])\n\t" \
+		RSEQ_INJECT_ASM(2) \
+		"cmpl %[start_event_counter], %[current_event_counter]\n\t" \
+		"jnz %l[failure]\n\t" \
+		RSEQ_INJECT_ASM(3) \
+		extra_store \
+		"movq %[to_write_final], (%[target_final])\n\t" \
+		"2:\n\t" \
+		RSEQ_INJECT_ASM(5) \
+		"movq $0, (%[rseq_cs])\n\t" \
+		: /* no outputs */ \
+		: [start_event_counter]"r"((_start_value).event_counter), \
+		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
+		  [to_write_final]"r"(_to_write_final), \
+		  [target_final]"r"(_target_final), \
+		  [rseq_cs]"r"(&(_start_value).rseqp->rseq_cs) \
+		  extra_input \
+		  RSEQ_INJECT_INPUT \
+		: "memory", "cc" \
+		  RSEQ_INJECT_CLOBBER \
+		: _failure \
+	);
 
-#ifdef __x86_64__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".quad 1f, 2f, %l[failure], 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"movq $3b, (%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"cmpl %[start_event_counter], %[current_event_counter]\n\t"
-		"jnz %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"movq %[to_write], (%[target])\n\t"
-		"2:\n\t"
+#define RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"movq %[to_write_spec], (%[target_spec])\n\t" \
 		RSEQ_INJECT_ASM(4)
-		"movq $0, (%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"r"(p),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
-	);
+
+/* x86-64 is TSO */
+#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
+	RSEQ_FINISH2_SPECULATIVE_STORE_ASM()
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM(_target_spec, _to_write_spec) \
+		, [to_write_spec]"r"(_to_write_spec), \
+		[target_spec]"r"(_target_spec)
+
 #elif defined(__i386__)
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".long 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"movl $3b, (%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"cmpl %[start_event_counter], %[current_event_counter]\n\t"
-		"jnz %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"movl %[to_write], (%[target])\n\t"
-		"2:\n\t"
-		RSEQ_INJECT_ASM(4)
-		"movl $0, (%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"r"(p),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
+
+#define RSEQ_FINISH_ASM(_target_final, _to_write_final, _start_value, \
+		_failure, extra_store, extra_input) \
+	__asm__ __volatile__ goto ( \
+		".pushsection __rseq_table, \"aw\"\n\t" \
+		".balign 32\n\t" \
+		"3:\n\t" \
+		".long 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t" \
+		".popsection\n\t" \
+		"1:\n\t" \
+		RSEQ_INJECT_ASM(1) \
+		"movl $3b, (%[rseq_cs])\n\t" \
+		RSEQ_INJECT_ASM(2) \
+		"cmpl %[start_event_counter], %[current_event_counter]\n\t" \
+		"jnz %l[failure]\n\t" \
+		RSEQ_INJECT_ASM(3) \
+		extra_store \
+		"movl %[to_write_final], (%[target_final])\n\t" \
+		"2:\n\t" \
+		RSEQ_INJECT_ASM(5) \
+		"movl $0, (%[rseq_cs])\n\t" \
+		: /* no outputs */ \
+		: [start_event_counter]"r"((_start_value).event_counter), \
+		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
+		  [to_write_final]"r"(_to_write_final), \
+		  [target_final]"r"(_target_final), \
+		  [rseq_cs]"r"(&(_start_value).rseqp->rseq_cs) \
+		  extra_input \
+		  RSEQ_INJECT_INPUT \
+		: "memory", "cc" \
+		  RSEQ_INJECT_CLOBBER \
+		: _failure \
 	);
-#elif defined(__ARMEL__)
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		".word 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"adr r0, 3f\n\t"
-		"str r0, [%[rseq_cs]]\n\t"
-		RSEQ_INJECT_ASM(2)
-		"ldr r0, %[current_event_counter]\n\t"
-		"mov r1, #0\n\t"
-		"cmp %[start_event_counter], r0\n\t"
-		"bne %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"str %[to_write], [%[target]]\n\t"
-		"2:\n\t"
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"movl %[to_write_spec], (%[target_spec])\n\t" \
 		RSEQ_INJECT_ASM(4)
-		"str r1, [%[rseq_cs]]\n\t"
-		"b 4f\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".word 1b, 0x0, 2b, 0x0, l[failure], 0x0, 0x0, 0x0\n\t"
-		"4:\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"r"(p),
-		  [rseq_cs]"r"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r0", "r1", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
+
+#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
+		RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"lock; addl $0,0(%%esp)\n\t"
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM(_target_spec, _to_write_spec) \
+		, [to_write_spec]"r"(_to_write_spec), \
+		[target_spec]"r"(_target_spec)
+
+#elif defined(__ARMEL__)
+
+#define RSEQ_FINISH_ASM(_target_final, _to_write_final, _start_value, \
+		_failure, extra_store, extra_input) \
+	__asm__ __volatile__ goto ( \
+		".pushsection __rseq_table, \"aw\"\n\t" \
+		".balign 32\n\t" \
+		".word 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t" \
+		".popsection\n\t" \
+		"1:\n\t" \
+		RSEQ_INJECT_ASM(1) \
+		"adr r0, 3f\n\t" \
+		"str r0, [%[rseq_cs]]\n\t" \
+		RSEQ_INJECT_ASM(2) \
+		"ldr r0, %[current_event_counter]\n\t" \
+		"mov r1, #0\n\t" \
+		"cmp %[start_event_counter], r0\n\t" \
+		"bne %l[failure]\n\t" \
+		RSEQ_INJECT_ASM(3) \
+		extra_store \
+		"str %[to_write_final], [%[target_final]]\n\t" \
+		"2:\n\t" \
+		RSEQ_INJECT_ASM(5) \
+		"str r1, [%[rseq_cs]]\n\t" \
+		"b 4f\n\t" \
+		".balign 32\n\t" \
+		"3:\n\t" \
+		".word 1b, 0x0, 2b, 0x0, l[failure], 0x0, 0x0, 0x0\n\t" \
+		"4:\n\t" \
+		: /* no outputs */ \
+		: [start_event_counter]"r"((_start_value).event_counter), \
+		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
+		  [to_write_final]"r"(_to_write_final), \
+		  [target_final]"r"(_target_final), \
+		  [rseq_cs]"r"(&(_start_value).rseqp->rseq_cs) \
+		  extra_input \
+		  RSEQ_INJECT_INPUT \
+		: "r0", "r1", "memory", "cc" \
+		  RSEQ_INJECT_CLOBBER \
+		: _failure \
 	);
-#elif __PPC64__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		".quad 1f, 2f, %l[failure], 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"lis %%r17, (3b)@highest\n\t"
-		"ori %%r17, %%r17, (3b)@higher\n\t"
-		"rldicr %%r17, %%r17, 32, 31\n\t"
-		"oris %%r17, %%r17, (3b)@h\n\t"
-		"ori %%r17, %%r17, (3b)@l\n\t"
-		"std %%r17, 0(%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"lwz %%r17, %[current_event_counter]\n\t"
-		"cmpw cr7, %[start_event_counter], %%r17\n\t"
-		"bne- cr7, %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"std %[to_write], 0(%[target])\n\t"
-		"2:\n\t"
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"str %[to_write_spec], [%[target_spec]]\n\t" \
 		RSEQ_INJECT_ASM(4)
-		"li %%r17, 0\n\t"
-		"std %%r17, 0(%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"b"(p),
-		  [rseq_cs]"b"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r17", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
+
+#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
+		RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"dmb\n\t"
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM(_target_spec, _to_write_spec) \
+		, [to_write_spec]"r"(_to_write_spec), \
+		[target_spec]"r"(_target_spec)
+
+#elif __PPC64__
+
+#define RSEQ_FINISH_ASM(_target_final, _to_write_final, _start_value, \
+		_failure, extra_store, extra_input) \
+	__asm__ __volatile__ goto ( \
+		".pushsection __rseq_table, \"aw\"\n\t" \
+		".balign 32\n\t" \
+		"3:\n\t" \
+		".quad 1f, 2f, %l[failure], 0x0\n\t" \
+		".popsection\n\t" \
+		"1:\n\t" \
+		RSEQ_INJECT_ASM(1) \
+		"lis %%r17, (3b)@highest\n\t" \
+		"ori %%r17, %%r17, (3b)@higher\n\t" \
+		"rldicr %%r17, %%r17, 32, 31\n\t" \
+		"oris %%r17, %%r17, (3b)@h\n\t" \
+		"ori %%r17, %%r17, (3b)@l\n\t" \
+		"std %%r17, 0(%[rseq_cs])\n\t" \
+		RSEQ_INJECT_ASM(2) \
+		"lwz %%r17, %[current_event_counter]\n\t" \
+		"cmpw cr7, %[start_event_counter], %%r17\n\t" \
+		"bne- cr7, %l[failure]\n\t" \
+		RSEQ_INJECT_ASM(3) \
+		extra_store \
+		"std %[to_write_final], 0(%[target_final])\n\t" \
+		"2:\n\t" \
+		RSEQ_INJECT_ASM(5) \
+		"li %%r17, 0\n\t" \
+		"std %%r17, 0(%[rseq_cs])\n\t" \
+		: /* no outputs */ \
+		: [start_event_counter]"r"((_start_value).event_counter), \
+		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
+		  [to_write_final]"r"(_to_write_final), \
+		  [target_final]"b"(_target_final), \
+		  [rseq_cs]"b"(&(_start_value).rseqp->rseq_cs) \
+		  extra_input \
+		  RSEQ_INJECT_INPUT \
+		: "r17", "memory", "cc" \
+		  RSEQ_INJECT_CLOBBER \
+		: _failure \
 	);
-#elif __PPC__
-	__asm__ __volatile__ goto (
-		".pushsection __rseq_table, \"aw\"\n\t"
-		".balign 32\n\t"
-		"3:\n\t"
-		/* 32-bit only supported on BE */
-		".long 0x0, 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0\n\t"
-		".popsection\n\t"
-		"1:\n\t"
-		RSEQ_INJECT_ASM(1)
-		"lis %%r17, (3b)@ha\n\t"
-		"addi %%r17, %%r17, (3b)@l\n\t"
-		"stw %%r17, 0(%[rseq_cs])\n\t"
-		RSEQ_INJECT_ASM(2)
-		"lwz %%r17, %[current_event_counter]\n\t"
-		"cmpw cr7, %[start_event_counter], %%r17\n\t"
-		"bne- cr7, %l[failure]\n\t"
-		RSEQ_INJECT_ASM(3)
-		"stw %[to_write], 0(%[target])\n\t"
-		"2:\n\t"
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"std %[to_write_spec], 0(%[target_spec])\n\t" \
 		RSEQ_INJECT_ASM(4)
-		"li %%r17, 0\n\t"
-		"stw %%r17, 0(%[rseq_cs])\n\t"
-		: /* no outputs */
-		: [start_event_counter]"r"(start_value.event_counter),
-		  [current_event_counter]"m"(start_value.rseqp->u.e.event_counter),
-		  [to_write]"r"(to_write),
-		  [target]"b"(p),
-		  [rseq_cs]"b"(&start_value.rseqp->rseq_cs)
-		  RSEQ_INJECT_INPUT
-		: "r17", "memory", "cc"
-		  RSEQ_INJECT_CLOBBER
-		: failure
+
+#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
+		RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"lwsync\n\t"
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM(_target_spec, _to_write_spec) \
+		, [to_write_spec]"r"(_to_write_spec), \
+		[target_spec]"b"(_target_spec)
+
+#elif __PPC__
+
+#define RSEQ_FINISH_ASM(_target_final, _to_write_final, _start_value, \
+		_failure, extra_store, extra_input) \
+	__asm__ __volatile__ goto ( \
+		".pushsection __rseq_table, \"aw\"\n\t" \
+		".balign 32\n\t" \
+		"3:\n\t" \
+		/* 32-bit only supported on BE */ \
+		".long 0x0, 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0\n\t" \
+		".popsection\n\t" \
+		"1:\n\t" \
+		RSEQ_INJECT_ASM(1) \
+		"lis %%r17, (3b)@ha\n\t" \
+		"addi %%r17, %%r17, (3b)@l\n\t" \
+		"stw %%r17, 0(%[rseq_cs])\n\t" \
+		RSEQ_INJECT_ASM(2) \
+		"lwz %%r17, %[current_event_counter]\n\t" \
+		"cmpw cr7, %[start_event_counter], %%r17\n\t" \
+		"bne- cr7, %l[failure]\n\t" \
+		RSEQ_INJECT_ASM(3) \
+		extra_store \
+		"stw %[to_write_final], 0(%[target_final])\n\t" \
+		"2:\n\t" \
+		RSEQ_INJECT_ASM(5) \
+		"li %%r17, 0\n\t" \
+		"stw %%r17, 0(%[rseq_cs])\n\t" \
+		: /* no outputs */ \
+		: [start_event_counter]"r"((_start_value).event_counter), \
+		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
+		  [to_write_final]"r"(_to_write_final), \
+		  [target_final]"b"(_target_final), \
+		  [rseq_cs]"b"(&(_start_value).rseqp->rseq_cs) \
+		  extra_input \
+		  RSEQ_INJECT_INPUT \
+		: "r17", "memory", "cc" \
+		  RSEQ_INJECT_CLOBBER \
+		: _failure \
 	);
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"stw %[to_write_spec], 0(%[target_spec])\n\t" \
+		RSEQ_INJECT_ASM(4)
+
+#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
+		RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
+		"lwsync\n\t"
+
+#define RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM(_target_spec, _to_write_spec) \
+		, [to_write_spec]"r"(_to_write_spec), \
+		[target_spec]"b"(_target_spec)
+
 #else
 #error unsupported target
 #endif
+
+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(8)
+
+	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;
+	}
+
+	RSEQ_FINISH_ASM(p, to_write, start_value, failure, , );
+
+	return true;
+failure:
+	RSEQ_INJECT_FAILED
+	ACCESS_ONCE(start_value.rseqp->rseq_cs) = 0;
+	return false;
+}
+
+/*
+ * p_spec and to_write_spec are used for a speculative write attempted
+ * near the end of the restartable sequence. A rseq_finish2 may fail
+ * even after this write takes place.
+ *
+ * p_final and to_write_final are used for the final write. If this
+ * write takes place, the rseq_finish2 is guaranteed to succeed.
+ */
+static inline __attribute__((always_inline))
+bool rseq_finish2(struct rseq_lock *rlock,
+		intptr_t *p_spec, intptr_t to_write_spec,
+		intptr_t *p_final, intptr_t to_write_final,
+		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;
+	}
+
+	RSEQ_FINISH_ASM(p_final, to_write_final, start_value, failure,
+		RSEQ_FINISH2_SPECULATIVE_STORE_ASM(),
+		RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM(p_spec, to_write_spec)
+	);
+	return true;
+failure:
+	RSEQ_INJECT_FAILED
+	ACCESS_ONCE(start_value.rseqp->rseq_cs) = 0;
+	return false;
+}
+
+static inline __attribute__((always_inline))
+bool rseq_finish2_release(struct rseq_lock *rlock,
+		intptr_t *p_spec, intptr_t to_write_spec,
+		intptr_t *p_final, intptr_t to_write_final,
+		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;
+	}
+
+	RSEQ_FINISH_ASM(p_final, to_write_final, start_value, failure,
+		RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM(),
+		RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM(p_spec, to_write_spec)
+	);
 	return true;
 failure:
 	RSEQ_INJECT_FAILED

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

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

#1461564

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 12, 2016 at 06:11:45PM +0000, Mathieu Desnoyers wrote:
> ----- On Aug 12, 2016, at 12:35 PM, Boqun Feng boqun.feng@gmail.com wrote:
> 
> > On Fri, Aug 12, 2016 at 01:30:15PM +0800, Boqun Feng wrote:
> > [snip]
> >> > > Besides, do we allow userspace programs do read-only access to the
> >> > > memory objects modified by do_rseq(). If so, we have a problem when
> >> > > there are two writes in a do_rseq()(either in the rseq critical section
> >> > > or in the asm block), because in current implemetation, these two writes
> >> > > are unordered, which makes the readers outside a do_rseq() could observe
> >> > > the ordering of writes differently.
> >> > > 
> >> > > For rseq_finish2(), a simple solution would be making the "final" write
> >> > > a RELEASE.
> >> > 
> >> > Indeed, we would need a release semantic for the final store here if this
> >> > is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
> >> > add a rseq_finish2_release. We should find a way to eliminate code duplication
> >> 
> >> I'm in favor of a separate rseq_finish2_release().
> >> 
> >> > there. I suspect we'll end up doing macros.
> >> > 
> >> 
> >> Me too. Lemme have a try ;-)
> >> 
> > 
> > How about this? Although a little messy, I separated the asm block into
> > several parts and implemented each part in a arch-diagnose way.
> 
> I find it rather hard to follow the per-arch assembly with this approach.
> It might prove to be troublesome if we want to do arch-specific optimizations
> in the future.
> 

It might be, but I was just trying to kill as much duplicate code as
possible, because the more duplicate we have, the more maintain effort
we need.

For example, PPC32 and PPC64 may have the same asm code to check the
event counter, but different code to do the final store.  Having the
same RSEQ_CHECK_COUNTER() for PPC32 and PPC64 actually makes it easy if
we come up a way to optimize the counter check code on PPC.

And if some arch wants to have some very specifical optimizations,
it could always write the whole asm block again rather than use the
helpers macros.

> I've come up with the following macro approach instead, feedback welcome!
> 
> 
> commit 4d27431d6aefaee617540ef04518962b0e4d14f4
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date:   Thu Aug 11 19:11:27 2016 -0400
> 
>     rseq_finish2, rseq_finish2_release (WIP)
> 
[...]
> +#elif defined(__ARMEL__)
> +
> +#define RSEQ_FINISH_ASM(_target_final, _to_write_final, _start_value, \
> +		_failure, extra_store, extra_input) \
> +	__asm__ __volatile__ goto ( \
> +		".pushsection __rseq_table, \"aw\"\n\t" \
> +		".balign 32\n\t" \
> +		".word 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t" \
> +		".popsection\n\t" \
> +		"1:\n\t" \
> +		RSEQ_INJECT_ASM(1) \
> +		"adr r0, 3f\n\t" \
> +		"str r0, [%[rseq_cs]]\n\t" \
> +		RSEQ_INJECT_ASM(2) \
> +		"ldr r0, %[current_event_counter]\n\t" \
> +		"mov r1, #0\n\t" \
> +		"cmp %[start_event_counter], r0\n\t" \
> +		"bne %l[failure]\n\t" \
> +		RSEQ_INJECT_ASM(3) \
> +		extra_store \
> +		"str %[to_write_final], [%[target_final]]\n\t" \
> +		"2:\n\t" \
> +		RSEQ_INJECT_ASM(5) \
> +		"str r1, [%[rseq_cs]]\n\t" \

I find this is a little weird here, that is having an extra register for
zeroing the rseq_cs. Could we

		"mov r0, #0\n\t"
		"str r0, [%[rseq_cs]]\n\t"

here? Which not only saves a register, but also an instruction "mov r1,
#0" in the fast path. Am I missing something subtle?

> +		"b 4f\n\t" \
> +		".balign 32\n\t" \
> +		"3:\n\t" \
> +		".word 1b, 0x0, 2b, 0x0, l[failure], 0x0, 0x0, 0x0\n\t" \
> +		"4:\n\t" \
> +		: /* no outputs */ \
> +		: [start_event_counter]"r"((_start_value).event_counter), \
> +		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
> +		  [to_write_final]"r"(_to_write_final), \
> +		  [target_final]"r"(_target_final), \
> +		  [rseq_cs]"r"(&(_start_value).rseqp->rseq_cs) \
> +		  extra_input \
> +		  RSEQ_INJECT_INPUT \
> +		: "r0", "r1", "memory", "cc" \
> +		  RSEQ_INJECT_CLOBBER \
> +		: _failure \
>  	);

[...]

> +
> +#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
> +		RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
> +		"dmb\n\t"
> +

Having a RELEASE barrier here may be OK for all current archs we
support, but there are archs which rather than have a lightweight
RELEASE barrier but use a special instruction for RELEASE operations,
for example, AArch64. Do we need to take that into consideration and
define a RSEQ_FINISH_ASM_RELEASE() rather than a
RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM()?

[...]

Regards
Boqun

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

#1462146

----- On Aug 12, 2016, at 9:28 PM, Boqun Feng boqun.feng@gmail.com wrote:

> On Fri, Aug 12, 2016 at 06:11:45PM +0000, Mathieu Desnoyers wrote:
>> ----- On Aug 12, 2016, at 12:35 PM, Boqun Feng boqun.feng@gmail.com wrote:
>> 
>> > On Fri, Aug 12, 2016 at 01:30:15PM +0800, Boqun Feng wrote:
>> > [snip]
>> >> > > Besides, do we allow userspace programs do read-only access to the
>> >> > > memory objects modified by do_rseq(). If so, we have a problem when
>> >> > > there are two writes in a do_rseq()(either in the rseq critical section
>> >> > > or in the asm block), because in current implemetation, these two writes
>> >> > > are unordered, which makes the readers outside a do_rseq() could observe
>> >> > > the ordering of writes differently.
>> >> > > 
>> >> > > For rseq_finish2(), a simple solution would be making the "final" write
>> >> > > a RELEASE.
>> >> > 
>> >> > Indeed, we would need a release semantic for the final store here if this
>> >> > is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
>> >> > add a rseq_finish2_release. We should find a way to eliminate code duplication
>> >> 
>> >> I'm in favor of a separate rseq_finish2_release().
>> >> 
>> >> > there. I suspect we'll end up doing macros.
>> >> > 
>> >> 
>> >> Me too. Lemme have a try ;-)
>> >> 
>> > 
>> > How about this? Although a little messy, I separated the asm block into
>> > several parts and implemented each part in a arch-diagnose way.
>> 
>> I find it rather hard to follow the per-arch assembly with this approach.
>> It might prove to be troublesome if we want to do arch-specific optimizations
>> in the future.
>> 
> 
> It might be, but I was just trying to kill as much duplicate code as
> possible, because the more duplicate we have, the more maintain effort
> we need.
> 
> For example, PPC32 and PPC64 may have the same asm code to check the
> event counter, but different code to do the final store.  Having the
> same RSEQ_CHECK_COUNTER() for PPC32 and PPC64 actually makes it easy if
> we come up a way to optimize the counter check code on PPC.
> 
> And if some arch wants to have some very specifical optimizations,
> it could always write the whole asm block again rather than use the
> helpers macros.

Creating macros for each assembly "operation" done in the restartable
sequence ends up requiring that people learn a new custom mini-language,
and implement those macros for each architecture.

I'd rather prefer to let each architecture maintainer express the
restartable sequence directly in assembly, which is already known to
them, than require them to learn a new small macro-based language.

Eliminating duplicated code is a goal I agree with, but there are
ways to achieve this which don't end up creating a macro-based custom
mini-language (such as what I proposed below).

> 
>> I've come up with the following macro approach instead, feedback welcome!
>> 
>> 
>> commit 4d27431d6aefaee617540ef04518962b0e4d14f4
>> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Date:   Thu Aug 11 19:11:27 2016 -0400
>> 
>>     rseq_finish2, rseq_finish2_release (WIP)
>> 
> [...]
>> +#elif defined(__ARMEL__)
>> +
>> +#define RSEQ_FINISH_ASM(_target_final, _to_write_final, _start_value, \
>> +		_failure, extra_store, extra_input) \
>> +	__asm__ __volatile__ goto ( \
>> +		".pushsection __rseq_table, \"aw\"\n\t" \
>> +		".balign 32\n\t" \
>> +		".word 1f, 0x0, 2f, 0x0, %l[failure], 0x0, 0x0, 0x0\n\t" \
>> +		".popsection\n\t" \
>> +		"1:\n\t" \
>> +		RSEQ_INJECT_ASM(1) \
>> +		"adr r0, 3f\n\t" \
>> +		"str r0, [%[rseq_cs]]\n\t" \
>> +		RSEQ_INJECT_ASM(2) \
>> +		"ldr r0, %[current_event_counter]\n\t" \
>> +		"mov r1, #0\n\t" \
>> +		"cmp %[start_event_counter], r0\n\t" \
>> +		"bne %l[failure]\n\t" \
>> +		RSEQ_INJECT_ASM(3) \
>> +		extra_store \
>> +		"str %[to_write_final], [%[target_final]]\n\t" \
>> +		"2:\n\t" \
>> +		RSEQ_INJECT_ASM(5) \
>> +		"str r1, [%[rseq_cs]]\n\t" \
> 
> I find this is a little weird here, that is having an extra register for
> zeroing the rseq_cs. Could we
> 
>		"mov r0, #0\n\t"
>		"str r0, [%[rseq_cs]]\n\t"
> 
> here? Which not only saves a register, but also an instruction "mov r1,
> #0" in the fast path. Am I missing something subtle?

In terms of fast-path, you would be trading:

(1)
	"ldr r0, %[current_event_counter]\n\t" \
	"mov r1, #0\n\t"
	"cmp %[start_event_counter], r0\n\t" \
	"bne %l[failure]\n\t" \
	"str %[to_write_final], [%[target_final]]\n\t" \
	"2:\n\t" \
	"str r1, [%[rseq_cs]]\n\t" \
for

(2)
	"ldr r0, %[current_event_counter]\n\t" \
	"cmp %[start_event_counter], r0\n\t" \
	"bne %l[failure]\n\t" \
	"str %[to_write_final], [%[target_final]]\n\t" \
	"2:\n\t" \
	"mov r0, #0\n\t"
	"str r0, [%[rseq_cs]]\n\t" \

Your proposal (2) saves a register (does not clobber r1), but this
is at the expense of a slower fast-path. In (1), loading the constant
0 is done while the processor is stalled on the current_event_counter
load, which is needed by a following comparison. Therefore, we can
increase instruction-level parallelism by placing the immediate value
0 load right after the ldr instruction. This, however, requires that
we use a different register than r0, because r0 is already used by the
ldr/cmp instructions.

Since this is a fast-path, achieving higher instruction throughput
is more important than saving a register.

I came up with this as an optimization while doing benchmarking
on a ARM32 Cubietruck as a reference architecture.

> 
>> +		"b 4f\n\t" \
>> +		".balign 32\n\t" \
>> +		"3:\n\t" \
>> +		".word 1b, 0x0, 2b, 0x0, l[failure], 0x0, 0x0, 0x0\n\t" \
>> +		"4:\n\t" \
>> +		: /* no outputs */ \
>> +		: [start_event_counter]"r"((_start_value).event_counter), \
>> +		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
>> +		  [to_write_final]"r"(_to_write_final), \
>> +		  [target_final]"r"(_target_final), \
>> +		  [rseq_cs]"r"(&(_start_value).rseqp->rseq_cs) \
>> +		  extra_input \
>> +		  RSEQ_INJECT_INPUT \
>> +		: "r0", "r1", "memory", "cc" \
>> +		  RSEQ_INJECT_CLOBBER \
>> +		: _failure \
>>  	);
> 
> [...]
> 
>> +
>> +#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
>> +		RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
>> +		"dmb\n\t"
>> +
> 
> Having a RELEASE barrier here may be OK for all current archs we
> support, but there are archs which rather than have a lightweight
> RELEASE barrier but use a special instruction for RELEASE operations,
> for example, AArch64. Do we need to take that into consideration and
> define a RSEQ_FINISH_ASM_RELEASE() rather than a
> RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM()?

Good point. We should introduce the barrier before the final
store to fit this scenario. This would also work if we want to
do many speculative stores followed by a final store: it really
makes sense to put the barrier just before the final store rather
than after each speculative store.

I just pushed a commit in my dev branch implementing this. Testing
is welcome.

Thanks!

Mathieu

> 
> [...]
> 
> Regards
> Boqun

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

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

#1462516

[Multipart message — attachments visible in raw view] — view raw

On Sun, Aug 14, 2016 at 03:02:20PM +0000, Mathieu Desnoyers wrote:
> ----- On Aug 12, 2016, at 9:28 PM, Boqun Feng boqun.feng@gmail.com wrote:
> 
> > On Fri, Aug 12, 2016 at 06:11:45PM +0000, Mathieu Desnoyers wrote:
> >> ----- On Aug 12, 2016, at 12:35 PM, Boqun Feng boqun.feng@gmail.com wrote:
> >> 
> >> > On Fri, Aug 12, 2016 at 01:30:15PM +0800, Boqun Feng wrote:
> >> > [snip]
> >> >> > > Besides, do we allow userspace programs do read-only access to the
> >> >> > > memory objects modified by do_rseq(). If so, we have a problem when
> >> >> > > there are two writes in a do_rseq()(either in the rseq critical section
> >> >> > > or in the asm block), because in current implemetation, these two writes
> >> >> > > are unordered, which makes the readers outside a do_rseq() could observe
> >> >> > > the ordering of writes differently.
> >> >> > > 
> >> >> > > For rseq_finish2(), a simple solution would be making the "final" write
> >> >> > > a RELEASE.
> >> >> > 
> >> >> > Indeed, we would need a release semantic for the final store here if this
> >> >> > is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
> >> >> > add a rseq_finish2_release. We should find a way to eliminate code duplication
> >> >> 
> >> >> I'm in favor of a separate rseq_finish2_release().
> >> >> 
> >> >> > there. I suspect we'll end up doing macros.
> >> >> > 
> >> >> 
> >> >> Me too. Lemme have a try ;-)
> >> >> 
> >> > 
> >> > How about this? Although a little messy, I separated the asm block into
> >> > several parts and implemented each part in a arch-diagnose way.
> >> 
> >> I find it rather hard to follow the per-arch assembly with this approach.
> >> It might prove to be troublesome if we want to do arch-specific optimizations
> >> in the future.
> >> 
> > 
> > It might be, but I was just trying to kill as much duplicate code as
> > possible, because the more duplicate we have, the more maintain effort
> > we need.
> > 
> > For example, PPC32 and PPC64 may have the same asm code to check the
> > event counter, but different code to do the final store.  Having the
> > same RSEQ_CHECK_COUNTER() for PPC32 and PPC64 actually makes it easy if
> > we come up a way to optimize the counter check code on PPC.
> > 
> > And if some arch wants to have some very specifical optimizations,
> > it could always write the whole asm block again rather than use the
> > helpers macros.
> 
> Creating macros for each assembly "operation" done in the restartable
> sequence ends up requiring that people learn a new custom mini-language,
> and implement those macros for each architecture.
> 
> I'd rather prefer to let each architecture maintainer express the
> restartable sequence directly in assembly, which is already known to
> them, than require them to learn a new small macro-based language.
> 
> Eliminating duplicated code is a goal I agree with, but there are
> ways to achieve this which don't end up creating a macro-based custom
> mini-language (such as what I proposed below).
> 

Fair point ;-)

One more thing, do we want to use arch-specific header files to put
arch-specific assembly code? For example, rseq-x86.h, rseq-powerpc.h,
etc. This may save readers a lot of time if he or she is only interested
in a particular arch, and also make maintaining a little easier(no need
to worry about breaking other archs accidentally)

[...]
> 
> In terms of fast-path, you would be trading:
> 
> (1)
> 	"ldr r0, %[current_event_counter]\n\t" \
> 	"mov r1, #0\n\t"
> 	"cmp %[start_event_counter], r0\n\t" \
> 	"bne %l[failure]\n\t" \
> 	"str %[to_write_final], [%[target_final]]\n\t" \
> 	"2:\n\t" \
> 	"str r1, [%[rseq_cs]]\n\t" \
> for
> 
> (2)
> 	"ldr r0, %[current_event_counter]\n\t" \
> 	"cmp %[start_event_counter], r0\n\t" \
> 	"bne %l[failure]\n\t" \
> 	"str %[to_write_final], [%[target_final]]\n\t" \
> 	"2:\n\t" \
> 	"mov r0, #0\n\t"
> 	"str r0, [%[rseq_cs]]\n\t" \
> 
> Your proposal (2) saves a register (does not clobber r1), but this
> is at the expense of a slower fast-path. In (1), loading the constant
> 0 is done while the processor is stalled on the current_event_counter
> load, which is needed by a following comparison. Therefore, we can
> increase instruction-level parallelism by placing the immediate value
> 0 load right after the ldr instruction. This, however, requires that
> we use a different register than r0, because r0 is already used by the
> ldr/cmp instructions.
> 
> Since this is a fast-path, achieving higher instruction throughput
> is more important than saving a register.
> 
> I came up with this as an optimization while doing benchmarking
> on a ARM32 Cubietruck as a reference architecture.
> 

Nice ;-) Better to put a comment there?

I should try to investigate something similar for powerpc.

> > 
> >> +		"b 4f\n\t" \
> >> +		".balign 32\n\t" \
> >> +		"3:\n\t" \
> >> +		".word 1b, 0x0, 2b, 0x0, l[failure], 0x0, 0x0, 0x0\n\t" \
> >> +		"4:\n\t" \
> >> +		: /* no outputs */ \
> >> +		: [start_event_counter]"r"((_start_value).event_counter), \
> >> +		  [current_event_counter]"m"((_start_value).rseqp->u.e.event_counter), \
> >> +		  [to_write_final]"r"(_to_write_final), \
> >> +		  [target_final]"r"(_target_final), \
> >> +		  [rseq_cs]"r"(&(_start_value).rseqp->rseq_cs) \
> >> +		  extra_input \
> >> +		  RSEQ_INJECT_INPUT \
> >> +		: "r0", "r1", "memory", "cc" \
> >> +		  RSEQ_INJECT_CLOBBER \
> >> +		: _failure \
> >>  	);
> > 
> > [...]
> > 
> >> +
> >> +#define RSEQ_FINISH2_RELEASE_SPECULATIVE_STORE_ASM() \
> >> +		RSEQ_FINISH2_SPECULATIVE_STORE_ASM() \
> >> +		"dmb\n\t"
> >> +
> > 
> > Having a RELEASE barrier here may be OK for all current archs we
> > support, but there are archs which rather than have a lightweight
> > RELEASE barrier but use a special instruction for RELEASE operations,
> > for example, AArch64. Do we need to take that into consideration and
> > define a RSEQ_FINISH_ASM_RELEASE() rather than a
> > RSEQ_FINISH2_SPECULATIVE_STORE_INPUT_ASM()?
> 
> Good point. We should introduce the barrier before the final
> store to fit this scenario. This would also work if we want to
> do many speculative stores followed by a final store: it really
> makes sense to put the barrier just before the final store rather
> than after each speculative store.
> 
> I just pushed a commit in my dev branch implementing this. Testing
> is welcome.
> 

Sure, let me play around ;-)

Regards,
Boqun

> Thanks!
> 
> Mathieu
> 
> > 
> > [...]
> > 
> > Regards
> > Boqun
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

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

#1463060

----- On Aug 14, 2016, at 8:56 PM, Boqun Feng boqun.feng@gmail.com wrote:

> On Sun, Aug 14, 2016 at 03:02:20PM +0000, Mathieu Desnoyers wrote:
>> ----- On Aug 12, 2016, at 9:28 PM, Boqun Feng boqun.feng@gmail.com wrote:
>> 
>> > On Fri, Aug 12, 2016 at 06:11:45PM +0000, Mathieu Desnoyers wrote:
>> >> ----- On Aug 12, 2016, at 12:35 PM, Boqun Feng boqun.feng@gmail.com wrote:
>> >> 
>> >> > On Fri, Aug 12, 2016 at 01:30:15PM +0800, Boqun Feng wrote:
>> >> > [snip]
>> >> >> > > Besides, do we allow userspace programs do read-only access to the
>> >> >> > > memory objects modified by do_rseq(). If so, we have a problem when
>> >> >> > > there are two writes in a do_rseq()(either in the rseq critical section
>> >> >> > > or in the asm block), because in current implemetation, these two writes
>> >> >> > > are unordered, which makes the readers outside a do_rseq() could observe
>> >> >> > > the ordering of writes differently.
>> >> >> > > 
>> >> >> > > For rseq_finish2(), a simple solution would be making the "final" write
>> >> >> > > a RELEASE.
>> >> >> > 
>> >> >> > Indeed, we would need a release semantic for the final store here if this
>> >> >> > is the common use. Or we could duplicate the "flavors" of rseq_finish2 and
>> >> >> > add a rseq_finish2_release. We should find a way to eliminate code duplication
>> >> >> 
>> >> >> I'm in favor of a separate rseq_finish2_release().
>> >> >> 
>> >> >> > there. I suspect we'll end up doing macros.
>> >> >> > 
>> >> >> 
>> >> >> Me too. Lemme have a try ;-)
>> >> >> 
>> >> > 
>> >> > How about this? Although a little messy, I separated the asm block into
>> >> > several parts and implemented each part in a arch-diagnose way.
>> >> 
>> >> I find it rather hard to follow the per-arch assembly with this approach.
>> >> It might prove to be troublesome if we want to do arch-specific optimizations
>> >> in the future.
>> >> 
>> > 
>> > It might be, but I was just trying to kill as much duplicate code as
>> > possible, because the more duplicate we have, the more maintain effort
>> > we need.
>> > 
>> > For example, PPC32 and PPC64 may have the same asm code to check the
>> > event counter, but different code to do the final store.  Having the
>> > same RSEQ_CHECK_COUNTER() for PPC32 and PPC64 actually makes it easy if
>> > we come up a way to optimize the counter check code on PPC.
>> > 
>> > And if some arch wants to have some very specifical optimizations,
>> > it could always write the whole asm block again rather than use the
>> > helpers macros.
>> 
>> Creating macros for each assembly "operation" done in the restartable
>> sequence ends up requiring that people learn a new custom mini-language,
>> and implement those macros for each architecture.
>> 
>> I'd rather prefer to let each architecture maintainer express the
>> restartable sequence directly in assembly, which is already known to
>> them, than require them to learn a new small macro-based language.
>> 
>> Eliminating duplicated code is a goal I agree with, but there are
>> ways to achieve this which don't end up creating a macro-based custom
>> mini-language (such as what I proposed below).
>> 
> 
> Fair point ;-)
> 
> One more thing, do we want to use arch-specific header files to put
> arch-specific assembly code? For example, rseq-x86.h, rseq-powerpc.h,
> etc. This may save readers a lot of time if he or she is only interested
> in a particular arch, and also make maintaining a little easier(no need
> to worry about breaking other archs accidentally)
> 
> [...]

Good point. I wanted to wait until we had enough architectures before
doing this, but now that we have x86 32/64, ppc 32/64 and arm 32, it
appears to be the right time. Done and pushed.

>> 
>> In terms of fast-path, you would be trading:
>> 
>> (1)
>> 	"ldr r0, %[current_event_counter]\n\t" \
>> 	"mov r1, #0\n\t"
>> 	"cmp %[start_event_counter], r0\n\t" \
>> 	"bne %l[failure]\n\t" \
>> 	"str %[to_write_final], [%[target_final]]\n\t" \
>> 	"2:\n\t" \
>> 	"str r1, [%[rseq_cs]]\n\t" \
>> for
>> 
>> (2)
>> 	"ldr r0, %[current_event_counter]\n\t" \
>> 	"cmp %[start_event_counter], r0\n\t" \
>> 	"bne %l[failure]\n\t" \
>> 	"str %[to_write_final], [%[target_final]]\n\t" \
>> 	"2:\n\t" \
>> 	"mov r0, #0\n\t"
>> 	"str r0, [%[rseq_cs]]\n\t" \
>> 
>> Your proposal (2) saves a register (does not clobber r1), but this
>> is at the expense of a slower fast-path. In (1), loading the constant
>> 0 is done while the processor is stalled on the current_event_counter
>> load, which is needed by a following comparison. Therefore, we can
>> increase instruction-level parallelism by placing the immediate value
>> 0 load right after the ldr instruction. This, however, requires that
>> we use a different register than r0, because r0 is already used by the
>> ldr/cmp instructions.
>> 
>> Since this is a fast-path, achieving higher instruction throughput
>> is more important than saving a register.
>> 
>> I came up with this as an optimization while doing benchmarking
>> on a ARM32 Cubietruck as a reference architecture.
>> 
> 
> Nice ;-) Better to put a comment there?

Done.

> 
> I should try to investigate something similar for powerpc.
> 

Yes, you could try clobbering one extra register to move the
"li %%r17, 0\n\t" right after the lwz instruction. Depending on
the architecture characteristics, it may speed it up a bit. I
would expect that benchmarks on older architectures (e.g. old ppc32)
might be more affected by such tweak than newer POWER8.

Thanks,

Mathieu

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

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

#1461434

----- On Aug 11, 2016, at 7:26 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- 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.
> 
> I've added a commit implementing rseq_finish2() in my rseq volatile
> dev branch. You can fetch it at:
> 
> https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback
> 
> I also have a separate test and benchmark tree in addition to the
> kernel selftests here:
> 
> https://github.com/compudj/rseq-test
> 
> I named the first write a "speculative" write, and the second write
> the "final" write.
> 
> Would you like to extend the test cases to cover your intended use-case ?
> 

Hi Dave!

I just pushed a rseq_finish2() test in my rseq-fallback branch. It implements
a per-cpu buffer holding pointers, and pushes/pops items to/from it.

To use it:

cd tools/testing/selftests/rseq
./param_test -T b

(see -h for advanced usage)

Let me know if I got it right!

Thanks,

Mathieu

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

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

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

#1461444

>>>> 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.
>>
>> I've added a commit implementing rseq_finish2() in my rseq volatile
>> dev branch. You can fetch it at:
>>
>> https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback
>>
>> I also have a separate test and benchmark tree in addition to the
>> kernel selftests here:
>>
>> https://github.com/compudj/rseq-test
>>
>> I named the first write a "speculative" write, and the second write
>> the "final" write.
>>
>> Would you like to extend the test cases to cover your intended use-case ?
>>
>
>Hi Dave!
>
>I just pushed a rseq_finish2() test in my rseq-fallback branch. It implements
>a per-cpu buffer holding pointers, and pushes/pops items to/from it.
>
>To use it:
>
>cd tools/testing/selftests/rseq
>./param_test -T b
>
>(see -h for advanced usage)
>
>Let me know if I got it right!

Hi Mathieu,

Thanks, you beat me to it.    I commented on the github, that's pretty much it.  

> 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).

Sounds reasonable to me.  I agree it would be best to move the failure path 
out of the asm if possible.

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

#1462175

----- On Aug 12, 2016, at 4:05 PM, Dave Watson davejwatson@fb.com wrote:

>>>>> 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.
>>>
>>> I've added a commit implementing rseq_finish2() in my rseq volatile
>>> dev branch. You can fetch it at:
>>>
>>> https://github.com/compudj/linux-percpu-dev/tree/rseq-fallback
>>>
>>> I also have a separate test and benchmark tree in addition to the
>>> kernel selftests here:
>>>
>>> https://github.com/compudj/rseq-test
>>>
>>> I named the first write a "speculative" write, and the second write
>>> the "final" write.
>>>
>>> Would you like to extend the test cases to cover your intended use-case ?
>>>
>>
>>Hi Dave!
>>
>>I just pushed a rseq_finish2() test in my rseq-fallback branch. It implements
>>a per-cpu buffer holding pointers, and pushes/pops items to/from it.
>>
>>To use it:
>>
>>cd tools/testing/selftests/rseq
>>./param_test -T b
>>
>>(see -h for advanced usage)
>>
>>Let me know if I got it right!
> 

FYI, I have started implementing rseq_finish_memcpy() and rseq_finish_memcpy_release().
The idea is to perform an inline memcpy as speculative writes before the
final store (offset). I have pushed the work in progress in my dev branch.

This would be an alternative to rseq_finish2() (which I still consider very useful)
in cases where we want to push a sequence of bytes into a ring buffer before updating
the offset counter, without having to rely on memory allocation.

Feedback is welcome!

Thanks,

Mathieu


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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web