Page MenuHomeFreeBSD

riscv: fix errors in some atomic type aliases
ClosedPublic

Authored by mhorne on Mar 4 2021, 6:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 9:12 AM
Unknown Object (File)
Mon, Mar 11, 11:58 PM
Unknown Object (File)
Fri, Mar 8, 5:48 AM
Unknown Object (File)
Dec 22 2023, 7:13 AM
Unknown Object (File)
Dec 20 2023, 12:49 AM
Unknown Object (File)
Dec 12 2023, 12:41 PM
Unknown Object (File)
Nov 25 2023, 5:54 PM
Unknown Object (File)
Nov 11 2023, 8:21 PM

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mhorne requested review of this revision.Mar 4 2021, 6:04 PM
mhorne added reviewers: arichardson, jrtc27, jhb.
This comment was removed by emaste.

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.

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

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.

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.

Seems like a fairly obvious thing to fix, yes.

This revision is now accepted and ready to land.Mar 4 2021, 7:22 PM

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).

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)?

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).

This revision was automatically updated to reflect the committed changes.

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?

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.