Received-SPF: pass (google.com: domain of linux-kernel+bounces-213647-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: iad51458e:Fastmail
Date: Thu, 13 Jun 2024 09:30:26 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Gary Guo <gary@garyguo.net>
Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arch@vger.kernel.org, llvm@lists.linux.dev,
	Miguel Ojeda <ojeda@kernel.org>,	Alex Gaynor <alex.gaynor@gmail.com>,
	Wedson Almeida Filho <wedsonaf@gmail.com>,
	=?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
	Benno Lossin <benno.lossin@proton.me>,
	Andreas Hindborg <a.hindborg@samsung.com>,
	Alice Ryhl <aliceryhl@google.com>,
	Alan Stern <stern@rowland.harvard.edu>,
	Andrea Parri <parri.andrea@gmail.com>,	Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Nicholas Piggin <npiggin@gmail.com>,	David Howells <dhowells@redhat.com>,
	Jade Alglave <j.alglave@ucl.ac.uk>,	Luc Maranget <luc.maranget@inria.fr>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Akira Yokosawa <akiyks@gmail.com>,	Daniel Lustig <dlustig@nvidia.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,	kent.overstreet@gmail.com,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, elver@google.com,
	Mark Rutland <mark.rutland@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>,	Ingo Molnar <mingo@redhat.com>,
 Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Catalin Marinas <catalin.marinas@arm.com>,	torvalds@linux-foundation.org,
 linux-arm-kernel@lists.infradead.org,	linux-fsdevel@vger.kernel.org,
 Trevor Gross <tmgross@umich.edu>,	dakr@redhat.com
Subject: Re: [RFC 2/2] rust: sync: Add atomic support
Message-ID: <ZmseosxVQXdsQjNB@boqun-archlinux>
References: <20240612223025.1158537-1-boqun.feng@gmail.com>
 <20240612223025.1158537-3-boqun.feng@gmail.com>
 <20240613144432.77711a3a@eugeo>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240613144432.77711a3a@eugeo>

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