From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f74.google.com (mail-wm1-f74.google.com [209.85.128.74]) (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 3BB32281351 for ; Tue, 22 Apr 2025 14:08:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1745330885; cv=none; b=TDLL3iInpmO0VelvxrgbGXqj04Ce9mNzOUkRl3TkJfxUZc5nGER6X3k8Sz0/k0jFuDhHZCTYiTucqq0ODab8ydm5XGcSLmGdqAevTPDBFn3eu4WAYph9L9maG+06qmpzbmgETuN/tKSpq0xAefNmHW4jJEo3xW6HgQRhSMQzsSg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1745330885; c=relaxed/simple; bh=MnhkHQmJvsmVgFvYaPYNciLeUrxdKNlcCbhqYEL83cY=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=pJZ+m73BmXg12f2N7nHrjiw2LUZe55oQw8fvZSk3xH7sCDJdG9sojv2wEhZ0LgHF1EkSOosDE7HBzc4bVL5uI3g9pTk5++gMb1yo3f2+eQX9T5zcDeL5Qz6bzneV4Nrob4Ozu4jjBRCKcHpIvgSXuaMCiTQLc9GiJAUj4rFv6IE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=GaAcWWVU; arc=none smtp.client-ip=209.85.128.74 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="GaAcWWVU" Received: by mail-wm1-f74.google.com with SMTP id 5b1f17b1804b1-44059976a1fso19856195e9.1 for ; Tue, 22 Apr 2025 07:08:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1745330881; x=1745935681; 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=5F6zoPHO0Vf2l1Fq062YutUIptN7EZL0klrG+bfSeIk=; b=GaAcWWVU/y6/Olyb40TKcMn1qDtxycffKAgv3bGtIxlH1Isv6Amf02gFPn7pZ+bGGC w9zjhyaaXpBn3s5CD6JeTXj47KWCfx/Qmd4xuvuV3Rzc+AfXOwez8QR5KnW65Tv50kFr wkjmrvgUZpgnJQyA3Nf73kal1X+KLbsc9DrQ6ltMntwzU5brufa/ga8S20a1wa6LNB5M P4Z3Gx+hYxszn+F0/BRwaSj928PjTMrqZ2YCsWT243lDTxYGlCcpHQdu9Lsiy6kJ5uzz +8ciQzirpKf9+51DheYsM/ML0VVMbjN7FE3C8i/DQAcSI64gVlAjUckXeixEQJ9Ura3Q o6xg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1745330881; x=1745935681; 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=5F6zoPHO0Vf2l1Fq062YutUIptN7EZL0klrG+bfSeIk=; b=OT7lXPKEHryR2bYf8MBZovjn4tLGFkpQQbbCRikMYHw4IpraT7yC5gDFhhZmRBm5SZ anlrwd+zMZnP7B+DM5q843xvaP7eRegynihPQu92na66VXuw1lPvmYX6mB3swxxznTVH kiqEvi7eLgVswNHSqRfG/TFkZYkWhU3aEsny4qIex4pXcj8XgYw7ZvfTrUvDiAu9Kebu XAgQezx97JE8QqYUgxYIYO1GGMej3HyAbRuJoXHaiTHjFLtx/1nDVMqkMuW8jpCbjN7C CrMk8T34JyMlSnmOdgEmEpuHtAE9ziWJNTc05hPHXA47H6dL7Lx/fG1luYnnyf4VyeQi ShLQ== X-Forwarded-Encrypted: i=1; AJvYcCVrCy0a821+Rb5SNUU+O1gpBBQyCADGI6+gjEYhsG+E9hR4aS08pq1M8+peMuAq7sxZ6DVmJGFiu1GXGafHgA==@vger.kernel.org X-Gm-Message-State: AOJu0YyCiUGvbLxJzZic1OSBqSIccWS90jvz8NEOUsx6tWS10D2cQR6v xzr6GLJKmJ8pqPVc52WU1PZAMysOiOfEzS8OSDrHOG+luI+urVkAtVC8/aV92RgnBrtsPei4oYX qxN86BLP66GZaHg== X-Google-Smtp-Source: AGHT+IFLX4DHr+/6tE1KLHYgawIPobjKGhOVsKXL6zG3Ab9H7Y3EWJMVI94rYDp55qKE6hSpaEWOyAkgUfcYthE= X-Received: from wmqe6.prod.google.com ([2002:a05:600c:4e46:b0:43c:f5f7:f76a]) (user=aliceryhl job=prod-delivery.src-stubby-dispatcher) by 2002:a05:600c:4706:b0:43c:e9f7:d6a3 with SMTP id 5b1f17b1804b1-4406ab97d53mr134849535e9.13.1745330881618; Tue, 22 Apr 2025 07:08:01 -0700 (PDT) Date: Tue, 22 Apr 2025 14:07:59 +0000 In-Reply-To: <20250324-list-no-offset-v1-5-afd2b7fc442a@gmail.com> Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20250324-list-no-offset-v1-0-afd2b7fc442a@gmail.com> <20250324-list-no-offset-v1-5-afd2b7fc442a@gmail.com> Message-ID: Subject: Re: [PATCH 5/5] rust: list: remove OFFSET constants From: Alice Ryhl To: Tamir Duberstein Cc: Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , "=?utf-8?B?QmrDtnJu?= Roy Baron" , Benno Lossin , Andreas Hindborg , Trevor Gross , Danilo Krummrich , Bjorn Helgaas , Greg Kroah-Hartman , "Rafael J. Wysocki" , rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org Content-Type: text/plain; charset="utf-8" On Mon, Mar 24, 2025 at 05:33:47PM -0400, Tamir Duberstein wrote: > Replace `ListLinksSelfPtr::LIST_LINKS_SELF_PTR_OFFSET` with `unsafe fn > raw_get_self_ptr` which returns a pointer to the field rather than > requiring the caller to do pointer arithmetic. > > Implement `HasListLinks::raw_get_list_links` in `impl_has_list_links!`, > narrowing the interface of `HasListLinks` and replacing pointer > arithmetic with `container_of!`. > > Modify `impl_list_item` to also invoke `impl_has_list_links!` or > `impl_has_list_links_self_ptr!`. This is necessary to allow > `impl_list_item` to see more of the tokens used by > `impl_has_list_links{,_self_ptr}!`. > > A similar API change was discussed on the hrtimer series[1]. > > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1] > Signed-off-by: Tamir Duberstein > --- > rust/kernel/list.rs | 18 +++--- > rust/kernel/list/impl_list_item_mod.rs | 100 +++++++++++++++------------------ > 2 files changed, 56 insertions(+), 62 deletions(-) > > diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs > index a335c3b1ff5e..f370a8c1df98 100644 > --- a/rust/kernel/list.rs > +++ b/rust/kernel/list.rs > @@ -212,9 +212,6 @@ unsafe impl Send for ListLinksSelfPtr {} > unsafe impl Sync for ListLinksSelfPtr {} > > impl ListLinksSelfPtr { > - /// The offset from the [`ListLinks`] to the self pointer field. > - pub const LIST_LINKS_SELF_PTR_OFFSET: usize = core::mem::offset_of!(Self, self_ptr); > - > /// Creates a new initializer for this type. > pub fn new() -> impl PinInit { > // INVARIANT: Pin-init initializers can't be used on an existing `Arc`, so this value will > @@ -229,6 +226,16 @@ pub fn new() -> impl PinInit { > self_ptr: Opaque::uninit(), > } > } > + > + /// Returns a pointer to the self pointer. > + /// > + /// # Safety > + /// > + /// The provided pointer must point at a valid struct of type `Self`. > + pub unsafe fn raw_get_self_ptr(me: *mut Self) -> *const Opaque<*const T> { > + // SAFETY: The caller promises that the pointer is valid. > + unsafe { ptr::addr_of!((*me).self_ptr) } > + } > } > > impl, const ID: u64> List { > @@ -603,14 +610,11 @@ fn next(&mut self) -> Option> { > /// } > /// } > /// > -/// kernel::list::impl_has_list_links! { > -/// impl HasListLinks<0> for ListItem { self.links } > -/// } > /// kernel::list::impl_list_arc_safe! { > /// impl ListArcSafe<0> for ListItem { untracked; } > /// } > /// kernel::list::impl_list_item! { > -/// impl ListItem<0> for ListItem { using ListLinks; } > +/// impl ListItem<0> for ListItem { using ListLinks { self.links }; } > /// } > /// > /// // Use a cursor to remove the first element with the given value. > diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs > index 705b46150b97..4f9100aadbce 100644 > --- a/rust/kernel/list/impl_list_item_mod.rs > +++ b/rust/kernel/list/impl_list_item_mod.rs > @@ -6,21 +6,18 @@ > > use crate::list::ListLinks; > > -/// Declares that this type has a `ListLinks` field at a fixed offset. > +/// Declares that this type has a [`ListLinks`] field. > /// > -/// This trait is only used to help implement `ListItem` safely. If `ListItem` is implemented > +/// This trait is only used to help implement [`ListItem`] safely. If [`ListItem`] is implemented > /// manually, then this trait is not needed. Use the [`impl_has_list_links!`] macro to implement > /// this trait. > /// > /// # Safety > /// > -/// All values of this type must have a `ListLinks` field at the given offset. > +/// The methods on this trait must have exactly the behavior that the definitions given below have. > /// > -/// The behavior of `raw_get_list_links` must not be changed. > +/// [`ListItem`]: crate::list::ListItem > pub unsafe trait HasListLinks { > - /// The offset of the `ListLinks` field. > - const OFFSET: usize; > - > /// Returns a pointer to the [`ListLinks`] field. > /// > /// # Safety > @@ -28,14 +25,7 @@ pub unsafe trait HasListLinks { > /// The provided pointer must point at a valid struct of type `Self`. > /// > /// [`ListLinks`]: ListLinks > - // We don't really need this method, but it's necessary for the implementation of > - // `impl_has_list_links!` to be correct. > - #[inline] > - unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut ListLinks { > - // SAFETY: The caller promises that the pointer is valid. The implementer promises that the > - // `OFFSET` constant is correct. > - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut ListLinks } > - } > + unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut ListLinks; > } > > /// Implements the [`HasListLinks`] trait for the given type. > @@ -48,14 +38,11 @@ macro_rules! impl_has_list_links { > )*) => {$( > // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has the > // right type. > - // > - // The behavior of `raw_get_list_links` is not changed since the `addr_of_mut!` macro is > - // equivalent to the pointer offset operation in the trait definition. > unsafe impl$(<$($generics),*>)? $crate::list::HasListLinks$(<$id>)? for $self { > - const OFFSET: usize = ::core::mem::offset_of!(Self, $($field).*) as usize; > - > #[inline] > unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$id>)? { > + const _: usize = ::core::mem::offset_of!($self, $($field).*); > + > // SAFETY: The caller promises that the pointer is not dangling. We know that this > // expression doesn't follow any pointers, as the `offset_of!` invocation above > // would otherwise not compile. > @@ -66,12 +53,15 @@ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$ > } > pub use impl_has_list_links; > > -/// Declares that the `ListLinks` field in this struct is inside a `ListLinksSelfPtr`. > +/// Declares that the [`ListLinks`] field in this struct is inside a > +/// [`ListLinksSelfPtr`]. > /// > /// # Safety > /// > -/// The `ListLinks` field of this struct at the offset `HasListLinks::OFFSET` must be > -/// inside a `ListLinksSelfPtr`. > +/// The [`ListLinks`] field of this struct at [`HasListLinks::raw_get_list_links`] must be > +/// inside a [`ListLinksSelfPtr`]. > +/// > +/// [`ListLinksSelfPtr`]: crate::list::ListLinksSelfPtr > pub unsafe trait HasSelfPtr > where > Self: HasListLinks, > @@ -91,8 +81,6 @@ macro_rules! impl_has_list_links_self_ptr { > unsafe impl$(<$($generics)*>)? $crate::list::HasSelfPtr<$item_type $(, $id)?> for $self {} > > unsafe impl$(<$($generics)*>)? $crate::list::HasListLinks$(<$id>)? for $self { > - const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize; > - > #[inline] > unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$id>)? { > // SAFETY: The caller promises that the pointer is not dangling. > @@ -115,9 +103,13 @@ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$ > macro_rules! impl_list_item { > ( > $(impl$({$($generics:tt)*})? ListItem<$num:tt> for $self:ty { > - using ListLinks; > + using ListLinks { self$(.$field:ident)* }; > })* > ) => {$( > + $crate::list::impl_has_list_links! { > + impl$({$($generics:tt)*})? HasListLinks<$num> for $self { self$(.$field)* } > + } > + > // SAFETY: See GUARANTEES comment on each method. > unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $self { > // GUARANTEES: > @@ -133,20 +125,19 @@ unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> { > } > > // GUARANTEES: > - // * `me` originates from the most recent call to `prepare_to_insert`, which just added > - // `offset` to the pointer passed to `prepare_to_insert`. This method subtracts > - // `offset` from `me` so it returns the pointer originally passed to > - // `prepare_to_insert`. > + // * `me` originates from the most recent call to `prepare_to_insert`, which calls > + // `raw_get_list_link`, which is implemented using `addr_of_mut!((*self).$field)`. > + // This method uses `container_of` to perform the inverse operation, so it returns the > + // pointer originally passed to `prepare_to_insert`. > // * The pointer remains valid until the next call to `post_remove` because the caller > // of the most recent call to `prepare_to_insert` promised to retain ownership of the > // `ListArc` containing `Self` until the next call to `post_remove`. The value cannot > // be destroyed while a `ListArc` reference exists. > unsafe fn view_value(me: *mut $crate::list::ListLinks<$num>) -> *const Self { > - let offset = >::OFFSET; > // SAFETY: `me` originates from the most recent call to `prepare_to_insert`, so it > - // points at the field at offset `offset` in a value of type `Self`. Thus, > - // subtracting `offset` from `me` is still in-bounds of the allocation. > - unsafe { (me as *const u8).sub(offset) as *const Self } > + // points at the field `$field` in a value of type `Self`. Thus, reversing that > + // operation is still in-bounds of the allocation. > + $crate::container_of!(me, Self, $($field)*) > } > > // GUARANTEES: > @@ -163,25 +154,28 @@ unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$nu > } > > // GUARANTEES: > - // * `me` originates from the most recent call to `prepare_to_insert`, which just added > - // `offset` to the pointer passed to `prepare_to_insert`. This method subtracts > - // `offset` from `me` so it returns the pointer originally passed to > - // `prepare_to_insert`. > + // * `me` originates from the most recent call to `prepare_to_insert`, which calls > + // `raw_get_list_link`, which is implemented using `addr_of_mut!((*self).$field)`. > + // This method uses `container_of` to perform the inverse operation, so it returns the > + // pointer originally passed to `prepare_to_insert`. > unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self { > - let offset = >::OFFSET; > // SAFETY: `me` originates from the most recent call to `prepare_to_insert`, so it > - // points at the field at offset `offset` in a value of type `Self`. Thus, > - // subtracting `offset` from `me` is still in-bounds of the allocation. > - unsafe { (me as *const u8).sub(offset) as *const Self } > + // points at the field `$field` in a value of type `Self`. Thus, reversing that > + // operation is still in-bounds of the allocation. > + $crate::container_of!(me, Self, $($field)*) > } > } > )*}; > > ( > $(impl$({$($generics:tt)*})? ListItem<$num:tt> for $self:ty { > - using ListLinksSelfPtr; > + using ListLinksSelfPtr { self$(.$field:ident)* }; > })* > ) => {$( > + $crate::list::impl_has_list_links_self_ptr! { > + impl$({$($generics:tt)*})? HasListLinks<$num> for $self { self$(.$field)* } > + } > + > // SAFETY: See GUARANTEES comment on each method. > unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $self { > // GUARANTEES: > @@ -196,13 +190,10 @@ unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$nu > // SAFETY: The caller promises that `me` points at a valid value of type `Self`. > let links_field = unsafe { >::view_links(me) }; > > - let spoff = $crate::list::ListLinksSelfPtr::::LIST_LINKS_SELF_PTR_OFFSET; > - // Goes via the offset as the field is private. > - // > - // SAFETY: The constant is equal to `offset_of!(ListLinksSelfPtr, self_ptr)`, so > - // the pointer stays in bounds of the allocation. > - let self_ptr = unsafe { (links_field as *const u8).add(spoff) } > - as *const $crate::types::Opaque<*const Self>; > + // SAFETY: By the same reasoning above, `links_field` is a valid pointer. > + let self_ptr = unsafe { > + $crate::list::ListLinksSelfPtr::::raw_get_self_ptr(links_field) > + }; > let cell_inner = $crate::types::Opaque::raw_get(self_ptr); > > // SAFETY: This value is not accessed in any other places than `prepare_to_insert`, > @@ -241,11 +232,10 @@ unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> { > // `ListArc` containing `Self` until the next call to `post_remove`. The value cannot > // be destroyed while a `ListArc` reference exists. > unsafe fn view_value(links_field: *mut $crate::list::ListLinks<$num>) -> *const Self { > - let spoff = $crate::list::ListLinksSelfPtr::::LIST_LINKS_SELF_PTR_OFFSET; > - // SAFETY: The constant is equal to `offset_of!(ListLinksSelfPtr, self_ptr)`, so > - // the pointer stays in bounds of the allocation. > - let self_ptr = unsafe { (links_field as *const u8).add(spoff) } > - as *const ::core::cell::UnsafeCell<*const Self>; > + // SAFETY: By the same reasoning above, `links_field` is a valid pointer. > + let self_ptr = unsafe { > + $crate::list::ListLinksSelfPtr::::raw_get_self_ptr(links_field) > + }; > let cell_inner = ::core::cell::UnsafeCell::raw_get(self_ptr); I ran this with Rust Binder. After adjusting the macro invocations, I got this error: error[E0308]: mismatched types --> /proc/self/cwd/common/drivers/android/binder/rust_binder.rs:178:1 | 178 | / kernel::list::impl_list_item! { 179 | | impl ListItem<0> for DTRWrap { 180 | | using ListLinksSelfPtr { self.links }; 181 | | } 182 | | } | | ^ | | | | |_expected `*mut ListLinksSelfPtr>`, found `*mut ListLinks` | arguments to this function are incorrect | = note: expected raw pointer `*mut ListLinksSelfPtr>` found raw pointer `*mut ListLinks` note: associated function defined here --> /proc/self/cwd/common/rust/kernel/list.rs:235:19 = note: this error originates in the macro `kernel::list::impl_list_item` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0308]: mismatched types --> /proc/self/cwd/common/drivers/android/binder/rust_binder.rs:178:1 | 178 | / kernel::list::impl_list_item! { 179 | | impl ListItem<0> for DTRWrap { 180 | | using ListLinksSelfPtr { self.links }; 181 | | } 182 | | } | | ^ | | | | |_expected `*const UnsafeCell<_>`, found `*const Opaque<*const DTRWrap<...>>` | arguments to this function are incorrect | = note: expected raw pointer `*const UnsafeCell<_>` found raw pointer `*const Opaque<*const DTRWrap<(dyn DeliverToRead + 'static)>>` note: associated function defined here --> /proc/self/cwd/prebuilts/rust/linux-x86/1.82.0/lib/rustlib/src/rust/library/core/src/cell.rs:2210:18 = note: this error originates in the macro `kernel::list::impl_list_item` (in Nightly builds, run with -Z macro-backtrace for more info) The relevant code is: #[pin_data] struct DTRWrap { #[pin] links: ListLinksSelfPtr>, #[pin] wrapped: T, } kernel::list::impl_list_arc_safe! { impl{T: ListArcSafe + ?Sized} ListArcSafe<0> for DTRWrap { tracked_by wrapped: T; } } kernel::list::impl_list_item! { impl ListItem<0> for DTRWrap { using ListLinksSelfPtr { self.links }; } } Alice