Received-SPF: pass (google.com: domain of linux-kernel+bounces-213733-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 13 Jun 2024 18:19:10 +0100
From: Gary Guo <gary@garyguo.net>
To: Boqun Feng <boqun.feng@gmail.com>
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>, =?UTF-8?B?QmrDtnJu?= 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: <20240613181910.74db809c@eugeo>
In-Reply-To: <ZmseosxVQXdsQjNB@boqun-archlinux>
References: <20240612223025.1158537-1-boqun.feng@gmail.com>
	<20240612223025.1158537-3-boqun.feng@gmail.com>
	<20240613144432.77711a3a@eugeo>
	<ZmseosxVQXdsQjNB@boqun-archlinux>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?RHNG2x6SwDE5iCEHZXMc/lucISQQAQgG40p2JdjXjTz03uisbY41/JoVjvpv?=
 =?us-ascii?Q?DLxpw7XeTk+pTXyRB20s4j+ArTs3/S348YOnoK2qfnsreleMpn5NAwrgdTmH?=
 =?us-ascii?Q?f72fU5Hi1/FkATEUwDRsEe3t1THnp5/gTRlT0RWkX/THXLM7KO9PhhUXPlFr?=
 =?us-ascii?Q?nO2GtJfoCDZIfhDFrfvbdeW0gzinXma0Yiqy0Q5gquMj2tPHyJw9ftst2/8/?=
 =?us-ascii?Q?5lae4fz5l1JXrCcdwN9kjB1VURcugLzGhMmCXjSUfrMD3hWyea6nr8D06zrU?=
 =?us-ascii?Q?dFMbMGn/ICawnrxgUDOY86foad8/VjYiosxKOVFfT5j1r7G6Fe39ejQuITcs?=
 =?us-ascii?Q?QnQEx0os6Bh26deuZ1znh/eRsYXr+8Cl+G53GN4YVUe2WQickiU2F7t6CanA?=
 =?us-ascii?Q?e12/dXAJCR2LgOk3jw42mE3rByMh6g1Nc8PwdngIE4kOmV2hPgLn2gL1Qp0y?=
 =?us-ascii?Q?Xg4QydRrtE+jHiQyCXzWWEEl8v6CyIizK7dQ0gE/KqflKxZXPHmZ9hhUIUgg?=
 =?us-ascii?Q?snvIQbFFUEnGc29V3lOzz0KkU2wtnjBD6VXf5MVG2GOWW7E/X+/GgV8hqP+z?=
 =?us-ascii?Q?5c4DPMn2i/pE0abNoKj/Z/jV1zEldJcEPehtj5dez0EMIDapfDK2DDdamYI3?=
 =?us-ascii?Q?92H4e2OebvS/4Bjpeg02Zd2ufxUvBKlizD/ect/nvzVbI0CTU+D3B9vte7bo?=
 =?us-ascii?Q?tieQnQWwZhQIBWIHp3PZCsyzjuLoK+NPg2oNzirtVmfmNgOvssn2s9G3AY9h?=
 =?us-ascii?Q?7u6r2qCh/iCrVQGDDjxqPy1MWoXKmk8c9ZNyhNPE+EmRWz9P5zOGW0ixckxJ?=
 =?us-ascii?Q?jjBuF+iFqMbQGU/op30h4SSsrpgd66mbtUx03I4EHxtmhUeGF0FihcUHAuN0?=
 =?us-ascii?Q?avrKEZSpmdzp06a0Oe4g3P34F4eWfYG07UYHB5YGnTZ2XnJ29ptF3XyWmn7H?=
 =?us-ascii?Q?ao+WXN79zadgiWIu5M1aXj7q98QX/Qy/ITR3q0lJjq4p5atsPrMo66MxyKjD?=
 =?us-ascii?Q?7ZkE5FMtot0UrRyGAEp+R/PM/MN5jN3mnDMtQCWLvNd+RRa4fF7gW9vw7fve?=
 =?us-ascii?Q?nRRYX8C5ovafWfSN26y3wFIMJH6PdFM9wRBjWECWNEeb6yinSxVGHigL8q45?=
 =?us-ascii?Q?yV78MM5gU3JbbeMpBxdet+JkRSNWAUM3UEtkAxTVh8ny79cwj3eaPkDrW+JK?=
 =?us-ascii?Q?VjAi1UWw0M6nZCFLUDE3GBFrSQ+oStv42BROcyxwgfIWqmgVRvBU5y5uRCi3?=
 =?us-ascii?Q?3qTvjEYYwk+hW++Uv0y8XZ1sBg8vFaAxDOC2C235cJ/xev/sn0Nz5dLQ4kAh?=
 =?us-ascii?Q?hGpszRALiMdH71wLRHB5bdNAr6ddQem3f5Q3D58g0dT8TE3poEzemHF1LuFC?=
 =?us-ascii?Q?phKADylw9kq8bwhsyOokOvQedaH1FvojTMH7XbfZjkB3otOWMPK4p+0QwUXM?=
 =?us-ascii?Q?3ormOxtIaoW10WwH/eO0GgpWVxkFmYgz3m8nRlRb8lJCW91kyxmYsYk6CKNt?=
 =?us-ascii?Q?Jj4CM27ssb6sbwQs9G4yLHixjt66d641EEtj6VJKP1uR2P+OHOcEjVL64a5V?=
 =?us-ascii?Q?GPWtBSmf9a2HZczkNQvICOwWUsouyuCyhEf7nA8E?=
X-OriginatorOrg: garyguo.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 79560d9f-84a4-4c66-2356-08dc8bccf172
X-MS-Exchange-CrossTenant-AuthSource: LO2P265MB5183.GBRP265.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2024 17:19:12.1616
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bbc898ad-b10f-4e10-8552-d9377b823d45
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fT8x6yZZy3OIvuAQ4OfZvO8cRk+glYF+vVJ3uDT/aw0jpit6BNw/8jDjggaAay7y1xMtBJyGNMVLpKpSV9IFnA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LO9P265MB7760

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