From 41f83ffade0fc2fd50eebd42b6d2fb67763787af Mon Sep 17 00:00:00 2001 From: Wedson Almeida Filho Date: Wed, 29 Mar 2023 03:14:54 -0300 Subject: [PATCH 1/2] rust: arc: rename `ArcInner` to `WithRef` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is in preparation for removing `ArcBorrow` and making `WithRef` public. Suggested-by: Björn Roy Baron Signed-off-by: Wedson Almeida Filho --- rust/kernel/sync/arc.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 3d496391a9bd86..2661dc8a278b77 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -126,13 +126,13 @@ mod std_vendor; /// # Ok::<(), Error>(()) /// ``` pub struct Arc { - ptr: NonNull>, - _p: PhantomData>, + ptr: NonNull>, + _p: PhantomData>, } #[pin_data] #[repr(C)] -struct ArcInner { +struct WithRef { refcount: Opaque, data: T, } @@ -164,7 +164,7 @@ impl Arc { /// Constructs a new reference counted instance of `T`. pub fn try_new(contents: T) -> Result { // INVARIANT: The refcount is initialised to a non-zero value. - let value = ArcInner { + let value = WithRef { // SAFETY: There are no safety requirements for this FFI call. refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }), data: contents, @@ -201,13 +201,13 @@ impl Arc { } impl Arc { - /// Constructs a new [`Arc`] from an existing [`ArcInner`]. + /// Constructs a new [`Arc`] from an existing [`WithRef`]. /// /// # Safety /// /// The caller must ensure that `inner` points to a valid location and has a non-zero reference /// count, one of which will be owned by the new [`Arc`] instance. - unsafe fn from_inner(inner: NonNull>) -> Self { + unsafe fn from_inner(inner: NonNull>) -> Self { // INVARIANT: By the safety requirements, the invariants hold. Arc { ptr: inner, @@ -243,7 +243,7 @@ impl ForeignOwnable for Arc { unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> { // SAFETY: By the safety requirement of this function, we know that `ptr` came from // a previous call to `Arc::into_foreign`. - let inner = NonNull::new(ptr as *mut ArcInner).unwrap(); + let inner = NonNull::new(ptr as *mut WithRef).unwrap(); // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive // for the lifetime of the returned value. @@ -376,7 +376,7 @@ impl From>> for Arc { /// # Ok::<(), Error>(()) /// ``` pub struct ArcBorrow<'a, T: ?Sized + 'a> { - inner: NonNull>, + inner: NonNull>, _p: PhantomData<&'a ()>, } @@ -406,7 +406,7 @@ impl ArcBorrow<'_, T> { /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance: /// 1. That `inner` remains valid; /// 2. That no mutable references to `inner` are created. - unsafe fn new(inner: NonNull>) -> Self { + unsafe fn new(inner: NonNull>) -> Self { // INVARIANT: The safety requirements guarantee the invariants. Self { inner, From 56ba416ed4835df0a142f79c59f3e3bb584001af Mon Sep 17 00:00:00 2001 From: Wedson Almeida Filho Date: Wed, 29 Mar 2023 03:53:37 -0300 Subject: [PATCH 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef` With GATs, we don't need a separate type to represent a borrowed object with a refcount, we can just use Rust's regular shared borrowing. In this case, we use `&WithRef` instead of `ArcBorrow<'_, T>`. Co-developed-by: Boqun Feng Signed-off-by: Boqun Feng Signed-off-by: Wedson Almeida Filho --- rust/kernel/sync.rs | 2 +- rust/kernel/sync/arc.rs | 182 ++++++++++------------------- rust/kernel/sync/arc/std_vendor.rs | 4 +- 3 files changed, 65 insertions(+), 123 deletions(-) diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index d219ee518eff15..0834948845000a 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -12,7 +12,7 @@ mod condvar; pub mod lock; mod locked_by; -pub use arc::{Arc, ArcBorrow, UniqueArc}; +pub use arc::{Arc, UniqueArc, WithRef}; pub use condvar::CondVar; pub use lock::{mutex::Mutex, spinlock::SpinLock}; pub use locked_by::LockedBy; diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 2661dc8a278b77..5948e42b9c8fa2 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -105,14 +105,14 @@ mod std_vendor; /// Coercion from `Arc` to `Arc`: /// /// ``` -/// use kernel::sync::{Arc, ArcBorrow}; +/// use kernel::sync::{Arc, WithRef}; /// /// trait MyTrait { /// // Trait has a function whose `self` type is `Arc`. /// fn example1(self: Arc) {} /// -/// // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`. -/// fn example2(self: ArcBorrow<'_, Self>) {} +/// // Trait has a function whose `self` type is `&WithRef`. +/// fn example2(self: &WithRef) {} /// } /// /// struct Example; @@ -130,9 +130,48 @@ pub struct Arc { _p: PhantomData>, } +/// An instance of `T` with an attached reference count. +/// +/// # Examples +/// +/// ``` +/// use kernel::sync::{Arc, WithRef}; +/// +/// struct Example; +/// +/// fn do_something(e: &WithRef) -> Arc { +/// e.into() +/// } +/// +/// let obj = Arc::try_new(Example)?; +/// let cloned = do_something(obj.as_with_ref()); +/// +/// // Assert that both `obj` and `cloned` point to the same underlying object. +/// assert!(core::ptr::eq(&*obj, &*cloned)); +/// ``` +/// +/// Using `WithRef` as the type of `self`: +/// +/// ``` +/// use kernel::sync::{Arc, WithRef}; +/// +/// struct Example { +/// _a: u32, +/// _b: u32, +/// } +/// +/// impl Example { +/// fn use_reference(self: &WithRef) { +/// // ... +/// } +/// } +/// +/// let obj = Arc::try_new(Example { _a: 10, _b: 20 })?; +/// obj.as_with_ref().use_reference(); +/// ``` #[pin_data] #[repr(C)] -struct WithRef { +pub struct WithRef { refcount: Opaque, data: T, } @@ -215,16 +254,16 @@ impl Arc { } } - /// Returns an [`ArcBorrow`] from the given [`Arc`]. + /// Returns a [`WithRef`] from the given [`Arc`]. /// - /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method - /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised. + /// This is useful when the argument of a function call is a [`WithRef`] (e.g., in a method + /// receiver), but we have an [`Arc`] instead. Getting a [`WithRef`] is free when optimised. #[inline] - pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> { + pub fn as_with_ref(&self) -> &WithRef { // SAFETY: The constraint that the lifetime of the shared reference must outlive that of - // the returned `ArcBorrow` ensures that the object remains alive and that no mutable + // the returned `WithRef` ensures that the object remains alive and that no mutable // reference can be created. - unsafe { ArcBorrow::new(self.ptr) } + unsafe { self.ptr.as_ref() } } /// Compare whether two [`Arc`] pointers reference the same underlying object. @@ -234,20 +273,17 @@ impl Arc { } impl ForeignOwnable for Arc { - type Borrowed<'a> = ArcBorrow<'a, T>; + type Borrowed<'a> = &'a WithRef; fn into_foreign(self) -> *const core::ffi::c_void { ManuallyDrop::new(self).ptr.as_ptr() as _ } - unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> { + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a WithRef { // SAFETY: By the safety requirement of this function, we know that `ptr` came from - // a previous call to `Arc::into_foreign`. - let inner = NonNull::new(ptr as *mut WithRef).unwrap(); - - // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive - // for the lifetime of the returned value. - unsafe { ArcBorrow::new(inner) } + // a previous call to `Arc::into_foreign`. The safety requirements of `from_foreign` ensure + // that the object remains alive for the lifetime of the returned value. + unsafe { &*(ptr.cast::>()) } } unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { @@ -320,119 +356,25 @@ impl From>> for Arc { } } -/// A borrowed reference to an [`Arc`] instance. -/// -/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler -/// to use just `&T`, which we can trivially get from an `Arc` instance. -/// -/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow` -/// over `&Arc` because the latter results in a double-indirection: a pointer (shared reference) -/// to a pointer (`Arc`) to the object (`T`). An [`ArcBorrow`] eliminates this double -/// indirection while still allowing one to increment the refcount and getting an `Arc` when/if -/// needed. -/// -/// # Invariants -/// -/// There are no mutable references to the underlying [`Arc`], and it remains valid for the -/// lifetime of the [`ArcBorrow`] instance. -/// -/// # Example -/// -/// ``` -/// use kernel::sync::{Arc, ArcBorrow}; -/// -/// struct Example; -/// -/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc { -/// e.into() -/// } -/// -/// let obj = Arc::try_new(Example)?; -/// let cloned = do_something(obj.as_arc_borrow()); -/// -/// // Assert that both `obj` and `cloned` point to the same underlying object. -/// assert!(core::ptr::eq(&*obj, &*cloned)); -/// # Ok::<(), Error>(()) -/// ``` -/// -/// Using `ArcBorrow` as the type of `self`: -/// -/// ``` -/// use kernel::sync::{Arc, ArcBorrow}; -/// -/// struct Example { -/// a: u32, -/// b: u32, -/// } -/// -/// impl Example { -/// fn use_reference(self: ArcBorrow<'_, Self>) { -/// // ... -/// } -/// } -/// -/// let obj = Arc::try_new(Example { a: 10, b: 20 })?; -/// obj.as_arc_borrow().use_reference(); -/// # Ok::<(), Error>(()) -/// ``` -pub struct ArcBorrow<'a, T: ?Sized + 'a> { - inner: NonNull>, - _p: PhantomData<&'a ()>, -} - -// This is to allow [`ArcBorrow`] (and variants) to be used as the type of `self`. -impl core::ops::Receiver for ArcBorrow<'_, T> {} - -// This is to allow `ArcBorrow` to be dispatched on when `ArcBorrow` can be coerced into -// `ArcBorrow`. -impl, U: ?Sized> core::ops::DispatchFromDyn> - for ArcBorrow<'_, T> -{ -} - -impl Clone for ArcBorrow<'_, T> { - fn clone(&self) -> Self { - *self - } -} - -impl Copy for ArcBorrow<'_, T> {} - -impl ArcBorrow<'_, T> { - /// Creates a new [`ArcBorrow`] instance. - /// - /// # Safety - /// - /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance: - /// 1. That `inner` remains valid; - /// 2. That no mutable references to `inner` are created. - unsafe fn new(inner: NonNull>) -> Self { - // INVARIANT: The safety requirements guarantee the invariants. - Self { - inner, - _p: PhantomData, - } - } -} +// This is to allow [`WithRef`] (and variants) to be used as the type of `self`. +impl core::ops::Receiver for WithRef {} -impl From> for Arc { - fn from(b: ArcBorrow<'_, T>) -> Self { +impl From<&WithRef> for Arc { + fn from(b: &WithRef) -> Self { // SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop` // guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the // increment. - ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) }) + ManuallyDrop::new(unsafe { Arc::from_inner(b.into()) }) .deref() .clone() } } -impl Deref for ArcBorrow<'_, T> { +impl Deref for WithRef { type Target = T; fn deref(&self) -> &Self::Target { - // SAFETY: By the type invariant, the underlying object is still alive with no mutable - // references to it, so it is safe to create a shared reference. - unsafe { &self.inner.as_ref().data } + &self.data } } @@ -526,7 +468,7 @@ impl UniqueArc { /// Tries to allocate a new [`UniqueArc`] instance whose contents are not initialised yet. pub fn try_new_uninit() -> Result>, AllocError> { // INVARIANT: The refcount is initialised to a non-zero value. - let inner = Box::try_init::(try_init!(ArcInner { + let inner = Box::try_init::(try_init!(WithRef { // SAFETY: There are no safety requirements for this FFI call. refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }), data <- init::uninit::(), diff --git a/rust/kernel/sync/arc/std_vendor.rs b/rust/kernel/sync/arc/std_vendor.rs index a66a0c2831b3ed..4b30e5597ba5dd 100644 --- a/rust/kernel/sync/arc/std_vendor.rs +++ b/rust/kernel/sync/arc/std_vendor.rs @@ -5,7 +5,7 @@ //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details, //! see . -use crate::sync::{arc::ArcInner, Arc}; +use crate::sync::{arc::WithRef, Arc}; use core::any::Any; impl Arc { @@ -17,7 +17,7 @@ impl Arc { if (*self).is::() { // SAFETY: We have just checked that the type is correct, so we can cast the pointer. unsafe { - let ptr = self.ptr.cast::>(); + let ptr = self.ptr.cast::>(); core::mem::forget(self); Ok(Arc::from_inner(ptr)) }