riscv: Implement 8-bit and 16-bit atomic operations

[zishun.yi.dev_gmail.com]

Summary

The base RISC-V 'A' extension lacks sub-word AMO instructions. This
implements the missing 8/16-bit atomic operations (add, sub, set,
clear) using fcmpset-based CAS loops.

This serves as a software fallback until the Zabha extension becomes
widely available.

[mchoo]

Rationale looks good. (Side note: RVA23 profile says Zabha will be mandatory in future RVA profiles)

sys/riscv/include/atomic.h

138

do you mean load_acq

150

ditto

205

ditto

216

ditto

[zishun.yi.dev_gmail.com]

sys/riscv/include/atomic.h

138

No, I don't think it's needed here. The correctness is guaranteed by the subsequent CAS.
And see _atomic_subword.h.

[jrtc27]

Surely these belong in atomic_subword.h or similar? They're not at all specific to RISC-V, and other architectures lack various of these.

[jrtc27]

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

[zishun.yi.dev_gmail.com]

In D57379#1314780, @jrtc27 wrote:

Surely these belong in atomic_subword.h or similar? They're not at all specific to RISC-V, and other architectures lack various of these.

Yeah, I think maybe putting them in atomic_subword.h is a better choice. I will update tomorrow.

[zishun.yi.dev_gmail.com]

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Ah, I didn't actually consider the performance before.
I added these to simplify the KASAN porting. KASAN needs to intercept these defines, but riscv doesn't have these definitions, so adding them ensures the KASAN common code won't be polluted by arch-specific code.

[jrtc27]

In D57379#1314783, @zishun.yi.dev_gmail.com wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Ah, I didn't actually consider the performance before.
I added these to simplify the KASAN porting. KASAN needs to intercept these defines, but riscv doesn't have these definitions, so adding them ensures the KASAN common code won't be polluted by arch-specific code.

KASAN should just not intercept things that don't exist

[mchoo]

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Just curious: do we have trampolines for sub-atomic operations at instruction level? Having them for Zabha or LDADD* would be great optimization opportunity.

[jrtc27]

In D57379#1314785, @minsoochoo0122_proton.me wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Just curious: do we have trampolines for sub-atomic operations at instruction level? Having them for Zabha or LDADD* would be great optimization opportunity.

I don't understand what your question means

[mchoo]

In D57379#1314786, @jrtc27 wrote:

In D57379#1314785, @minsoochoo0122_proton.me wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Just curious: do we have trampolines for sub-atomic operations at instruction level? Having them for Zabha or LDADD* would be great optimization opportunity.

I don't understand what your question means

The code has todo for Zabha support. I guess this means there will be implementation in future

if (has_zabha) {
    AMOADD.{B,H};
} else {
    load and fcmpset;
}

Oh, I think I used the term "trampoline" incorrectly here. I meant branching based on detection of Zabha (or equivalent on other ISA).

Back to my original question: How can we achieve this "special path" so processors with Zabha don't have severe performance impact?

[jrtc27]

In D57379#1314791, @minsoochoo0122_proton.me wrote:

In D57379#1314786, @jrtc27 wrote:

In D57379#1314785, @minsoochoo0122_proton.me wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Just curious: do we have trampolines for sub-atomic operations at instruction level? Having them for Zabha or LDADD* would be great optimization opportunity.

I don't understand what your question means

The code has todo for Zabha support. I guess this means there will be implementation in future

if (has_zabha) {
    AMOADD.{B,H};
} else {
    load and fcmpset;
}

Oh, I think I used the term "trampoline" incorrectly here. I meant branching based on detection of Zabha (or equivalent on other ISA).

Back to my original question: How can we achieve this "special path" so processors with Zabha don't have severe performance impact?

Same as LSE on arm64

[mchoo]

In D57379#1314792, @jrtc27 wrote:

In D57379#1314791, @minsoochoo0122_proton.me wrote:

Back to my original question: How can we achieve this "special path" so processors with Zabha don't have severe performance impact?

Same as LSE on arm64

Found it. Thanks:)

[zishun.yi.dev_gmail.com]

In D57379#1314784, @jrtc27 wrote:

In D57379#1314783, @zishun.yi.dev_gmail.com wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Ah, I didn't actually consider the performance before.
I added these to simplify the KASAN porting. KASAN needs to intercept these defines, but riscv doesn't have these definitions, so adding them ensures the KASAN common code won't be polluted by arch-specific code.

KASAN should just not intercept things that don't exist

Fair enough. I should fix the KASAN side instead of adding these to riscv.
Thanks, I will do it tomorrow.

[mchoo]

sys/riscv/include/atomic.h

138

Oh wait, I somehow saw these as load/store. I apologize for that. You can mark them all as resolved.

[mchoo]

sys/riscv/include/atomic.h

138

(another mistake, ignore my comment above. I think I'm too tired today)

[zishun.yi.dev_gmail.com]

As suggested by @jrtc27, KASAN shouldn't intercept unsupported riscv atomic functions.

I've updated the KASAN code to handle this rather than adding them to riscv. These KASAN changes are now included in D57381, so this revision is being abandoned.