On 26.03.2024 20:02, Oleksii wrote:
> On Mon, 2024-03-25 at 09:18 +0100, Jan Beulich wrote:
>> On 22.03.2024 13:25, Oleksii wrote:
>>> On Thu, 2024-03-21 at 14:03 +0100, Jan Beulich wrote:
>>>> On 15.03.2024 19:06, Oleksii Kurochko wrote:
>>>>> + */
>>>>> +static always_inline void read_atomic_size(const volatile void
>>>>> *p,
>>>>> + void *res,
>>>>> + unsigned int size)
>>>>> +{
>>>>> + switch ( size )
>>>>> + {
>>>>> + case 1: *(uint8_t *)res = readb(p); break;
>>>>> + case 2: *(uint16_t *)res = readw(p); break;
>>>>> + case 4: *(uint32_t *)res = readl(p); break;
>>>>> + case 8: *(uint32_t *)res = readq(p); break;
>>>>
>>>> Nit: Excess blank before =.
>>>>
>>>> Also - no #ifdef here to be RV32-ready?
>>> Because there is #ifdef RV32 in io.h for readq().
>>
>> There you'd run into __raw_readq()'s BUILD_BUG_ON(), afaict even for
>> 1-, 2-, or 4-byte accesses. That's not quite what we want here.
> Do you mean that if someone will redefine readq() in another way and
> not wrap it by #ifdef RV32? Except this I am not sure that there is an
> issue as it will be still a compilation error, so anyway it will be
> needed to provide an implementation for __raw_readq().
No. BUILD_BUG_ON() is a compile-time thing. The compiler will encounter
this construct. And hence it will necessarily fail. Which is why the
other approach (causing a linker error) is used elsewhere. And we're
still only in the course of considering to utilize DCE for something
like STATIC_ASSERT_UNREACHABLE(); iirc there was something getting in
the way there.
> One of the reason why I decided to wrap with #ifdef RV32 in io.h to not
> go over the source code and add wrapping. Also for some code it will be
> needed to rewrite it. For example, I am not sure that I can add #ifdef
> inside macros, f.e.:
> #define write_atomic(p, x) \
> ({ \
> typeof(*(p)) x__ = (x); \
> switch ( sizeof(*(p)) ) \
> { \
> case 1: writeb(x__, p); break; \
> case 2: writew(x__, p); break; \
> case 4: writel(x__, p); break; \
> case 8: writeq(x__, p); break; \
> default: __bad_atomic_size(); break; \
> } \
> x__; \
> })
You can't add #ifdef there. Such needs abstracting differently.
But of course there's the option of simply not making any of these
constructs RV32-ready. Yet if so, that then will want doing
consistently.
>>>>> +/*
>>>>> + * First, the atomic ops that have no ordering constraints and
>>>>> therefor don't
>>>>> + * have the AQ or RL bits set. These don't return anything,
>>>>> so
>>>>> there's only
>>>>> + * one version to worry about.
>>>>> + */
>>>>> +#define ATOMIC_OP(op, asm_op, I, asm_type, c_type, prefix) \
>>>>> +static inline \
>>>>> +void atomic##prefix##_##op(c_type i, atomic##prefix##_t *v) \
>>>>> +{ \
>>>>> + asm volatile ( \
>>>>> + " amo" #asm_op "." #asm_type " zero, %1, %0" \
>>>>> + : "+A" (v->counter) \
>>>>> + : "r" (I) \
>>>>
>>>> Btw, I consider this pretty confusing. At the 1st and 2nd glance
>>>> this
>>>> looks like a mistake, i.e. as if i was meant. Imo ...
>>>>
>>>>> + : "memory" ); \
>>>>> +} \
>>>>> +
>>>>> +/*
>>>>> + * Only CONFIG_GENERIC_ATOMIC64=y was ported to Xen that is
>>>>> the
>>>>> reason why
>>>>> + * last argument for ATOMIC_OP isn't used.
>>>>> + */
>>>>> +#define ATOMIC_OPS(op, asm_op, I) \
>>>>> + ATOMIC_OP (op, asm_op, I, w, int, )
>>>>> +
>>>>> +ATOMIC_OPS(add, add, i)
>>>>> +ATOMIC_OPS(sub, add, -i)
>>>>> +ATOMIC_OPS(and, and, i)
>>>>> +ATOMIC_OPS( or, or, i)
>>>>> +ATOMIC_OPS(xor, xor, i)
>>>>
>>>> ... here you want to only pass the (unary) operator (and leaving
>>>> that
>>>> blank
>>>> is as fine as using +).
>>> I agree that a game with 'i' and 'I' looks confusing, but I am not
>>> really understand what is wrong with using ' i' here. It seems that
>>> preprocessed macros looks fine:
>>> static inline void atomic_add(int i, atomic_t *v) { asm volatile
>>> ( "
>>> amo" "add" "." "w" " zero, %1, %0" : "+A" (v->counter) : "r" (i)
>>> :
>>> "memory" ); }
>>>
>>> static inline void atomic_sub(int i, atomic_t *v) { asm volatile
>>> ( "
>>> amo" "add" "." "w" " zero, %1, %0" : "+A" (v->counter) : "r" (-
>>> i) :
>>> "memory" ); }
>>
>> I didn't question the pre-processed result being correct. Instead I
>> said
>> that I consider the construct confusing to the reader, for looking as
>> if
>> there was a mistake (in the case of the letter i used). Note also in
>> particular how the macro invocations need to be in sync with the
>> macro
>> implementation, for lower case i being used both in the macro and in
>> its
>> invocations. Anything parameterized would better be fully so, at the
>> very least to avoid, as said, confusion. (Having macros depend on
>> context at their use sites _may_ be okay for local helper macros, but
>> here we're talking about a not even private header file.)
> I am not sure then I understand how mentioning '+i' will help
> significantly remove confusion.
I'm afraid I don't understand: What "mentioning '+i'" are you referring
to? I'm pretty sure I didn't suggest that. What I suggested was to pass
a bare operator (- or +) as macro argument.
>>>>> +/* 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?
>>> 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?
Jan
