From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Boqun Feng <boqun.feng@gmail.com>
Cc: Dave Watson <davejwatson@fb.com>,
Andrew Morton <akpm@linux-foundation.org>,
Russell King <linux@arm.linux.org.uk>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
linux-api <linux-api@vger.kernel.org>,
Paul Turner <pjt@google.com>, Andrew Hunter <ahh@google.com>,
Peter Zijlstra <peterz@infradead.org>,
Andy Lutomirski <luto@amacapital.net>,
Andi Kleen <andi@firstfloor.org>, Chris Lameter <cl@linux.com>,
Ben Maurer <bmaurer@fb.com>, rostedt <rostedt@goodmis.org>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
Josh Triplett <josh@joshtriplett.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will.deacon@arm.com>,
Michael Kerrisk <mtk.manpages@gmail.com>
Subject: Re: [RFC PATCH v7 7/7] Restartable sequences: self-tests
Date: Fri, 12 Aug 2016 03:10:38 +0000 (UTC) [thread overview]
Message-ID: <365498072.8664.1470971438957.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <20160812012854.GC1740@tardis.cn.ibm.com>
----- 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
next prev parent reply other threads:[~2016-08-12 3:17 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-21 21:14 [RFC PATCH v7 0/7] Restartable sequences system call Mathieu Desnoyers
2016-07-21 21:14 ` [RFC PATCH v7 1/7] " Mathieu Desnoyers
2016-07-25 23:02 ` Andy Lutomirski
2016-07-26 3:02 ` Mathieu Desnoyers
2016-08-03 12:27 ` Peter Zijlstra
2016-08-03 16:37 ` Andy Lutomirski
2016-08-03 18:31 ` Christoph Lameter
2016-08-04 5:01 ` Andy Lutomirski
2016-08-04 4:27 ` Boqun Feng
2016-08-04 5:03 ` Andy Lutomirski
2016-08-09 16:13 ` Boqun Feng
2016-08-10 8:01 ` Andy Lutomirski
2016-08-10 17:40 ` Mathieu Desnoyers
2016-08-10 17:33 ` Mathieu Desnoyers
2016-08-11 4:54 ` Boqun Feng
2016-08-10 8:13 ` Andy Lutomirski
2016-08-03 18:29 ` Christoph Lameter
2016-08-10 16:47 ` Mathieu Desnoyers
2016-08-10 16:59 ` Christoph Lameter
2016-07-27 15:03 ` Boqun Feng
2016-07-27 15:05 ` [RFC 1/4] rseq/param_test: Convert test_data_entry::count to intptr_t Boqun Feng
2016-07-27 15:05 ` [RFC 2/4] Restartable sequences: powerpc architecture support Boqun Feng
2016-07-28 3:13 ` Mathieu Desnoyers
2016-07-27 15:05 ` [RFC 3/4] Restartable sequences: Wire up powerpc system call Boqun Feng
2016-07-28 3:13 ` Mathieu Desnoyers
2016-07-27 15:05 ` [RFC 4/4] Restartable sequences: Add self-tests for PPC Boqun Feng
2016-07-28 2:59 ` Mathieu Desnoyers
2016-07-28 4:43 ` Boqun Feng
2016-07-28 7:37 ` [RFC v2] " Boqun Feng
2016-07-28 14:04 ` Mathieu Desnoyers
2016-07-28 13:42 ` [RFC 4/4] " Mathieu Desnoyers
2016-07-28 3:07 ` [RFC 1/4] rseq/param_test: Convert test_data_entry::count to intptr_t Mathieu Desnoyers
2016-07-28 3:10 ` [RFC PATCH v7 1/7] Restartable sequences system call Mathieu Desnoyers
2016-08-03 13:19 ` Peter Zijlstra
2016-08-03 14:53 ` Paul E. McKenney
2016-08-03 15:45 ` Boqun Feng
2016-08-07 15:36 ` Mathieu Desnoyers
2016-08-07 23:35 ` Boqun Feng
2016-08-09 13:22 ` Mathieu Desnoyers
2016-08-09 20:06 ` Mathieu Desnoyers
2016-08-09 21:33 ` Peter Zijlstra
2016-08-09 22:41 ` Mathieu Desnoyers
2016-08-10 7:50 ` Peter Zijlstra
2016-08-10 13:26 ` Mathieu Desnoyers
2016-08-10 13:33 ` Peter Zijlstra
2016-08-10 14:04 ` Mathieu Desnoyers
2016-08-10 8:10 ` Andy Lutomirski
2016-08-10 19:04 ` Mathieu Desnoyers
2016-08-10 19:16 ` Andy Lutomirski
2016-08-10 20:06 ` Mathieu Desnoyers
2016-08-10 20:09 ` Andy Lutomirski
2016-08-10 21:01 ` Mathieu Desnoyers
2016-08-11 7:23 ` Andy Lutomirski
2016-08-10 8:43 ` Peter Zijlstra
2016-08-10 13:57 ` Mathieu Desnoyers
2016-08-10 14:28 ` Peter Zijlstra
2016-08-10 14:44 ` Mathieu Desnoyers
2016-08-10 13:29 ` Peter Zijlstra
2016-07-21 21:14 ` [RFC PATCH v7 2/7] tracing: instrument restartable sequences Mathieu Desnoyers
2016-07-21 21:14 ` [RFC PATCH v7 3/7] Restartable sequences: ARM 32 architecture support Mathieu Desnoyers
2016-07-21 21:14 ` [RFC PATCH v7 4/7] Restartable sequences: wire up ARM 32 system call Mathieu Desnoyers
2016-07-21 21:14 ` [RFC PATCH v7 5/7] Restartable sequences: x86 32/64 architecture support Mathieu Desnoyers
2016-07-21 21:14 ` [RFC PATCH v7 6/7] Restartable sequences: wire up x86 32/64 system call Mathieu Desnoyers
2016-07-21 21:14 ` [RFC PATCH v7 7/7] Restartable sequences: self-tests Mathieu Desnoyers
[not found] ` <CO1PR15MB09822FC140F84DCEEF2004CDDD0B0@CO1PR15MB0982.namprd15.prod.outlook.com>
2016-07-24 3:09 ` Mathieu Desnoyers
2016-07-24 18:01 ` Dave Watson
2016-07-25 16:43 ` Mathieu Desnoyers
2016-08-11 23:26 ` Mathieu Desnoyers
2016-08-12 1:28 ` Boqun Feng
2016-08-12 3:10 ` Mathieu Desnoyers [this message]
2016-08-12 3:13 ` Mathieu Desnoyers
2016-08-12 5:30 ` Boqun Feng
2016-08-12 16:35 ` Boqun Feng
2016-08-12 18:11 ` Mathieu Desnoyers
2016-08-13 1:28 ` Boqun Feng
2016-08-14 15:02 ` Mathieu Desnoyers
2016-08-15 0:56 ` Boqun Feng
2016-08-15 18:06 ` Mathieu Desnoyers
2016-08-12 19:36 ` Mathieu Desnoyers
2016-08-12 20:05 ` Dave Watson
2016-08-14 17:09 ` Mathieu Desnoyers
2016-07-25 18:12 ` Mathieu Desnoyers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=365498072.8664.1470971438957.JavaMail.zimbra@efficios.com \
--to=mathieu.desnoyers@efficios.com \
--cc=ahh@google.com \
--cc=akpm@linux-foundation.org \
--cc=andi@firstfloor.org \
--cc=bmaurer@fb.com \
--cc=boqun.feng@gmail.com \
--cc=catalin.marinas@arm.com \
--cc=cl@linux.com \
--cc=davejwatson@fb.com \
--cc=hpa@zytor.com \
--cc=josh@joshtriplett.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=luto@amacapital.net \
--cc=mingo@redhat.com \
--cc=mtk.manpages@gmail.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).