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

From: Boqun Feng
Date: Sun Aug 14 2016 - 20:56:55 EST


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@xxxxxxxxx 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@xxxxxxxxx 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

Attachment: signature.asc
Description: PGP signature