From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 41CB71DDC03 for ; Tue, 26 Nov 2024 18:57:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732647467; cv=none; b=clqCV0tQcePJZnHCOXy3U1kImknu0B3iASmzaxFSemytc8tFRYTIr6uzubje4ZnjzZ1Z0R1cVkWBj6xsp7fa4fGiXXFeQUV+dj9wQNlEO47CjLFV1HcyVSxsT1ssShWDrLuR/qd1dPbGWxUZCS4f8+F/XnH60n14pU2iOWJxCyk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732647467; c=relaxed/simple; bh=FQdexGUZlaE4KFC2ddg2t0lNwQkVn1TXZdOgt3YQ8z0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=FnAOeYYPb9QSoX06ILLpNvMxmb6f49XG464cK6RSUJb93YxrWRB/0DTkCbzvhs9DFXRIts/hM3BCgGKDY+whgsW5NTD6Cj/0bVD9xlkiSuY1xEj82aH0PMb8Kl3pKmzVlaOIRHfxpYmf4p+GJwnyWlUnCNe5Xwok4ICvvFqQAtI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=sedlak.dev; spf=none smtp.mailfrom=sedlak.dev; dkim=pass (2048-bit key) header.d=sedlak-dev.20230601.gappssmtp.com header.i=@sedlak-dev.20230601.gappssmtp.com header.b=eWFkV+Fj; arc=none smtp.client-ip=209.85.128.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=sedlak.dev Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=sedlak.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=sedlak-dev.20230601.gappssmtp.com header.i=@sedlak-dev.20230601.gappssmtp.com header.b="eWFkV+Fj" Received: by mail-wm1-f44.google.com with SMTP id 5b1f17b1804b1-4349fd77b33so20715095e9.2 for ; Tue, 26 Nov 2024 10:57:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sedlak-dev.20230601.gappssmtp.com; s=20230601; t=1732647463; x=1733252263; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=lH+EuN7XG/opD7jydmM6mpYlIbILE0ML+h/J6BW4iZQ=; b=eWFkV+FjJX6XeZ0HBcMg3E9cGdSQwFY+IjsxjkcBg9DjN7AeounDmCV/j0XZ2fUthw H8IXKVeoDK44Pn1KGrtXIa+lenvnJxXRYHc0C+vj24WgGuH3gyxeZBi/15VvVz2uYp5w JNW2ye/srmkJfz5GiBrrbb1ssMMzK+AR/qu3jzauWuHlFPTOKwJCPFbEj+yHLdkgPOyL QdMOdMAIJHgG/jUlDII76Ay8C2s3KAk967v/HnftdYnbFpsLBkHoMqvvBQlyo+p2RiFA 9C8eax4SxQ00KZrUKve7ujuxgNrtG0tZCMamBEkrouJLemfEVZtPAX8pfhfAxzM0ggXU a8XQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732647463; x=1733252263; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=lH+EuN7XG/opD7jydmM6mpYlIbILE0ML+h/J6BW4iZQ=; b=juiXJc1+Db8zBGfBOAgHPJncq1HrlZEuFeatxxw0PRFd2gs9df2fkLhDhCeDlH8rQa HkwpH2iBKz9AziC40n50OmgMO+0YmYN/HhKvYcRKwPUYBUmicXVycO7EVmsfGHhtL5oH LYFXAsXfndEspOx7t66qPD5QAcg5ZXFZAlRfvKW6ObNTwsjfODuW1lykC4lmFLOn7hcC pu8B62ez5mtoxmB13KSZMrqOq3V0p6oLZVXBF+uYVgmR6PCQ5GBNVibWegWJVZ1ZmCiL wS0aY3cje1w9i544sTxl175S8/3NO1hCuk0Bqd874+eemgscK+XGKEFhEFevd9pXP3wn PAOw== X-Gm-Message-State: AOJu0YxuioXU+xbpfGqYiNcskE2UkwfHrXYZsCXeMa1NfveHtLS7Igdm 9oFD+/28IzQF5eL2GZ1ExGq2hMfnXFlewqlgcKnpnYo27mzYjNyKbwDfUWXPsHk= X-Gm-Gg: ASbGncvfaHRWK7ODABF+LQDgGMmqH5njGAtz+b/35knWbimP41/mjkFEgQjuGwLyn86 pq2Z8nIpo951hPStFmvwaO2cUSZ0NTWPHlJZmjNmx32C2YhYnSGgiqlEqijfVmzYjY/MGMkrQyC SuVLtFxK4Ztt0VhWERVxXmnDqZsDvd6pOLoTmzySHjqooor5lrGrhwbgB3qZDn+6dUlnCChzGji nw02bMT1nmKJxTkuMBHIHcgh2j7/dULh9rONTXG+DYJe30D8Bk= X-Google-Smtp-Source: AGHT+IGqhYhQxwauHfnqcW1RwJZkdzHaoXh+dpbEcywdiTUJgvy/B3VA3nxqbz3Ou/oyHhKC09ewsQ== X-Received: by 2002:a5d:6484:0:b0:382:455b:eec6 with SMTP id ffacd0b85a97d-385c6ec0cebmr32663f8f.35.1732647463113; Tue, 26 Nov 2024 10:57:43 -0800 (PST) Received: from [192.168.88.249] ([95.85.217.110]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3825fd04ef5sm13970343f8f.111.2024.11.26.10.57.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 26 Nov 2024 10:57:42 -0800 (PST) Message-ID: <230b3602-5d68-4e79-969d-0d2df1fdf033@sedlak.dev> Date: Tue, 26 Nov 2024 19:57:40 +0100 Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 1/3] rust: Add `OnceLite` for executing code once To: jens.korinth@tuta.io, Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Alice Ryhl , Trevor Gross Cc: rust-for-linux@vger.kernel.org, FUJITA Tomonori , Dirk Behme , linux-kernel@vger.kernel.org References: <20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io> <20241126-pr_once_macros-v4-1-410b8ca9643e@tuta.io> Content-Language: en-US From: Daniel Sedlak In-Reply-To: <20241126-pr_once_macros-v4-1-410b8ca9643e@tuta.io> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 11/26/24 5:40 PM, Jens Korinth via B4 Relay wrote: > From: Jens Korinth > > Similar to `Once` in Rust's standard library, but with the same > non-blocking behavior as the kernel's `DO_ONCE_LITE` macro. Abstraction > allows easy replacement of the underlying sync mechanisms, see > > https://lore.kernel.org/rust-for-linux/20241109-pr_once_macros-v3-0-6beb24e0cac8@tuta.io/. > > Suggested-by: Boqun Feng > Signed-off-by: Jens Korinth > --- > rust/kernel/lib.rs | 1 + > rust/kernel/once_lite.rs | 127 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 128 insertions(+) > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index bf8d7f841f9425d19a24f3910929839cfe705c7f..2b0a80435d24f5e168679ec2e25bd68cd970dcdd 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -44,6 +44,7 @@ > pub mod list; > #[cfg(CONFIG_NET)] > pub mod net; > +pub mod once_lite; > pub mod page; > pub mod prelude; > pub mod print; > diff --git a/rust/kernel/once_lite.rs b/rust/kernel/once_lite.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..723c3244fc856fe974ddd33de5415e7ced37f315 > --- /dev/null > +++ b/rust/kernel/once_lite.rs > @@ -0,0 +1,127 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! A one-time only global execution primitive. > +//! > +//! This primitive is meant to be used to execute code only once. It is > +//! similar in design to Rust's > +//! [`std::sync:Once`](https://doc.rust-lang.org/std/sync/struct.Once.html), > +//! but borrowing the non-blocking mechanism used in the kernel's > +//! [`DO_ONCE_LITE`] macro. > +//! > +//! An example use case would be to print a message only the first time. > +//! > +//! [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h > +//! > +//! C header: [`include/linux/once_lite.h`](srctree/include/linux/once_lite.h) > +//! > +//! Reference: > + > +use core::sync::atomic::{AtomicBool, Ordering::Relaxed}; > + > +/// A low-level synchronization primitive for one-time global execution. > +/// > +/// Based on the > +/// [`std::sync:Once`](https://doc.rust-lang.org/std/sync/struct.Once.html) > +/// interface, but internally equivalent the kernel's [`DO_ONCE_LITE`] > +/// macro. The Rust macro `do_once_lite` replacing it uses `OnceLite` > +/// internally. > +/// > +/// [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h > +/// > +/// # Examples > +/// > +/// ```rust The `rust` part should be default value for rustdoc tests, can we please omit that? > +/// static START: kernel::once_lite::OnceLite = > +/// kernel::once_lite::OnceLite::new(); > +/// > +/// let mut x: i32 = 0; > +/// > +/// START.call_once(|| { > +/// // run initialization here > +/// x = 42; > +/// }); > +/// while !START.is_completed() { /* busy wait */ } > +/// assert_eq!(x, 42); > +/// ``` > +/// > +pub struct OnceLite(AtomicBool, AtomicBool); Have you considered it to be implemented like `AtomicU32`? I think™ that one atomic variable is more than enough. > + > +impl OnceLite { > + /// Creates a new `OnceLite` value. > + #[inline(always)] > + pub const fn new() -> Self { > + Self(AtomicBool::new(false), AtomicBool::new(false)) > + } > + > + /// Performs an initialization routine once and only once. The given > + /// closure will be executed if this is the first time `call_once` has > + /// been called, and otherwise the routine will not be invoked. > + /// > + /// This method will _not_ block the calling thread if another > + /// initialization is currently running. It is _not_ guaranteed that the > + /// initialization routine will have completed by the time the calling > + /// thread continues execution. > + /// > + /// Note that this is different from the guarantees made by > + /// [`std::sync::Once`], but identical to the way the implementation of > + /// the kernel's [`DO_ONCE_LITE_IF`] macro is behaving at the time of > + /// writing. > + /// > + /// [`std::sync::Once`]: https://doc.rust-lang.org/std/sync/struct.Once.html > + /// [`DO_ONCE_LITE_IF`]: srctree/include/once_lite.h > + #[inline(always)] > + pub fn call_once(&self, f: F) { > + if !self.0.load(Relaxed) && !self.0.swap(true, Relaxed) { > + f() > + }; > + self.1.store(true, Relaxed); I think the memory ordering is wrong here. Since it is `Relaxed`, the `self.1` and `self.0` can get reordered during execution. So `self.0` can be false, even though `self.1` will be true. Not on x86, but on architectures with weaker memory models it can happen. Even the implementation in the Rust std, uses at least `Acquire`/`Release`, where the reordering shouldn't be possible see [1]. [1]: https://github.com/rust-lang/rust/blob/master/library/std/src/sys/sync/once/futex.rs > + } > + > + /// Returns `true` if some `call_once` call has completed successfully. > + /// Specifically, `is_completed` will return `false` in the following > + /// situations: > + /// > + /// 1. `call_once()` was not called at all, > + /// 2. `call_once()` was called, but has not yet completed. > + /// > + /// # Examples > + /// > + /// ```rust Also here the `rust` part should be the default value. > + /// static INIT: kernel::once_lite::OnceLite = > + /// kernel::once_lite::OnceLite::new(); > + /// > + /// assert_eq!(INIT.is_completed(), false); > + /// INIT.call_once(|| { > + /// assert_eq!(INIT.is_completed(), false); > + /// }); > + /// assert_eq!(INIT.is_completed(), true); > + /// ``` > + #[inline(always)] > + pub fn is_completed(&self) -> bool { > + self.1.load(Relaxed) > + } > +} > + > +/// Executes code only once. > +/// > +/// Equivalent to the kernel's [`DO_ONCE_LITE`] macro: Expression is > +/// evaluated at most once by the first thread, other threads will not be > +/// blocked while executing in first thread, nor are there any guarantees > +/// regarding the visibility of side-effects of the called expression. > +/// > +/// [`DO_ONCE_LITE`]: srctree/include/linux/once_lite.h > +/// > +/// # Examples > +/// > +/// ``` > +/// let mut x: i32 = 0; > +/// kernel::do_once_lite!((||{x = 42;})()); > +/// ``` > +#[macro_export] > +macro_rules! do_once_lite { > + ($e: expr) => {{ > + #[link_section = ".data.once"] > + static ONCE: $crate::once_lite::OnceLite = $crate::once_lite::OnceLite::new(); > + ONCE.call_once(|| $e); > + }}; > +} > Thanks for working on that! Daniel