From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-4322.protonmail.ch (mail-4322.protonmail.ch [185.70.43.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2A8BF21B8E1 for ; Fri, 7 Mar 2025 14:30:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.70.43.22 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741357811; cv=none; b=mDMYUg4M/hQjEoZW/mfoB0j3wVCuUfA5Z/p7Cxr5CbH8SBp2ERQrTRfKbGlbs2Y3wG3Xze7kNltpxOMyqDJJDN1a7zGP3YzqnHGlOztm/UBt/YxKQCRcC480GH1A5ffBA4xCuTRIIdyoxxcwxBC4ZgYPwg/TNjbbN4w9ZcrSk3c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741357811; c=relaxed/simple; bh=fleqeP/FCDmwdLw5F27AS7KWR4YOCNwwEmIYnsgNX5I=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=lbksIN4aczgGrFGhS3rMbhO+HCeLU45e/+hYAeg5uvt4tfkuDItxZrCL1364v4ZFTNL/DqMTTgIpyiq7soTGMsHY93v3pgjE/CHNHiCps6yqsBCsQJQAmNq9o7e40E0DVO2csGlbNDql9oQrxsy8XlJANlG34bG4UUCZd3zotzo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me; spf=pass smtp.mailfrom=proton.me; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b=gjyO6s3L; arc=none smtp.client-ip=185.70.43.22 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=proton.me Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b="gjyO6s3L" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=ropyt3ciq5djhfusp5t3zvr6ym.protonmail; t=1741357807; x=1741617007; bh=BzotiybtxOMnzczcpfh7Ghvit0KX8yvXjEj11c8kWdo=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=gjyO6s3LumOMFUMfZ0XjK0YNnrzenw1MA6Y3M6Tzh13R7qjSt2OrhsPKsGEUcBHcp um6ePatPVyAbw+yRF3ef6otirtceNn/hSB15/1su7L/Wtrg7+dT1fxi8ZvrHAS5R8w 9ajuviPo8bPug+Gt6Rc068SphJsIDUvyPGUYCi+jBczNCYt4yStMnyD3C051IvOBIa bCswLxH6y47UDwttD5LJq7J8ew7+36m0uzQy0oYh5TCzL6Jsbad0rISEpHOktGX7aR TKV0Z1hoKFH/wLj6pXKLfZREoD0uw4sgoir7E6Q6C/alYcFx3HBmB8RJ/keOqzQBkh fsMUHV3vyBm7A== Date: Fri, 07 Mar 2025 14:29:58 +0000 To: Andreas Hindborg From: Benno Lossin Cc: Miguel Ojeda , Anna-Maria Behnsen , Frederic Weisbecker , Thomas Gleixner , Danilo Krummrich , Alex Gaynor , Boqun Feng , Gary Guo , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Alice Ryhl , Trevor Gross , Lyude Paul , Guangbo Cui <2407018371@qq.com>, Dirk Behme , Daniel Almeida , Tamir Duberstein , Markus Elfring , rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v10 10/13] rust: hrtimer: implement `HrTimerPointer` for `Pin>` Message-ID: In-Reply-To: <87bjud3po0.fsf@kernel.org> References: <20250307-hrtimer-v3-v6-12-rc2-v10-0-0cf7e9491da4@kernel.org> <20250307-hrtimer-v3-v6-12-rc2-v10-10-0cf7e9491da4@kernel.org> <87bjud3po0.fsf@kernel.org> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: 61db47778e6dfdfa1623c35b5aab7d7c3edffb7a Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On Fri Mar 7, 2025 at 3:01 PM CET, Andreas Hindborg wrote: > "Benno Lossin" writes: > >> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote: >>> Allow `Pin>` to be the target of a timer callback. >>> >>> Acked-by: Frederic Weisbecker >>> Reviewed-by: Lyude Paul >>> Signed-off-by: Andreas Hindborg >>> --- >>> rust/kernel/time/hrtimer.rs | 3 ++ >>> rust/kernel/time/hrtimer/tbox.rs | 109 +++++++++++++++++++++++++++++++= ++++++++ >>> 2 files changed, 112 insertions(+) >>> >>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs >>> index d2791fd624b7..991d37b0524a 100644 >>> --- a/rust/kernel/time/hrtimer.rs >>> +++ b/rust/kernel/time/hrtimer.rs >>> @@ -443,3 +443,6 @@ unsafe fn timer_container_of(ptr: *mut $crate::time= ::hrtimer::HrTimer<$timer_typ >>> pub use pin::PinHrTimerHandle; >>> mod pin_mut; >>> pub use pin_mut::PinMutHrTimerHandle; >>> +// `box` is a reserved keyword, so prefix with `t` for timer >>> +mod tbox; >>> +pub use tbox::BoxHrTimerHandle; >>> diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtime= r/tbox.rs >>> new file mode 100644 >>> index 000000000000..a3b2ed849050 >>> --- /dev/null >>> +++ b/rust/kernel/time/hrtimer/tbox.rs >>> @@ -0,0 +1,109 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +use super::HasHrTimer; >>> +use super::HrTimer; >>> +use super::HrTimerCallback; >>> +use super::HrTimerHandle; >>> +use super::HrTimerPointer; >>> +use super::RawHrTimerCallback; >>> +use crate::prelude::*; >>> +use crate::time::Ktime; >>> +use core::mem::ManuallyDrop; >>> +use core::ptr::NonNull; >>> + >>> +/// A handle for a [`Box>`] returned by a call to >>> +/// [`HrTimerPointer::start`]. >>> +pub struct BoxHrTimerHandle >> >> Should this type implement `Send` and `Sync` depending on `T`? > > Yes. In practice `T` will always be `Send` and `Sync` because of bounds > on other traits. > > I don't think we have to require `T: Sync`, because the handle does not e= ver > create shared references to the underlying `T`? Oh I meant to do: unsafe impl Send for BoxHrTimerHandle {} But since you don't have it, it might be unnecessary. >>> +where >>> + T: HasHrTimer, >>> + A: crate::alloc::Allocator, >>> +{ >>> + pub(crate) inner: NonNull, >>> + _p: core::marker::PhantomData, >>> +} >>> + >>> +// SAFETY: We implement drop below, and we cancel the timer in the dro= p >>> +// implementation. >>> +unsafe impl HrTimerHandle for BoxHrTimerHandle >>> +where >>> + T: HasHrTimer, >>> + A: crate::alloc::Allocator, >>> +{ >>> + fn cancel(&mut self) -> bool { >>> + // SAFETY: As we obtained `self.inner` from a valid reference = when we >>> + // created `self`, it must point to a valid `T`. >>> + let timer_ptr =3D unsafe { >::raw_get_timer= (self.inner.as_ptr()) }; >>> + >>> + // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `t= imer_ptr` >>> + // must point to a valid `HrTimer` instance. >>> + unsafe { HrTimer::::raw_cancel(timer_ptr) } >>> + } >>> +} >>> + >>> +impl Drop for BoxHrTimerHandle >>> +where >>> + T: HasHrTimer, >>> + A: crate::alloc::Allocator, >>> +{ >>> + fn drop(&mut self) { >>> + self.cancel(); >>> + // SAFETY: `self.inner` came from a `Box::into_raw` call >> >> Please add this as an invariant to `Self`. > > OK. > >> >>> + drop(unsafe { Box::::from_raw(self.inner.as_ptr()) }) >>> + } >>> +} >>> + >>> +impl HrTimerPointer for Pin> >>> +where >>> + T: 'static, >>> + T: Send + Sync, >>> + T: HasHrTimer, >>> + T: for<'a> HrTimerCallback =3D Pin>>, >>> + Pin>: for<'a> RawHrTimerCallback =3D = Pin<&'a T>>, >> >> I don't think this is necessary. > > Should I remove it? I feel like it communicates intent. What intent? >>> + A: crate::alloc::Allocator, >>> +{ >>> + type TimerHandle =3D BoxHrTimerHandle; >>> + >>> + fn start(self, expires: Ktime) -> Self::TimerHandle { >>> + // SAFETY: >>> + // - We will not move out of this box during timer callback (= we pass an >>> + // immutable reference to the callback). >>> + // - `Box::into_raw` is guaranteed to return a valid pointer. >>> + let inner =3D >>> + unsafe { NonNull::new_unchecked(Box::into_raw(Pin::into_in= ner_unchecked(self))) }; >>> + >>> + // SAFETY: >>> + // - We keep `self` alive by wrapping it in a handle below. >>> + // - Since we generate the pointer passed to `start` from a v= alid >>> + // reference, it is a valid pointer. >>> + unsafe { T::start(inner.as_ptr(), expires) }; >>> + >>> + BoxHrTimerHandle { >>> + inner, >>> + _p: core::marker::PhantomData, >>> + } >>> + } >>> +} >>> + >>> +impl RawHrTimerCallback for Pin> >>> +where >>> + T: 'static, >>> + T: HasHrTimer, >>> + T: for<'a> HrTimerCallback =3D Pin>>, >>> + A: crate::alloc::Allocator, >>> +{ >>> + type CallbackTarget<'a> =3D Pin<&'a T>; >> >> Why isn't this `Pin<&'a mut T>`? > > I don't think it matters much? There can be no other mutable references > while the callback is running, so why not a shared ref? IIUC there can be no references to the value, since the user used a `Pin>` to schedule the timer. I thought it might make sense to give a pinned mutable reference, since you explicitly implement the `RawHrTimerCallback` for `Pin<&mut T>`. Which made me believe one sometimes needs to modify the `T` from the callback. Since we're able to do that when the user used a `Box`, I think we should just do it. >>> + >>> + unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings:= :hrtimer_restart { >>> + // `HrTimer` is `repr(C)` >>> + let timer_ptr =3D ptr.cast::>(); >>> + >>> + // SAFETY: By C API contract `ptr` is the pointer we passed wh= en >>> + // queuing the timer, so it is a `HrTimer` embedded in a `T= `. >>> + let data_ptr =3D unsafe { T::timer_container_of(timer_ptr) }; >>> + >>> + // SAFETY: We called `Box::into_raw` when we queued the timer. >>> + let tbox =3D ManuallyDrop::new(Box::into_pin(unsafe { Box::::from_raw(data_ptr) })); >> >> Since you turn this into a reference below and never run the drop, why >> not turn the pointer directly into a reference? > > You mean replace with `unsafe {&*data_ptr};`? I guess that could work, > but it hinges on `Box` being transparent which is more subtle than going > through the API. I think that's cleaner. Also why does that rely on `Box` being transparent? --- Cheers, Benno