riscv: fix errors in some atomic type aliases

[mhorne]

Summary

This appears to be a copy-and-paste error that has simply been
overlooked. The tree contains only two calls to any of the affected
variants, but recent additions to the test suite started exercising the
call to atomic_clear_rel_int() in ng_leave_write(), reliably causing
panics.

Note that the particular test case ng_macfilter_test:main still appears
to fail on this platform, but this change reduces the panic to a
timeout.

PR: 253237

Test Plan

ngctl mkpeer vtnet0 no longer panics the machine.

I looked through atomic.h to see if there were any other errors of this kind, and it seemed like this was all. If someone else could give a second look-through, it would be helpful.

[emaste]

This comment was removed by emaste.

[arichardson]

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

[arichardson]

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

Hmm actually, atomic_clear_acq_long and atomic_clear_acq_ptr are still broken. See c90baf6817a0fa3a864c8213f46b15c8f49bf761

[mhorne]

In D29064#650982, @arichardson wrote:

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

Hmm actually, atomic_clear_acq_long and atomic_clear_acq_ptr are still broken. See c90baf6817a0fa3a864c8213f46b15c8f49bf761

In what way are they still broken? The diffs looks identical to me.

[arichardson]

In D29064#650993, @mhorne wrote:

In D29064#650982, @arichardson wrote:

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

Hmm actually, atomic_clear_acq_long and atomic_clear_acq_ptr are still broken. See c90baf6817a0fa3a864c8213f46b15c8f49bf761

In what way are they still broken? The diffs looks identical to me.

I must have looked at the current file instead of the version with this change. Please ignore.

[kp]

Seems like a fairly obvious thing to fix, yes.

[jrtc27]

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

The need for libatomic on some architectures in some cases is the only thing getting in the way of that (which might just be sub-word atomics for RISC-V when using GCC?) :(

Though if our compiler-rt provides them (for userspace consumers that haven't discovered C11) and we add the necessary implementations to the kernel for the affected architectures, perhaps that's fine? Yeah, you'll pay the cost of a function call, but if you want to avoid that then go get a better compiler (or fix the one you're using to not suck).

[arichardson]

In D29064#651014, @jrtc27 wrote:

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

The need for libatomic on some architectures in some cases is the only thing getting in the way of that (which might just be sub-word atomics for RISC-V when using GCC?) :(

Though if our compiler-rt provides them (for userspace consumers that haven't discovered C11) and we add the necessary implementations to the kernel for the affected architectures, perhaps that's fine? Yeah, you'll pay the cost of a function call, but if you want to avoid that then go get a better compiler (or fix the one you're using to not suck).

I think last time I checked, GCC generated the code inline for MIPS/PPC/ARM/AARCH64, but was missing support for RISC-V. I guess we could keep the sub-word inline asm for !defined(clang)?

[jrtc27]

In D29064#651016, @arichardson wrote:

In D29064#651014, @jrtc27 wrote:

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

The need for libatomic on some architectures in some cases is the only thing getting in the way of that (which might just be sub-word atomics for RISC-V when using GCC?) :(

Though if our compiler-rt provides them (for userspace consumers that haven't discovered C11) and we add the necessary implementations to the kernel for the affected architectures, perhaps that's fine? Yeah, you'll pay the cost of a function call, but if you want to avoid that then go get a better compiler (or fix the one you're using to not suck).

I think last time I checked, GCC generated the code inline for MIPS/PPC/ARM/AARCH64, but was missing support for RISC-V. I guess we could keep the sub-word inline asm for !defined(clang)?

That'd just be asking for it to bit-rot, but should improve performance slightly when building with Clang so maybe worth it. If it's just RISC-V/GCC that's the issue I have no qualms about adding the necessary assembly to support.S or similar and taking a slight performance hit for already-slow operations that you probably shouldn't be doing in a hot path anyway. There are various related issues with GCC's half-baked implementation tracked in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86005 (which I believe don't affect LLVM unless you mix non-RVA code with RVA code).

[mhorne]

In D29064#651022, @jrtc27 wrote:

In D29064#651016, @arichardson wrote:

In D29064#651014, @jrtc27 wrote:

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

The need for libatomic on some architectures in some cases is the only thing getting in the way of that (which might just be sub-word atomics for RISC-V when using GCC?) :(

Though if our compiler-rt provides them (for userspace consumers that haven't discovered C11) and we add the necessary implementations to the kernel for the affected architectures, perhaps that's fine? Yeah, you'll pay the cost of a function call, but if you want to avoid that then go get a better compiler (or fix the one you're using to not suck).

I think last time I checked, GCC generated the code inline for MIPS/PPC/ARM/AARCH64, but was missing support for RISC-V. I guess we could keep the sub-word inline asm for !defined(clang)?

That'd just be asking for it to bit-rot, but should improve performance slightly when building with Clang so maybe worth it. If it's just RISC-V/GCC that's the issue I have no qualms about adding the necessary assembly to support.S or similar and taking a slight performance hit for already-slow operations that you probably shouldn't be doing in a hot path anyway. There are various related issues with GCC's half-baked implementation tracked in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86005 (which I believe don't affect LLVM unless you mix non-RVA code with RVA code).

Can someone provide a summary of the issues surrounding GCC and libatomic? I'm having a hard time following this discussion.

Is there a reason that we don't/shouldn't ship libatomic with the devel/freebsd-gcc* toolchain ports?

[jrtc27]

In D29064#651072, @mhorne wrote:

In D29064#651022, @jrtc27 wrote:

In D29064#651016, @arichardson wrote:

In D29064#651014, @jrtc27 wrote:

In D29064#650980, @arichardson wrote:

Couldn't see anything else in this file, but maybe other architectures are affected? I'd really like it if we could have a single atomic.h that uses compiler intrinsics so there's only one file to scan for typos.

The need for libatomic on some architectures in some cases is the only thing getting in the way of that (which might just be sub-word atomics for RISC-V when using GCC?) :(

Though if our compiler-rt provides them (for userspace consumers that haven't discovered C11) and we add the necessary implementations to the kernel for the affected architectures, perhaps that's fine? Yeah, you'll pay the cost of a function call, but if you want to avoid that then go get a better compiler (or fix the one you're using to not suck).

I think last time I checked, GCC generated the code inline for MIPS/PPC/ARM/AARCH64, but was missing support for RISC-V. I guess we could keep the sub-word inline asm for !defined(clang)?

That'd just be asking for it to bit-rot, but should improve performance slightly when building with Clang so maybe worth it. If it's just RISC-V/GCC that's the issue I have no qualms about adding the necessary assembly to support.S or similar and taking a slight performance hit for already-slow operations that you probably shouldn't be doing in a hot path anyway. There are various related issues with GCC's half-baked implementation tracked in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86005 (which I believe don't affect LLVM unless you mix non-RVA code with RVA code).

Can someone provide a summary of the issues surrounding GCC and libatomic? I'm having a hard time following this discussion.

Is there a reason that we don't/shouldn't ship libatomic with the devel/freebsd-gcc* toolchain ports?

The summary is that, currently, if you use machine/atomic.h then you don't need libatomic, but if you change the implementation to use the intrinsics instead of assembly and do all of (a) compile with GCC (b) compile for RISC-V (c) perform sub-word atomics, then you will end up needing libatomic as GCC doesn't inline the implementation of sub-word atomics even despite the A extension being enabled. LLVM will just provide an inline implementation like any proper compiler should.