Re: [PATCH v6 10/20] xen/riscv: introduce atomic.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-03-27 at 08:40 +0100, Jan Beulich wrote:
...

> > > > > > +/* This is required to provide a full barrier on success.
> > > > > > */
> > > > > > +static inline int atomic_add_unless(atomic_t *v, int a,
> > > > > > int u)
> > > > > > +{
> > > > > > +       int prev, rc;
> > > > > > +
> > > > > > +    asm volatile (
> > > > > > +        "0: lr.w     %[p],  %[c]\n"
> > > > > > +        "   beq      %[p],  %[u], 1f\n"
> > > > > > +        "   add      %[rc], %[p], %[a]\n"
> > > > > > +        "   sc.w.rl  %[rc], %[rc], %[c]\n"
> > > > > > +        "   bnez     %[rc], 0b\n"
> > > > > > +        RISCV_FULL_BARRIER
> > > > > 
> > > > > With this and no .aq on the load, why the .rl on the store?
> > > > It is something that LKMM requires [1].
> > > > 
> > > > This is not fully clear to me what is so specific in LKMM, but
> > > > accoring
> > > > to the spec:
> > > >    Ordering Annotation Fence-based Equivalent
> > > >    l{b|h|w|d|r}.aq     l{b|h|w|d|r}; fence r,rw
> > > >    l{b|h|w|d|r}.aqrl   fence rw,rw; l{b|h|w|d|r}; fence r,rw
> > > >    s{b|h|w|d|c}.rl     fence rw,w; s{b|h|w|d|c}
> > > >    s{b|h|w|d|c}.aqrl   fence rw,w; s{b|h|w|d|c}
> > > >    amo<op>.aq          amo<op>; fence r,rw
> > > >    amo<op>.rl          fence rw,w; amo<op>
> > > >    amo<op>.aqrl        fence rw,rw; amo<op>; fence rw,rw
> > > >    Table 2.2: Mappings from .aq and/or .rl to fence-based
> > > > equivalents.
> > > >    An alternative mapping places a fence rw,rw after the
> > > > existing 
> > > >    s{b|h|w|d|c} mapping rather than at the front of the
> > > >    l{b|h|w|d|r} mapping.
> > > 
> > > I'm afraid I can't spot the specific case in this table. None of
> > > the
> > > stores in the right column have a .rl suffix.
> > Yes, it is expected.
> > 
> > I am reading this table as (f.e.) amo<op>.rl is an equivalent of
> > fence
> > rw,w; amo<op>. (without .rl) 
> 
> In which case: How does quoting the table answer my original
> question?
Agree, it is starting to be confusing, so let me give an answer to your
question below.

> 
> > > >    It is also safe to translate any .aq, .rl, or .aqrl
> > > > annotation
> > > > into
> > > >    the fence-based snippets of
> > > >    Table 2.2. These can also be used as a legal implementation
> > > > of
> > > >    l{b|h|w|d} or s{b|h|w|d} pseu-
> > > >    doinstructions for as long as those instructions are not
> > > > added
> > > > to
> > > >    the ISA.
> > > > 
> > > > So according to the spec, it should be:
> > > >  sc.w ...
> > > >  RISCV_FULL_BARRIER.
> > > > 
> > > > Considering [1] and how this code looks before, it seems to me
> > > > that
> > > > it
> > > > is safe to use lr.w.aq/sc.w.rl here or an fence equivalent.
> > > 
> > > Here you say "or". Then why dos the code use sc.?.rl _and_ a
> > > fence?
> > I confused this line with amo<op>.aqrl, so based on the table 2.2
> > above
> > s{b|h|w|d|c}.aqrl is transformed to "fence rw,w; s{b|h|w|d|c}", but
> > Linux kernel decided to strengthen it with full barrier:
> >    -              "0:\n\t"
> >    -              "lr.w.aqrl  %[p],  %[c]\n\t"
> >    -              "beq        %[p],  %[u], 1f\n\t"
> >    -              "add       %[rc],  %[p], %[a]\n\t"
> >    -              "sc.w.aqrl %[rc], %[rc], %[c]\n\t"
> >    -              "bnez      %[rc], 0b\n\t"
> >    -              "1:"
> >    +               "0:     lr.w     %[p],  %[c]\n"
> >    +               "       beq      %[p],  %[u], 1f\n"
> >    +               "       add      %[rc], %[p], %[a]\n"
> >    +               "       sc.w.rl  %[rc], %[rc], %[c]\n"
> >    +               "       bnez     %[rc], 0b\n"
> >    +               "       fence    rw, rw\n"
> >    +               "1:\n"
> > As they have the following 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.
> 
> Is that really "current implementations", not "the abstract model"?
> If so, the use of an explicit fence would be more like a workaround
> (and would hence want commenting to that effect).
> 
> In neither case can I see my original question answered: Why the .rl
> on the store when there is a full fence later?
The good explanation for that was provided in the commit addressing a
similar issue for ARM64 [
https://patchwork.kernel.org/project/linux-arm-kernel/patch/1391516953-14541-1-git-send-email-will.deacon@xxxxxxx/
].
The same holds true for RISC-V since ARM also employs WMO.

I would also like to mention another point, as I indicated in another
email thread
[ https://lists.xen.org/archives/html/xen-devel/2024-03/msg01582.html ]
, that now this fence can be omitted and .aqrl can be used instead.

This was confirmed by Dan (the author of the RVWMO spec)
[https://lore.kernel.org/linux-riscv/41e01514-74ca-84f2-f5cc-2645c444fd8e@xxxxxxxxxx/
]

I hope this addresses your original question. Does it?

~ Oleksii