Message ID | 20231206-rb-new-condvar-methods-v1-2-33a4cab7fdaa@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp4004426vqy; Wed, 6 Dec 2023 02:10:10 -0800 (PST) X-Google-Smtp-Source: AGHT+IFlcne//yEhQlWQYQKBL9VKkGF6EnXIx5KvRQDs5qXwmXQ9Zu3+MGFokbn6w/GbAYR1WKtN X-Received: by 2002:a05:6808:2396:b0:3a7:d566:8b5e with SMTP id bp22-20020a056808239600b003a7d5668b5emr810142oib.44.1701857410521; Wed, 06 Dec 2023 02:10:10 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701857410; cv=none; d=google.com; s=arc-20160816; b=IeQitpSsNy6kYk9dWvcT2u6fA1ANnxfuB0bmDcgl1y8oGKgetcHDkmxEs91rtbh2Xa o//ceLWh6SGCEtL0VSip6jIljXcPKnmdCAdVUjd+MnLl02wL6yP2bqMZkPYBpToT9CbA HuI/Mff9+ckBDx8mTbyhOrCf7YXfAK6Ix8dp7Ur42btbAJWKPDJLrh6qaFPa4sk4JseE YSbtoACxnhVCMODYazGVbjbjAwaLH7kHqVYbBYdOvZgzfmFK72Qznb1eCy9PI7n37gJX rnhZ6Pa5PFX8asCqcb3VJ0WXTtBE9rr41Y6HOcSZfGdhj4Ku5c8H/qOIVI3+6vl+pYJA R9KA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:dkim-signature; bh=Yyo/AOmj9zcd4Cgit94NCPm2Zi7Z6JIsUXb5F1cqXp4=; fh=4Ll0qDznmJCNJvJ+PsATqWDPrXYFX8AsCU+k6YytCaY=; b=y4rEn9mQw/pYfr+63wksVkwMJcgclsLX5dc+ki3HaPvFzaGUDIc/sLCblDn33gQBSy u+b1NmDJ46JdKYKzZfv8dprNtBCjLzvevw5Gw0epFiZ1/vuunxPC+dXvaJw9p0v/3TyC oCjKO8xpSO0V7BrIsen1CLw9iZzGg/p9CO1LKmdCxf0TLBFeoYaOVFR4KPm1J2WDwgSj Zpqx3+g3vEj6e4xL8Ay/4/HZLQUGV/HPW59BjuZ0iXRfuyq6wEXM7wqJ9w0to4dd8F6n vXD8CyT21zoozCVHjugvvAc/ik6rD0saHgoQIPsLvAbdfGoLUINsuLC9rXlzZURSgLLq ZaPQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b="gvXVxW/F"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id q11-20020a056a00088b00b006cbd8179b4csi11325127pfj.108.2023.12.06.02.10.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Dec 2023 02:10:10 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b="gvXVxW/F"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 974B48132A6E; Wed, 6 Dec 2023 02:10:03 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1377431AbjLFKJx (ORCPT <rfc822;pusanteemu@gmail.com> + 99 others); Wed, 6 Dec 2023 05:09:53 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37574 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1377427AbjLFKJq (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 6 Dec 2023 05:09:46 -0500 Received: from mail-lj1-x249.google.com (mail-lj1-x249.google.com [IPv6:2a00:1450:4864:20::249]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6469ED4D for <linux-kernel@vger.kernel.org>; Wed, 6 Dec 2023 02:09:51 -0800 (PST) Received: by mail-lj1-x249.google.com with SMTP id 38308e7fff4ca-2ca005e8de4so30800611fa.3 for <linux-kernel@vger.kernel.org>; Wed, 06 Dec 2023 02:09:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1701857389; x=1702462189; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=Yyo/AOmj9zcd4Cgit94NCPm2Zi7Z6JIsUXb5F1cqXp4=; b=gvXVxW/F6Y3HWq4D3dGFCWtlGUZIcrxiJz8iAPXDOXnzu8ttJTKFPpMvKC3HQXTtcH YZ/I8LU14+aMVnMZ/D7VVdj9Uk03JDXhDtL9wiCeWdS9fkAcmBruOBho6vVgKOga7GZh GNB8+NJES9nUZGfP0TmPPtbACLCx9LFZCGagGW1d7fuRndoWVNf3dlnDEzlHJKWUS9uI NaBIjuRLtDYSQCCzdckOB18146TF20Hh6Sv4GobKr2mSwKSX/fFzmMB1LcxZHx2ecAyx 9GlCJIRKo5Tiq6M03Bw2nYvEjQexn4MvydA54h5JqC0EEHLZ7ujeauv8sq39s6DYO3/l ySGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701857389; x=1702462189; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Yyo/AOmj9zcd4Cgit94NCPm2Zi7Z6JIsUXb5F1cqXp4=; b=Qfv6LjLAvLLpc9j0LkYOuNTE/1jENg6FaszyA4cHtrJDF3orl5nXuuokbh5E8DrcLE K46GH8RIRHtwJ6FaS65OVNge7e41kUAU3/TtZWE/vipR3EourUcWtK6ZLk9gKq8VAsMN BWO8+gXZA2KAA7fMWBFJtCiMypUGD46xvCyJDAYeVqoZTcEjNy9FWg/+7hKJN+S3qcXr KOjjAeiQXyolHaUeP8VAd2XXVQ7MVUMm2DVJZ/MKJqom+//fb2wlCP1RLBEnP9W13LOz c2zGdiezitFs95tLkGtL60jLrL5eiRIIOurx3KjngF/1lvRf7GEsM/T/kH/o1HBeMvmP OmKw== X-Gm-Message-State: AOJu0Yzdu2RVYjjvcLF8n1mP0Tadf2/zP+qegj7ckWc42gMLATTXMzvD tN5gYzF6b7eBrGM9htDeiQgnGwtsvD38Qi8= X-Received: from aliceryhl2.c.googlers.com ([fda3:e722:ac3:cc00:68:949d:c0a8:572]) (user=aliceryhl job=sendgmr) by 2002:a05:6512:39d6:b0:50b:f0f6:259d with SMTP id k22-20020a05651239d600b0050bf0f6259dmr6812lfu.11.1701857389217; Wed, 06 Dec 2023 02:09:49 -0800 (PST) Date: Wed, 06 Dec 2023 10:09:24 +0000 In-Reply-To: <20231206-rb-new-condvar-methods-v1-0-33a4cab7fdaa@google.com> Mime-Version: 1.0 References: <20231206-rb-new-condvar-methods-v1-0-33a4cab7fdaa@google.com> X-Developer-Key: i=aliceryhl@google.com; a=openpgp; fpr=49F6C1FAA74960F43A5B86A1EE7A392FDE96209F X-Developer-Signature: v=1; a=openpgp-sha256; l=5270; i=aliceryhl@google.com; h=from:subject:message-id; bh=/VMhS2n7v+OrDjmNW0iMiPb1VnK9+s+caIt3RRCeoD0=; b=owEBbQKS/ZANAwAKAQRYvu5YxjlGAcsmYgBlcEhkME2PSBggPDlUK+JjUepphkl7X+XwFkwnM 3RoI5m0NtiJAjMEAAEKAB0WIQSDkqKUTWQHCvFIvbIEWL7uWMY5RgUCZXBIZAAKCRAEWL7uWMY5 RspjD/9yieSBoVrrLuRCIT7d3bnc3302+K9bE/cgtJckVgMlEnSX4oOE229OHdMZ0gsCXRVXeHg GqNZ8htlUk7d8wgt/7dNx2SH4TWYHEznDsS26uePexTkNOYVJX+ce+uREA9pc/J/Rwqx6um8UqR j4hv/M81LKuyptxs6WR0EyBY7fATbrgTIzgP6ajdRJ9FSjQVEfxKy06utISRfa2MwroT0TCx78X oaPaz943kPiB72dUCqAo5hGkYHQtHB8dGdbR5qapdEyN/XQ0GbWV5lp44/oje3wpi87jA/CASGS YcL2qnLJmz+M5WfVwNNWA4IECjePWyJajCFKzUWgMn8HDNjEx/vmLN70AajfhXxrwlK6indrGMT Ly1HSzCqZ9v/Kwg8qvHm54CqkWGv56yz7NDMlQNzByCrUQZ3R/mDyksBwZo6tDOMixnTakzyyTF ZXgqZNfaK+zHEKtoTiXd/xkrPyIVNv4+144VmtGG+8sbVBst9B+HprWXJV4J83J+aYTJIa/Ayyc UkfPKorryorYhJ+Rnvj66IDRZZMt40SvkTxVoBANWCdQm//fqdPckCs9V5ZTSo8WRLt2o9gPPWA oqwuFFLv4RzbZpb/bAMQKi2O0btVjRG6osyoYXr4VFQuf+Fnpii6u6S2kCHuW4kcLwq+VwmFa6x I3f8/hOBATRSRwQ== X-Mailer: b4 0.13-dev-26615 Message-ID: <20231206-rb-new-condvar-methods-v1-2-33a4cab7fdaa@google.com> Subject: [PATCH 2/2] rust: sync: add `CondVar::wait_timeout` From: Alice Ryhl <aliceryhl@google.com> To: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, " =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= " <bjorn3_gh@protonmail.com>, Benno Lossin <benno.lossin@proton.me>, Andreas Hindborg <a.hindborg@samsung.com>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>, Waiman Long <longman@redhat.com> Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, Alice Ryhl <aliceryhl@google.com> Content-Type: text/plain; charset="utf-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 06 Dec 2023 02:10:04 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784526835606384124 X-GMAIL-MSGID: 1784526835606384124 |
Series |
Additional CondVar methods needed by Rust Binder
|
|
Commit Message
Alice Ryhl
Dec. 6, 2023, 10:09 a.m. UTC
Sleep on a condition variable with a timeout.
This is used by Rust Binder for process freezing. There, we want to
sleep until the freeze operation completes, but we want to be able to
abort the process freezing if it doesn't complete within some timeout.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/sync.rs | 2 +-
rust/kernel/sync/condvar.rs | 73 +++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/sync/lock.rs | 4 +--
3 files changed, 76 insertions(+), 3 deletions(-)
Comments
On 12/6/23 07:09, Alice Ryhl wrote: > Sleep on a condition variable with a timeout. > > This is used by Rust Binder for process freezing. There, we want to > sleep until the freeze operation completes, but we want to be able to > abort the process freezing if it doesn't complete within some timeout. > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > [...] > > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > + /// [`CondVar::notify_all`], or when the thread receives a signal. > + /// > + /// Returns whether there is a signal pending. Remaining jiffies or zero on timeout? > + fn wait_internal_timeout<T, B>( > + &self, > + wait_state: u32, > + guard: &mut Guard<'_, T, B>, > + timeout: u64, > + ) -> u64 > [...] > + > + /// Releases the lock and waits for a notification in interruptible mode. > + /// > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > + /// [`CondVar::notify_all`], or when a timeout occurs, or when the thread receives a signal. > + /// > + /// Returns whether there is a signal pending. This one is correct. > + #[must_use = "wait_timeout returns if a signal is pending, so the caller must check the return value"] > + pub fn wait_timeout<T: ?Sized, B: Backend>( > + &self, > + guard: &mut Guard<'_, T, B>, > + jiffies: u64, > + ) -> CondVarTimeoutResult { > [...]
On Wed, Dec 06, 2023 at 10:09:24AM +0000, Alice Ryhl wrote: [...] > + > +/// The return type of `wait_timeout`. > +pub enum CondVarTimeoutResult { > + /// The timeout was reached. > + Timeout, > + /// Somebody woke us up. > + Woken { > + /// Remaining sleep duration. > + jiffies: u64, I have a Jiffies definition in the my upcoming timer patchset: /// The time unit of Linux kernel. One jiffy equals (1/HZ) second. pub type Jiffies = core::ffi::c_ulong; Maybe you can add that (in a separate patch) in kernel::time? Regards, Boqun > + }, > + /// A signal occurred. > + Signal { > + /// Remaining sleep duration. > + jiffies: u64, > + }, > +} [...]
On Wed, Dec 6, 2023 at 4:53 PM Martin Rodriguez Reboredo <yakoyoku@gmail.com> wrote: > > On 12/6/23 07:09, Alice Ryhl wrote: > > Sleep on a condition variable with a timeout. > > > > This is used by Rust Binder for process freezing. There, we want to > > sleep until the freeze operation completes, but we want to be able to > > abort the process freezing if it doesn't complete within some timeout. > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > --- > > [...] > > > > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > > + /// [`CondVar::notify_all`], or when the thread receives a signal. > > + /// > > + /// Returns whether there is a signal pending. > > Remaining jiffies or zero on timeout? Seems like I just copied this typo from the other `_internal` method. I'll fix it on both. Alice
On Wed, Dec 06, 2023 at 08:30:06AM -0800, Boqun Feng wrote: > On Wed, Dec 06, 2023 at 10:09:24AM +0000, Alice Ryhl wrote: > [...] > > + > > +/// The return type of `wait_timeout`. > > +pub enum CondVarTimeoutResult { > > + /// The timeout was reached. > > + Timeout, > > + /// Somebody woke us up. > > + Woken { > > + /// Remaining sleep duration. > > + jiffies: u64, > > I have a Jiffies definition in the my upcoming timer patchset: > > /// The time unit of Linux kernel. One jiffy equals (1/HZ) second. > pub type Jiffies = core::ffi::c_ulong; > > Maybe you can add that (in a separate patch) in kernel::time? Urgh, why are we using jiffies in 2023?
On Wed, Dec 6, 2023 at 5:39 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Dec 06, 2023 at 08:30:06AM -0800, Boqun Feng wrote: > > On Wed, Dec 06, 2023 at 10:09:24AM +0000, Alice Ryhl wrote: > > [...] > > > + > > > +/// The return type of `wait_timeout`. > > > +pub enum CondVarTimeoutResult { > > > + /// The timeout was reached. > > > + Timeout, > > > + /// Somebody woke us up. > > > + Woken { > > > + /// Remaining sleep duration. > > > + jiffies: u64, > > > > I have a Jiffies definition in the my upcoming timer patchset: > > > > /// The time unit of Linux kernel. One jiffy equals (1/HZ) second. > > pub type Jiffies = core::ffi::c_ulong; > > > > Maybe you can add that (in a separate patch) in kernel::time? > > Urgh, why are we using jiffies in 2023? I assumed that the correct thing here would be to accept the same unit as what schedule_timeout takes. Should I be doing something else? Alice
On Wed, Dec 06, 2023 at 05:42:29PM +0100, Alice Ryhl wrote: > On Wed, Dec 6, 2023 at 5:39 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Wed, Dec 06, 2023 at 08:30:06AM -0800, Boqun Feng wrote: > > > On Wed, Dec 06, 2023 at 10:09:24AM +0000, Alice Ryhl wrote: > > > [...] > > > > + > > > > +/// The return type of `wait_timeout`. > > > > +pub enum CondVarTimeoutResult { > > > > + /// The timeout was reached. > > > > + Timeout, > > > > + /// Somebody woke us up. > > > > + Woken { > > > > + /// Remaining sleep duration. > > > > + jiffies: u64, > > > > > > I have a Jiffies definition in the my upcoming timer patchset: > > > > > > /// The time unit of Linux kernel. One jiffy equals (1/HZ) second. > > > pub type Jiffies = core::ffi::c_ulong; > > > > > > Maybe you can add that (in a separate patch) in kernel::time? > > > > Urgh, why are we using jiffies in 2023? > > I assumed that the correct thing here would be to accept the same unit > as what schedule_timeout takes. Should I be doing something else? Bah, so we have schedule_hrtimeout() that takes ktime/u64 nsec. But the 'problem' is that hrtimers are written with the expectation to fire, while the old timers are written with the expectation to not fire. Timeouts are typically best done with the latter, so in that regard using schedule_timeout() is right. But it is sad to inflict the brain-damage that is jiffies onto new code. Perhaps add schedule_timeout_*msec() wrappers around schedule_timeout*() and use a consistent sane time unit? Thomas?
On Wed, Dec 6, 2023 at 5:53 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Dec 06, 2023 at 05:42:29PM +0100, Alice Ryhl wrote: > > On Wed, Dec 6, 2023 at 5:39 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > On Wed, Dec 06, 2023 at 08:30:06AM -0800, Boqun Feng wrote: > > > > On Wed, Dec 06, 2023 at 10:09:24AM +0000, Alice Ryhl wrote: > > > > [...] > > > > > + > > > > > +/// The return type of `wait_timeout`. > > > > > +pub enum CondVarTimeoutResult { > > > > > + /// The timeout was reached. > > > > > + Timeout, > > > > > + /// Somebody woke us up. > > > > > + Woken { > > > > > + /// Remaining sleep duration. > > > > > + jiffies: u64, > > > > > > > > I have a Jiffies definition in the my upcoming timer patchset: > > > > > > > > /// The time unit of Linux kernel. One jiffy equals (1/HZ) second. > > > > pub type Jiffies = core::ffi::c_ulong; > > > > > > > > Maybe you can add that (in a separate patch) in kernel::time? > > > > > > Urgh, why are we using jiffies in 2023? > > > > I assumed that the correct thing here would be to accept the same unit > > as what schedule_timeout takes. Should I be doing something else? > > Bah, so we have schedule_hrtimeout() that takes ktime/u64 nsec. But the > 'problem' is that hrtimers are written with the expectation to fire, > while the old timers are written with the expectation to not fire. > > Timeouts are typically best done with the latter, so in that regard > using schedule_timeout() is right. But it is sad to inflict the > brain-damage that is jiffies onto new code. > > Perhaps add schedule_timeout_*msec() wrappers around schedule_timeout*() > and use a consistent sane time unit? > > Thomas? Hmm, looking over my usage in Rust Binder again ... the unit I need *is* msec, but when we are woken up, we sometimes just go to sleep again, which means that we need to be able to pass the remaining duration back to `wait_timeout` to continue sleeping. I'm guessing that I would lose precision if I converted back/forth to msecs here? Alice
On 06/12/2023 10:09, Alice Ryhl wrote: [...] > diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs > index 9861c6749ad0..a6a6b6ab0c39 100644 > --- a/rust/kernel/sync/condvar.rs > +++ b/rust/kernel/sync/condvar.rs > @@ -120,6 +120,63 @@ fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guar > unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) }; > } > > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > + /// [`CondVar::notify_all`], or when the thread receives a signal. > + /// > + /// Returns whether there is a signal pending. > + fn wait_internal_timeout<T, B>( > + &self, > + wait_state: u32, > + guard: &mut Guard<'_, T, B>, > + timeout: u64, > + ) -> u64 > + where > + T: ?Sized, > + B: Backend, > + { > + let wait = Opaque::<bindings::wait_queue_entry>::uninit(); > + > + // SAFETY: `wait` points to valid memory. > + unsafe { bindings::init_wait(wait.get()) }; > + > + // SAFETY: Both `wait` and `wait_list` point to valid memory. > + unsafe { > + bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _) > + }; > + > + // SAFETY: Switches to another thread. > + let timeout = > + guard.do_unlocked(|| unsafe { bindings::schedule_timeout(timeout as _) as _ }); It looks like `schedule_timeout()` simply calls `schedule()` when the timeout passed is `MAX_SCHEDULE_TIMEOUT`, so `wait_internal_timeout()` could be merged together with the already existing `wait_internal()`, where `wait_internal()` would always call `schedule_timeout()`? I may be missing something, so just wondering why you decided to introduce another method. > + > + // SAFETY: Both `wait` and `wait_list` point to valid memory. > + unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) }; > + > + timeout > + } > + > + /// Releases the lock and waits for a notification in interruptible mode. > + /// > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > + /// [`CondVar::notify_all`], or when a timeout occurs, or when the thread receives a signal. > + /// > + /// Returns whether there is a signal pending. > + #[must_use = "wait_timeout returns if a signal is pending, so the caller must check the return value"] > + pub fn wait_timeout<T: ?Sized, B: Backend>( > + &self, > + guard: &mut Guard<'_, T, B>, > + jiffies: u64, > + ) -> CondVarTimeoutResult { Should this be called `wait_timeout_interruptable` instead, so that if we need to add one using the `TASK_INTERRUPTIBLE` state later we don't need to modfy it again? It also matches the `schedule_timeout_interruptible` one in the kernel (although that's not a reason to change it just in itself). > + let res = self.wait_internal_timeout(bindings::TASK_INTERRUPTIBLE, guard, jiffies); > + > + match (res as _, crate::current!().signal_pending()) { > + (jiffies, true) => CondVarTimeoutResult::Signal { jiffies }, > + (0, false) => CondVarTimeoutResult::Timeout, > + (jiffies, false) => CondVarTimeoutResult::Woken { jiffies }, > + } > + } > + > /// Releases the lock and waits for a notification in interruptible mode. > /// > /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > @@ -177,3 +234,19 @@ pub fn notify_all(&self) { > self.notify(0, 0); > } > } > + > +/// The return type of `wait_timeout`. > +pub enum CondVarTimeoutResult { > + /// The timeout was reached. > + Timeout, > + /// Somebody woke us up. > + Woken { > + /// Remaining sleep duration. > + jiffies: u64, > + }, > + /// A signal occurred. > + Signal { > + /// Remaining sleep duration. > + jiffies: u64, > + }, > +} Is `Signal` and `Woken` only going to hold a single value? Would it be best represented as a tuple struct instead, like so? pub enum CondVarTimeoutResult { /// The timeout was reached. Timeout, /// Somebody woke us up. Woken (u64), /// A signal occurred. Signal (u64), } Regard, Tiago.
On Wed, Dec 6, 2023 at 6:05 PM Tiago Lam <tiagolam@gmail.com> wrote: > > > On 06/12/2023 10:09, Alice Ryhl wrote: > [...] > > diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs > > index 9861c6749ad0..a6a6b6ab0c39 100644 > > --- a/rust/kernel/sync/condvar.rs > > +++ b/rust/kernel/sync/condvar.rs > > @@ -120,6 +120,63 @@ fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guar > > unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) }; > > } > > > > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > > + /// [`CondVar::notify_all`], or when the thread receives a signal. > > + /// > > + /// Returns whether there is a signal pending. > > + fn wait_internal_timeout<T, B>( > > + &self, > > + wait_state: u32, > > + guard: &mut Guard<'_, T, B>, > > + timeout: u64, > > + ) -> u64 > > + where > > + T: ?Sized, > > + B: Backend, > > + { > > + let wait = Opaque::<bindings::wait_queue_entry>::uninit(); > > + > > + // SAFETY: `wait` points to valid memory. > > + unsafe { bindings::init_wait(wait.get()) }; > > + > > + // SAFETY: Both `wait` and `wait_list` point to valid memory. > > + unsafe { > > + bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _) > > + }; > > + > > + // SAFETY: Switches to another thread. > > + let timeout = > > + guard.do_unlocked(|| unsafe { bindings::schedule_timeout(timeout as _) as _ }); > > It looks like `schedule_timeout()` simply calls `schedule()` when the > timeout passed is `MAX_SCHEDULE_TIMEOUT`, so `wait_internal_timeout()` > could be merged together with the already existing `wait_internal()`, > where `wait_internal()` would always call `schedule_timeout()`? I may be > missing something, so just wondering why you decided to introduce > another method. Ah, nice! I didn't notice that I could combine them. > > + /// Releases the lock and waits for a notification in interruptible mode. > > + /// > > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > > + /// [`CondVar::notify_all`], or when a timeout occurs, or when the thread receives a signal. > > + /// > > + /// Returns whether there is a signal pending. > > + #[must_use = "wait_timeout returns if a signal is pending, so the caller must check the return value"] > > + pub fn wait_timeout<T: ?Sized, B: Backend>( > > + &self, > > + guard: &mut Guard<'_, T, B>, > > + jiffies: u64, > > + ) -> CondVarTimeoutResult { > > Should this be called `wait_timeout_interruptable` instead, so that if > we need to add one using the `TASK_INTERRUPTIBLE` state later we don't > need to modfy it again? It also matches the > `schedule_timeout_interruptible` one in the kernel (although that's not > a reason to change it just in itself). I don't mind changing the names, but in this patch I was just consistent with what was already there. > > +/// The return type of `wait_timeout`. > > +pub enum CondVarTimeoutResult { > > + /// The timeout was reached. > > + Timeout, > > + /// Somebody woke us up. > > + Woken { > > + /// Remaining sleep duration. > > + jiffies: u64, > > + }, > > + /// A signal occurred. > > + Signal { > > + /// Remaining sleep duration. > > + jiffies: u64, > > + }, > > +} > > > Is `Signal` and `Woken` only going to hold a single value? Would it be > best represented as a tuple struct instead, like so? > > pub enum CondVarTimeoutResult { > /// The timeout was reached. > Timeout, > /// Somebody woke us up. > Woken (u64), > /// A signal occurred. > Signal (u64), > } I could do that, but I like the explicitly named version as it makes it clear that the unit is jiffies. Alice
On 12/8/23 08:37, Alice Ryhl wrote: > On Wed, Dec 6, 2023 at 6:05 PM Tiago Lam <tiagolam@gmail.com> wrote: >> On 06/12/2023 10:09, Alice Ryhl wrote: >>> +/// The return type of `wait_timeout`. >>> +pub enum CondVarTimeoutResult { >>> + /// The timeout was reached. >>> + Timeout, >>> + /// Somebody woke us up. >>> + Woken { >>> + /// Remaining sleep duration. >>> + jiffies: u64, >>> + }, >>> + /// A signal occurred. >>> + Signal { >>> + /// Remaining sleep duration. >>> + jiffies: u64, >>> + }, >>> +} >> >> Is `Signal` and `Woken` only going to hold a single value? Would it be >> best represented as a tuple struct instead, like so? >> >> pub enum CondVarTimeoutResult { >> /// The timeout was reached. >> Timeout, >> /// Somebody woke us up. >> Woken (u64), >> /// A signal occurred. >> Signal (u64), >> } > > I could do that, but I like the explicitly named version as it makes > it clear that the unit is jiffies. Why not use `type Jiffies = u64;` until we have proper bindings for them? That way we can have both.
On 12/6/23 11:09, Alice Ryhl wrote: > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > + /// [`CondVar::notify_all`], or when the thread receives a signal. > + /// > + /// Returns whether there is a signal pending. > + fn wait_internal_timeout<T, B>( > + &self, > + wait_state: u32, > + guard: &mut Guard<'_, T, B>, > + timeout: u64, > + ) -> u64 > + where > + T: ?Sized, > + B: Backend, > + { > + let wait = Opaque::<bindings::wait_queue_entry>::uninit(); > + > + // SAFETY: `wait` points to valid memory. > + unsafe { bindings::init_wait(wait.get()) }; > + > + // SAFETY: Both `wait` and `wait_list` point to valid memory. > + unsafe { > + bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _) Does `.into()` work here? If for some reason the type here changes, we probably want to know about it. > + }; > + > + // SAFETY: Switches to another thread. > + let timeout = > + guard.do_unlocked(|| unsafe { bindings::schedule_timeout(timeout as _) as _ }); Ditto.
On Fri, Dec 8, 2023 at 10:27 AM Benno Lossin <benno.lossin@proton.me> wrote: > > On 12/8/23 08:37, Alice Ryhl wrote: > > On Wed, Dec 6, 2023 at 6:05 PM Tiago Lam <tiagolam@gmail.com> wrote: > >> On 06/12/2023 10:09, Alice Ryhl wrote: > >>> +/// The return type of `wait_timeout`. > >>> +pub enum CondVarTimeoutResult { > >>> + /// The timeout was reached. > >>> + Timeout, > >>> + /// Somebody woke us up. > >>> + Woken { > >>> + /// Remaining sleep duration. > >>> + jiffies: u64, > >>> + }, > >>> + /// A signal occurred. > >>> + Signal { > >>> + /// Remaining sleep duration. > >>> + jiffies: u64, > >>> + }, > >>> +} > >> > >> Is `Signal` and `Woken` only going to hold a single value? Would it be > >> best represented as a tuple struct instead, like so? > >> > >> pub enum CondVarTimeoutResult { > >> /// The timeout was reached. > >> Timeout, > >> /// Somebody woke us up. > >> Woken (u64), > >> /// A signal occurred. > >> Signal (u64), > >> } > > > > I could do that, but I like the explicitly named version as it makes > > it clear that the unit is jiffies. > > Why not use `type Jiffies = u64;` until we have proper bindings for > them? That way we can have both. I do not mind adding and using a type alias, but I still think the named fields are better. Alice
On Fri, Dec 8, 2023 at 8:04 PM Benno Lossin <benno.lossin@proton.me> wrote: > > On 12/6/23 11:09, Alice Ryhl wrote: > > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > > + /// [`CondVar::notify_all`], or when the thread receives a signal. > > + /// > > + /// Returns whether there is a signal pending. > > + fn wait_internal_timeout<T, B>( > > + &self, > > + wait_state: u32, > > + guard: &mut Guard<'_, T, B>, > > + timeout: u64, > > + ) -> u64 > > + where > > + T: ?Sized, > > + B: Backend, > > + { > > + let wait = Opaque::<bindings::wait_queue_entry>::uninit(); > > + > > + // SAFETY: `wait` points to valid memory. > > + unsafe { bindings::init_wait(wait.get()) }; > > + > > + // SAFETY: Both `wait` and `wait_list` point to valid memory. > > + unsafe { > > + bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _) > > Does `.into()` work here? If for some reason the type here changes, we > probably want to know about it. I think we may be able to eliminate this cast by using c_int for the integer type. > > + }; > > + > > + // SAFETY: Switches to another thread. > > + let timeout = > > + guard.do_unlocked(|| unsafe { bindings::schedule_timeout(timeout as _) as _ }); > > Ditto. Here, we're casting u64->long and then long->u64. How about this? u64->long - Use timeout.try_into().unwrap_or(MAX_SCHEDULE_TIMEOUT), since MAX_SCHEDULE_TIMEOUT is LONG_MAX. long->u64 - This value is guaranteed to be less than the argument passed to schedule_timeout. Use .into() for infallible cast. Alice
On 12/12/23 10:51, Alice Ryhl wrote: > On Fri, Dec 8, 2023 at 8:04 PM Benno Lossin <benno.lossin@proton.me> wrote: >> >> On 12/6/23 11:09, Alice Ryhl wrote: >>> + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the >>> + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or >>> + /// [`CondVar::notify_all`], or when the thread receives a signal. >>> + /// >>> + /// Returns whether there is a signal pending. >>> + fn wait_internal_timeout<T, B>( >>> + &self, >>> + wait_state: u32, >>> + guard: &mut Guard<'_, T, B>, >>> + timeout: u64, >>> + ) -> u64 >>> + where >>> + T: ?Sized, >>> + B: Backend, >>> + { >>> + let wait = Opaque::<bindings::wait_queue_entry>::uninit(); >>> + >>> + // SAFETY: `wait` points to valid memory. >>> + unsafe { bindings::init_wait(wait.get()) }; >>> + >>> + // SAFETY: Both `wait` and `wait_list` point to valid memory. >>> + unsafe { >>> + bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _) >> >> Does `.into()` work here? If for some reason the type here changes, we >> probably want to know about it. > > I think we may be able to eliminate this cast by using c_int for the > integer type. Sounds good. >>> + }; >>> + >>> + // SAFETY: Switches to another thread. >>> + let timeout = >>> + guard.do_unlocked(|| unsafe { bindings::schedule_timeout(timeout as _) as _ }); >> >> Ditto. > > Here, we're casting u64->long and then long->u64. How about this? > > u64->long - Use timeout.try_into().unwrap_or(MAX_SCHEDULE_TIMEOUT), > since MAX_SCHEDULE_TIMEOUT is LONG_MAX. > > long->u64 - This value is guaranteed to be less than the argument > passed to schedule_timeout. Use .into() for infallible cast. Also sounds good :)
On Fri, Dec 08, 2023 at 08:37:27AM +0100, Alice Ryhl wrote: [...] > > > + /// Releases the lock and waits for a notification in interruptible mode. > > > + /// > > > + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the > > > + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or > > > + /// [`CondVar::notify_all`], or when a timeout occurs, or when the thread receives a signal. > > > + /// > > > + /// Returns whether there is a signal pending. > > > + #[must_use = "wait_timeout returns if a signal is pending, so the caller must check the return value"] > > > + pub fn wait_timeout<T: ?Sized, B: Backend>( > > > + &self, > > > + guard: &mut Guard<'_, T, B>, > > > + jiffies: u64, > > > + ) -> CondVarTimeoutResult { > > > > Should this be called `wait_timeout_interruptable` instead, so that if > > we need to add one using the `TASK_INTERRUPTIBLE` state later we don't > > need to modfy it again? It also matches the > > `schedule_timeout_interruptible` one in the kernel (although that's not > > a reason to change it just in itself). > > I don't mind changing the names, but in this patch I was just > consistent with what was already there. > Hmm.. so Rust's wait() is actually interruptible wait and we have wait_uninterruptible(), while C API is wait_event() is uninterruptible, and we have a wait_event_interruptible(), I think it makes sense we follow what C API has. Will send a patch soon. Regards, Boqun [...]
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index d219ee518eff..c1fb10fc64f4 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -13,7 +13,7 @@ mod locked_by; pub use arc::{Arc, ArcBorrow, UniqueArc}; -pub use condvar::CondVar; +pub use condvar::{CondVar, CondVarTimeoutResult}; pub use lock::{mutex::Mutex, spinlock::SpinLock}; pub use locked_by::LockedBy; diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index 9861c6749ad0..a6a6b6ab0c39 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -120,6 +120,63 @@ fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guar unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) }; } + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or + /// [`CondVar::notify_all`], or when the thread receives a signal. + /// + /// Returns whether there is a signal pending. + fn wait_internal_timeout<T, B>( + &self, + wait_state: u32, + guard: &mut Guard<'_, T, B>, + timeout: u64, + ) -> u64 + where + T: ?Sized, + B: Backend, + { + let wait = Opaque::<bindings::wait_queue_entry>::uninit(); + + // SAFETY: `wait` points to valid memory. + unsafe { bindings::init_wait(wait.get()) }; + + // SAFETY: Both `wait` and `wait_list` point to valid memory. + unsafe { + bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _) + }; + + // SAFETY: Switches to another thread. + let timeout = + guard.do_unlocked(|| unsafe { bindings::schedule_timeout(timeout as _) as _ }); + + // SAFETY: Both `wait` and `wait_list` point to valid memory. + unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) }; + + timeout + } + + /// Releases the lock and waits for a notification in interruptible mode. + /// + /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the + /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or + /// [`CondVar::notify_all`], or when a timeout occurs, or when the thread receives a signal. + /// + /// Returns whether there is a signal pending. + #[must_use = "wait_timeout returns if a signal is pending, so the caller must check the return value"] + pub fn wait_timeout<T: ?Sized, B: Backend>( + &self, + guard: &mut Guard<'_, T, B>, + jiffies: u64, + ) -> CondVarTimeoutResult { + let res = self.wait_internal_timeout(bindings::TASK_INTERRUPTIBLE, guard, jiffies); + + match (res as _, crate::current!().signal_pending()) { + (jiffies, true) => CondVarTimeoutResult::Signal { jiffies }, + (0, false) => CondVarTimeoutResult::Timeout, + (jiffies, false) => CondVarTimeoutResult::Woken { jiffies }, + } + } + /// Releases the lock and waits for a notification in interruptible mode. /// /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the @@ -177,3 +234,19 @@ pub fn notify_all(&self) { self.notify(0, 0); } } + +/// The return type of `wait_timeout`. +pub enum CondVarTimeoutResult { + /// The timeout was reached. + Timeout, + /// Somebody woke us up. + Woken { + /// Remaining sleep duration. + jiffies: u64, + }, + /// A signal occurred. + Signal { + /// Remaining sleep duration. + jiffies: u64, + }, +} diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index f12a684bc957..149a5259d431 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -139,7 +139,7 @@ pub struct Guard<'a, T: ?Sized, B: Backend> { unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {} impl<T: ?Sized, B: Backend> Guard<'_, T, B> { - pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) { + pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U { // SAFETY: The caller owns the lock, so it is safe to unlock it. unsafe { B::unlock(self.lock.state.get(), &self.state) }; @@ -147,7 +147,7 @@ pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) { let _relock = ScopeGuard::new(|| unsafe { B::relock(self.lock.state.get(), &mut self.state) }); - cb(); + cb() } }