| Message ID | 20240612223025.1158537-3-boqun.feng@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] rust: Introduce atomic API helpers | expand |
On Wed, Jun 12, 2024 at 03:30:25PM -0700, Boqun Feng wrote: > --- /dev/null > +++ b/rust/kernel/sync/atomic/impl.rs > @@ -0,0 +1,1375 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Generated by scripts/atomic/gen-rust-atomic.sh > +//! DO NOT MODIFY THIS FILE DIRECTLY Again, why not generate at build time? thanks, greg k-h
On Wed, 12 Jun 2024 15:30:25 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: > Provide two atomic types: AtomicI32 and AtomicI64 with the existing > implemenation of C atomics. These atomics have the same semantics of the > corresponding LKMM C atomics, and using one memory (ordering) model > certainly reduces the reasoning difficulty and potential bugs from the > interaction of two different memory models. > > Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect > my responsiblity on these Rust APIs. > > Note that `Atomic*::new()`s are implemented vi open coding on struct > atomic*_t. This allows `new()` being a `const` function, so that it can > be used in constant contexts. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > MAINTAINERS | 4 +- > arch/arm64/kernel/cpufeature.c | 2 + > rust/kernel/sync.rs | 1 + > rust/kernel/sync/atomic.rs | 63 ++ > rust/kernel/sync/atomic/impl.rs | 1375 +++++++++++++++++++++++++++++ > scripts/atomic/gen-atomics.sh | 1 + > scripts/atomic/gen-rust-atomic.sh | 136 +++ > 7 files changed, 1581 insertions(+), 1 deletion(-) > create mode 100644 rust/kernel/sync/atomic.rs > create mode 100644 rust/kernel/sync/atomic/impl.rs > create mode 100755 scripts/atomic/gen-rust-atomic.sh > > diff --git a/MAINTAINERS b/MAINTAINERS > index d6c90161c7bf..a8528d27b260 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3458,7 +3458,7 @@ F: drivers/input/touchscreen/atmel_mxt_ts.c > ATOMIC INFRASTRUCTURE > M: Will Deacon <will@kernel.org> > M: Peter Zijlstra <peterz@infradead.org> > -R: Boqun Feng <boqun.feng@gmail.com> > +M: Boqun Feng <boqun.feng@gmail.com> > R: Mark Rutland <mark.rutland@arm.com> > L: linux-kernel@vger.kernel.org > S: Maintained > @@ -3467,6 +3467,8 @@ F: arch/*/include/asm/atomic*.h > F: include/*/atomic*.h > F: include/linux/refcount.h > F: scripts/atomic/ > +F: rust/kernel/sync/atomic.rs > +F: rust/kernel/sync/atomic/ > > ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER > M: Bradley Grove <linuxdrivers@attotech.com> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 48e7029f1054..99e6e2b2867f 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1601,6 +1601,8 @@ static bool > has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope) > { > u64 val = read_scoped_sysreg(entry, scope); > + if (entry->capability == ARM64_HAS_LSE_ATOMICS) > + return false; > return feature_matches(val, entry); > } > > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > index 0ab20975a3b5..66ac3752ca71 100644 > --- a/rust/kernel/sync.rs > +++ b/rust/kernel/sync.rs > @@ -8,6 +8,7 @@ > use crate::types::Opaque; > > mod arc; > +pub mod atomic; > mod condvar; > pub mod lock; > mod locked_by; > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs > new file mode 100644 > index 000000000000..b0f852cf1741 > --- /dev/null > +++ b/rust/kernel/sync/atomic.rs > @@ -0,0 +1,63 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Atomic primitives. > +//! > +//! These primitives have the same semantics as their C counterparts, for precise definitions of > +//! the semantics, please refer to tools/memory-model. Note that Linux Kernel Memory (Consistency) > +//! Model is the only model for Rust development in kernel right now, please avoid to use Rust's > +//! own atomics. > + > +use crate::bindings::{atomic64_t, atomic_t}; > +use crate::types::Opaque; > + > +mod r#impl; > + > +/// Atomic 32bit signed integers. > +pub struct AtomicI32(Opaque<atomic_t>); > + > +/// Atomic 64bit signed integers. > +pub struct AtomicI64(Opaque<atomic64_t>); Can we avoid two types and use a generic `Atomic<T>` and then implement on `Atomic<i32>` and `Atomic<i64>` instead? Like the recent generic NonZero in Rust standard library or the atomic crate (https://docs.rs/atomic/). I think this is better for ergonomics. The impl do need some extra casting though. Best, Gary
On Thu, Jun 13, 2024 at 02:44:32PM +0100, Gary Guo wrote: > On Wed, 12 Jun 2024 15:30:25 -0700 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > Provide two atomic types: AtomicI32 and AtomicI64 with the existing > > implemenation of C atomics. These atomics have the same semantics of the > > corresponding LKMM C atomics, and using one memory (ordering) model > > certainly reduces the reasoning difficulty and potential bugs from the > > interaction of two different memory models. > > > > Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect > > my responsiblity on these Rust APIs. > > > > Note that `Atomic*::new()`s are implemented vi open coding on struct > > atomic*_t. This allows `new()` being a `const` function, so that it can > > be used in constant contexts. > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > --- > > MAINTAINERS | 4 +- > > arch/arm64/kernel/cpufeature.c | 2 + > > rust/kernel/sync.rs | 1 + > > rust/kernel/sync/atomic.rs | 63 ++ > > rust/kernel/sync/atomic/impl.rs | 1375 +++++++++++++++++++++++++++++ > > scripts/atomic/gen-atomics.sh | 1 + > > scripts/atomic/gen-rust-atomic.sh | 136 +++ > > 7 files changed, 1581 insertions(+), 1 deletion(-) > > create mode 100644 rust/kernel/sync/atomic.rs > > create mode 100644 rust/kernel/sync/atomic/impl.rs > > create mode 100755 scripts/atomic/gen-rust-atomic.sh > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index d6c90161c7bf..a8528d27b260 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3458,7 +3458,7 @@ F: drivers/input/touchscreen/atmel_mxt_ts.c > > ATOMIC INFRASTRUCTURE > > M: Will Deacon <will@kernel.org> > > M: Peter Zijlstra <peterz@infradead.org> > > -R: Boqun Feng <boqun.feng@gmail.com> > > +M: Boqun Feng <boqun.feng@gmail.com> > > R: Mark Rutland <mark.rutland@arm.com> > > L: linux-kernel@vger.kernel.org > > S: Maintained > > @@ -3467,6 +3467,8 @@ F: arch/*/include/asm/atomic*.h > > F: include/*/atomic*.h > > F: include/linux/refcount.h > > F: scripts/atomic/ > > +F: rust/kernel/sync/atomic.rs > > +F: rust/kernel/sync/atomic/ > > > > ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER > > M: Bradley Grove <linuxdrivers@attotech.com> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 48e7029f1054..99e6e2b2867f 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -1601,6 +1601,8 @@ static bool > > has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope) > > { > > u64 val = read_scoped_sysreg(entry, scope); > > + if (entry->capability == ARM64_HAS_LSE_ATOMICS) > > + return false; > > return feature_matches(val, entry); > > } > > > > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > > index 0ab20975a3b5..66ac3752ca71 100644 > > --- a/rust/kernel/sync.rs > > +++ b/rust/kernel/sync.rs > > @@ -8,6 +8,7 @@ > > use crate::types::Opaque; > > > > mod arc; > > +pub mod atomic; > > mod condvar; > > pub mod lock; > > mod locked_by; > > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs > > new file mode 100644 > > index 000000000000..b0f852cf1741 > > --- /dev/null > > +++ b/rust/kernel/sync/atomic.rs > > @@ -0,0 +1,63 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Atomic primitives. > > +//! > > +//! These primitives have the same semantics as their C counterparts, for precise definitions of > > +//! the semantics, please refer to tools/memory-model. Note that Linux Kernel Memory (Consistency) > > +//! Model is the only model for Rust development in kernel right now, please avoid to use Rust's > > +//! own atomics. > > + > > +use crate::bindings::{atomic64_t, atomic_t}; > > +use crate::types::Opaque; > > + > > +mod r#impl; > > + > > +/// Atomic 32bit signed integers. > > +pub struct AtomicI32(Opaque<atomic_t>); > > + > > +/// Atomic 64bit signed integers. > > +pub struct AtomicI64(Opaque<atomic64_t>); > > > Can we avoid two types and use a generic `Atomic<T>` and then implement > on `Atomic<i32>` and `Atomic<i64>` instead? Like the recent > generic NonZero in Rust standard library or the atomic crate > (https://docs.rs/atomic/). > We can always add a layer on top of what we have here to provide the generic `Atomic<T>`. However, I personally don't think generic `Atomic<T>` is a good idea, for a few reasons: * I'm not sure it will bring benefits to users, the current atomic users in kernel are pretty specific on the size of atomic they use, so they want to directly use AtomicI32 or AtomicI64 in their type definitions rather than use a `Atomic<T>` where their users can provide type later. * I can also see the future where we have different APIs on different types of atomics, for example, we could have a: impl AtomicI64 { pub fn split(&self) -> (&AtomicI32, &AtomicI32) } which doesn't exist for AtomicI32. Note this is not a UB because we write our atomic implementation in asm, so it's perfectly fine for mix-sized atomics. So let's start with some basic and simple until we really have a need for generic `Atomic<T>`. Thoughts? Regards, Boqun > I think this is better for ergonomics. The impl do need some extra > casting though. > > Best, > Gary
On Thu, 13 Jun 2024 09:30:26 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: > > > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs > > > new file mode 100644 > > > index 000000000000..b0f852cf1741 > > > --- /dev/null > > > +++ b/rust/kernel/sync/atomic.rs > > > @@ -0,0 +1,63 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +//! Atomic primitives. > > > +//! > > > +//! These primitives have the same semantics as their C counterparts, for precise definitions of > > > +//! the semantics, please refer to tools/memory-model. Note that Linux Kernel Memory (Consistency) > > > +//! Model is the only model for Rust development in kernel right now, please avoid to use Rust's > > > +//! own atomics. > > > + > > > +use crate::bindings::{atomic64_t, atomic_t}; > > > +use crate::types::Opaque; > > > + > > > +mod r#impl; > > > + > > > +/// Atomic 32bit signed integers. > > > +pub struct AtomicI32(Opaque<atomic_t>); > > > + > > > +/// Atomic 64bit signed integers. > > > +pub struct AtomicI64(Opaque<atomic64_t>); > > > > > > Can we avoid two types and use a generic `Atomic<T>` and then implement > > on `Atomic<i32>` and `Atomic<i64>` instead? Like the recent > > generic NonZero in Rust standard library or the atomic crate > > (https://docs.rs/atomic/). > > > > We can always add a layer on top of what we have here to provide the > generic `Atomic<T>`. However, I personally don't think generic > `Atomic<T>` is a good idea, for a few reasons: > > * I'm not sure it will bring benefits to users, the current atomic > users in kernel are pretty specific on the size of atomic they > use, so they want to directly use AtomicI32 or AtomicI64 in > their type definitions rather than use a `Atomic<T>` where their > users can provide type later. You can write `Atomic<i32>` and `Atomic<i64>`. > > * I can also see the future where we have different APIs on > different types of atomics, for example, we could have a: > > impl AtomicI64 { > pub fn split(&self) -> (&AtomicI32, &AtomicI32) > } > > which doesn't exist for AtomicI32. Note this is not a UB because > we write our atomic implementation in asm, so it's perfectly > fine for mix-sized atomics. You can still have impl Atomic<i64> { pub fn split(&self) -> (&Atomic<i32>, &Atomic<i32>) } I see `Atomic<i32>/Atomic<i64>` being generally more flexible than `AtomicI32/AtomicI64`, without very little downsides. We may not have generic users but I think it doesn't do any harm to have it in a form that makes future generics easy. Best, Gary
On Thu, Jun 13, 2024 at 6:31 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > So let's start with some basic and simple until we really have a need > for generic `Atomic<T>`. Thoughts? I don't want to delay this, but using generics would be more flexible, right? e.g. it could allow us to have atomics of, say, newtypes, if that were to be useful. Is there a particular disadvantage of using the generics? The two cases you mentioned would just be written explicitly, right? One disadvantage would be that they are different from the Rust standard library ones, e.g. in case we wanted third-party code to use them, but could be provided if needed later on. Cheers, Miguel
On Thu, Jun 13, 2024 at 07:22:54PM +0200, Miguel Ojeda wrote: > On Thu, Jun 13, 2024 at 6:31 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > So let's start with some basic and simple until we really have a need > > for generic `Atomic<T>`. Thoughts? > > I don't want to delay this, but using generics would be more flexible, > right? e.g. it could allow us to have atomics of, say, newtypes, if > that were to be useful. > > Is there a particular disadvantage of using the generics? The two > cases you mentioned would just be written explicitly, right? > > One disadvantage would be that they are different from the Rust > standard library ones, e.g. in case we wanted third-party code to use > them, but could be provided if needed later on. > Well, the other thing is AtomicI32 -> atomic_t and AtomicI64 -> atomic64_t are perfect mappings, and we can treat AtomicI32 and AtomicI64 as a separate layer that wires C atomics into Rust. As I said, we can build `Atomic<T>` on top of this layer, like: Atomic<T> | V AtomicI{32,64} | V atomic{,64}_t and if we drop this layer, the dependencies become: Atomic<i32,i64> <- Atomic<u32,u64> | V atomic{,64}_t i.e. in the same layer of Atomic<T>, some of them directly depend on some other Atomic<T> types, which doesn't look very clean to me. And it might be difficult for architecture maintainers to track the exact dependency for Rust code. This is also the reason why I didn't use Rust macros to generate AtomicI32 and AtomicI64 implementation: I use a script to generate .rs file. This ensures AtomicI32 and AtomicI64 stay with the exact same set of APIs as atomic{,64}_t (described by scripts/atomic/atomics.tbl. Put it in another way, I guess you can think AtomicI32 and AtomicI64 as some sort of intrinsic layer provided by C. And should we need it, we can build an Atomic<T> layer on top of it. Does this make sense? Regards, Boqun > Cheers, > Miguel
On Wed, Jun 12, 2024 at 03:30:25PM -0700, Boqun Feng wrote: [...] > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 48e7029f1054..99e6e2b2867f 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1601,6 +1601,8 @@ static bool > has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope) > { > u64 val = read_scoped_sysreg(entry, scope); > + if (entry->capability == ARM64_HAS_LSE_ATOMICS) > + return false; > return feature_matches(val, entry); > } > Yeah, this part was mis-committed, will remove it in the next version. Regards, Boqun [...]
On Thu, Jun 13, 2024 at 09:30:26AM -0700, Boqun Feng wrote: > We can always add a layer on top of what we have here to provide the > generic `Atomic<T>`. However, I personally don't think generic > `Atomic<T>` is a good idea, for a few reasons: > > * I'm not sure it will bring benefits to users, the current atomic > users in kernel are pretty specific on the size of atomic they > use, so they want to directly use AtomicI32 or AtomicI64 in > their type definitions rather than use a `Atomic<T>` where their > users can provide type later. > > * I can also see the future where we have different APIs on > different types of atomics, for example, we could have a: > > impl AtomicI64 { > pub fn split(&self) -> (&AtomicI32, &AtomicI32) > } > > which doesn't exist for AtomicI32. Note this is not a UB because > we write our atomic implementation in asm, so it's perfectly > fine for mix-sized atomics. > > So let's start with some basic and simple until we really have a need > for generic `Atomic<T>`. Thoughts? Not on the generic thing, but on the lack of long. atomic_long_t is often used when we have pointers with extra bits on. Then you want a number type in order to be able to manipulate the low bits.
On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > Does this make sense? Implementation-wise, if you think it is simpler or more clear/elegant to have the extra lower level layer, then that sounds fine. However, I was mainly talking about what we would eventually expose to users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, then we could make the lower layer private already. We can defer that extra layer/work if needed even if we go for `Atomic<T>`, but it would be nice to understand if we have consensus for an eventual user-facing API, or if someone has any other opinion or concerns on one vs. the other. Cheers, Miguel
On Wed, Jun 12, 2024 at 03:30:25PM -0700, Boqun Feng wrote: > Provide two atomic types: AtomicI32 and AtomicI64 with the existing > implemenation of C atomics. These atomics have the same semantics of the > corresponding LKMM C atomics, and using one memory (ordering) model > certainly reduces the reasoning difficulty and potential bugs from the > interaction of two different memory models. > > Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect > my responsiblity on these Rust APIs. > > Note that `Atomic*::new()`s are implemented vi open coding on struct > atomic*_t. This allows `new()` being a `const` function, so that it can > be used in constant contexts. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> I have a few minor comments below. > --- > MAINTAINERS | 4 +- > arch/arm64/kernel/cpufeature.c | 2 + > rust/kernel/sync.rs | 1 + > rust/kernel/sync/atomic.rs | 63 ++ > rust/kernel/sync/atomic/impl.rs | 1375 +++++++++++++++++++++++++++++ > scripts/atomic/gen-atomics.sh | 1 + > scripts/atomic/gen-rust-atomic.sh | 136 +++ > 7 files changed, 1581 insertions(+), 1 deletion(-) > create mode 100644 rust/kernel/sync/atomic.rs > create mode 100644 rust/kernel/sync/atomic/impl.rs > create mode 100755 scripts/atomic/gen-rust-atomic.sh > > diff --git a/MAINTAINERS b/MAINTAINERS > index d6c90161c7bf..a8528d27b260 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3458,7 +3458,7 @@ F: drivers/input/touchscreen/atmel_mxt_ts.c > ATOMIC INFRASTRUCTURE > M: Will Deacon <will@kernel.org> > M: Peter Zijlstra <peterz@infradead.org> > -R: Boqun Feng <boqun.feng@gmail.com> > +M: Boqun Feng <boqun.feng@gmail.com> > R: Mark Rutland <mark.rutland@arm.com> > L: linux-kernel@vger.kernel.org > S: Maintained > @@ -3467,6 +3467,8 @@ F: arch/*/include/asm/atomic*.h > F: include/*/atomic*.h > F: include/linux/refcount.h > F: scripts/atomic/ > +F: rust/kernel/sync/atomic.rs > +F: rust/kernel/sync/atomic/ > > ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER > M: Bradley Grove <linuxdrivers@attotech.com> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 48e7029f1054..99e6e2b2867f 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1601,6 +1601,8 @@ static bool > has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope) > { > u64 val = read_scoped_sysreg(entry, scope); > + if (entry->capability == ARM64_HAS_LSE_ATOMICS) > + return false; > return feature_matches(val, entry); > } As per other replies, this'll obviously need to go. > diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs > new file mode 100644 > index 000000000000..b0f852cf1741 > --- /dev/null > +++ b/rust/kernel/sync/atomic.rs > @@ -0,0 +1,63 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Atomic primitives. > +//! > +//! These primitives have the same semantics as their C counterparts, for precise definitions of > +//! the semantics, please refer to tools/memory-model. Note that Linux Kernel Memory (Consistency) > +//! Model is the only model for Rust development in kernel right now, please avoid to use Rust's > +//! own atomics. > + > +use crate::bindings::{atomic64_t, atomic_t}; As with the last patch, why no atomic_long_t? [...] > +#gen_proto_order_variant(meta, pfx, name, sfx, order, atomic, ty, int, raw, arg...) > +gen_proto_order_variant() > +{ > + local meta="$1"; shift > + local pfx="$1"; shift > + local name="$1"; shift > + local sfx="$1"; shift > + local order="$1"; shift > + local atomic="$1"; shift > + local ty="$1"; shift > + local int="$1"; shift > + local raw="$1"; shift > + > + local fn_name="${raw}${pfx}${name}${sfx}${order}" > + local atomicname="${raw}${atomic}_${pfx}${name}${sfx}${order}" > + > + local ret="$(gen_ret_type "${meta}" "${int}")" > + local params="$(gen_params "${int}" $@)" > + local args="$(gen_args "$@")" > + local retstmt="$(gen_ret_stmt "${meta}")" > + > +cat <<EOF > + /// See \`${atomicname}\`. > + #[inline(always)] > + pub fn ${fn_name}(&self${params}) ${ret}{ > + // SAFETY:\`self.0.get()\` is a valid pointer. > + unsafe { > + ${retstmt}${atomicname}(${args}); > + } > + } > +EOF > +} AFAICT the 'ty' argument (AtomicI32/AtomicI64) isn't used and can be removed. Likewise for 'raw'. > + > +cat << EOF > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Generated by $0 > +//! DO NOT MODIFY THIS FILE DIRECTLY > + > +use super::*; > +use crate::bindings::*; > + > +impl AtomicI32 { > +EOF > + > +grep '^[a-z]' "$1" | while read name meta args; do > + gen_proto "${meta}" "${name}" "atomic" "AtomicI32" "i32" "" ${args} With 'ty' and 'raw' gone, this'd be: gen_proto "${meta}" "${name}" "atomic" "i32" ${args} > +done > + > +cat << EOF > +} > + > +impl AtomicI64 { > +EOF > + > +grep '^[a-z]' "$1" | while read name meta args; do > + gen_proto "${meta}" "${name}" "atomic64" "AtomicI64" "i64" "" ${args} With 'ty' and 'raw' gone, this'd be: gen_proto "${meta}" "${name}" "atomic64" "i64" ${args} Mark. > +done > + > +cat << EOF > +} > + > +EOF > -- > 2.45.2 >
On Fri, Jun 14, 2024 at 11:51:24AM +0200, Peter Zijlstra wrote: > On Thu, Jun 13, 2024 at 09:30:26AM -0700, Boqun Feng wrote: > > > We can always add a layer on top of what we have here to provide the > > generic `Atomic<T>`. However, I personally don't think generic > > `Atomic<T>` is a good idea, for a few reasons: > > > > * I'm not sure it will bring benefits to users, the current atomic > > users in kernel are pretty specific on the size of atomic they > > use, so they want to directly use AtomicI32 or AtomicI64 in > > their type definitions rather than use a `Atomic<T>` where their > > users can provide type later. > > > > * I can also see the future where we have different APIs on > > different types of atomics, for example, we could have a: > > > > impl AtomicI64 { > > pub fn split(&self) -> (&AtomicI32, &AtomicI32) > > } > > > > which doesn't exist for AtomicI32. Note this is not a UB because > > we write our atomic implementation in asm, so it's perfectly > > fine for mix-sized atomics. > > > > So let's start with some basic and simple until we really have a need > > for generic `Atomic<T>`. Thoughts? > > Not on the generic thing, but on the lack of long. atomic_long_t is > often used when we have pointers with extra bits on. Then you want a > number type in order to be able to manipulate the low bits. I mentioned my plan on AtomicPtr, but I think I should have clarified this more. My plan is: pub struct AtomicIsize { #[cfg(CONFIG_64BIT)] inner: AtomicI64 #[cfg(not(CONFIG_64BIT))] inner: AtomicI32 } i.e. building AtomicIsize (Rust's atomic_long_t) based on AtomicI64 and AtomicI32. And we can a AtomicPtr type on it: pub struct AtomicPtr<T> { inner: AtomicIsize, _type: PhantomData<*mut T> } Of course, I need to do some code generating work for AtomicIsize and AtomicPtr, I plan to do that in Rust not in scripts, this will keep the rust/kernel/sync/atomic/impl.rs relatively small (i.e. the Rust/C interface is smaller). I can include this part in the next version, if you want to see it. Regards, Boqun
On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: > On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > Does this make sense? > > Implementation-wise, if you think it is simpler or more clear/elegant > to have the extra lower level layer, then that sounds fine. > > However, I was mainly talking about what we would eventually expose to > users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, The truth is I don't know ;-) I don't have much data on which one is better. Personally, I think AtomicI32 and AtomicI64 make the users have to think about size, alignment, etc, and I think that's important for atomic users and people who review their code, because before one uses atomics, one should ask themselves: why don't I use a lock? Atomics provide the ablities to do low level stuffs and when doing low level stuffs, you want to be more explicit than ergonomic. That said, I keep an open mind on `Atomic<T>`, maybe it will show its value at last. But right now, I'm not convinced personally. > then we could make the lower layer private already. > > We can defer that extra layer/work if needed even if we go for > `Atomic<T>`, but it would be nice to understand if we have consensus > for an eventual user-facing API, or if someone has any other opinion > or concerns on one vs. the other. > Yes, that'll be great. I'd love to see others' inputs! Regards, Boqun > Cheers, > Miguel
On Fri, Jun 14, 2024 at 11:40:47AM +0100, Mark Rutland wrote: [...] > > +#gen_proto_order_variant(meta, pfx, name, sfx, order, atomic, ty, int, raw, arg...) > > +gen_proto_order_variant() > > +{ > > + local meta="$1"; shift > > + local pfx="$1"; shift > > + local name="$1"; shift > > + local sfx="$1"; shift > > + local order="$1"; shift > > + local atomic="$1"; shift > > + local ty="$1"; shift > > + local int="$1"; shift > > + local raw="$1"; shift > > + > > + local fn_name="${raw}${pfx}${name}${sfx}${order}" > > + local atomicname="${raw}${atomic}_${pfx}${name}${sfx}${order}" > > + > > + local ret="$(gen_ret_type "${meta}" "${int}")" > > + local params="$(gen_params "${int}" $@)" > > + local args="$(gen_args "$@")" > > + local retstmt="$(gen_ret_stmt "${meta}")" > > + > > +cat <<EOF > > + /// See \`${atomicname}\`. > > + #[inline(always)] > > + pub fn ${fn_name}(&self${params}) ${ret}{ > > + // SAFETY:\`self.0.get()\` is a valid pointer. > > + unsafe { > > + ${retstmt}${atomicname}(${args}); > > + } > > + } > > +EOF > > +} > > AFAICT the 'ty' argument (AtomicI32/AtomicI64) isn't used and can be > removed. > Good catch. > Likewise for 'raw'. > > > + > > +cat << EOF > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Generated by $0 > > +//! DO NOT MODIFY THIS FILE DIRECTLY > > + > > +use super::*; > > +use crate::bindings::*; > > + > > +impl AtomicI32 { > > +EOF > > + > > +grep '^[a-z]' "$1" | while read name meta args; do > > + gen_proto "${meta}" "${name}" "atomic" "AtomicI32" "i32" "" ${args} > > With 'ty' and 'raw' gone, this'd be: > > gen_proto "${meta}" "${name}" "atomic" "i32" ${args} > > > +done > > + > > +cat << EOF > > +} > > + > > +impl AtomicI64 { > > +EOF > > + > > +grep '^[a-z]' "$1" | while read name meta args; do > > + gen_proto "${meta}" "${name}" "atomic64" "AtomicI64" "i64" "" ${args} > > With 'ty' and 'raw' gone, this'd be: > > gen_proto "${meta}" "${name}" "atomic64" "i64" ${args} > All fixed locally, thanks! Regards, Boqun > Mark. > > > +done > > + > > +cat << EOF > > +} > > + > > +EOF > > -- > > 2.45.2 > >
On 14.06.24 16:33, Boqun Feng wrote: > On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: >> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >>> >>> Does this make sense? >> >> Implementation-wise, if you think it is simpler or more clear/elegant >> to have the extra lower level layer, then that sounds fine. >> >> However, I was mainly talking about what we would eventually expose to >> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > > The truth is I don't know ;-) I don't have much data on which one is > better. Personally, I think AtomicI32 and AtomicI64 make the users have > to think about size, alignment, etc, and I think that's important for > atomic users and people who review their code, because before one uses > atomics, one should ask themselves: why don't I use a lock? Atomics > provide the ablities to do low level stuffs and when doing low level > stuffs, you want to be more explicit than ergonomic. How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just because the underlying `Atomic<I>` type is generic shouldn't change this, right? --- Cheers, Benno
On 6/14/24 2:59 AM, Miguel Ojeda wrote: > On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >> >> Does this make sense? > > Implementation-wise, if you think it is simpler or more clear/elegant > to have the extra lower level layer, then that sounds fine. > > However, I was mainly talking about what we would eventually expose to > users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > then we could make the lower layer private already. > > We can defer that extra layer/work if needed even if we go for > `Atomic<T>`, but it would be nice to understand if we have consensus > for an eventual user-facing API, or if someone has any other opinion > or concerns on one vs. the other. Well, here's one: The reason that we have things like atomic64_read() in the C code is because C doesn't have generics. In Rust, we should simply move directly to Atomic<T>, as there are, after all, associated benefits. And it's very easy to see the connection between the C types and the Atomic<T> types. thanks,
On Fri, Jun 14, 2024 at 06:03:37PM -0700, John Hubbard wrote: > On 6/14/24 2:59 AM, Miguel Ojeda wrote: > > On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > Does this make sense? > > > > Implementation-wise, if you think it is simpler or more clear/elegant > > to have the extra lower level layer, then that sounds fine. > > > > However, I was mainly talking about what we would eventually expose to > > users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > > then we could make the lower layer private already. > > > > We can defer that extra layer/work if needed even if we go for > > `Atomic<T>`, but it would be nice to understand if we have consensus > > for an eventual user-facing API, or if someone has any other opinion > > or concerns on one vs. the other. > > Well, here's one: > > The reason that we have things like atomic64_read() in the C code is > because C doesn't have generics. > > In Rust, we should simply move directly to Atomic<T>, as there are, > after all, associated benefits. And it's very easy to see the connection What are the associated benefits you are referring to? Rust std doesn't use Atomic<T>, that somewhat proves that we don't need it. Regards, Boqun > between the C types and the Atomic<T> types. > > > thanks, > -- > John Hubbard > NVIDIA >
On 6/14/24 6:24 PM, Boqun Feng wrote: > On Fri, Jun 14, 2024 at 06:03:37PM -0700, John Hubbard wrote: >> On 6/14/24 2:59 AM, Miguel Ojeda wrote: >>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >>>> >>>> Does this make sense? >>> >>> Implementation-wise, if you think it is simpler or more clear/elegant >>> to have the extra lower level layer, then that sounds fine. >>> >>> However, I was mainly talking about what we would eventually expose to >>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, >>> then we could make the lower layer private already. >>> >>> We can defer that extra layer/work if needed even if we go for >>> `Atomic<T>`, but it would be nice to understand if we have consensus >>> for an eventual user-facing API, or if someone has any other opinion >>> or concerns on one vs. the other. >> >> Well, here's one: >> >> The reason that we have things like atomic64_read() in the C code is >> because C doesn't have generics. >> >> In Rust, we should simply move directly to Atomic<T>, as there are, >> after all, associated benefits. And it's very easy to see the connection > > What are the associated benefits you are referring to? Rust std doesn't > use Atomic<T>, that somewhat proves that we don't need it. Just the stock things that a generic provides: less duplicated code, automatic support for future types (although here it's really just integer types we care about of course). thanks,
On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: > On 14.06.24 16:33, Boqun Feng wrote: > > On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: > >> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > >>> > >>> Does this make sense? > >> > >> Implementation-wise, if you think it is simpler or more clear/elegant > >> to have the extra lower level layer, then that sounds fine. > >> > >> However, I was mainly talking about what we would eventually expose to > >> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > > > > The truth is I don't know ;-) I don't have much data on which one is > > better. Personally, I think AtomicI32 and AtomicI64 make the users have > > to think about size, alignment, etc, and I think that's important for > > atomic users and people who review their code, because before one uses > > atomics, one should ask themselves: why don't I use a lock? Atomics > > provide the ablities to do low level stuffs and when doing low level > > stuffs, you want to be more explicit than ergonomic. > > How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just The difference is that with Atomic{I32,I64} APIs, one has to choose (and think about) the size when using atomics, and cannot leave that option open. It's somewhere unconvenient, but as I said, atomics variables are different. For example, if someone is going to implement a reference counter struct, they can define as follow: struct Refcount<T> { refcount: AtomicI32, data: UnsafeCell<T> } but with atomic generic, people can leave that option open and do: struct Refcount<R, T> { refcount: Atomic<R>, data: UnsafeCell<T> } while it provides configurable options for experienced users, but it also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: on ll/sc architectures, because `data` and `refcount` can be in the same machine-word, the accesses of `refcount` are affected by the accesses of `data`. The point I'm trying to make here is: when you are using atomics, you care about performance a lot (otherwise, why don't you use a lock?), and because of that, you should care about the size of the atomics, because it may affect the performance significantly. Regards, Boqun > because the underlying `Atomic<I>` type is generic shouldn't change > this, right? > > --- > Cheers, > Benno >
On Fri, Jun 14, 2024 at 06:28:00PM -0700, John Hubbard wrote: > On 6/14/24 6:24 PM, Boqun Feng wrote: > > On Fri, Jun 14, 2024 at 06:03:37PM -0700, John Hubbard wrote: > > > On 6/14/24 2:59 AM, Miguel Ojeda wrote: > > > > On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > Does this make sense? > > > > > > > > Implementation-wise, if you think it is simpler or more clear/elegant > > > > to have the extra lower level layer, then that sounds fine. > > > > > > > > However, I was mainly talking about what we would eventually expose to > > > > users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > > > > then we could make the lower layer private already. > > > > > > > > We can defer that extra layer/work if needed even if we go for > > > > `Atomic<T>`, but it would be nice to understand if we have consensus > > > > for an eventual user-facing API, or if someone has any other opinion > > > > or concerns on one vs. the other. > > > > > > Well, here's one: > > > > > > The reason that we have things like atomic64_read() in the C code is > > > because C doesn't have generics. > > > > > > In Rust, we should simply move directly to Atomic<T>, as there are, > > > after all, associated benefits. And it's very easy to see the connection > > > > What are the associated benefits you are referring to? Rust std doesn't > > use Atomic<T>, that somewhat proves that we don't need it. > Just the stock things that a generic provides: less duplicated code, It's still a bit handwavy, sorry. Admittedly, I haven't looked into too much Rust concurrent code, maybe it's even true for C code ;-) So I took a look at the crate that Gary mentioned (the one provides generic atomic APIs): https://crates.io/crates/atomic there's a "Dependent" tab where you can see the other crates that depends on it. With a quick look, I haven't found any Rust concurrent project I'm aware of (no crossbeam, no tokio, no futures). On the other hand, there is a non-generic based atomic library: https://crates.io/crates/portable-atomic which has more projects depend on it, and there are some Rust concurrent projects that I'm aware of: futures, async-task etc. Note that people can get the non-generic based atomic API from Rust std library, and the "portable-atomic" crate is only 2-year old, while "atomic" crate is 8-year old. More interestingly, the same author of "atomic" crate, who is an expert in concurrent areas, has another project (there are a lot projects from the author, but this is the one I'm mostly aware of) "parking_lot", which "provides implementations of Mutex, RwLock, Condvar and Once that are smaller, faster and more flexible than those in the Rust standard library, as well as a ReentrantMutex type which supports recursive locking.", and it doesn't use the "atomic" crate either. These data could mean nothing, there are multiple reasons affecting the popularity of a library. But all the above seems to suggests that you don't really need generic on atomic, at least for a lot of meaningful concurent code. So if we were to make a decision right now, I don't see that generic atomics are winning. Of course, as I said previously, we can always add them if we have learned more and have the consensus. (Don't make me wrong, I love generic in general, I just want to avoid the "I have a generic hammer and everything looks like generic nails" situation.) Regards, Boqun > automatic support for future types (although here it's really just > integer types we care about of course). > > > thanks, > -- > John Hubbard > NVIDIA >
On 6/14/24 7:39 PM, Boqun Feng wrote: > On Fri, Jun 14, 2024 at 06:28:00PM -0700, John Hubbard wrote: >> On 6/14/24 6:24 PM, Boqun Feng wrote: >>> On Fri, Jun 14, 2024 at 06:03:37PM -0700, John Hubbard wrote: >>>> On 6/14/24 2:59 AM, Miguel Ojeda wrote: >>>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >>>>>> >>>>>> Does this make sense? >>>>> >>>>> Implementation-wise, if you think it is simpler or more clear/elegant >>>>> to have the extra lower level layer, then that sounds fine. >>>>> >>>>> However, I was mainly talking about what we would eventually expose to >>>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, >>>>> then we could make the lower layer private already. >>>>> >>>>> We can defer that extra layer/work if needed even if we go for >>>>> `Atomic<T>`, but it would be nice to understand if we have consensus >>>>> for an eventual user-facing API, or if someone has any other opinion >>>>> or concerns on one vs. the other. >>>> >>>> Well, here's one: >>>> >>>> The reason that we have things like atomic64_read() in the C code is >>>> because C doesn't have generics. >>>> >>>> In Rust, we should simply move directly to Atomic<T>, as there are, >>>> after all, associated benefits. And it's very easy to see the connection >>> >>> What are the associated benefits you are referring to? Rust std doesn't >>> use Atomic<T>, that somewhat proves that we don't need it. >> Just the stock things that a generic provides: less duplicated code, > > It's still a bit handwavy, sorry. > > Admittedly, I haven't looked into too much Rust concurrent code, maybe > it's even true for C code ;-) So I took a look at the crate that Gary > mentioned (the one provides generic atomic APIs): > > https://crates.io/crates/atomic > > there's a "Dependent" tab where you can see the other crates that > depends on it. With a quick look, I haven't found any Rust concurrent > project I'm aware of (no crossbeam, no tokio, no futures). On the other > hand, there is a non-generic based atomic library: > > https://crates.io/crates/portable-atomic > > which has more projects depend on it, and there are some Rust concurrent > projects that I'm aware of: futures, async-task etc. Note that people > can get the non-generic based atomic API from Rust std library, and > the "portable-atomic" crate is only 2-year old, while "atomic" crate is > 8-year old. > > More interestingly, the same author of "atomic" crate, who is an expert > in concurrent areas, has another project (there are a lot projects from > the author, but this is the one I'm mostly aware of) "parking_lot", > which "provides implementations of Mutex, RwLock, Condvar and Once that > are smaller, faster and more flexible than those in the Rust standard > library, as well as a ReentrantMutex type which supports recursive > locking.", and it doesn't use the "atomic" crate either. > > These data could mean nothing, there are multiple reasons affecting the > popularity of a library. But all the above seems to suggests that you > don't really need generic on atomic, at least for a lot of meaningful > concurent code. > > > So if we were to make a decision right now, I don't see that generic > atomics are winning. Of course, as I said previously, we can always add That does seem to be the case: the non-generic flavor looks more popular so far. > them if we have learned more and have the consensus. Yes, I suppose waiting might be better. I expected the Atomic<T> to be more popular than it actually is... thanks,
On 15.06.24 03:33, Boqun Feng wrote: > On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: >> On 14.06.24 16:33, Boqun Feng wrote: >>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: >>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >>>>> >>>>> Does this make sense? >>>> >>>> Implementation-wise, if you think it is simpler or more clear/elegant >>>> to have the extra lower level layer, then that sounds fine. >>>> >>>> However, I was mainly talking about what we would eventually expose to >>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, >>> >>> The truth is I don't know ;-) I don't have much data on which one is >>> better. Personally, I think AtomicI32 and AtomicI64 make the users have >>> to think about size, alignment, etc, and I think that's important for >>> atomic users and people who review their code, because before one uses >>> atomics, one should ask themselves: why don't I use a lock? Atomics >>> provide the ablities to do low level stuffs and when doing low level >>> stuffs, you want to be more explicit than ergonomic. >> >> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just > > The difference is that with Atomic{I32,I64} APIs, one has to choose (and > think about) the size when using atomics, and cannot leave that option > open. It's somewhere unconvenient, but as I said, atomics variables are > different. For example, if someone is going to implement a reference > counter struct, they can define as follow: > > struct Refcount<T> { > refcount: AtomicI32, > data: UnsafeCell<T> > } > > but with atomic generic, people can leave that option open and do: > > struct Refcount<R, T> { > refcount: Atomic<R>, > data: UnsafeCell<T> > } > > while it provides configurable options for experienced users, but it > also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: > on ll/sc architectures, because `data` and `refcount` can be in the same > machine-word, the accesses of `refcount` are affected by the accesses of > `data`. I think this is a non-issue. We have two options of counteracting this: 1. We can just point this out in reviews and force people to use `Atomic<T>` with a concrete type. In cases where there really is the need to be generic, we can have it. 2. We can add a private trait in the bounds for the generic, nobody outside of the module can access it and thus they need to use a concrete type: // needs a better name trait Integer {} impl Integer for i32 {} impl Integer for i64 {} pub struct Atomic<T: Integer> { /* ... */ } And then in the other module, you can't do this (with compiler error): pub struct Refcount<R: Integer, T> { // ^^^^^^^ not found in this scope // note: trait `crate::atomic::Integer` exists but is inaccessible refcount: Atomic<R>, data: UnsafeCell<T>, } I think that we can start with approach 2 and if we find a use-case where generics are really unavoidable, we can either put it in the same module as `Atomic<T>`, or change the access of `Integer`. --- Cheers, Benno > The point I'm trying to make here is: when you are using atomics, you > care about performance a lot (otherwise, why don't you use a lock?), and > because of that, you should care about the size of the atomics, because > it may affect the performance significantly.
On Sat, Jun 15, 2024 at 07:09:30AM +0000, Benno Lossin wrote: > On 15.06.24 03:33, Boqun Feng wrote: > > On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: > >> On 14.06.24 16:33, Boqun Feng wrote: > >>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: > >>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > >>>>> > >>>>> Does this make sense? > >>>> > >>>> Implementation-wise, if you think it is simpler or more clear/elegant > >>>> to have the extra lower level layer, then that sounds fine. > >>>> > >>>> However, I was mainly talking about what we would eventually expose to > >>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > >>> > >>> The truth is I don't know ;-) I don't have much data on which one is > >>> better. Personally, I think AtomicI32 and AtomicI64 make the users have > >>> to think about size, alignment, etc, and I think that's important for > >>> atomic users and people who review their code, because before one uses > >>> atomics, one should ask themselves: why don't I use a lock? Atomics > >>> provide the ablities to do low level stuffs and when doing low level > >>> stuffs, you want to be more explicit than ergonomic. > >> > >> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just > > > > The difference is that with Atomic{I32,I64} APIs, one has to choose (and > > think about) the size when using atomics, and cannot leave that option > > open. It's somewhere unconvenient, but as I said, atomics variables are > > different. For example, if someone is going to implement a reference > > counter struct, they can define as follow: > > > > struct Refcount<T> { > > refcount: AtomicI32, > > data: UnsafeCell<T> > > } > > > > but with atomic generic, people can leave that option open and do: > > > > struct Refcount<R, T> { > > refcount: Atomic<R>, > > data: UnsafeCell<T> > > } > > > > while it provides configurable options for experienced users, but it > > also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: > > on ll/sc architectures, because `data` and `refcount` can be in the same > > machine-word, the accesses of `refcount` are affected by the accesses of > > `data`. > > I think this is a non-issue. We have two options of counteracting this: > 1. We can just point this out in reviews and force people to use > `Atomic<T>` with a concrete type. In cases where there really is the > need to be generic, we can have it. > 2. We can add a private trait in the bounds for the generic, nobody > outside of the module can access it and thus they need to use a > concrete type: > > // needs a better name > trait Integer {} > impl Integer for i32 {} > impl Integer for i64 {} > > pub struct Atomic<T: Integer> { > /* ... */ > } > > And then in the other module, you can't do this (with compiler error): > > pub struct Refcount<R: Integer, T> { > // ^^^^^^^ not found in this scope > // note: trait `crate::atomic::Integer` exists but is inaccessible > refcount: Atomic<R>, > data: UnsafeCell<T>, > } > > I think that we can start with approach 2 and if we find a use-case > where generics are really unavoidable, we can either put it in the same > module as `Atomic<T>`, or change the access of `Integer`. > What's the issue of having AtomicI32 and AtomicI64 first then? We don't need to do 1 or 2 until the real users show up. And I'd like also to point out that there are a few more trait bound designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> have different sets of API (no inc_unless_negative() for u32). Don't make me wrong, I have no doubt we can handle this in the type system, but given the design work need, won't it make sense that we take baby steps on this? We can first introduce AtomicI32 and AtomicI64 which already have real users, and then if there are some values of generic atomics, we introduce them and have proper discussion on design. To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. What's the downside? A bit specific example would help me understand the real concern here. Regards, Boqun > --- > Cheers, > Benno > > > The point I'm trying to make here is: when you are using atomics, you > > care about performance a lot (otherwise, why don't you use a lock?), and > > because of that, you should care about the size of the atomics, because > > it may affect the performance significantly. >
On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: > On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > Does this make sense? > > Implementation-wise, if you think it is simpler or more clear/elegant > to have the extra lower level layer, then that sounds fine. > > However, I was mainly talking about what we would eventually expose to > users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > then we could make the lower layer private already. > > We can defer that extra layer/work if needed even if we go for > `Atomic<T>`, but it would be nice to understand if we have consensus > for an eventual user-facing API, or if someone has any other opinion > or concerns on one vs. the other. Since this is fully compatible to LKMM atomic operations, is there a use case for C and Rust operating on the same atomic value? And then you will need to specify the size, or odd things are likely to happen if they disagree on size. With Atomic<T> can we easily say what type the underlying implementation uses? Andrew
On 16.06.24 00:12, Boqun Feng wrote: > On Sat, Jun 15, 2024 at 07:09:30AM +0000, Benno Lossin wrote: >> On 15.06.24 03:33, Boqun Feng wrote: >>> On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: >>>> On 14.06.24 16:33, Boqun Feng wrote: >>>>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: >>>>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >>>>>>> >>>>>>> Does this make sense? >>>>>> >>>>>> Implementation-wise, if you think it is simpler or more clear/elegant >>>>>> to have the extra lower level layer, then that sounds fine. >>>>>> >>>>>> However, I was mainly talking about what we would eventually expose to >>>>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, >>>>> >>>>> The truth is I don't know ;-) I don't have much data on which one is >>>>> better. Personally, I think AtomicI32 and AtomicI64 make the users have >>>>> to think about size, alignment, etc, and I think that's important for >>>>> atomic users and people who review their code, because before one uses >>>>> atomics, one should ask themselves: why don't I use a lock? Atomics >>>>> provide the ablities to do low level stuffs and when doing low level >>>>> stuffs, you want to be more explicit than ergonomic. >>>> >>>> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just >>> >>> The difference is that with Atomic{I32,I64} APIs, one has to choose (and >>> think about) the size when using atomics, and cannot leave that option >>> open. It's somewhere unconvenient, but as I said, atomics variables are >>> different. For example, if someone is going to implement a reference >>> counter struct, they can define as follow: >>> >>> struct Refcount<T> { >>> refcount: AtomicI32, >>> data: UnsafeCell<T> >>> } >>> >>> but with atomic generic, people can leave that option open and do: >>> >>> struct Refcount<R, T> { >>> refcount: Atomic<R>, >>> data: UnsafeCell<T> >>> } >>> >>> while it provides configurable options for experienced users, but it >>> also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: >>> on ll/sc architectures, because `data` and `refcount` can be in the same >>> machine-word, the accesses of `refcount` are affected by the accesses of >>> `data`. >> >> I think this is a non-issue. We have two options of counteracting this: >> 1. We can just point this out in reviews and force people to use >> `Atomic<T>` with a concrete type. In cases where there really is the >> need to be generic, we can have it. >> 2. We can add a private trait in the bounds for the generic, nobody >> outside of the module can access it and thus they need to use a >> concrete type: >> >> // needs a better name >> trait Integer {} >> impl Integer for i32 {} >> impl Integer for i64 {} >> >> pub struct Atomic<T: Integer> { >> /* ... */ >> } >> >> And then in the other module, you can't do this (with compiler error): >> >> pub struct Refcount<R: Integer, T> { >> // ^^^^^^^ not found in this scope >> // note: trait `crate::atomic::Integer` exists but is inaccessible >> refcount: Atomic<R>, >> data: UnsafeCell<T>, >> } >> >> I think that we can start with approach 2 and if we find a use-case >> where generics are really unavoidable, we can either put it in the same >> module as `Atomic<T>`, or change the access of `Integer`. >> > > What's the issue of having AtomicI32 and AtomicI64 first then? We don't > need to do 1 or 2 until the real users show up. Generics allow you to avoid code duplication (I don't think that you want to create the `Atomic{I32,I64}` types via macros...). We would have to do a lot of refactoring, when we want to introduce it. I don't see the harm of introducing generics from the get-go. > And I'd like also to point out that there are a few more trait bound > designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> > have different sets of API (no inc_unless_negative() for u32). Sure, just like Gary said, you can just do: impl Atomic<i32> { pub fn inc_unless_negative(&self, ordering: Ordering) -> bool; } Or add a `HasNegative` trait. > Don't make me wrong, I have no doubt we can handle this in the type > system, but given the design work need, won't it make sense that we take > baby steps on this? We can first introduce AtomicI32 and AtomicI64 which > already have real users, and then if there are some values of generic > atomics, we introduce them and have proper discussion on design. I don't understand this point, why can't we put in the effort for a good design? AFAIK we normally spend considerable time to get the API right and I think in this case it would include making it generic. > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. > What's the downside? A bit specific example would help me understand > the real concern here. I don't like that, why have two ways of doing the same thing? People will be confused whether they should use `AtomicI32` vs `Atomic<i32>`... --- Cheers, Benno
On Sat, Jun 15, 2024 at 03:12:33PM -0700, Boqun Feng wrote: > What's the issue of having AtomicI32 and AtomicI64 first then? We don't > need to do 1 or 2 until the real users show up. > > And I'd like also to point out that there are a few more trait bound > designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> > have different sets of API (no inc_unless_negative() for u32). > > Don't make me wrong, I have no doubt we can handle this in the type > system, but given the design work need, won't it make sense that we take > baby steps on this? We can first introduce AtomicI32 and AtomicI64 which > already have real users, and then if there are some values of generic > atomics, we introduce them and have proper discussion on design. > > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. > What's the downside? A bit specific example would help me understand > the real concern here. Err, what? Of course we want generic atomics, and we need that for properly supporting cmpxchg. Bogun, you've got all the rust guys pushing for doing this with generics, I'm not sure why you're being stubborn here?
On Sun, Jun 16, 2024 at 09:46:45AM +0000, Benno Lossin wrote: > On 16.06.24 00:12, Boqun Feng wrote: > > On Sat, Jun 15, 2024 at 07:09:30AM +0000, Benno Lossin wrote: > >> On 15.06.24 03:33, Boqun Feng wrote: > >>> On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: > >>>> On 14.06.24 16:33, Boqun Feng wrote: > >>>>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: > >>>>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > >>>>>>> > >>>>>>> Does this make sense? > >>>>>> > >>>>>> Implementation-wise, if you think it is simpler or more clear/elegant > >>>>>> to have the extra lower level layer, then that sounds fine. > >>>>>> > >>>>>> However, I was mainly talking about what we would eventually expose to > >>>>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > >>>>> > >>>>> The truth is I don't know ;-) I don't have much data on which one is > >>>>> better. Personally, I think AtomicI32 and AtomicI64 make the users have > >>>>> to think about size, alignment, etc, and I think that's important for > >>>>> atomic users and people who review their code, because before one uses > >>>>> atomics, one should ask themselves: why don't I use a lock? Atomics > >>>>> provide the ablities to do low level stuffs and when doing low level > >>>>> stuffs, you want to be more explicit than ergonomic. > >>>> > >>>> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just > >>> > >>> The difference is that with Atomic{I32,I64} APIs, one has to choose (and > >>> think about) the size when using atomics, and cannot leave that option > >>> open. It's somewhere unconvenient, but as I said, atomics variables are > >>> different. For example, if someone is going to implement a reference > >>> counter struct, they can define as follow: > >>> > >>> struct Refcount<T> { > >>> refcount: AtomicI32, > >>> data: UnsafeCell<T> > >>> } > >>> > >>> but with atomic generic, people can leave that option open and do: > >>> > >>> struct Refcount<R, T> { > >>> refcount: Atomic<R>, > >>> data: UnsafeCell<T> > >>> } > >>> > >>> while it provides configurable options for experienced users, but it > >>> also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: > >>> on ll/sc architectures, because `data` and `refcount` can be in the same > >>> machine-word, the accesses of `refcount` are affected by the accesses of > >>> `data`. > >> > >> I think this is a non-issue. We have two options of counteracting this: > >> 1. We can just point this out in reviews and force people to use > >> `Atomic<T>` with a concrete type. In cases where there really is the > >> need to be generic, we can have it. > >> 2. We can add a private trait in the bounds for the generic, nobody > >> outside of the module can access it and thus they need to use a > >> concrete type: > >> > >> // needs a better name > >> trait Integer {} > >> impl Integer for i32 {} > >> impl Integer for i64 {} > >> > >> pub struct Atomic<T: Integer> { > >> /* ... */ > >> } > >> > >> And then in the other module, you can't do this (with compiler error): > >> > >> pub struct Refcount<R: Integer, T> { > >> // ^^^^^^^ not found in this scope > >> // note: trait `crate::atomic::Integer` exists but is inaccessible > >> refcount: Atomic<R>, > >> data: UnsafeCell<T>, > >> } > >> > >> I think that we can start with approach 2 and if we find a use-case > >> where generics are really unavoidable, we can either put it in the same > >> module as `Atomic<T>`, or change the access of `Integer`. > >> > > > > What's the issue of having AtomicI32 and AtomicI64 first then? We don't > > need to do 1 or 2 until the real users show up. > > Generics allow you to avoid code duplication (I don't think that you > want to create the `Atomic{I32,I64}` types via macros...). We would have > to do a lot of refactoring, when we want to introduce it. I don't see You can simply do type AtomicI32=Atomic<i32>; Plus, we always do refactoring in kernel, because it's impossible to get everything right at the first time. TBH, it's too confident to think one can. > the harm of introducing generics from the get-go. > > > And I'd like also to point out that there are a few more trait bound > > designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> > > have different sets of API (no inc_unless_negative() for u32). > > Sure, just like Gary said, you can just do: > > impl Atomic<i32> { > pub fn inc_unless_negative(&self, ordering: Ordering) -> bool; > } > > Or add a `HasNegative` trait. > > > Don't make me wrong, I have no doubt we can handle this in the type > > system, but given the design work need, won't it make sense that we take > > baby steps on this? We can first introduce AtomicI32 and AtomicI64 which > > already have real users, and then if there are some values of generic > > atomics, we introduce them and have proper discussion on design. > > I don't understand this point, why can't we put in the effort for a good > design? AFAIK we normally spend considerable time to get the API right > and I think in this case it would include making it generic. > What's the design you propose here? Well, the conversation between us is only the design bit I saw, elsewhere it's all handwaving that "generics are overall really good". I'm happy to get the API right, and it's easy and simple to do on concrete types. But IIUC, Gary's suggestion is to only have Atomic<i32> and Atomic<i64> first, and do the design later, which I really don't like. It may not be a complete design, but I need to see the design now to understand whether we need to go to that direction. I cannot just introduce a TBD generic. Regards, Boqun > > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. > > What's the downside? A bit specific example would help me understand > > the real concern here. > > I don't like that, why have two ways of doing the same thing? People > will be confused whether they should use `AtomicI32` vs `Atomic<i32>`... > > --- > Cheers, > Benno >
On Sun, Jun 16, 2024 at 05:51:07AM -0400, Kent Overstreet wrote: > On Sat, Jun 15, 2024 at 03:12:33PM -0700, Boqun Feng wrote: > > What's the issue of having AtomicI32 and AtomicI64 first then? We don't > > need to do 1 or 2 until the real users show up. > > > > And I'd like also to point out that there are a few more trait bound > > designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> > > have different sets of API (no inc_unless_negative() for u32). > > > > Don't make me wrong, I have no doubt we can handle this in the type > > system, but given the design work need, won't it make sense that we take > > baby steps on this? We can first introduce AtomicI32 and AtomicI64 which > > already have real users, and then if there are some values of generic > > atomics, we introduce them and have proper discussion on design. > > > > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. > > What's the downside? A bit specific example would help me understand > > the real concern here. > > Err, what? > > Of course we want generic atomics, and we need that for properly > supporting cmpxchg. > Nope. Note this series only introduces the atomic types (atomic_ C APIs), but cmpxchg C APIs (no atomic_ prefix) are probably presented via a different API, where we need to make it easier to interact with normal types, and we may use generic there. > Bogun, you've got all the rust guys pushing for doing this with > generics, I'm not sure why you're being stubborn here? Hmm? Have you seen the email I replied to John, a broader Rust community seems doesn't appreciate the idea of generic atomics. Regards, Boqun
On Sun, Jun 16, 2024 at 07:16:30AM -0700, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 05:51:07AM -0400, Kent Overstreet wrote: > > On Sat, Jun 15, 2024 at 03:12:33PM -0700, Boqun Feng wrote: > > > What's the issue of having AtomicI32 and AtomicI64 first then? We don't > > > need to do 1 or 2 until the real users show up. > > > > > > And I'd like also to point out that there are a few more trait bound > > > designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> > > > have different sets of API (no inc_unless_negative() for u32). > > > > > > Don't make me wrong, I have no doubt we can handle this in the type > > > system, but given the design work need, won't it make sense that we take > > > baby steps on this? We can first introduce AtomicI32 and AtomicI64 which > > > already have real users, and then if there are some values of generic > > > atomics, we introduce them and have proper discussion on design. > > > > > > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. > > > What's the downside? A bit specific example would help me understand > > > the real concern here. > > > > Err, what? > > > > Of course we want generic atomics, and we need that for properly > > supporting cmpxchg. > > > > Nope. Note this series only introduces the atomic types (atomic_ C > APIs), but cmpxchg C APIs (no atomic_ prefix) are probably presented via > a different API, where we need to make it easier to interact with normal > types, and we may use generic there. > Or it could be a generic function instead of generic type like: pub unsafe fn cmpxchg<T>(ptr: * mut T, old: T, new T) -> T the "unsafe" part is due to `ptr` may not be a valid pointer or this may make normal accesses data race. Regards, Boqun > > Bogun, you've got all the rust guys pushing for doing this with > > generics, I'm not sure why you're being stubborn here? > > Hmm? Have you seen the email I replied to John, a broader Rust community > seems doesn't appreciate the idea of generic atomics. > > Regards, > Boqun
On Fri, 14 Jun 2024 19:39:27 -0700 Boqun Feng <boqun.feng@gmail.com> wrote: > On Fri, Jun 14, 2024 at 06:28:00PM -0700, John Hubbard wrote: > > On 6/14/24 6:24 PM, Boqun Feng wrote: > > > On Fri, Jun 14, 2024 at 06:03:37PM -0700, John Hubbard wrote: > > > > On 6/14/24 2:59 AM, Miguel Ojeda wrote: > > > > > On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > > > Does this make sense? > > > > > > > > > > Implementation-wise, if you think it is simpler or more clear/elegant > > > > > to have the extra lower level layer, then that sounds fine. > > > > > > > > > > However, I was mainly talking about what we would eventually expose to > > > > > users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > > > > > then we could make the lower layer private already. > > > > > > > > > > We can defer that extra layer/work if needed even if we go for > > > > > `Atomic<T>`, but it would be nice to understand if we have consensus > > > > > for an eventual user-facing API, or if someone has any other opinion > > > > > or concerns on one vs. the other. > > > > > > > > Well, here's one: > > > > > > > > The reason that we have things like atomic64_read() in the C code is > > > > because C doesn't have generics. > > > > > > > > In Rust, we should simply move directly to Atomic<T>, as there are, > > > > after all, associated benefits. And it's very easy to see the connection > > > > > > What are the associated benefits you are referring to? Rust std doesn't > > > use Atomic<T>, that somewhat proves that we don't need it. > > Just the stock things that a generic provides: less duplicated code, > > It's still a bit handwavy, sorry. > > Admittedly, I haven't looked into too much Rust concurrent code, maybe > it's even true for C code ;-) So I took a look at the crate that Gary > mentioned (the one provides generic atomic APIs): > > https://crates.io/crates/atomic > > there's a "Dependent" tab where you can see the other crates that > depends on it. With a quick look, I haven't found any Rust concurrent > project I'm aware of (no crossbeam, no tokio, no futures). On the other > hand, there is a non-generic based atomic library: > > https://crates.io/crates/portable-atomic > > which has more projects depend on it, and there are some Rust concurrent > projects that I'm aware of: futures, async-task etc. Note that people > can get the non-generic based atomic API from Rust std library, and > the "portable-atomic" crate is only 2-year old, while "atomic" crate is > 8-year old. > > More interestingly, the same author of "atomic" crate, who is an expert > in concurrent areas, has another project (there are a lot projects from > the author, but this is the one I'm mostly aware of) "parking_lot", > which "provides implementations of Mutex, RwLock, Condvar and Once that > are smaller, faster and more flexible than those in the Rust standard > library, as well as a ReentrantMutex type which supports recursive > locking.", and it doesn't use the "atomic" crate either. Note that crossbeam's AtomicCell is also generic, and crossbeam is used by tons of crates. As Miguel mentioned, I think it's very likely that in the future we want be able to do atomics on new types (e.g. for seqlocks perhaps). We probably don't need the non-lock-free fallback of crossbeam's AtomicCell, but the lock-free subset with newtype support is desirable. People in general don't use the `atomic` crate because it provides no additional feature compared to the standard library. But it doesn't really mean that the standard library's atomic design is good. People decided to use AtomicT and NonZeroT instead of Atomic<T> or NonZero<T> long time ago, but many now thinks the decision was bad. Introduction of NonZero<T> is a good example of it. NonZeroT are now all type aliases of NonZero<T>. I also don't see any downside in using generics. We can provide type aliases so people can use `AtomicI32` and `AtomicI64` when they want their code to be compatible with userspace Rust can still do so. `Atomic<i32>` is also just aesthetically better than `AtomicI32` IMO. When all other types look like `NonZero<i32>`, `Wrapping<i32>`, I don't think we should have `AtomicI32` just because "it's done this way in Rust std". Our alloc already deviates a lot from Rust std. Best, Gary
On Sun, Jun 16, 2024 at 03:51:45PM +0100, Gary Guo wrote: > On Fri, 14 Jun 2024 19:39:27 -0700 > Boqun Feng <boqun.feng@gmail.com> wrote: > > > On Fri, Jun 14, 2024 at 06:28:00PM -0700, John Hubbard wrote: > > > On 6/14/24 6:24 PM, Boqun Feng wrote: > > > > On Fri, Jun 14, 2024 at 06:03:37PM -0700, John Hubbard wrote: > > > > > On 6/14/24 2:59 AM, Miguel Ojeda wrote: > > > > > > On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > > > > > Does this make sense? > > > > > > > > > > > > Implementation-wise, if you think it is simpler or more clear/elegant > > > > > > to have the extra lower level layer, then that sounds fine. > > > > > > > > > > > > However, I was mainly talking about what we would eventually expose to > > > > > > users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > > > > > > then we could make the lower layer private already. > > > > > > > > > > > > We can defer that extra layer/work if needed even if we go for > > > > > > `Atomic<T>`, but it would be nice to understand if we have consensus > > > > > > for an eventual user-facing API, or if someone has any other opinion > > > > > > or concerns on one vs. the other. > > > > > > > > > > Well, here's one: > > > > > > > > > > The reason that we have things like atomic64_read() in the C code is > > > > > because C doesn't have generics. > > > > > > > > > > In Rust, we should simply move directly to Atomic<T>, as there are, > > > > > after all, associated benefits. And it's very easy to see the connection > > > > > > > > What are the associated benefits you are referring to? Rust std doesn't > > > > use Atomic<T>, that somewhat proves that we don't need it. > > > Just the stock things that a generic provides: less duplicated code, > > > > It's still a bit handwavy, sorry. > > > > Admittedly, I haven't looked into too much Rust concurrent code, maybe > > it's even true for C code ;-) So I took a look at the crate that Gary > > mentioned (the one provides generic atomic APIs): > > > > https://crates.io/crates/atomic > > > > there's a "Dependent" tab where you can see the other crates that > > depends on it. With a quick look, I haven't found any Rust concurrent > > project I'm aware of (no crossbeam, no tokio, no futures). On the other > > hand, there is a non-generic based atomic library: > > > > https://crates.io/crates/portable-atomic > > > > which has more projects depend on it, and there are some Rust concurrent > > projects that I'm aware of: futures, async-task etc. Note that people > > can get the non-generic based atomic API from Rust std library, and > > the "portable-atomic" crate is only 2-year old, while "atomic" crate is > > 8-year old. > > > > More interestingly, the same author of "atomic" crate, who is an expert > > in concurrent areas, has another project (there are a lot projects from > > the author, but this is the one I'm mostly aware of) "parking_lot", > > which "provides implementations of Mutex, RwLock, Condvar and Once that > > are smaller, faster and more flexible than those in the Rust standard > > library, as well as a ReentrantMutex type which supports recursive > > locking.", and it doesn't use the "atomic" crate either. > > Note that crossbeam's AtomicCell is also generic, and crossbeam is used > by tons of crates. As Miguel mentioned, I think it's very likely that in > the future we want be able to do atomics on new types (e.g. for > seqlocks perhaps). We probably don't need the non-lock-free fallback of Good, another design bit, thank you! What's our overall idea on sub-word types, like Atomic<u8> and Atomic<u16>, do we plan to say no to them, or they could have a limited APIs? IIUC, some operations on them are relatively sub-optimal on some architectures, supporting the same set of API as i32 and i64 is probably a bad idea. Another thing in my mind is making `Atomic<T>` pub struct Atomic<T: Send + ...> { ... } so that `Atomic<T>` will always be `Sync`, because quite frankly, an atomic type that cannot `Sync` is pointless. Regards, Boqun > crossbeam's AtomicCell, but the lock-free subset with newtype support > is desirable. > > People in general don't use the `atomic` crate because it provides no > additional feature compared to the standard library. But it doesn't > really mean that the standard library's atomic design is good. > > People decided to use AtomicT and NonZeroT instead of Atomic<T> or > NonZero<T> long time ago, but many now thinks the decision was bad. > Introduction of NonZero<T> is a good example of it. NonZeroT are now > all type aliases of NonZero<T>. > > I also don't see any downside in using generics. We can provide type > aliases so people can use `AtomicI32` and `AtomicI64` when they want > their code to be compatible with userspace Rust can still do so. > > `Atomic<i32>` is also just aesthetically better than `AtomicI32` IMO. > When all other types look like `NonZero<i32>`, `Wrapping<i32>`, I don't > think we should have `AtomicI32` just because "it's done this way in > Rust std". Our alloc already deviates a lot from Rust std. > > Best, > Gary
On 16.06.24 16:08, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 09:46:45AM +0000, Benno Lossin wrote: >> On 16.06.24 00:12, Boqun Feng wrote: >>> On Sat, Jun 15, 2024 at 07:09:30AM +0000, Benno Lossin wrote: >>>> On 15.06.24 03:33, Boqun Feng wrote: >>>>> On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: >>>>>> On 14.06.24 16:33, Boqun Feng wrote: >>>>>>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: >>>>>>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >>>>>>>>> >>>>>>>>> Does this make sense? >>>>>>>> >>>>>>>> Implementation-wise, if you think it is simpler or more clear/elegant >>>>>>>> to have the extra lower level layer, then that sounds fine. >>>>>>>> >>>>>>>> However, I was mainly talking about what we would eventually expose to >>>>>>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, >>>>>>> >>>>>>> The truth is I don't know ;-) I don't have much data on which one is >>>>>>> better. Personally, I think AtomicI32 and AtomicI64 make the users have >>>>>>> to think about size, alignment, etc, and I think that's important for >>>>>>> atomic users and people who review their code, because before one uses >>>>>>> atomics, one should ask themselves: why don't I use a lock? Atomics >>>>>>> provide the ablities to do low level stuffs and when doing low level >>>>>>> stuffs, you want to be more explicit than ergonomic. >>>>>> >>>>>> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just >>>>> >>>>> The difference is that with Atomic{I32,I64} APIs, one has to choose (and >>>>> think about) the size when using atomics, and cannot leave that option >>>>> open. It's somewhere unconvenient, but as I said, atomics variables are >>>>> different. For example, if someone is going to implement a reference >>>>> counter struct, they can define as follow: >>>>> >>>>> struct Refcount<T> { >>>>> refcount: AtomicI32, >>>>> data: UnsafeCell<T> >>>>> } >>>>> >>>>> but with atomic generic, people can leave that option open and do: >>>>> >>>>> struct Refcount<R, T> { >>>>> refcount: Atomic<R>, >>>>> data: UnsafeCell<T> >>>>> } >>>>> >>>>> while it provides configurable options for experienced users, but it >>>>> also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: >>>>> on ll/sc architectures, because `data` and `refcount` can be in the same >>>>> machine-word, the accesses of `refcount` are affected by the accesses of >>>>> `data`. >>>> >>>> I think this is a non-issue. We have two options of counteracting this: >>>> 1. We can just point this out in reviews and force people to use >>>> `Atomic<T>` with a concrete type. In cases where there really is the >>>> need to be generic, we can have it. >>>> 2. We can add a private trait in the bounds for the generic, nobody >>>> outside of the module can access it and thus they need to use a >>>> concrete type: >>>> >>>> // needs a better name >>>> trait Integer {} >>>> impl Integer for i32 {} >>>> impl Integer for i64 {} >>>> >>>> pub struct Atomic<T: Integer> { >>>> /* ... */ >>>> } >>>> >>>> And then in the other module, you can't do this (with compiler error): >>>> >>>> pub struct Refcount<R: Integer, T> { >>>> // ^^^^^^^ not found in this scope >>>> // note: trait `crate::atomic::Integer` exists but is inaccessible >>>> refcount: Atomic<R>, >>>> data: UnsafeCell<T>, >>>> } >>>> >>>> I think that we can start with approach 2 and if we find a use-case >>>> where generics are really unavoidable, we can either put it in the same >>>> module as `Atomic<T>`, or change the access of `Integer`. >>>> >>> >>> What's the issue of having AtomicI32 and AtomicI64 first then? We don't >>> need to do 1 or 2 until the real users show up. >> >> Generics allow you to avoid code duplication (I don't think that you >> want to create the `Atomic{I32,I64}` types via macros...). We would have >> to do a lot of refactoring, when we want to introduce it. I don't see > > You can simply do > > type AtomicI32=Atomic<i32>; Eh, I would think that we could just do a text replacement in this case. Or if that doesn't work, Coccinelle should be able to do this... > Plus, we always do refactoring in kernel, because it's impossible to get > everything right at the first time. TBH, it's too confident to think one > can. I don't think that we're at the "let's just put it in" stage. This is an RFC version, so it should be fine to completely change the approach. I agree, that we can't get it 100% right the first time, but we should at least strive to get a good version. >> the harm of introducing generics from the get-go. >> >>> And I'd like also to point out that there are a few more trait bound >>> designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> >>> have different sets of API (no inc_unless_negative() for u32). >> >> Sure, just like Gary said, you can just do: >> >> impl Atomic<i32> { >> pub fn inc_unless_negative(&self, ordering: Ordering) -> bool; >> } >> >> Or add a `HasNegative` trait. >> >>> Don't make me wrong, I have no doubt we can handle this in the type >>> system, but given the design work need, won't it make sense that we take >>> baby steps on this? We can first introduce AtomicI32 and AtomicI64 which >>> already have real users, and then if there are some values of generic >>> atomics, we introduce them and have proper discussion on design. >> >> I don't understand this point, why can't we put in the effort for a good >> design? AFAIK we normally spend considerable time to get the API right >> and I think in this case it would include making it generic. >> > > What's the design you propose here? Well, the conversation between us is > only the design bit I saw, elsewhere it's all handwaving that "generics > are overall really good". I'm happy to get the API right, and it's easy > and simple to do on concrete types. But IIUC, Gary's suggestion is to > only have Atomic<i32> and Atomic<i64> first, and do the design later, > which I really don't like. It may not be a complete design, but I need > to see the design now to understand whether we need to go to that > direction. I cannot just introduce a TBD generic. I don't think that the idea was to "do the design later". I don't even know how you would do that, since you need the design to submit a patch. I can't offer you a complete API description, since that would require me writing it up myself. But I would recommend trying to get it to work with generics. I got a few other comments: - I don't think that we should resort to a script to generate the Rust code since it prevents adding good documentation & examples to the various methods. AFAIU you want to generate the functions from `scripts/atomic/atomics.tbl` to keep it in sync with the C side. I looked at the git log of that file and it hasn't been changed significantly since its inception. I don't think that there is any benefit to generating the functions from that file. - most of the documented functions say "See `c_function`", I don't like this, can we either copy the C documentation (I imagine it not changing that often, or is that assumption wrong?) or write our own? - we should try to use either const generic or normal parameters for the access ordering instead of putting it in the function name. - why do we need both non-return and return variants? I think it is probably a good idea to discuss this in our meeting. --- Cheers, Benno
On Sun, Jun 16, 2024 at 4:16 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > Hmm? Have you seen the email I replied to John, a broader Rust community > seems doesn't appreciate the idea of generic atomics. I don't think we can easily draw that conclusion from those download numbers / dependent crates. portable-atomic may be more popular simply because it provides features for platforms the standard library does not. The interface being generic or not may have nothing to do with it. Or perhaps because it has a 1.x version, while the other doesn't, etc. In fact, the atomic crate is essentially about providing `Atomic<T>`, so one could argue that all those downloads are precisely from people that want a generic atomic. Moreover, I noticed portable-atomic's issue #1 in GitHub is, precisely, adding `Atomic<T>` support. The maintainer has a PR for that updated over time, most recently a few hours ago. There is also `AtomicCell<T>` from crossbeam, which is the first feature listed in its docs. Anyway... The way I see it, both approaches seem similar (i.e. for what we are going to use them for today, at least) and neither apparently has a major downside today for those use cases (apart from needed refactors later to go to another approach). (By the "generic approach", by the way, I mean just providing `Atomic<{i32,i64}>`, not a complex design) So it is up to you on what you send for the non-RFC patches, of course, and if nobody has the time / wants to do the work for the "simple" generic approach, then we can just go ahead with this for the moment. But I think it would be nice to at least consider the "simple" generic approach to see how much worse it would be. Other bits to consider, that perhaps give you arguments for one or the other: consequences on the compilation time, on inlining, on the error messages for new users, on the generated documentation, on how easy to grep they are, etc. Cheers, Miguel
On Sun, Jun 16, 2024 at 07:16:30AM -0700, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 05:51:07AM -0400, Kent Overstreet wrote: > > On Sat, Jun 15, 2024 at 03:12:33PM -0700, Boqun Feng wrote: > > > What's the issue of having AtomicI32 and AtomicI64 first then? We don't > > > need to do 1 or 2 until the real users show up. > > > > > > And I'd like also to point out that there are a few more trait bound > > > designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> > > > have different sets of API (no inc_unless_negative() for u32). > > > > > > Don't make me wrong, I have no doubt we can handle this in the type > > > system, but given the design work need, won't it make sense that we take > > > baby steps on this? We can first introduce AtomicI32 and AtomicI64 which > > > already have real users, and then if there are some values of generic > > > atomics, we introduce them and have proper discussion on design. > > > > > > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. > > > What's the downside? A bit specific example would help me understand > > > the real concern here. > > > > Err, what? > > > > Of course we want generic atomics, and we need that for properly > > supporting cmpxchg. > > > > Nope. Note this series only introduces the atomic types (atomic_ C > APIs), but cmpxchg C APIs (no atomic_ prefix) are probably presented via > a different API, where we need to make it easier to interact with normal > types, and we may use generic there. > > > Bogun, you've got all the rust guys pushing for doing this with > > generics, I'm not sure why you're being stubborn here? > > Hmm? Have you seen the email I replied to John, a broader Rust community > seems doesn't appreciate the idea of generic atomics. Apologies, I appear to have gotten things backwards in my pre-coffee reading, I'll have to catch up on the whole thread :)
On Sun, Jun 16, 2024 at 05:14:56PM +0200, Miguel Ojeda wrote: > On Sun, Jun 16, 2024 at 4:16 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > Hmm? Have you seen the email I replied to John, a broader Rust community > > seems doesn't appreciate the idea of generic atomics. > > I don't think we can easily draw that conclusion from those download > numbers / dependent crates. > > portable-atomic may be more popular simply because it provides > features for platforms the standard library does not. The interface > being generic or not may have nothing to do with it. Or perhaps > because it has a 1.x version, while the other doesn't, etc. > > In fact, the atomic crate is essentially about providing `Atomic<T>`, > so one could argue that all those downloads are precisely from people > that want a generic atomic. > > Moreover, I noticed portable-atomic's issue #1 in GitHub is, > precisely, adding `Atomic<T>` support. The maintainer has a PR for > that updated over time, most recently a few hours ago. > > There is also `AtomicCell<T>` from crossbeam, which is the first > feature listed in its docs. > > Anyway... > > The way I see it, both approaches seem similar (i.e. for what we are > going to use them for today, at least) and neither apparently has a > major downside today for those use cases (apart from needed refactors > later to go to another approach). > > (By the "generic approach", by the way, I mean just providing > `Atomic<{i32,i64}>`, not a complex design) > > So it is up to you on what you send for the non-RFC patches, of > course, and if nobody has the time / wants to do the work for the > "simple" generic approach, then we can just go ahead with this for the > moment. But I think it would be nice to at least consider the "simple" > generic approach to see how much worse it would be. > > Other bits to consider, that perhaps give you arguments for one or the > other: consequences on the compilation time, on inlining, on the error > messages for new users, on the generated documentation, on how easy to > grep they are, etc. Yeah, rereading the thread - I'm with Miguel and Gary. Generics are simply the correct way to do it, if the wider rust community didn't do it that way I think that can be chalked up more to historical baggage or needlessly copying the base integer type scheme. Let's please do it right here, and generics are the correct approach.
On Sun, Jun 16, 2024 at 03:06:36PM +0000, Benno Lossin wrote: > On 16.06.24 16:08, Boqun Feng wrote: > > On Sun, Jun 16, 2024 at 09:46:45AM +0000, Benno Lossin wrote: > >> On 16.06.24 00:12, Boqun Feng wrote: > >>> On Sat, Jun 15, 2024 at 07:09:30AM +0000, Benno Lossin wrote: > >>>> On 15.06.24 03:33, Boqun Feng wrote: > >>>>> On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: > >>>>>> On 14.06.24 16:33, Boqun Feng wrote: > >>>>>>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: > >>>>>>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: > >>>>>>>>> > >>>>>>>>> Does this make sense? > >>>>>>>> > >>>>>>>> Implementation-wise, if you think it is simpler or more clear/elegant > >>>>>>>> to have the extra lower level layer, then that sounds fine. > >>>>>>>> > >>>>>>>> However, I was mainly talking about what we would eventually expose to > >>>>>>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, > >>>>>>> > >>>>>>> The truth is I don't know ;-) I don't have much data on which one is > >>>>>>> better. Personally, I think AtomicI32 and AtomicI64 make the users have > >>>>>>> to think about size, alignment, etc, and I think that's important for > >>>>>>> atomic users and people who review their code, because before one uses > >>>>>>> atomics, one should ask themselves: why don't I use a lock? Atomics > >>>>>>> provide the ablities to do low level stuffs and when doing low level > >>>>>>> stuffs, you want to be more explicit than ergonomic. > >>>>>> > >>>>>> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just > >>>>> > >>>>> The difference is that with Atomic{I32,I64} APIs, one has to choose (and > >>>>> think about) the size when using atomics, and cannot leave that option > >>>>> open. It's somewhere unconvenient, but as I said, atomics variables are > >>>>> different. For example, if someone is going to implement a reference > >>>>> counter struct, they can define as follow: > >>>>> > >>>>> struct Refcount<T> { > >>>>> refcount: AtomicI32, > >>>>> data: UnsafeCell<T> > >>>>> } > >>>>> > >>>>> but with atomic generic, people can leave that option open and do: > >>>>> > >>>>> struct Refcount<R, T> { > >>>>> refcount: Atomic<R>, > >>>>> data: UnsafeCell<T> > >>>>> } > >>>>> > >>>>> while it provides configurable options for experienced users, but it > >>>>> also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: > >>>>> on ll/sc architectures, because `data` and `refcount` can be in the same > >>>>> machine-word, the accesses of `refcount` are affected by the accesses of > >>>>> `data`. > >>>> > >>>> I think this is a non-issue. We have two options of counteracting this: > >>>> 1. We can just point this out in reviews and force people to use > >>>> `Atomic<T>` with a concrete type. In cases where there really is the > >>>> need to be generic, we can have it. > >>>> 2. We can add a private trait in the bounds for the generic, nobody > >>>> outside of the module can access it and thus they need to use a > >>>> concrete type: > >>>> > >>>> // needs a better name > >>>> trait Integer {} > >>>> impl Integer for i32 {} > >>>> impl Integer for i64 {} > >>>> > >>>> pub struct Atomic<T: Integer> { > >>>> /* ... */ > >>>> } > >>>> > >>>> And then in the other module, you can't do this (with compiler error): > >>>> > >>>> pub struct Refcount<R: Integer, T> { > >>>> // ^^^^^^^ not found in this scope > >>>> // note: trait `crate::atomic::Integer` exists but is inaccessible > >>>> refcount: Atomic<R>, > >>>> data: UnsafeCell<T>, > >>>> } > >>>> > >>>> I think that we can start with approach 2 and if we find a use-case > >>>> where generics are really unavoidable, we can either put it in the same > >>>> module as `Atomic<T>`, or change the access of `Integer`. > >>>> > >>> > >>> What's the issue of having AtomicI32 and AtomicI64 first then? We don't > >>> need to do 1 or 2 until the real users show up. > >> > >> Generics allow you to avoid code duplication (I don't think that you > >> want to create the `Atomic{I32,I64}` types via macros...). We would have > >> to do a lot of refactoring, when we want to introduce it. I don't see > > > > You can simply do > > > > type AtomicI32=Atomic<i32>; > > Eh, I would think that we could just do a text replacement in this case. > Or if that doesn't work, Coccinelle should be able to do this... > > > Plus, we always do refactoring in kernel, because it's impossible to get > > everything right at the first time. TBH, it's too confident to think one > > can. > > I don't think that we're at the "let's just put it in" stage. This is an > RFC version, so it should be fine to completely change the approach. I'm fine as well. I wasn't trying to rush anything, but as I mentioned below, I need some more design from people who want it to understand whether that's a good idea. > I agree, that we can't get it 100% right the first time, but we should > at least strive to get a good version. > > >> the harm of introducing generics from the get-go. > >> > >>> And I'd like also to point out that there are a few more trait bound > >>> designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> > >>> have different sets of API (no inc_unless_negative() for u32). > >> > >> Sure, just like Gary said, you can just do: > >> > >> impl Atomic<i32> { > >> pub fn inc_unless_negative(&self, ordering: Ordering) -> bool; > >> } > >> > >> Or add a `HasNegative` trait. > >> > >>> Don't make me wrong, I have no doubt we can handle this in the type > >>> system, but given the design work need, won't it make sense that we take > >>> baby steps on this? We can first introduce AtomicI32 and AtomicI64 which > >>> already have real users, and then if there are some values of generic > >>> atomics, we introduce them and have proper discussion on design. > >> > >> I don't understand this point, why can't we put in the effort for a good > >> design? AFAIK we normally spend considerable time to get the API right > >> and I think in this case it would include making it generic. > >> > > > > What's the design you propose here? Well, the conversation between us is > > only the design bit I saw, elsewhere it's all handwaving that "generics > > are overall really good". I'm happy to get the API right, and it's easy > > and simple to do on concrete types. But IIUC, Gary's suggestion is to > > only have Atomic<i32> and Atomic<i64> first, and do the design later, > > which I really don't like. It may not be a complete design, but I need > > to see the design now to understand whether we need to go to that > > direction. I cannot just introduce a TBD generic. > > I don't think that the idea was to "do the design later". I don't even > know how you would do that, since you need the design to submit a patch. > Then I might mis-understand Gary? He said: "Can we avoid two types and use a generic `Atomic<T>` and then implement on `Atomic<i32>` and `Atomic<i64>` instead?" , which means just replace `impl AtomicI32` with `impl Atomic<i32>` to me. > I can't offer you a complete API description, since that would require > me writing it up myself. But I would recommend trying to get it to work > with generics. I got a few other comments: We should work on things that are concrete, right? It's fine that the design is not complete, and it's fine if you just recommend. But without a somewhat concrete design (doesn't have to be complete), I cannot be sure about whether we have the same vision of the future of generic atomics (see my question to Gary), that's a bit hard for me to try to work something out (plus I personally don't think it's a good idea, it's OK to me, but not good). Anyway, I wasn't trying to refuse to do this just based on personal reasons, but I do need to understand what you are all proposing, because I don't have one myself. > - I don't think that we should resort to a script to generate the Rust > code since it prevents adding good documentation & examples to the > various methods. AFAIU you want to generate the functions from > `scripts/atomic/atomics.tbl` to keep it in sync with the C side. I > looked at the git log of that file and it hasn't been changed > significantly since its inception. I don't think that there is any > benefit to generating the functions from that file. I'll leave this to other atomic maintainers. > - most of the documented functions say "See `c_function`", I don't like > this, can we either copy the C documentation (I imagine it not > changing that often, or is that assumption wrong?) or write our own? You're not wrong. AN in C side, we do have some documentation template to generate the comments (see scripts/atomic/kerneldoc). But first the format is for doxygen(I guess?), and second as you just bring up, the templates are tied with the bash script. > - we should try to use either const generic or normal parameters for the > access ordering instead of putting it in the function name. Why is it important? Keeping it in the current way brings the value that it's not too much different than their C counterparts. Could you explain a bit the pros and cons on suffix vs const generic approach? > - why do we need both non-return and return variants? > Historical reason I guess. Plus maybe some architectures have a better implementation on non-return atomics IIRC. Regards, Boqun > I think it is probably a good idea to discuss this in our meeting. > > --- > Cheers, > Benno >
On Sun, Jun 16, 2024 at 05:14:56PM +0200, Miguel Ojeda wrote: > On Sun, Jun 16, 2024 at 4:16 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > Hmm? Have you seen the email I replied to John, a broader Rust community > > seems doesn't appreciate the idea of generic atomics. > > I don't think we can easily draw that conclusion from those download > numbers / dependent crates. > > portable-atomic may be more popular simply because it provides > features for platforms the standard library does not. The interface > being generic or not may have nothing to do with it. Or perhaps > because it has a 1.x version, while the other doesn't, etc. > Totally agreed, but that was the information all I could get at that time ;-) > In fact, the atomic crate is essentially about providing `Atomic<T>`, > so one could argue that all those downloads are precisely from people > that want a generic atomic. > > Moreover, I noticed portable-atomic's issue #1 in GitHub is, > precisely, adding `Atomic<T>` support. The maintainer has a PR for > that updated over time, most recently a few hours ago. > Wait! Could it be because of me? Or I'm thinking too much about myself ;-) > There is also `AtomicCell<T>` from crossbeam, which is the first > feature listed in its docs. > > Anyway... > > The way I see it, both approaches seem similar (i.e. for what we are > going to use them for today, at least) and neither apparently has a > major downside today for those use cases (apart from needed refactors > later to go to another approach). > > (By the "generic approach", by the way, I mean just providing > `Atomic<{i32,i64}>`, not a complex design) > > So it is up to you on what you send for the non-RFC patches, of > course, and if nobody has the time / wants to do the work for the > "simple" generic approach, then we can just go ahead with this for the > moment. But I think it would be nice to at least consider the "simple" > generic approach to see how much worse it would be. > What would work for me is a somewhat concrete design consensus (on what sizes we are going to support, the expectation on how many traits we are going to introduce, etc.) And then a simple generic approach is not a problem. (But I remain my right to say "I told you so" ;-)) As I said, we cannot just do generic because of generic, we have to have at least some idea abou the things we are generify on (the scope and the cost in term of how many more traits people need to understand). > Other bits to consider, that perhaps give you arguments for one or the > other: consequences on the compilation time, on inlining, on the error > messages for new users, on the generated documentation, on how easy to > grep they are, etc. > These seem non-issues to me (except the grep part), but I'm relatively more familiar with atomics and Rust, so it's good to hear others thought, or wait the feedback until we have the patchset to review. Regards, Boqun > Cheers, > Miguel
On Sun, Jun 16, 2024 at 11:32:31AM -0400, Kent Overstreet wrote: > On Sun, Jun 16, 2024 at 05:14:56PM +0200, Miguel Ojeda wrote: > > On Sun, Jun 16, 2024 at 4:16 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > Hmm? Have you seen the email I replied to John, a broader Rust community > > > seems doesn't appreciate the idea of generic atomics. > > > > I don't think we can easily draw that conclusion from those download > > numbers / dependent crates. > > > > portable-atomic may be more popular simply because it provides > > features for platforms the standard library does not. The interface > > being generic or not may have nothing to do with it. Or perhaps > > because it has a 1.x version, while the other doesn't, etc. > > > > In fact, the atomic crate is essentially about providing `Atomic<T>`, > > so one could argue that all those downloads are precisely from people > > that want a generic atomic. > > > > Moreover, I noticed portable-atomic's issue #1 in GitHub is, > > precisely, adding `Atomic<T>` support. The maintainer has a PR for > > that updated over time, most recently a few hours ago. > > > > There is also `AtomicCell<T>` from crossbeam, which is the first > > feature listed in its docs. > > > > Anyway... > > > > The way I see it, both approaches seem similar (i.e. for what we are > > going to use them for today, at least) and neither apparently has a > > major downside today for those use cases (apart from needed refactors > > later to go to another approach). > > > > (By the "generic approach", by the way, I mean just providing > > `Atomic<{i32,i64}>`, not a complex design) > > > > So it is up to you on what you send for the non-RFC patches, of > > course, and if nobody has the time / wants to do the work for the > > "simple" generic approach, then we can just go ahead with this for the > > moment. But I think it would be nice to at least consider the "simple" > > generic approach to see how much worse it would be. > > > > Other bits to consider, that perhaps give you arguments for one or the > > other: consequences on the compilation time, on inlining, on the error > > messages for new users, on the generated documentation, on how easy to > > grep they are, etc. > > Yeah, rereading the thread - I'm with Miguel and Gary. > > Generics are simply the correct way to do it, if the wider rust > community didn't do it that way I think that can be chalked up more to > historical baggage or needlessly copying the base integer type scheme. > > Let's please do it right here, and generics are the correct approach. If so, maybe we should do u<Wide> instead of u8, u16, oh, and probably just Integer<Sign, Wide> instead of i{8,16,32,64) and u{8,16,32,64} ;-) Regards, Boqun
On 16.06.24 17:34, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 03:06:36PM +0000, Benno Lossin wrote: >> On 16.06.24 16:08, Boqun Feng wrote: >>> On Sun, Jun 16, 2024 at 09:46:45AM +0000, Benno Lossin wrote: >>>> On 16.06.24 00:12, Boqun Feng wrote: >>>>> On Sat, Jun 15, 2024 at 07:09:30AM +0000, Benno Lossin wrote: >>>>>> On 15.06.24 03:33, Boqun Feng wrote: >>>>>>> On Fri, Jun 14, 2024 at 09:22:24PM +0000, Benno Lossin wrote: >>>>>>>> On 14.06.24 16:33, Boqun Feng wrote: >>>>>>>>> On Fri, Jun 14, 2024 at 11:59:58AM +0200, Miguel Ojeda wrote: >>>>>>>>>> On Thu, Jun 13, 2024 at 9:05 PM Boqun Feng <boqun.feng@gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>> Does this make sense? >>>>>>>>>> >>>>>>>>>> Implementation-wise, if you think it is simpler or more clear/elegant >>>>>>>>>> to have the extra lower level layer, then that sounds fine. >>>>>>>>>> >>>>>>>>>> However, I was mainly talking about what we would eventually expose to >>>>>>>>>> users, i.e. do we want to provide `Atomic<T>` to begin with? If yes, >>>>>>>>> >>>>>>>>> The truth is I don't know ;-) I don't have much data on which one is >>>>>>>>> better. Personally, I think AtomicI32 and AtomicI64 make the users have >>>>>>>>> to think about size, alignment, etc, and I think that's important for >>>>>>>>> atomic users and people who review their code, because before one uses >>>>>>>>> atomics, one should ask themselves: why don't I use a lock? Atomics >>>>>>>>> provide the ablities to do low level stuffs and when doing low level >>>>>>>>> stuffs, you want to be more explicit than ergonomic. >>>>>>>> >>>>>>>> How would this be different with `Atomic<i32>` and `Atomic<i64>`? Just >>>>>>> >>>>>>> The difference is that with Atomic{I32,I64} APIs, one has to choose (and >>>>>>> think about) the size when using atomics, and cannot leave that option >>>>>>> open. It's somewhere unconvenient, but as I said, atomics variables are >>>>>>> different. For example, if someone is going to implement a reference >>>>>>> counter struct, they can define as follow: >>>>>>> >>>>>>> struct Refcount<T> { >>>>>>> refcount: AtomicI32, >>>>>>> data: UnsafeCell<T> >>>>>>> } >>>>>>> >>>>>>> but with atomic generic, people can leave that option open and do: >>>>>>> >>>>>>> struct Refcount<R, T> { >>>>>>> refcount: Atomic<R>, >>>>>>> data: UnsafeCell<T> >>>>>>> } >>>>>>> >>>>>>> while it provides configurable options for experienced users, but it >>>>>>> also provides opportunities for sub-optimal types, e.g. Refcount<u8, T>: >>>>>>> on ll/sc architectures, because `data` and `refcount` can be in the same >>>>>>> machine-word, the accesses of `refcount` are affected by the accesses of >>>>>>> `data`. >>>>>> >>>>>> I think this is a non-issue. We have two options of counteracting this: >>>>>> 1. We can just point this out in reviews and force people to use >>>>>> `Atomic<T>` with a concrete type. In cases where there really is the >>>>>> need to be generic, we can have it. >>>>>> 2. We can add a private trait in the bounds for the generic, nobody >>>>>> outside of the module can access it and thus they need to use a >>>>>> concrete type: >>>>>> >>>>>> // needs a better name >>>>>> trait Integer {} >>>>>> impl Integer for i32 {} >>>>>> impl Integer for i64 {} >>>>>> >>>>>> pub struct Atomic<T: Integer> { >>>>>> /* ... */ >>>>>> } >>>>>> >>>>>> And then in the other module, you can't do this (with compiler error): >>>>>> >>>>>> pub struct Refcount<R: Integer, T> { >>>>>> // ^^^^^^^ not found in this scope >>>>>> // note: trait `crate::atomic::Integer` exists but is inaccessible >>>>>> refcount: Atomic<R>, >>>>>> data: UnsafeCell<T>, >>>>>> } >>>>>> >>>>>> I think that we can start with approach 2 and if we find a use-case >>>>>> where generics are really unavoidable, we can either put it in the same >>>>>> module as `Atomic<T>`, or change the access of `Integer`. >>>>>> >>>>> >>>>> What's the issue of having AtomicI32 and AtomicI64 first then? We don't >>>>> need to do 1 or 2 until the real users show up. >>>> >>>> Generics allow you to avoid code duplication (I don't think that you >>>> want to create the `Atomic{I32,I64}` types via macros...). We would have >>>> to do a lot of refactoring, when we want to introduce it. I don't see >>> >>> You can simply do >>> >>> type AtomicI32=Atomic<i32>; >> >> Eh, I would think that we could just do a text replacement in this case. >> Or if that doesn't work, Coccinelle should be able to do this... >> >>> Plus, we always do refactoring in kernel, because it's impossible to get >>> everything right at the first time. TBH, it's too confident to think one >>> can. >> >> I don't think that we're at the "let's just put it in" stage. This is an >> RFC version, so it should be fine to completely change the approach. > > I'm fine as well. I wasn't trying to rush anything, but as I mentioned > below, I need some more design from people who want it to understand > whether that's a good idea. > >> I agree, that we can't get it 100% right the first time, but we should >> at least strive to get a good version. >> >>>> the harm of introducing generics from the get-go. >>>> >>>>> And I'd like also to point out that there are a few more trait bound >>>>> designs needed for Atomic<T>, for example, Atomic<u32> and Atomic<i32> >>>>> have different sets of API (no inc_unless_negative() for u32). >>>> >>>> Sure, just like Gary said, you can just do: >>>> >>>> impl Atomic<i32> { >>>> pub fn inc_unless_negative(&self, ordering: Ordering) -> bool; >>>> } >>>> >>>> Or add a `HasNegative` trait. >>>> >>>>> Don't make me wrong, I have no doubt we can handle this in the type >>>>> system, but given the design work need, won't it make sense that we take >>>>> baby steps on this? We can first introduce AtomicI32 and AtomicI64 which >>>>> already have real users, and then if there are some values of generic >>>>> atomics, we introduce them and have proper discussion on design. >>>> >>>> I don't understand this point, why can't we put in the effort for a good >>>> design? AFAIK we normally spend considerable time to get the API right >>>> and I think in this case it would include making it generic. >>>> >>> >>> What's the design you propose here? Well, the conversation between us is >>> only the design bit I saw, elsewhere it's all handwaving that "generics >>> are overall really good". I'm happy to get the API right, and it's easy >>> and simple to do on concrete types. But IIUC, Gary's suggestion is to >>> only have Atomic<i32> and Atomic<i64> first, and do the design later, >>> which I really don't like. It may not be a complete design, but I need >>> to see the design now to understand whether we need to go to that >>> direction. I cannot just introduce a TBD generic. >> >> I don't think that the idea was to "do the design later". I don't even >> know how you would do that, since you need the design to submit a patch. >> > > Then I might mis-understand Gary? He said: > > "Can we avoid two types and use a generic `Atomic<T>` and then implement > on `Atomic<i32>` and `Atomic<i64>` instead?" > > , which means just replace `impl AtomicI32` with `impl Atomic<i32>` to > me. This is a fair interpretation, but what prevents you from merging the impls of functions that can be? I assumed that you would do that automatically. >> I can't offer you a complete API description, since that would require >> me writing it up myself. But I would recommend trying to get it to work >> with generics. I got a few other comments: > > We should work on things that are concrete, right? It's fine that the > design is not complete, and it's fine if you just recommend. But without > a somewhat concrete design (doesn't have to be complete), I cannot be > sure about whether we have the same vision of the future of generic > atomics (see my question to Gary), that's a bit hard for me to try to Sorry, which question? Also to be aligned on the vision, I think we should rather talk about the vision and not the design, since the design that we want to have now can be different from the vision. On that note, what do you envision the future of the atomic API? > work something out (plus I personally don't think it's a good idea, it's > OK to me, but not good). Anyway, I wasn't trying to refuse to do this > just based on personal reasons, but I do need to understand what you are > all proposing, because I don't have one myself. I went back through the thread and here is what I think your argument against generics is: people should think about size and alignment when using atomics, which is problematic when allowing users to leave the atomic generic. But as I argued before, this is not an issue. Have I overlooked another argument? Because I don't see anything else. >> - I don't think that we should resort to a script to generate the Rust >> code since it prevents adding good documentation & examples to the >> various methods. AFAIU you want to generate the functions from >> `scripts/atomic/atomics.tbl` to keep it in sync with the C side. I >> looked at the git log of that file and it hasn't been changed >> significantly since its inception. I don't think that there is any >> benefit to generating the functions from that file. > > I'll leave this to other atomic maintainers. > >> - most of the documented functions say "See `c_function`", I don't like >> this, can we either copy the C documentation (I imagine it not >> changing that often, or is that assumption wrong?) or write our own? > > You're not wrong. AN in C side, we do have some documentation template > to generate the comments (see scripts/atomic/kerneldoc). But first the > format is for doxygen(I guess?), and second as you just bring up, the > templates are tied with the bash script. I see a bash script similarly to how Wedson sees proc macros ;) We should try *hard* to avoid them and only use them when there is no other way. >> - we should try to use either const generic or normal parameters for the >> access ordering instead of putting it in the function name. > > Why is it important? Keeping it in the current way brings the value that > it's not too much different than their C counterparts. Could you explain > a bit the pros and cons on suffix vs const generic approach? Reduce code duplication, instead of 3 different variants, we can have one. It allows people to build ergonomic APIs that allows the user to decide which synchronization they use under the hood. >> - why do we need both non-return and return variants? >> > > Historical reason I guess. Plus maybe some architectures have a better > implementation on non-return atomics IIRC. Could we get some more concrete arguments for why we would need these in rust? If the reason is purely historical, then we shouldn't expose the non-return variant IMO. If it is because of performance, then we can only expose them in the respective arches. --- Cheers, Benno
On Sun, Jun 16, 2024 at 03:55:12PM +0000, Benno Lossin wrote: [...] > >> > >> I don't think that the idea was to "do the design later". I don't even > >> know how you would do that, since you need the design to submit a patch. > >> > > > > Then I might mis-understand Gary? He said: > > > > "Can we avoid two types and use a generic `Atomic<T>` and then implement > > on `Atomic<i32>` and `Atomic<i64>` instead?" > > > > , which means just replace `impl AtomicI32` with `impl Atomic<i32>` to > > me. > > This is a fair interpretation, but what prevents you from merging the > impls of functions that can be? I assumed that you would do that > automatically. > I think you missed the point, Gary's suggestion at that time sounds like: let's impl Atomic<i32> and Atomic<i64> first, and leave rest of the work for later, that is a "do the design later" to me. > >> I can't offer you a complete API description, since that would require > >> me writing it up myself. But I would recommend trying to get it to work > >> with generics. I got a few other comments: > > > > We should work on things that are concrete, right? It's fine that the > > design is not complete, and it's fine if you just recommend. But without > > a somewhat concrete design (doesn't have to be complete), I cannot be > > sure about whether we have the same vision of the future of generic > > atomics (see my question to Gary), that's a bit hard for me to try to > > Sorry, which question? https://lore.kernel.org/rust-for-linux/Zm7_XWe6ciy1yN-h@Boquns-Mac-mini.home/ > Also to be aligned on the vision, I think we should rather talk about > the vision and not the design, since the design that we want to have now > can be different from the vision. On that note, what do you envision the > future of the atomic API? > Mine is simple, just providing AtomicI32 and AtomicI64 first, since there are immediate users right now, and should we learn more needs from the users, we get more idea about what a good API is, and we evolve from there. > > work something out (plus I personally don't think it's a good idea, it's > > OK to me, but not good). Anyway, I wasn't trying to refuse to do this > > just based on personal reasons, but I do need to understand what you are > > all proposing, because I don't have one myself. > > I went back through the thread and here is what I think your argument > against generics is: people should think about size and alignment when > using atomics, which is problematic when allowing users to leave the > atomic generic. > But as I argued before, this is not an issue. Have I overlooked another You mean you said it's a non-issue but gave me two counteract? If it's really a non-issue, you won't need a couneraction, right? In other words non-generic atomics do provide some value. > argument? Because I don't see anything else. > > >> - I don't think that we should resort to a script to generate the Rust > >> code since it prevents adding good documentation & examples to the > >> various methods. AFAIU you want to generate the functions from > >> `scripts/atomic/atomics.tbl` to keep it in sync with the C side. I > >> looked at the git log of that file and it hasn't been changed > >> significantly since its inception. I don't think that there is any > >> benefit to generating the functions from that file. > > > > I'll leave this to other atomic maintainers. > > > >> - most of the documented functions say "See `c_function`", I don't like > >> this, can we either copy the C documentation (I imagine it not > >> changing that often, or is that assumption wrong?) or write our own? > > > > You're not wrong. AN in C side, we do have some documentation template > > to generate the comments (see scripts/atomic/kerneldoc). But first the > > format is for doxygen(I guess?), and second as you just bring up, the > > templates are tied with the bash script. > > I see a bash script similarly to how Wedson sees proc macros ;) > We should try *hard* to avoid them and only use them when there is no > other way. > I will just start with the existing mechanism and try to evolve, whether it's a script or proc macro, I don't mind, I want to get the work done and most relevant people can understand in the way the they prefer and step-by-step, move it to the place I think is the best for the project. > >> - we should try to use either const generic or normal parameters for the > >> access ordering instead of putting it in the function name. > > > > Why is it important? Keeping it in the current way brings the value that > > it's not too much different than their C counterparts. Could you explain > > a bit the pros and cons on suffix vs const generic approach? > > Reduce code duplication, instead of 3 different variants, we can have > one. It allows people to build ergonomic APIs that allows the user to > decide which synchronization they use under the hood. > I already mentioned why I think it's good in the current way, I will defer it to others on their inputs. > >> - why do we need both non-return and return variants? > >> > > > > Historical reason I guess. Plus maybe some architectures have a better > > implementation on non-return atomics IIRC. > > Could we get some more concrete arguments for why we would need these in > rust? If the reason is purely historical, then we shouldn't expose the Sure. Look like my memory is correct, at least on ARM64 they are different instructions (see arch/arm64/include/asm/atomic_lse.h) non-return atomics on ARM64: #define ATOMIC_OP(op, asm_op) \ static __always_inline void \ __lse_atomic_##op(int i, atomic_t *v) \ { \ asm volatile( \ __LSE_PREAMBLE \ " " #asm_op " %w[i], %[v]\n" \ : [v] "+Q" (v->counter) \ : [i] "r" (i)); \ } value-return atomics on ARM64: #define ATOMIC_FETCH_OP(name, mb, op, asm_op, cl...) \ static __always_inline int \ __lse_atomic_fetch_##op##name(int i, atomic_t *v) \ { \ int old; \ \ asm volatile( \ __LSE_PREAMBLE \ " " #asm_op #mb " %w[i], %w[old], %[v]" \ : [v] "+Q" (v->counter), \ [old] "=r" (old) \ : [i] "r" (i) \ : cl); \ \ return old; \ } It may not be easy to see the different instructions from the pasted code, but you can find them in the header file, also you could notice that the number of operands is different? > non-return variant IMO. If it is because of performance, then we can > only expose them in the respective arches. > Hmm.. so we ask user to write arch-specific code like: pub fn increase_counter(&self) { #[cfg(CONFIG_ARM64)] self.counter.inc(); #[cfg(CONFIG_X86_64)] let _ = self.counter.inc_return(); } are you sure it's a good idea? Regards, Boqun > --- > Cheers, > Benno >
On Sun, Jun 16, 2024 at 09:46:45AM +0000, Benno Lossin wrote: [...] > > To me, it's perfectly fine that Atomic{I32,I64} co-exist with Atomic<T>. > > What's the downside? A bit specific example would help me understand > > the real concern here. > > I don't like that, why have two ways of doing the same thing? People > will be confused whether they should use `AtomicI32` vs `Atomic<i32>`... > BTW, we already have something similar like this in kernel, we have SpinLock<T> and Lock<T, SpinLockBackend>, how should we do about this? Regards, Boqun > --- > Cheers, > Benno >
On Sun, Jun 16, 2024 at 08:54:09AM -0700, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 11:32:31AM -0400, Kent Overstreet wrote: > > On Sun, Jun 16, 2024 at 05:14:56PM +0200, Miguel Ojeda wrote: > > > On Sun, Jun 16, 2024 at 4:16 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > Hmm? Have you seen the email I replied to John, a broader Rust community > > > > seems doesn't appreciate the idea of generic atomics. > > > > > > I don't think we can easily draw that conclusion from those download > > > numbers / dependent crates. > > > > > > portable-atomic may be more popular simply because it provides > > > features for platforms the standard library does not. The interface > > > being generic or not may have nothing to do with it. Or perhaps > > > because it has a 1.x version, while the other doesn't, etc. > > > > > > In fact, the atomic crate is essentially about providing `Atomic<T>`, > > > so one could argue that all those downloads are precisely from people > > > that want a generic atomic. > > > > > > Moreover, I noticed portable-atomic's issue #1 in GitHub is, > > > precisely, adding `Atomic<T>` support. The maintainer has a PR for > > > that updated over time, most recently a few hours ago. > > > > > > There is also `AtomicCell<T>` from crossbeam, which is the first > > > feature listed in its docs. > > > > > > Anyway... > > > > > > The way I see it, both approaches seem similar (i.e. for what we are > > > going to use them for today, at least) and neither apparently has a > > > major downside today for those use cases (apart from needed refactors > > > later to go to another approach). > > > > > > (By the "generic approach", by the way, I mean just providing > > > `Atomic<{i32,i64}>`, not a complex design) > > > > > > So it is up to you on what you send for the non-RFC patches, of > > > course, and if nobody has the time / wants to do the work for the > > > "simple" generic approach, then we can just go ahead with this for the > > > moment. But I think it would be nice to at least consider the "simple" > > > generic approach to see how much worse it would be. > > > > > > Other bits to consider, that perhaps give you arguments for one or the > > > other: consequences on the compilation time, on inlining, on the error > > > messages for new users, on the generated documentation, on how easy to > > > grep they are, etc. > > > > Yeah, rereading the thread - I'm with Miguel and Gary. > > > > Generics are simply the correct way to do it, if the wider rust > > community didn't do it that way I think that can be chalked up more to > > historical baggage or needlessly copying the base integer type scheme. > > > > Let's please do it right here, and generics are the correct approach. > I think the disagreement here is not non-generic atomic vs generic atomic, it's pure generic atomic vs. AtomicI{32,64} etc + generic atomic. I said multiple times that I'm OK with generic atomics if there are real users, just I'm not sure it's something we want to do right now (or we have enough information to go fully on that direction). And I think it's fine to have non-generic atomic and generic atomic coexist. Regards, Boqun > If so, maybe we should do u<Wide> instead of u8, u16, oh, and probably > just Integer<Sign, Wide> instead of i{8,16,32,64) and u{8,16,32,64} ;-) > > Regards, > Boqun
On Sun, Jun 16, 2024 at 10:30:03AM -0700, Boqun Feng wrote: > I think the disagreement here is not non-generic atomic vs generic > atomic, it's pure generic atomic vs. AtomicI{32,64} etc + generic > atomic. I said multiple times that I'm OK with generic atomics if there > are real users, just I'm not sure it's something we want to do right now > (or we have enough information to go fully on that direction). And I > think it's fine to have non-generic atomic and generic atomic coexist. Well, having the generic interface from the start does matter, that's what we (myself included) want to code to. No need to overcomplicate it, just Atomic<u8> Atomic<u16> etc... As long as that's available, the internal implementation shouldn't have to change.
On Sun, Jun 16, 2024 at 08:06:05AM -0700, Boqun Feng wrote: [...] > > > > Note that crossbeam's AtomicCell is also generic, and crossbeam is used > > by tons of crates. As Miguel mentioned, I think it's very likely that in > > the future we want be able to do atomics on new types (e.g. for > > seqlocks perhaps). We probably don't need the non-lock-free fallback of > > Good, another design bit, thank you! > > What's our overall idea on sub-word types, like Atomic<u8> and > Atomic<u16>, do we plan to say no to them, or they could have a limited > APIs? IIUC, some operations on them are relatively sub-optimal on some > architectures, supporting the same set of API as i32 and i64 is probably > a bad idea. > > Another thing in my mind is making `Atomic<T>` > > pub struct Atomic<T: Send + ...> { ... } > > so that `Atomic<T>` will always be `Sync`, because quite frankly, an > atomic type that cannot `Sync` is pointless. > Also, how do we avoid this issue [1] in kernel? `atomic_load()` in C is implemented as READ_ONCE() and it's, at most time, a volatile read, so the eventual code is: let a: (u8, u16) = (1, 2); let b = unsafe { core::ptr::read_volatile::<i32>(&a as *const _ as *const i32) }; I know we probably ignore data race here and treat `read_volatile` as a dependency read per LKMM [2]. But this is an using of uninitialized data, so it's a bit different. We can do what https://crates.io/crates/atomic does: pub struct Atomic<T: NoUninit + ..> { ... } , where `NoUinit` means no internal padding bytes, but it loses the ability to put a #[repr(u32)] pub enum Foo { .. } into `Atomic<T>`, right? Which is probably a case you want to support? Regards, Boqun [1]: https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1133926617 [2]: tools/memory-model/Documentation/access-marking.txt > Regards, > Boqun > > > crossbeam's AtomicCell, but the lock-free subset with newtype support > > is desirable. > > > > People in general don't use the `atomic` crate because it provides no > > additional feature compared to the standard library. But it doesn't > > really mean that the standard library's atomic design is good. > > > > People decided to use AtomicT and NonZeroT instead of Atomic<T> or > > NonZero<T> long time ago, but many now thinks the decision was bad. > > Introduction of NonZero<T> is a good example of it. NonZeroT are now > > all type aliases of NonZero<T>. > > > > I also don't see any downside in using generics. We can provide type > > aliases so people can use `AtomicI32` and `AtomicI64` when they want > > their code to be compatible with userspace Rust can still do so. > > > > `Atomic<i32>` is also just aesthetically better than `AtomicI32` IMO. > > When all other types look like `NonZero<i32>`, `Wrapping<i32>`, I don't > > think we should have `AtomicI32` just because "it's done this way in > > Rust std". Our alloc already deviates a lot from Rust std. > > > > Best, > > Gary
On Sun, Jun 16, 2024 at 10:36:13PM -0700, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 08:06:05AM -0700, Boqun Feng wrote: > [...] > > > > > > Note that crossbeam's AtomicCell is also generic, and crossbeam is used > > > by tons of crates. As Miguel mentioned, I think it's very likely that in > > > the future we want be able to do atomics on new types (e.g. for > > > seqlocks perhaps). We probably don't need the non-lock-free fallback of > > > > Good, another design bit, thank you! > > > > What's our overall idea on sub-word types, like Atomic<u8> and > > Atomic<u16>, do we plan to say no to them, or they could have a limited > > APIs? IIUC, some operations on them are relatively sub-optimal on some > > architectures, supporting the same set of API as i32 and i64 is probably > > a bad idea. > > > > Another thing in my mind is making `Atomic<T>` > > > > pub struct Atomic<T: Send + ...> { ... } > > > > so that `Atomic<T>` will always be `Sync`, because quite frankly, an > > atomic type that cannot `Sync` is pointless. > > > > Also, how do we avoid this issue [1] in kernel? > > `atomic_load()` in C is implemented as READ_ONCE() and it's, at most > time, a volatile read, so the eventual code is: > > let a: (u8, u16) = (1, 2); > let b = unsafe { core::ptr::read_volatile::<i32>(&a as *const _ as *const i32) }; > ^^^^ this line should really be: let b: (u8, u16) = unsafe { transmute_copy(&read_volatile::<i32>(&a as *const _ as *const i32)) }; but you get the idea. Regards, Boqun > I know we probably ignore data race here and treat `read_volatile` as a > dependency read per LKMM [2]. But this is an using of uninitialized > data, so it's a bit different. > > We can do what https://crates.io/crates/atomic does: > > pub struct Atomic<T: NoUninit + ..> { ... } > > , where `NoUinit` means no internal padding bytes, but it loses the > ability to put a > > #[repr(u32)] > pub enum Foo { .. } > > into `Atomic<T>`, right? Which is probably a case you want to support? > > Regards, > Boqun > > [1]: https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1133926617 > [2]: tools/memory-model/Documentation/access-marking.txt > > > Regards, > > Boqun > > > > > crossbeam's AtomicCell, but the lock-free subset with newtype support > > > is desirable. > > > > > > People in general don't use the `atomic` crate because it provides no > > > additional feature compared to the standard library. But it doesn't > > > really mean that the standard library's atomic design is good. > > > > > > People decided to use AtomicT and NonZeroT instead of Atomic<T> or > > > NonZero<T> long time ago, but many now thinks the decision was bad. > > > Introduction of NonZero<T> is a good example of it. NonZeroT are now > > > all type aliases of NonZero<T>. > > > > > > I also don't see any downside in using generics. We can provide type > > > aliases so people can use `AtomicI32` and `AtomicI64` when they want > > > their code to be compatible with userspace Rust can still do so. > > > > > > `Atomic<i32>` is also just aesthetically better than `AtomicI32` IMO. > > > When all other types look like `NonZero<i32>`, `Wrapping<i32>`, I don't > > > think we should have `AtomicI32` just because "it's done this way in > > > Rust std". Our alloc already deviates a lot from Rust std. > > > > > > Best, > > > Gary
On 16.06.24 18:30, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 03:55:12PM +0000, Benno Lossin wrote: > [...] >>>> >>>> I don't think that the idea was to "do the design later". I don't even >>>> know how you would do that, since you need the design to submit a patch. >>>> >>> >>> Then I might mis-understand Gary? He said: >>> >>> "Can we avoid two types and use a generic `Atomic<T>` and then implement >>> on `Atomic<i32>` and `Atomic<i64>` instead?" >>> >>> , which means just replace `impl AtomicI32` with `impl Atomic<i32>` to >>> me. >> >> This is a fair interpretation, but what prevents you from merging the >> impls of functions that can be? I assumed that you would do that >> automatically. >> > > I think you missed the point, Gary's suggestion at that time sounds > like: let's impl Atomic<i32> and Atomic<i64> first, and leave rest of > the work for later, that is a "do the design later" to me. Hmm, but wouldn't that just be less work for you? >>>> I can't offer you a complete API description, since that would require >>>> me writing it up myself. But I would recommend trying to get it to work >>>> with generics. I got a few other comments: >>> >>> We should work on things that are concrete, right? It's fine that the >>> design is not complete, and it's fine if you just recommend. But without >>> a somewhat concrete design (doesn't have to be complete), I cannot be >>> sure about whether we have the same vision of the future of generic >>> atomics (see my question to Gary), that's a bit hard for me to try to >> >> Sorry, which question? > > https://lore.kernel.org/rust-for-linux/Zm7_XWe6ciy1yN-h@Boquns-Mac-mini.home/ > >> Also to be aligned on the vision, I think we should rather talk about >> the vision and not the design, since the design that we want to have now >> can be different from the vision. On that note, what do you envision the >> future of the atomic API? >> > > Mine is simple, just providing AtomicI32 and AtomicI64 first, since > there are immediate users right now, and should we learn more needs from > the users, we get more idea about what a good API is, and we evolve from > there. That is fine, but since we want to get generics in the future anyways, I think it would be good to already implement them now to also gather feedback on them. >>> work something out (plus I personally don't think it's a good idea, it's >>> OK to me, but not good). Anyway, I wasn't trying to refuse to do this >>> just based on personal reasons, but I do need to understand what you are >>> all proposing, because I don't have one myself. >> >> I went back through the thread and here is what I think your argument >> against generics is: people should think about size and alignment when >> using atomics, which is problematic when allowing users to leave the >> atomic generic. >> But as I argued before, this is not an issue. Have I overlooked another > > You mean you said it's a non-issue but gave me two counteract? If it's > really a non-issue, you won't need a couneraction, right? In other words > non-generic atomics do provide some value. The second counteractions would provide exactly the same API surface as your non-generic version, so I don't see how going non-generic provides any value over going generic. The first approach was intended for a future in which we are not scared of people using generic atomics in their structs. I don't know if we are going to be in that future, so I think we should go with the second approach for the time being. >> argument? Because I don't see anything else. >> >>>> - I don't think that we should resort to a script to generate the Rust >>>> code since it prevents adding good documentation & examples to the >>>> various methods. AFAIU you want to generate the functions from >>>> `scripts/atomic/atomics.tbl` to keep it in sync with the C side. I >>>> looked at the git log of that file and it hasn't been changed >>>> significantly since its inception. I don't think that there is any >>>> benefit to generating the functions from that file. >>> >>> I'll leave this to other atomic maintainers. >>> >>>> - most of the documented functions say "See `c_function`", I don't like >>>> this, can we either copy the C documentation (I imagine it not >>>> changing that often, or is that assumption wrong?) or write our own? >>> >>> You're not wrong. AN in C side, we do have some documentation template >>> to generate the comments (see scripts/atomic/kerneldoc). But first the >>> format is for doxygen(I guess?), and second as you just bring up, the >>> templates are tied with the bash script. >> >> I see a bash script similarly to how Wedson sees proc macros ;) >> We should try *hard* to avoid them and only use them when there is no >> other way. >> > > I will just start with the existing mechanism and try to evolve, whether > it's a script or proc macro, I don't mind, I want to get the work done > and most relevant people can understand in the way the they prefer and > step-by-step, move it to the place I think is the best for the project. I don't think that we need a script or a proc macro. A few declarative macros probably suffice if we go the way of generics. >>>> - we should try to use either const generic or normal parameters for the >>>> access ordering instead of putting it in the function name. >>> >>> Why is it important? Keeping it in the current way brings the value that >>> it's not too much different than their C counterparts. Could you explain >>> a bit the pros and cons on suffix vs const generic approach? >> >> Reduce code duplication, instead of 3 different variants, we can have >> one. It allows people to build ergonomic APIs that allows the user to >> decide which synchronization they use under the hood. >> > > I already mentioned why I think it's good in the current way, I will > defer it to others on their inputs. > >>>> - why do we need both non-return and return variants? >>>> >>> >>> Historical reason I guess. Plus maybe some architectures have a better >>> implementation on non-return atomics IIRC. >> >> Could we get some more concrete arguments for why we would need these in >> rust? If the reason is purely historical, then we shouldn't expose the > > Sure. Look like my memory is correct, at least on ARM64 they are > different instructions (see arch/arm64/include/asm/atomic_lse.h) > > non-return atomics on ARM64: > > #define ATOMIC_OP(op, asm_op) \ > static __always_inline void \ > __lse_atomic_##op(int i, atomic_t *v) \ > { \ > asm volatile( \ > __LSE_PREAMBLE \ > " " #asm_op " %w[i], %[v]\n" \ > : [v] "+Q" (v->counter) \ > : [i] "r" (i)); \ > } > > value-return atomics on ARM64: > > #define ATOMIC_FETCH_OP(name, mb, op, asm_op, cl...) \ > static __always_inline int \ > __lse_atomic_fetch_##op##name(int i, atomic_t *v) \ > { \ > int old; \ > \ > asm volatile( \ > __LSE_PREAMBLE \ > " " #asm_op #mb " %w[i], %w[old], %[v]" \ > : [v] "+Q" (v->counter), \ > [old] "=r" (old) \ > : [i] "r" (i) \ > : cl); \ > \ > return old; \ > } > > It may not be easy to see the different instructions from the pasted > code, but you can find them in the header file, also you could notice > that the number of operands is different? This is not my expertise, so I believe you :) >> non-return variant IMO. If it is because of performance, then we can >> only expose them in the respective arches. >> > > Hmm.. so we ask user to write arch-specific code like: > > pub fn increase_counter(&self) { > #[cfg(CONFIG_ARM64)] > self.counter.inc(); > > #[cfg(CONFIG_X86_64)] > let _ = self.counter.inc_return(); > } > > are you sure it's a good idea? No that looks horrible. Maybe there is something that we can do with generics, but I don't know if it is worth it. I guess we can leave that open for the time being. --- Cheers, Benno
On 17.06.24 07:36, Boqun Feng wrote: > On Sun, Jun 16, 2024 at 08:06:05AM -0700, Boqun Feng wrote: > [...] >>> >>> Note that crossbeam's AtomicCell is also generic, and crossbeam is used >>> by tons of crates. As Miguel mentioned, I think it's very likely that in >>> the future we want be able to do atomics on new types (e.g. for >>> seqlocks perhaps). We probably don't need the non-lock-free fallback of >> >> Good, another design bit, thank you! >> >> What's our overall idea on sub-word types, like Atomic<u8> and >> Atomic<u16>, do we plan to say no to them, or they could have a limited >> APIs? IIUC, some operations on them are relatively sub-optimal on some >> architectures, supporting the same set of API as i32 and i64 is probably >> a bad idea. >> >> Another thing in my mind is making `Atomic<T>` >> >> pub struct Atomic<T: Send + ...> { ... } >> >> so that `Atomic<T>` will always be `Sync`, because quite frankly, an >> atomic type that cannot `Sync` is pointless. That is true, but adding semantically "unnecessary" bounds can be bad. This is because they infect everything that wants to use `Atomic<T>`, since they also need to add that bound. > Also, how do we avoid this issue [1] in kernel? I think that we can first go the way of my second approach (ie adding a private trait as a bound on `Atomic<T>` to prevent generic usage). And only allow primitives. If we then see that people would like to put their own (u8, u16) tuple structs into `Atomic<T>`, we have multiple options: 1. Field projection: Only primitives can be `load`ed and `store`ed, to access the values of the tuple, one would need to project to each field and read them. 2. Disallow padding: We add an `unsafe` trait that asserts there are no padding bytes in there (like `NoUinit` from below) and also add a macro that implements the trait safely. 3. Use `MaybeUninit` under the hood: I don't know if this would fix the issue entirely, since that is what crossbeam currently uses (but the issue remains open). But I don't think that we should encourage large structs to be put into `Atomic<T>`, since that would be bad for perf, right? So I think that going the way of 1 would be great (if we had FP, otherwise 2 seems fine). > `atomic_load()` in C is implemented as READ_ONCE() and it's, at most > time, a volatile read, so the eventual code is: > > let a: (u8, u16) = (1, 2); > let b = unsafe { core::ptr::read_volatile::<i32>(&a as *const _ as *const i32) }; > > I know we probably ignore data race here and treat `read_volatile` as a > dependency read per LKMM [2]. But this is an using of uninitialized > data, so it's a bit different. But would we implement it this way? Or would it go through a C function? If we entirely do it in Rust, then yes this is bad. --- Cheers, Benno > We can do what https://crates.io/crates/atomic does: > > pub struct Atomic<T: NoUninit + ..> { ... } > > , where `NoUinit` means no internal padding bytes, but it loses the > ability to put a > > #[repr(u32)] > pub enum Foo { .. } > > into `Atomic<T>`, right? Which is probably a case you want to support? > > Regards, > Boqun > > [1]: https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1133926617 > [2]: tools/memory-model/Documentation/access-marking.txt
On Wed, Jun 19, 2024 at 09:09:46AM +0000, Benno Lossin wrote: [...] > >>> Then I might mis-understand Gary? He said: > >>> > >>> "Can we avoid two types and use a generic `Atomic<T>` and then implement > >>> on `Atomic<i32>` and `Atomic<i64>` instead?" > >>> > >>> , which means just replace `impl AtomicI32` with `impl Atomic<i32>` to > >>> me. > >> > >> This is a fair interpretation, but what prevents you from merging the > >> impls of functions that can be? I assumed that you would do that > >> automatically. > >> > > > > I think you missed the point, Gary's suggestion at that time sounds > > like: let's impl Atomic<i32> and Atomic<i64> first, and leave rest of > > the work for later, that is a "do the design later" to me. > > Hmm, but wouldn't that just be less work for you? > Not really ;-) Committing to a generic interface without a vision/design is going to be a maintenace nightmare. I will need to deal the soundness issues and different ideas about what can be put in <..>. So I'd suggest we take great caution before we make our decision, and I may even ask for a formal proof of the soundness of a generic interface for key components such as atomics. Because last time I check, safety is what Rust brings on the table, right? Someone may say that non generic atomics is one of the biggest mistakes that Rust ever made. But I disagree. In fact when I saw this, my first impression was "these guys know their stuff": there are only a few types and operations that make sense, so putting them in a generic interface would be over-engineer (at least at the beginning). It's better correct than convenient/popular/aesthetical. So would you and Gary be interested at working on such a design and proof? Maybe someone else Cced would also be interested. Eventually we will need the tool of soundness proof for other types as well, that's going to be very vaulable. And having that would be the real "less work for me" ;-) Right now, I still plan to do in the current way (i.e. non-generic atomcis for i32, i64, isize) because there are already users [1], the correctness is easy to confirm and we won't confuse users with half-baked generic design. But should we have a sound Atomic<T> design, I'm happy to adopt it ;-) [...] > >> Also to be aligned on the vision, I think we should rather talk about > >> the vision and not the design, since the design that we want to have now > >> can be different from the vision. On that note, what do you envision the > >> future of the atomic API? > >> > > > > Mine is simple, just providing AtomicI32 and AtomicI64 first, since > > there are immediate users right now, and should we learn more needs from > > the users, we get more idea about what a good API is, and we evolve from > > there. > > That is fine, but since we want to get generics in the future anyways, I > think it would be good to already implement them now to also gather > feedback on them. > [...] > > > > You mean you said it's a non-issue but gave me two counteract? If it's > > really a non-issue, you won't need a couneraction, right? In other words > > non-generic atomics do provide some value. > > The second counteractions would provide exactly the same API surface as > your non-generic version, so I don't see how going non-generic provides > any value over going generic. > The first approach was intended for a future in which we are not scared > of people using generic atomics in their structs. I don't know if we are > going to be in that future, so I think we should go with the second > approach for the time being. > Not going to reply the above right now, because I'm leaning more torwards switching when we have a sound Atomic<T> design, but I want you to know I appreciate your input and explanation. > >> argument? Because I don't see anything else. > >> > >>>> - I don't think that we should resort to a script to generate the Rust > >>>> code since it prevents adding good documentation & examples to the > >>>> various methods. AFAIU you want to generate the functions from > >>>> `scripts/atomic/atomics.tbl` to keep it in sync with the C side. I > >>>> looked at the git log of that file and it hasn't been changed > >>>> significantly since its inception. I don't think that there is any > >>>> benefit to generating the functions from that file. > >>> > >>> I'll leave this to other atomic maintainers. > >>> > >>>> - most of the documented functions say "See `c_function`", I don't like > >>>> this, can we either copy the C documentation (I imagine it not > >>>> changing that often, or is that assumption wrong?) or write our own? > >>> > >>> You're not wrong. AN in C side, we do have some documentation template > >>> to generate the comments (see scripts/atomic/kerneldoc). But first the > >>> format is for doxygen(I guess?), and second as you just bring up, the > >>> templates are tied with the bash script. > >> > >> I see a bash script similarly to how Wedson sees proc macros ;) > >> We should try *hard* to avoid them and only use them when there is no > >> other way. > >> > > > > I will just start with the existing mechanism and try to evolve, whether > > it's a script or proc macro, I don't mind, I want to get the work done > > and most relevant people can understand in the way the they prefer and > > step-by-step, move it to the place I think is the best for the project. > > I don't think that we need a script or a proc macro. A few declarative > macros probably suffice if we go the way of generics. > Ah right. And even without generics we could use some declarative macros, and I'm happy to take a look at this and provide the alternative approach so we can choose a better one. [1]: https://lore.kernel.org/rust-for-linux/20240611114551.228679-2-nmi@metaspace.dk/ Regards, Boqun
diff --git a/MAINTAINERS b/MAINTAINERS index d6c90161c7bf..a8528d27b260 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3458,7 +3458,7 @@ F: drivers/input/touchscreen/atmel_mxt_ts.c ATOMIC INFRASTRUCTURE M: Will Deacon <will@kernel.org> M: Peter Zijlstra <peterz@infradead.org> -R: Boqun Feng <boqun.feng@gmail.com> +M: Boqun Feng <boqun.feng@gmail.com> R: Mark Rutland <mark.rutland@arm.com> L: linux-kernel@vger.kernel.org S: Maintained @@ -3467,6 +3467,8 @@ F: arch/*/include/asm/atomic*.h F: include/*/atomic*.h F: include/linux/refcount.h F: scripts/atomic/ +F: rust/kernel/sync/atomic.rs +F: rust/kernel/sync/atomic/ ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER M: Bradley Grove <linuxdrivers@attotech.com> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 48e7029f1054..99e6e2b2867f 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1601,6 +1601,8 @@ static bool has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope) { u64 val = read_scoped_sysreg(entry, scope); + if (entry->capability == ARM64_HAS_LSE_ATOMICS) + return false; return feature_matches(val, entry); } diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 0ab20975a3b5..66ac3752ca71 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -8,6 +8,7 @@ use crate::types::Opaque; mod arc; +pub mod atomic; mod condvar; pub mod lock; mod locked_by; diff --git a/rust/kernel/sync/atomic.rs b/rust/kernel/sync/atomic.rs new file mode 100644 index 000000000000..b0f852cf1741 --- /dev/null +++ b/rust/kernel/sync/atomic.rs @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Atomic primitives. +//! +//! These primitives have the same semantics as their C counterparts, for precise definitions of +//! the semantics, please refer to tools/memory-model. Note that Linux Kernel Memory (Consistency) +//! Model is the only model for Rust development in kernel right now, please avoid to use Rust's +//! own atomics. + +use crate::bindings::{atomic64_t, atomic_t}; +use crate::types::Opaque; + +mod r#impl; + +/// Atomic 32bit signed integers. +pub struct AtomicI32(Opaque<atomic_t>); + +/// Atomic 64bit signed integers. +pub struct AtomicI64(Opaque<atomic64_t>); + +impl AtomicI32 { + /// Creates an atomic variable with initial value `v`. + /// + /// # Examples + /// + /// ```rust + /// # use kernel::sync::atomic::*; + /// + /// let x = AtomicI32::new(0); + /// + /// assert_eq!(x.read(), 0); + /// assert_eq!(x.fetch_add_relaxed(12), 0); + /// assert_eq!(x.read(), 12); + /// ``` + pub const fn new(v: i32) -> Self { + AtomicI32(Opaque::new(atomic_t { counter: v })) + } +} + +// SAFETY: `AtomicI32` is safe to share among execution contexts because all accesses are atomic. +unsafe impl Sync for AtomicI32 {} + +impl AtomicI64 { + /// Creates an atomic variable with initial value `v`. + /// + /// # Examples + /// + /// ```rust + /// # use kernel::sync::atomic::*; + /// + /// let x = AtomicI64::new(0); + /// + /// assert_eq!(x.read(), 0); + /// assert_eq!(x.fetch_add_relaxed(12), 0); + /// assert_eq!(x.read(), 12); + /// ``` + pub const fn new(v: i64) -> Self { + AtomicI64(Opaque::new(atomic64_t { counter: v })) + } +} + +// SAFETY: `AtomicI64` is safe to share among execution contexts because all accesses are atomic. +unsafe impl Sync for AtomicI64 {} diff --git a/rust/kernel/sync/atomic/impl.rs b/rust/kernel/sync/atomic/impl.rs new file mode 100644 index 000000000000..1bbb0a714834 --- /dev/null +++ b/rust/kernel/sync/atomic/impl.rs @@ -0,0 +1,1375 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Generated by scripts/atomic/gen-rust-atomic.sh +//! DO NOT MODIFY THIS FILE DIRECTLY + +use super::*; +use crate::bindings::*; + +impl AtomicI32 { + /// See `atomic_read`. + #[inline(always)] + pub fn read(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_read(self.0.get()); + } + } + /// See `atomic_read_acquire`. + #[inline(always)] + pub fn read_acquire(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_read_acquire(self.0.get()); + } + } + /// See `atomic_set`. + #[inline(always)] + pub fn set(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_set(self.0.get(), i); + } + } + /// See `atomic_set_release`. + #[inline(always)] + pub fn set_release(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_set_release(self.0.get(), i); + } + } + /// See `atomic_add`. + #[inline(always)] + pub fn add(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_add(i, self.0.get()); + } + } + /// See `atomic_add_return`. + #[inline(always)] + pub fn add_return(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_return(i, self.0.get()); + } + } + /// See `atomic_add_return_acquire`. + #[inline(always)] + pub fn add_return_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_return_acquire(i, self.0.get()); + } + } + /// See `atomic_add_return_release`. + #[inline(always)] + pub fn add_return_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_return_release(i, self.0.get()); + } + } + /// See `atomic_add_return_relaxed`. + #[inline(always)] + pub fn add_return_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_return_relaxed(i, self.0.get()); + } + } + /// See `atomic_fetch_add`. + #[inline(always)] + pub fn fetch_add(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_add(i, self.0.get()); + } + } + /// See `atomic_fetch_add_acquire`. + #[inline(always)] + pub fn fetch_add_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_add_acquire(i, self.0.get()); + } + } + /// See `atomic_fetch_add_release`. + #[inline(always)] + pub fn fetch_add_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_add_release(i, self.0.get()); + } + } + /// See `atomic_fetch_add_relaxed`. + #[inline(always)] + pub fn fetch_add_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_add_relaxed(i, self.0.get()); + } + } + /// See `atomic_sub`. + #[inline(always)] + pub fn sub(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_sub(i, self.0.get()); + } + } + /// See `atomic_sub_return`. + #[inline(always)] + pub fn sub_return(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_sub_return(i, self.0.get()); + } + } + /// See `atomic_sub_return_acquire`. + #[inline(always)] + pub fn sub_return_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_sub_return_acquire(i, self.0.get()); + } + } + /// See `atomic_sub_return_release`. + #[inline(always)] + pub fn sub_return_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_sub_return_release(i, self.0.get()); + } + } + /// See `atomic_sub_return_relaxed`. + #[inline(always)] + pub fn sub_return_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_sub_return_relaxed(i, self.0.get()); + } + } + /// See `atomic_fetch_sub`. + #[inline(always)] + pub fn fetch_sub(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_sub(i, self.0.get()); + } + } + /// See `atomic_fetch_sub_acquire`. + #[inline(always)] + pub fn fetch_sub_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_sub_acquire(i, self.0.get()); + } + } + /// See `atomic_fetch_sub_release`. + #[inline(always)] + pub fn fetch_sub_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_sub_release(i, self.0.get()); + } + } + /// See `atomic_fetch_sub_relaxed`. + #[inline(always)] + pub fn fetch_sub_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_sub_relaxed(i, self.0.get()); + } + } + /// See `atomic_inc`. + #[inline(always)] + pub fn inc(&self) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_inc(self.0.get()); + } + } + /// See `atomic_inc_return`. + #[inline(always)] + pub fn inc_return(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_inc_return(self.0.get()); + } + } + /// See `atomic_inc_return_acquire`. + #[inline(always)] + pub fn inc_return_acquire(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_inc_return_acquire(self.0.get()); + } + } + /// See `atomic_inc_return_release`. + #[inline(always)] + pub fn inc_return_release(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_inc_return_release(self.0.get()); + } + } + /// See `atomic_inc_return_relaxed`. + #[inline(always)] + pub fn inc_return_relaxed(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_inc_return_relaxed(self.0.get()); + } + } + /// See `atomic_fetch_inc`. + #[inline(always)] + pub fn fetch_inc(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_inc(self.0.get()); + } + } + /// See `atomic_fetch_inc_acquire`. + #[inline(always)] + pub fn fetch_inc_acquire(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_inc_acquire(self.0.get()); + } + } + /// See `atomic_fetch_inc_release`. + #[inline(always)] + pub fn fetch_inc_release(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_inc_release(self.0.get()); + } + } + /// See `atomic_fetch_inc_relaxed`. + #[inline(always)] + pub fn fetch_inc_relaxed(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_inc_relaxed(self.0.get()); + } + } + /// See `atomic_dec`. + #[inline(always)] + pub fn dec(&self) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_dec(self.0.get()); + } + } + /// See `atomic_dec_return`. + #[inline(always)] + pub fn dec_return(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_dec_return(self.0.get()); + } + } + /// See `atomic_dec_return_acquire`. + #[inline(always)] + pub fn dec_return_acquire(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_dec_return_acquire(self.0.get()); + } + } + /// See `atomic_dec_return_release`. + #[inline(always)] + pub fn dec_return_release(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_dec_return_release(self.0.get()); + } + } + /// See `atomic_dec_return_relaxed`. + #[inline(always)] + pub fn dec_return_relaxed(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_dec_return_relaxed(self.0.get()); + } + } + /// See `atomic_fetch_dec`. + #[inline(always)] + pub fn fetch_dec(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_dec(self.0.get()); + } + } + /// See `atomic_fetch_dec_acquire`. + #[inline(always)] + pub fn fetch_dec_acquire(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_dec_acquire(self.0.get()); + } + } + /// See `atomic_fetch_dec_release`. + #[inline(always)] + pub fn fetch_dec_release(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_dec_release(self.0.get()); + } + } + /// See `atomic_fetch_dec_relaxed`. + #[inline(always)] + pub fn fetch_dec_relaxed(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_dec_relaxed(self.0.get()); + } + } + /// See `atomic_and`. + #[inline(always)] + pub fn and(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_and(i, self.0.get()); + } + } + /// See `atomic_fetch_and`. + #[inline(always)] + pub fn fetch_and(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_and(i, self.0.get()); + } + } + /// See `atomic_fetch_and_acquire`. + #[inline(always)] + pub fn fetch_and_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_and_acquire(i, self.0.get()); + } + } + /// See `atomic_fetch_and_release`. + #[inline(always)] + pub fn fetch_and_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_and_release(i, self.0.get()); + } + } + /// See `atomic_fetch_and_relaxed`. + #[inline(always)] + pub fn fetch_and_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_and_relaxed(i, self.0.get()); + } + } + /// See `atomic_andnot`. + #[inline(always)] + pub fn andnot(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_andnot(i, self.0.get()); + } + } + /// See `atomic_fetch_andnot`. + #[inline(always)] + pub fn fetch_andnot(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_andnot(i, self.0.get()); + } + } + /// See `atomic_fetch_andnot_acquire`. + #[inline(always)] + pub fn fetch_andnot_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_andnot_acquire(i, self.0.get()); + } + } + /// See `atomic_fetch_andnot_release`. + #[inline(always)] + pub fn fetch_andnot_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_andnot_release(i, self.0.get()); + } + } + /// See `atomic_fetch_andnot_relaxed`. + #[inline(always)] + pub fn fetch_andnot_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_andnot_relaxed(i, self.0.get()); + } + } + /// See `atomic_or`. + #[inline(always)] + pub fn or(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_or(i, self.0.get()); + } + } + /// See `atomic_fetch_or`. + #[inline(always)] + pub fn fetch_or(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_or(i, self.0.get()); + } + } + /// See `atomic_fetch_or_acquire`. + #[inline(always)] + pub fn fetch_or_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_or_acquire(i, self.0.get()); + } + } + /// See `atomic_fetch_or_release`. + #[inline(always)] + pub fn fetch_or_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_or_release(i, self.0.get()); + } + } + /// See `atomic_fetch_or_relaxed`. + #[inline(always)] + pub fn fetch_or_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_or_relaxed(i, self.0.get()); + } + } + /// See `atomic_xor`. + #[inline(always)] + pub fn xor(&self, i: i32) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic_xor(i, self.0.get()); + } + } + /// See `atomic_fetch_xor`. + #[inline(always)] + pub fn fetch_xor(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_xor(i, self.0.get()); + } + } + /// See `atomic_fetch_xor_acquire`. + #[inline(always)] + pub fn fetch_xor_acquire(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_xor_acquire(i, self.0.get()); + } + } + /// See `atomic_fetch_xor_release`. + #[inline(always)] + pub fn fetch_xor_release(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_xor_release(i, self.0.get()); + } + } + /// See `atomic_fetch_xor_relaxed`. + #[inline(always)] + pub fn fetch_xor_relaxed(&self, i: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_xor_relaxed(i, self.0.get()); + } + } + /// See `atomic_xchg`. + #[inline(always)] + pub fn xchg(&self, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_xchg(self.0.get(), new); + } + } + /// See `atomic_xchg_acquire`. + #[inline(always)] + pub fn xchg_acquire(&self, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_xchg_acquire(self.0.get(), new); + } + } + /// See `atomic_xchg_release`. + #[inline(always)] + pub fn xchg_release(&self, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_xchg_release(self.0.get(), new); + } + } + /// See `atomic_xchg_relaxed`. + #[inline(always)] + pub fn xchg_relaxed(&self, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_xchg_relaxed(self.0.get(), new); + } + } + /// See `atomic_cmpxchg`. + #[inline(always)] + pub fn cmpxchg(&self, old: i32, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_cmpxchg(self.0.get(), old, new); + } + } + /// See `atomic_cmpxchg_acquire`. + #[inline(always)] + pub fn cmpxchg_acquire(&self, old: i32, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_cmpxchg_acquire(self.0.get(), old, new); + } + } + /// See `atomic_cmpxchg_release`. + #[inline(always)] + pub fn cmpxchg_release(&self, old: i32, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_cmpxchg_release(self.0.get(), old, new); + } + } + /// See `atomic_cmpxchg_relaxed`. + #[inline(always)] + pub fn cmpxchg_relaxed(&self, old: i32, new: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_cmpxchg_relaxed(self.0.get(), old, new); + } + } + /// See `atomic_try_cmpxchg`. + #[inline(always)] + pub fn try_cmpxchg(&self, old: &mut i32, new: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_try_cmpxchg(self.0.get(), old, new); + } + } + /// See `atomic_try_cmpxchg_acquire`. + #[inline(always)] + pub fn try_cmpxchg_acquire(&self, old: &mut i32, new: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_try_cmpxchg_acquire(self.0.get(), old, new); + } + } + /// See `atomic_try_cmpxchg_release`. + #[inline(always)] + pub fn try_cmpxchg_release(&self, old: &mut i32, new: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_try_cmpxchg_release(self.0.get(), old, new); + } + } + /// See `atomic_try_cmpxchg_relaxed`. + #[inline(always)] + pub fn try_cmpxchg_relaxed(&self, old: &mut i32, new: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_try_cmpxchg_relaxed(self.0.get(), old, new); + } + } + /// See `atomic_sub_and_test`. + #[inline(always)] + pub fn sub_and_test(&self, i: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_sub_and_test(i, self.0.get()); + } + } + /// See `atomic_dec_and_test`. + #[inline(always)] + pub fn dec_and_test(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_dec_and_test(self.0.get()); + } + } + /// See `atomic_inc_and_test`. + #[inline(always)] + pub fn inc_and_test(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_inc_and_test(self.0.get()); + } + } + /// See `atomic_add_negative`. + #[inline(always)] + pub fn add_negative(&self, i: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_negative(i, self.0.get()); + } + } + /// See `atomic_add_negative_acquire`. + #[inline(always)] + pub fn add_negative_acquire(&self, i: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_negative_acquire(i, self.0.get()); + } + } + /// See `atomic_add_negative_release`. + #[inline(always)] + pub fn add_negative_release(&self, i: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_negative_release(i, self.0.get()); + } + } + /// See `atomic_add_negative_relaxed`. + #[inline(always)] + pub fn add_negative_relaxed(&self, i: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_negative_relaxed(i, self.0.get()); + } + } + /// See `atomic_fetch_add_unless`. + #[inline(always)] + pub fn fetch_add_unless(&self, a: i32, u: i32) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_fetch_add_unless(self.0.get(), a, u); + } + } + /// See `atomic_add_unless`. + #[inline(always)] + pub fn add_unless(&self, a: i32, u: i32) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_add_unless(self.0.get(), a, u); + } + } + /// See `atomic_inc_not_zero`. + #[inline(always)] + pub fn inc_not_zero(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_inc_not_zero(self.0.get()); + } + } + /// See `atomic_inc_unless_negative`. + #[inline(always)] + pub fn inc_unless_negative(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_inc_unless_negative(self.0.get()); + } + } + /// See `atomic_dec_unless_positive`. + #[inline(always)] + pub fn dec_unless_positive(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_dec_unless_positive(self.0.get()); + } + } + /// See `atomic_dec_if_positive`. + #[inline(always)] + pub fn dec_if_positive(&self) -> i32 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic_dec_if_positive(self.0.get()); + } + } +} + +impl AtomicI64 { + /// See `atomic64_read`. + #[inline(always)] + pub fn read(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_read(self.0.get()); + } + } + /// See `atomic64_read_acquire`. + #[inline(always)] + pub fn read_acquire(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_read_acquire(self.0.get()); + } + } + /// See `atomic64_set`. + #[inline(always)] + pub fn set(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_set(self.0.get(), i); + } + } + /// See `atomic64_set_release`. + #[inline(always)] + pub fn set_release(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_set_release(self.0.get(), i); + } + } + /// See `atomic64_add`. + #[inline(always)] + pub fn add(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_add(i, self.0.get()); + } + } + /// See `atomic64_add_return`. + #[inline(always)] + pub fn add_return(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_return(i, self.0.get()); + } + } + /// See `atomic64_add_return_acquire`. + #[inline(always)] + pub fn add_return_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_return_acquire(i, self.0.get()); + } + } + /// See `atomic64_add_return_release`. + #[inline(always)] + pub fn add_return_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_return_release(i, self.0.get()); + } + } + /// See `atomic64_add_return_relaxed`. + #[inline(always)] + pub fn add_return_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_return_relaxed(i, self.0.get()); + } + } + /// See `atomic64_fetch_add`. + #[inline(always)] + pub fn fetch_add(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_add(i, self.0.get()); + } + } + /// See `atomic64_fetch_add_acquire`. + #[inline(always)] + pub fn fetch_add_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_add_acquire(i, self.0.get()); + } + } + /// See `atomic64_fetch_add_release`. + #[inline(always)] + pub fn fetch_add_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_add_release(i, self.0.get()); + } + } + /// See `atomic64_fetch_add_relaxed`. + #[inline(always)] + pub fn fetch_add_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_add_relaxed(i, self.0.get()); + } + } + /// See `atomic64_sub`. + #[inline(always)] + pub fn sub(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_sub(i, self.0.get()); + } + } + /// See `atomic64_sub_return`. + #[inline(always)] + pub fn sub_return(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_sub_return(i, self.0.get()); + } + } + /// See `atomic64_sub_return_acquire`. + #[inline(always)] + pub fn sub_return_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_sub_return_acquire(i, self.0.get()); + } + } + /// See `atomic64_sub_return_release`. + #[inline(always)] + pub fn sub_return_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_sub_return_release(i, self.0.get()); + } + } + /// See `atomic64_sub_return_relaxed`. + #[inline(always)] + pub fn sub_return_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_sub_return_relaxed(i, self.0.get()); + } + } + /// See `atomic64_fetch_sub`. + #[inline(always)] + pub fn fetch_sub(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_sub(i, self.0.get()); + } + } + /// See `atomic64_fetch_sub_acquire`. + #[inline(always)] + pub fn fetch_sub_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_sub_acquire(i, self.0.get()); + } + } + /// See `atomic64_fetch_sub_release`. + #[inline(always)] + pub fn fetch_sub_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_sub_release(i, self.0.get()); + } + } + /// See `atomic64_fetch_sub_relaxed`. + #[inline(always)] + pub fn fetch_sub_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_sub_relaxed(i, self.0.get()); + } + } + /// See `atomic64_inc`. + #[inline(always)] + pub fn inc(&self) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_inc(self.0.get()); + } + } + /// See `atomic64_inc_return`. + #[inline(always)] + pub fn inc_return(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_inc_return(self.0.get()); + } + } + /// See `atomic64_inc_return_acquire`. + #[inline(always)] + pub fn inc_return_acquire(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_inc_return_acquire(self.0.get()); + } + } + /// See `atomic64_inc_return_release`. + #[inline(always)] + pub fn inc_return_release(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_inc_return_release(self.0.get()); + } + } + /// See `atomic64_inc_return_relaxed`. + #[inline(always)] + pub fn inc_return_relaxed(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_inc_return_relaxed(self.0.get()); + } + } + /// See `atomic64_fetch_inc`. + #[inline(always)] + pub fn fetch_inc(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_inc(self.0.get()); + } + } + /// See `atomic64_fetch_inc_acquire`. + #[inline(always)] + pub fn fetch_inc_acquire(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_inc_acquire(self.0.get()); + } + } + /// See `atomic64_fetch_inc_release`. + #[inline(always)] + pub fn fetch_inc_release(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_inc_release(self.0.get()); + } + } + /// See `atomic64_fetch_inc_relaxed`. + #[inline(always)] + pub fn fetch_inc_relaxed(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_inc_relaxed(self.0.get()); + } + } + /// See `atomic64_dec`. + #[inline(always)] + pub fn dec(&self) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_dec(self.0.get()); + } + } + /// See `atomic64_dec_return`. + #[inline(always)] + pub fn dec_return(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_dec_return(self.0.get()); + } + } + /// See `atomic64_dec_return_acquire`. + #[inline(always)] + pub fn dec_return_acquire(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_dec_return_acquire(self.0.get()); + } + } + /// See `atomic64_dec_return_release`. + #[inline(always)] + pub fn dec_return_release(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_dec_return_release(self.0.get()); + } + } + /// See `atomic64_dec_return_relaxed`. + #[inline(always)] + pub fn dec_return_relaxed(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_dec_return_relaxed(self.0.get()); + } + } + /// See `atomic64_fetch_dec`. + #[inline(always)] + pub fn fetch_dec(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_dec(self.0.get()); + } + } + /// See `atomic64_fetch_dec_acquire`. + #[inline(always)] + pub fn fetch_dec_acquire(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_dec_acquire(self.0.get()); + } + } + /// See `atomic64_fetch_dec_release`. + #[inline(always)] + pub fn fetch_dec_release(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_dec_release(self.0.get()); + } + } + /// See `atomic64_fetch_dec_relaxed`. + #[inline(always)] + pub fn fetch_dec_relaxed(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_dec_relaxed(self.0.get()); + } + } + /// See `atomic64_and`. + #[inline(always)] + pub fn and(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_and(i, self.0.get()); + } + } + /// See `atomic64_fetch_and`. + #[inline(always)] + pub fn fetch_and(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_and(i, self.0.get()); + } + } + /// See `atomic64_fetch_and_acquire`. + #[inline(always)] + pub fn fetch_and_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_and_acquire(i, self.0.get()); + } + } + /// See `atomic64_fetch_and_release`. + #[inline(always)] + pub fn fetch_and_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_and_release(i, self.0.get()); + } + } + /// See `atomic64_fetch_and_relaxed`. + #[inline(always)] + pub fn fetch_and_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_and_relaxed(i, self.0.get()); + } + } + /// See `atomic64_andnot`. + #[inline(always)] + pub fn andnot(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_andnot(i, self.0.get()); + } + } + /// See `atomic64_fetch_andnot`. + #[inline(always)] + pub fn fetch_andnot(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_andnot(i, self.0.get()); + } + } + /// See `atomic64_fetch_andnot_acquire`. + #[inline(always)] + pub fn fetch_andnot_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_andnot_acquire(i, self.0.get()); + } + } + /// See `atomic64_fetch_andnot_release`. + #[inline(always)] + pub fn fetch_andnot_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_andnot_release(i, self.0.get()); + } + } + /// See `atomic64_fetch_andnot_relaxed`. + #[inline(always)] + pub fn fetch_andnot_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_andnot_relaxed(i, self.0.get()); + } + } + /// See `atomic64_or`. + #[inline(always)] + pub fn or(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_or(i, self.0.get()); + } + } + /// See `atomic64_fetch_or`. + #[inline(always)] + pub fn fetch_or(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_or(i, self.0.get()); + } + } + /// See `atomic64_fetch_or_acquire`. + #[inline(always)] + pub fn fetch_or_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_or_acquire(i, self.0.get()); + } + } + /// See `atomic64_fetch_or_release`. + #[inline(always)] + pub fn fetch_or_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_or_release(i, self.0.get()); + } + } + /// See `atomic64_fetch_or_relaxed`. + #[inline(always)] + pub fn fetch_or_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_or_relaxed(i, self.0.get()); + } + } + /// See `atomic64_xor`. + #[inline(always)] + pub fn xor(&self, i: i64) { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + atomic64_xor(i, self.0.get()); + } + } + /// See `atomic64_fetch_xor`. + #[inline(always)] + pub fn fetch_xor(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_xor(i, self.0.get()); + } + } + /// See `atomic64_fetch_xor_acquire`. + #[inline(always)] + pub fn fetch_xor_acquire(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_xor_acquire(i, self.0.get()); + } + } + /// See `atomic64_fetch_xor_release`. + #[inline(always)] + pub fn fetch_xor_release(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_xor_release(i, self.0.get()); + } + } + /// See `atomic64_fetch_xor_relaxed`. + #[inline(always)] + pub fn fetch_xor_relaxed(&self, i: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_xor_relaxed(i, self.0.get()); + } + } + /// See `atomic64_xchg`. + #[inline(always)] + pub fn xchg(&self, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_xchg(self.0.get(), new); + } + } + /// See `atomic64_xchg_acquire`. + #[inline(always)] + pub fn xchg_acquire(&self, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_xchg_acquire(self.0.get(), new); + } + } + /// See `atomic64_xchg_release`. + #[inline(always)] + pub fn xchg_release(&self, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_xchg_release(self.0.get(), new); + } + } + /// See `atomic64_xchg_relaxed`. + #[inline(always)] + pub fn xchg_relaxed(&self, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_xchg_relaxed(self.0.get(), new); + } + } + /// See `atomic64_cmpxchg`. + #[inline(always)] + pub fn cmpxchg(&self, old: i64, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_cmpxchg(self.0.get(), old, new); + } + } + /// See `atomic64_cmpxchg_acquire`. + #[inline(always)] + pub fn cmpxchg_acquire(&self, old: i64, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_cmpxchg_acquire(self.0.get(), old, new); + } + } + /// See `atomic64_cmpxchg_release`. + #[inline(always)] + pub fn cmpxchg_release(&self, old: i64, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_cmpxchg_release(self.0.get(), old, new); + } + } + /// See `atomic64_cmpxchg_relaxed`. + #[inline(always)] + pub fn cmpxchg_relaxed(&self, old: i64, new: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_cmpxchg_relaxed(self.0.get(), old, new); + } + } + /// See `atomic64_try_cmpxchg`. + #[inline(always)] + pub fn try_cmpxchg(&self, old: &mut i64, new: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_try_cmpxchg(self.0.get(), old, new); + } + } + /// See `atomic64_try_cmpxchg_acquire`. + #[inline(always)] + pub fn try_cmpxchg_acquire(&self, old: &mut i64, new: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_try_cmpxchg_acquire(self.0.get(), old, new); + } + } + /// See `atomic64_try_cmpxchg_release`. + #[inline(always)] + pub fn try_cmpxchg_release(&self, old: &mut i64, new: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_try_cmpxchg_release(self.0.get(), old, new); + } + } + /// See `atomic64_try_cmpxchg_relaxed`. + #[inline(always)] + pub fn try_cmpxchg_relaxed(&self, old: &mut i64, new: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_try_cmpxchg_relaxed(self.0.get(), old, new); + } + } + /// See `atomic64_sub_and_test`. + #[inline(always)] + pub fn sub_and_test(&self, i: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_sub_and_test(i, self.0.get()); + } + } + /// See `atomic64_dec_and_test`. + #[inline(always)] + pub fn dec_and_test(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_dec_and_test(self.0.get()); + } + } + /// See `atomic64_inc_and_test`. + #[inline(always)] + pub fn inc_and_test(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_inc_and_test(self.0.get()); + } + } + /// See `atomic64_add_negative`. + #[inline(always)] + pub fn add_negative(&self, i: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_negative(i, self.0.get()); + } + } + /// See `atomic64_add_negative_acquire`. + #[inline(always)] + pub fn add_negative_acquire(&self, i: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_negative_acquire(i, self.0.get()); + } + } + /// See `atomic64_add_negative_release`. + #[inline(always)] + pub fn add_negative_release(&self, i: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_negative_release(i, self.0.get()); + } + } + /// See `atomic64_add_negative_relaxed`. + #[inline(always)] + pub fn add_negative_relaxed(&self, i: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_negative_relaxed(i, self.0.get()); + } + } + /// See `atomic64_fetch_add_unless`. + #[inline(always)] + pub fn fetch_add_unless(&self, a: i64, u: i64) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_fetch_add_unless(self.0.get(), a, u); + } + } + /// See `atomic64_add_unless`. + #[inline(always)] + pub fn add_unless(&self, a: i64, u: i64) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_add_unless(self.0.get(), a, u); + } + } + /// See `atomic64_inc_not_zero`. + #[inline(always)] + pub fn inc_not_zero(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_inc_not_zero(self.0.get()); + } + } + /// See `atomic64_inc_unless_negative`. + #[inline(always)] + pub fn inc_unless_negative(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_inc_unless_negative(self.0.get()); + } + } + /// See `atomic64_dec_unless_positive`. + #[inline(always)] + pub fn dec_unless_positive(&self) -> bool { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_dec_unless_positive(self.0.get()); + } + } + /// See `atomic64_dec_if_positive`. + #[inline(always)] + pub fn dec_if_positive(&self) -> i64 { + // SAFETY:`self.0.get()` is a valid pointer. + unsafe { + return atomic64_dec_if_positive(self.0.get()); + } + } +} + +// 258c6b7d580a83146e21973b1068cc92d9e65b87 diff --git a/scripts/atomic/gen-atomics.sh b/scripts/atomic/gen-atomics.sh index 3927aee034c8..8d250c885c24 100755 --- a/scripts/atomic/gen-atomics.sh +++ b/scripts/atomic/gen-atomics.sh @@ -12,6 +12,7 @@ gen-atomic-instrumented.sh linux/atomic/atomic-instrumented.h gen-atomic-long.sh linux/atomic/atomic-long.h gen-atomic-fallback.sh linux/atomic/atomic-arch-fallback.h gen-rust-atomic-helpers.sh ../rust/atomic_helpers.h +gen-rust-atomic.sh ../rust/kernel/sync/atomic/impl.rs EOF while read script header args; do /bin/sh ${ATOMICDIR}/${script} ${ATOMICTBL} ${args} > ${LINUXDIR}/include/${header} diff --git a/scripts/atomic/gen-rust-atomic.sh b/scripts/atomic/gen-rust-atomic.sh new file mode 100755 index 000000000000..491c643301a9 --- /dev/null +++ b/scripts/atomic/gen-rust-atomic.sh @@ -0,0 +1,136 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +ATOMICDIR=$(dirname $0) + +. ${ATOMICDIR}/atomic-tbl.sh + +#gen_ret_type(meta, int) +gen_ret_type() { + local meta="$1"; shift + local int="$1"; shift + + case "${meta}" in + [sv]) printf "";; + [bB]) printf -- "-> bool ";; + [aiIfFlR]) printf -- "-> ${int} ";; + esac +} + +# gen_param_type(arg, int) +gen_param_type() +{ + local type="${1%%:*}"; shift + local int="$1"; shift + + case "${type}" in + i) type="${int}";; + p) type="&mut ${int}";; + esac + + printf "${type}" +} + +#gen_param(arg, int) +gen_param() +{ + local arg="$1"; shift + local int="$1"; shift + local name="$(gen_param_name "${arg}")" + local type="$(gen_param_type "${arg}" "${int}")" + + printf "${name}: ${type}" +} + +#gen_params(int, arg...) +gen_params() +{ + local int="$1"; shift + + while [ "$#" -gt 0 ]; do + if [[ "$1" != "v" && "$1" != "cv" ]]; then + printf ", " + gen_param "$1" "${int}" + fi + shift; + done +} + +#gen_args(arg...) +gen_args() +{ + while [ "$#" -gt 0 ]; do + if [[ "$1" == "v" || "$1" == "cv" ]]; then + printf "self.0.get()" + [ "$#" -gt 1 ] && printf ", " + else + printf "$(gen_param_name "$1")" + [ "$#" -gt 1 ] && printf ", " + fi + shift; + done +} + +#gen_proto_order_variant(meta, pfx, name, sfx, order, atomic, ty, int, raw, arg...) +gen_proto_order_variant() +{ + local meta="$1"; shift + local pfx="$1"; shift + local name="$1"; shift + local sfx="$1"; shift + local order="$1"; shift + local atomic="$1"; shift + local ty="$1"; shift + local int="$1"; shift + local raw="$1"; shift + + local fn_name="${raw}${pfx}${name}${sfx}${order}" + local atomicname="${raw}${atomic}_${pfx}${name}${sfx}${order}" + + local ret="$(gen_ret_type "${meta}" "${int}")" + local params="$(gen_params "${int}" $@)" + local args="$(gen_args "$@")" + local retstmt="$(gen_ret_stmt "${meta}")" + +cat <<EOF + /// See \`${atomicname}\`. + #[inline(always)] + pub fn ${fn_name}(&self${params}) ${ret}{ + // SAFETY:\`self.0.get()\` is a valid pointer. + unsafe { + ${retstmt}${atomicname}(${args}); + } + } +EOF +} + +cat << EOF +// SPDX-License-Identifier: GPL-2.0 + +//! Generated by $0 +//! DO NOT MODIFY THIS FILE DIRECTLY + +use super::*; +use crate::bindings::*; + +impl AtomicI32 { +EOF + +grep '^[a-z]' "$1" | while read name meta args; do + gen_proto "${meta}" "${name}" "atomic" "AtomicI32" "i32" "" ${args} +done + +cat << EOF +} + +impl AtomicI64 { +EOF + +grep '^[a-z]' "$1" | while read name meta args; do + gen_proto "${meta}" "${name}" "atomic64" "AtomicI64" "i64" "" ${args} +done + +cat << EOF +} + +EOF
Provide two atomic types: AtomicI32 and AtomicI64 with the existing implemenation of C atomics. These atomics have the same semantics of the corresponding LKMM C atomics, and using one memory (ordering) model certainly reduces the reasoning difficulty and potential bugs from the interaction of two different memory models. Also bump my role to the maintainer of ATOMIC INFRASTRUCTURE to reflect my responsiblity on these Rust APIs. Note that `Atomic*::new()`s are implemented vi open coding on struct atomic*_t. This allows `new()` being a `const` function, so that it can be used in constant contexts. Signed-off-by: Boqun Feng <boqun.feng@gmail.com> --- MAINTAINERS | 4 +- arch/arm64/kernel/cpufeature.c | 2 + rust/kernel/sync.rs | 1 + rust/kernel/sync/atomic.rs | 63 ++ rust/kernel/sync/atomic/impl.rs | 1375 +++++++++++++++++++++++++++++ scripts/atomic/gen-atomics.sh | 1 + scripts/atomic/gen-rust-atomic.sh | 136 +++ 7 files changed, 1581 insertions(+), 1 deletion(-) create mode 100644 rust/kernel/sync/atomic.rs create mode 100644 rust/kernel/sync/atomic/impl.rs create mode 100755 scripts/atomic/gen-rust-atomic.sh