|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 12/30] xen/riscv: introduce cmpxchg.h
On Mon, 2024-02-26 at 15:20 +0100, Jan Beulich wrote:
> On 26.02.2024 13:58, Oleksii wrote:
> > On Mon, 2024-02-26 at 12:28 +0100, Jan Beulich wrote:
> > > On 26.02.2024 12:18, Oleksii wrote:
> > > > On Mon, 2024-02-26 at 10:45 +0100, Jan Beulich wrote:
> > > > > On 23.02.2024 13:23, Oleksii wrote:
> > > > > > >
> > > > > > > > > > As 1- and 2-byte cases are emulated I decided that
> > > > > > > > > > is
> > > > > > > > > > not
> > > > > > > > > > to
> > > > > > > > > > provide
> > > > > > > > > > sfx argument for emulation macros as it will not
> > > > > > > > > > have
> > > > > > > > > > to
> > > > > > > > > > much
> > > > > > > > > > affect on
> > > > > > > > > > emulated types and just consume more performance on
> > > > > > > > > > acquire
> > > > > > > > > > and
> > > > > > > > > > release
> > > > > > > > > > version of sc/ld instructions.
> > > > > > > > >
> > > > > > > > > Question is whether the common case (4- and 8-byte
> > > > > > > > > accesses)
> > > > > > > > > shouldn't
> > > > > > > > > be valued higher, with 1- and 2-byte emulation being
> > > > > > > > > there
> > > > > > > > > just
> > > > > > > > > to
> > > > > > > > > allow things to not break altogether.
> > > > > > > > If I understand you correctly, it would make sense to
> > > > > > > > add
> > > > > > > > the
> > > > > > > > 'sfx'
> > > > > > > > argument for the 1/2-byte access case, ensuring that
> > > > > > > > all
> > > > > > > > options
> > > > > > > > are
> > > > > > > > available for 1/2-byte access case as well.
> > > > > > >
> > > > > > > That's one of the possibilities. As said, I'm not overly
> > > > > > > worried
> > > > > > > about
> > > > > > > the emulated cases. For the initial implementation I'd
> > > > > > > recommend
> > > > > > > going
> > > > > > > with what is easiest there, yielding the best possible
> > > > > > > result
> > > > > > > for
> > > > > > > the
> > > > > > > 4- and 8-byte cases. If later it turns out repeated
> > > > > > > acquire/release
> > > > > > > accesses are a problem in the emulation loop, things can
> > > > > > > be
> > > > > > > changed
> > > > > > > to explicit barriers, without touching the 4- and 8-byte
> > > > > > > cases.
> > > > > > I am confused then a little bit if emulated case is not an
> > > > > > issue.
> > > > > >
> > > > > > For 4- and 8-byte cases for xchg .aqrl is used, for relaxed
> > > > > > and
> > > > > > aqcuire
> > > > > > version of xchg barries are used.
> > > > > >
> > > > > > The similar is done for cmpxchg.
> > > > > >
> > > > > > If something will be needed to change in emulation loop it
> > > > > > won't
> > > > > > require to change 4- and 8-byte cases.
> > > > >
> > > > > I'm afraid I don't understand your reply.
> > > > IIUC, emulated cases it is implemented correctly in terms of
> > > > usage
> > > > barriers. And it also OK not to use sfx for lr/sc instructions
> > > > and
> > > > use
> > > > only barriers.
> > > >
> > > > For 4- and 8-byte cases are used sfx + barrier depending on the
> > > > specific case ( relaxed, acquire, release, generic xchg/cmpxchg
> > > > ).
> > > > What also looks to me correct. But you suggested to provide the
> > > > best
> > > > possible result for 4- and 8-byte cases.
> > > >
> > > > So I don't understand what the best possible result is as the
> > > > current
> > > > one usage of __{cmp}xchg_generic for each specific case (
> > > > relaxed,
> > > > acquire, release, generic xchg/cmpxchg ) looks correct to me:
> > > > xchg -> (..., ".aqrl", "", "") just suffix .aqrl suffix without
> > > > barriers.
> > > > xchg_release -> (..., "", RISCV_RELEASE_BARRIER, "" ) use only
> > > > release
> > > > barrier
> > > > xchg_acquire -> (..., "", "", RISCV_ACQUIRE_BARRIER ), only
> > > > acquire
> > > > barrier
> > > > xchg_relaxed -> (..., "", "", "") - no barries, no sfx
> > >
> > > So first: While explicit barriers are technically okay, I don't
> > > follow why
> > > you insist on using them when you can achieve the same by
> > > suitably
> > > tagging
> > > the actual insn doing the exchange. Then second: It's somewhat
> > > hard
> > > for me
> > > to see the final effect on the emulation paths without you
> > > actually
> > > having
> > > done the switch. Maybe no special handling is necessary there
> > > anymore
> > > then. And as said, it may actually be acceptable for the
> > > emulation
> > > paths
> > > to "only" be correct, but not be ideal in terms of performance.
> > > After
> > > all,
> > > if you use the normal 4-byte primitive in there, more (non-
> > > explicit)
> > > barriers than needed would occur if the involved loop has to take
> > > more
> > > than one iteration. Which could (but imo doesn't need to be)
> > > avoided
> > > by
> > > using a more relaxed 4-byte primitive there and an explicit
> > > barrier
> > > outside of the loop.
> >
> > According to the spec:
> > Table A.5 ( part of the table only I copied here )
> >
> > Linux Construct RVWMO Mapping
> > atomic <op> relaxed amo<op>.{w|d}
> > atomic <op> acquire amo<op>.{w|d}.aq
> > atomic <op> release amo<op>.{w|d}.rl
> > atomic <op> amo<op>.{w|d}.aqrl
> >
> > Linux Construct RVWMO LR/SC Mapping
> > atomic <op> relaxed loop: lr.{w|d}; <op>; sc.{w|d}; bnez loop
> > atomic <op> acquire loop: lr.{w|d}.aq; <op>; sc.{w|d}; bnez
> > loop
> > atomic <op> release loop: lr.{w|d}; <op>; sc.{w|d}.aqrl∗ ;
> > bnez
> > loop OR
> > fence.tso; loop: lr.{w|d}; <op>;
> > sc.{w|d}∗ ;
> > bnez loop
> > atomic <op> loop: lr.{w|d}.aq; <op>; sc.{w|d}.aqrl;
> > bnez
> > loop
>
> In your consideration what to implement you will want to limit
> things to constructs we actually use. I can't find any use of the
> relaxed, acquire, or release forms of atomics as mentioned above.
>
> > The Linux mappings for release operations may seem stronger than
> > necessary, but these mappings
> > are needed to cover some cases in which Linux requires stronger
> > orderings than the more intuitive
> > mappings would provide. In particular, as of the time this text is
> > being written, Linux is actively
> > debating whether to require load-load, load-store, and store-store
> > orderings between accesses in one
> > critical section and accesses in a subsequent critical section in
> > the
> > same hart and protected by the
> > same synchronization object. Not all combinations of FENCE
> > RW,W/FENCE
> > R,RW mappings
> > with aq/rl mappings combine to provide such orderings. There are a
> > few
> > ways around this problem,
> > including:
> > 1. Always use FENCE RW,W/FENCE R,RW, and never use aq/rl. This
> > suffices
> > but is undesir-
> > able, as it defeats the purpose of the aq/rl modifiers.
> > 2. Always use aq/rl, and never use FENCE RW,W/FENCE R,RW. This does
> > not
> > currently work
> > due to the lack of load and store opcodes with aq and rl modifiers.
>
> I don't understand this point: Which specific load and/or store forms
> are missing? According to my reading of the A extension spec all
> combination of aq/rl exist with both lr and sc.
I think this is not about lr and sc instructions.
It is about l{b|h|w|d} and s{b|h|w|d}, which should be used with fence
in case of acquire and seq_cst.
>
> > 3. Strengthen the mappings of release operations such that they
> > would
> > enforce sufficient order-
> > ings in the presence of either type of acquire mapping. This is the
> > currently-recommended
> > solution, and the one shown in Table A.5.
> >
> >
> > Based on this it is enough in our case use only suffixed
> > istructions
> > (amo<op>.{w|d}{.aq, .rl, .aqrl, .aqrl }, lr.{w|d}.{.aq, .aqrl }.
> >
> >
> > But as far as I understand in Linux atomics were strengthen with
> > fences:
> > Atomics present the same issue with locking: release and
> > acquire
> > variants need to be strengthened to meet the constraints
> > defined
> > by the Linux-kernel memory consistency model [1].
> >
> > Atomics present a further issue: implementations of atomics
> > such
> > as atomic_cmpxchg() and atomic_add_unless() rely on LR/SC
> > pairs,
> > which do not give full-ordering with .aqrl; for example,
> > current
> > implementations allow the "lr-sc-aqrl-pair-vs-full-barrier"
> > test
> > below to end up with the state indicated in the "exists"
> > clause.
> >
> > In order to "synchronize" LKMM and RISC-V's implementation,
> > this
> > commit strengthens the implementations of the atomics
> > operations
> > by replacing .rl and .aq with the use of ("lightweigth")
> > fences,
> > and by replacing .aqrl LR/SC pairs in sequences such as:
> >
> > 0: lr.w.aqrl %0, %addr
> > bne %0, %old, 1f
> > ...
> > sc.w.aqrl %1, %new, %addr
> > bnez %1, 0b
> > 1:
> >
> > with sequences of the form:
> >
> > 0: lr.w %0, %addr
> > bne %0, %old, 1f
> > ...
> > sc.w.rl %1, %new, %addr /* SC-release */
> > bnez %1, 0b
> > fence rw, rw /* "full" fence */
> > 1:
> >
> > following Daniel's suggestion.
>
> I'm likely missing something, yet as it looks it does help that the
> code fragment above appears to be ...
>
> > These modifications were validated with simulation of the RISC-
> > V
> > with sequences of the form:
> >
> > 0: lr.w %0, %addr
> > bne %0, %old, 1f
> > ...
> > sc.w.rl %1, %new, %addr /* SC-release */
> > bnez %1, 0b
> > fence rw, rw /* "full" fence */
> > 1:
> >
> > following Daniel's suggestion.
>
> ... entirely the same as this one. Yet there's presumably a reason
> for quoting it twice?
I think it was done by accident
~ Oleksii
>
> > These modifications were validated with simulation of the RISC-
> > V
> > memory consistency model.
> >
> > C lr-sc-aqrl-pair-vs-full-barrier
> >
> > {}
> >
> > P0(int *x, int *y, atomic_t *u)
> > {
> > int r0;
> > int r1;
> >
> > WRITE_ONCE(*x, 1);
> > r0 = atomic_cmpxchg(u, 0, 1);
> > r1 = READ_ONCE(*y);
> > }
> >
> > P1(int *x, int *y, atomic_t *v)
> > {
> > int r0;
> > int r1;
> >
> > WRITE_ONCE(*y, 1);
> > r0 = atomic_cmpxchg(v, 0, 1);
> > r1 = READ_ONCE(*x);
> > }
> >
> > exists (u=1 /\ v=1 /\ 0:r1=0 /\ 1:r1=0)
> >
> > [1] https://marc.info/?l=linux-kernel&m=151930201102853&w=2
> >
> > https://groups.google.com/a/groups.riscv.org/forum/#!topic/isa-dev/hKywNHBkAXM
> > https://marc.info/?l=linux-kernel&m=151633436614259&w=2
> >
> >
> > Thereby Linux kernel implementation seems to me more safe and it is
> > a
> > reason why I want/wanted to be aligned with it.
>
> Which may end up being okay. I hope you realize though that there's a
> lot more explanation needed in the respective commits then compared
> to
> what you've had so far. As a minimum, absolutely anything remotely
> unexpected needs to be explained.
>
> Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |