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]
Tested-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
rust/kernel/list.rs | 23 +++---
rust/kernel/list/impl_list_item_mod.rs | 143 +++++++++++++++------------------
2 files changed, 79 insertions(+), 87 deletions(-)
diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index fe58a3920e70..d976b31d1639 100644
--- a/rust/kernel/list.rs
+++ b/rust/kernel/list.rs
@@ -57,14 +57,11 @@
/// }
/// }
///
-/// impl_has_list_links! {
-/// impl HasListLinks<0> for BasicItem { self.links }
-/// }
/// impl_list_arc_safe! {
/// impl ListArcSafe<0> for BasicItem { untracked; }
/// }
/// impl_list_item! {
-/// impl ListItem<0> for BasicItem { using ListLinks; }
+/// impl ListItem<0> for BasicItem { using ListLinks { self.links }; }
/// }
///
/// // Create a new empty list.
@@ -320,9 +317,6 @@ unsafe impl<T: ?Sized + Send, const ID: u64> Send for ListLinksSelfPtr<T, ID> {}
unsafe impl<T: ?Sized + Sync, const ID: u64> Sync for ListLinksSelfPtr<T, ID> {}
impl<T: ?Sized, const ID: u64> ListLinksSelfPtr<T, ID> {
- /// 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<Self> {
// INVARIANT: Pin-init initializers can't be used on an existing `Arc`, so this value will
@@ -337,6 +331,16 @@ pub fn new() -> impl PinInit<Self> {
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: *const Self) -> *const Opaque<*const T> {
+ // SAFETY: The caller promises that the pointer is valid.
+ unsafe { ptr::addr_of!((*me).self_ptr) }
+ }
}
impl<T: ?Sized + ListItem<ID>, const ID: u64> List<T, ID> {
@@ -711,14 +715,11 @@ fn next(&mut self) -> Option<ArcBorrow<'a, T>> {
/// }
/// }
///
-/// 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 6b67c0ecc57b..289101e0ab5e 100644
--- a/rust/kernel/list/impl_list_item_mod.rs
+++ b/rust/kernel/list/impl_list_item_mod.rs
@@ -4,21 +4,18 @@
//! Helpers for implementing list traits safely.
-/// Declares that this type has a `ListLinks<ID>` field at a fixed offset.
+/// Declares that this type has a [`ListLinks<ID>`] 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<ID>` 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<const ID: u64 = 0> {
- /// The offset of the `ListLinks` field.
- const OFFSET: usize;
-
/// Returns a pointer to the [`ListLinks<T, ID>`] field.
///
/// # Safety
@@ -26,14 +23,7 @@ pub unsafe trait HasListLinks<const ID: u64 = 0> {
/// The provided pointer must point at a valid struct of type `Self`.
///
/// [`ListLinks<T, ID>`]: crate::list::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 crate::list::ListLinks<ID> {
- // SAFETY: The caller promises that the pointer is valid. The implementer promises that the
- // `OFFSET` constant is correct.
- unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast() }
- }
+ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut crate::list::ListLinks<ID>;
}
/// Implements the [`HasListLinks`] trait for the given type.
@@ -46,14 +36,15 @@ 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>)? {
+ // Statically ensure that `$(.field)*` doesn't follow any pointers.
+ //
+ // Cannot be `const` because `$self` may contain generics and E0401 says constants
+ // "can't use {`Self`,generic parameters} from outer item".
+ if false { let _: 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.
@@ -64,12 +55,15 @@ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$
}
pub use impl_has_list_links;
-/// Declares that the `ListLinks<ID>` field in this struct is inside a `ListLinksSelfPtr<T, ID>`.
+/// Declares that the [`ListLinks<ID>`] field in this struct is inside a
+/// [`ListLinksSelfPtr<T, ID>`].
///
/// # Safety
///
-/// The `ListLinks<ID>` field of this struct at the offset `HasListLinks<ID>::OFFSET` must be
-/// inside a `ListLinksSelfPtr<T, ID>`.
+/// The [`ListLinks<ID>`] field of this struct at [`HasListLinks<ID>::raw_get_list_links`] must be
+/// inside a [`ListLinksSelfPtr<T, ID>`].
+///
+/// [`ListLinksSelfPtr<T, ID>`]: crate::list::ListLinksSelfPtr
pub unsafe trait HasSelfPtr<T: ?Sized, const ID: u64 = 0>
where
Self: HasListLinks<ID>,
@@ -89,8 +83,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.
@@ -120,16 +112,12 @@ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$
/// links: kernel::list::ListLinks,
/// }
///
-/// kernel::list::impl_has_list_links! {
-/// impl HasListLinks<0> for SimpleListItem { self.links }
-/// }
-///
/// kernel::list::impl_list_arc_safe! {
/// impl ListArcSafe<0> for SimpleListItem { untracked; }
/// }
///
/// kernel::list::impl_list_item! {
-/// impl ListItem<0> for SimpleListItem { using ListLinks; }
+/// impl ListItem<0> for SimpleListItem { using ListLinks { self.links }; }
/// }
///
/// struct ListLinksHolder {
@@ -143,16 +131,12 @@ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$
/// links: ListLinksHolder,
/// }
///
-/// kernel::list::impl_has_list_links! {
-/// impl{T, U} HasListLinks<0> for ComplexListItem<T, U> { self.links.inner }
-/// }
-///
/// kernel::list::impl_list_arc_safe! {
/// impl{T, U} ListArcSafe<0> for ComplexListItem<T, U> { untracked; }
/// }
///
/// kernel::list::impl_list_item! {
-/// impl{T, U} ListItem<0> for ComplexListItem<T, U> { using ListLinks; }
+/// impl{T, U} ListItem<0> for ComplexListItem<T, U> { using ListLinks { self.links.inner }; }
/// }
/// ```
///
@@ -168,12 +152,8 @@ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$
/// impl ListArcSafe<0> for SimpleListItem { untracked; }
/// }
///
-/// kernel::list::impl_has_list_links_self_ptr! {
-/// impl HasSelfPtr<SimpleListItem> for SimpleListItem { self.links }
-/// }
-///
/// kernel::list::impl_list_item! {
-/// impl ListItem<0> for SimpleListItem { using ListLinksSelfPtr; }
+/// impl ListItem<0> for SimpleListItem { using ListLinksSelfPtr { self.links }; }
/// }
///
/// struct ListLinksSelfPtrHolder<T, U> {
@@ -191,21 +171,23 @@ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$
/// impl{T, U} ListArcSafe<0> for ComplexListItem<T, U> { untracked; }
/// }
///
-/// kernel::list::impl_has_list_links_self_ptr! {
-/// impl{T, U} HasSelfPtr<ComplexListItem<T, U>> for ComplexListItem<T, U> { self.links.inner }
-/// }
-///
/// kernel::list::impl_list_item! {
-/// impl{T, U} ListItem<0> for ComplexListItem<T, U> { using ListLinksSelfPtr; }
+/// impl{T, U} ListItem<0> for ComplexListItem<T, U> {
+/// using ListLinksSelfPtr { self.links.inner };
+/// }
/// }
/// ```
#[macro_export]
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)*})? HasListLinks<$num> for $self { self$(.$field)* }
+ }
+
// SAFETY: See GUARANTEES comment on each method.
unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $self {
// GUARANTEES:
@@ -221,20 +203,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 = <Self as $crate::list::HasListLinks<$num>>::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:
@@ -251,25 +232,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 = <Self as $crate::list::HasListLinks<$num>>::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)*})? HasSelfPtr<$self> for $self { self$(.$field)* }
+ }
+
// SAFETY: See GUARANTEES comment on each method.
unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $self {
// GUARANTEES:
@@ -284,13 +268,15 @@ 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 { <Self as $crate::list::ListItem<$num>>::view_links(me) };
- let spoff = $crate::list::ListLinksSelfPtr::<Self, $num>::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>;
+ let container = $crate::container_of!(
+ links_field, $crate::list::ListLinksSelfPtr<Self, $num>, inner
+ );
+
+ // SAFETY: By the same reasoning above, `links_field` is a valid pointer.
+ let self_ptr = unsafe {
+ $crate::list::ListLinksSelfPtr::raw_get_self_ptr(container)
+ };
+
let cell_inner = $crate::types::Opaque::raw_get(self_ptr);
// SAFETY: This value is not accessed in any other places than `prepare_to_insert`,
@@ -331,12 +317,17 @@ 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::<Self, $num>::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>;
- let cell_inner = ::core::cell::UnsafeCell::raw_get(self_ptr);
+ let container = $crate::container_of!(
+ links_field, $crate::list::ListLinksSelfPtr<Self, $num>, inner
+ );
+
+ // SAFETY: By the same reasoning above, `links_field` is a valid pointer.
+ let self_ptr = unsafe {
+ $crate::list::ListLinksSelfPtr::raw_get_self_ptr(container)
+ };
+
+ let cell_inner = $crate::types::Opaque::raw_get(self_ptr);
+
// SAFETY: This is not a data race, because the only function that writes to this
// value is `prepare_to_insert`, but by the safety requirements the
// `prepare_to_insert` method may not be called in parallel with `view_value` or
--
2.50.0On Wed, Jul 9, 2025 at 9:31 PM Tamir Duberstein <tamird@gmail.com> wrote: > > -/// Declares that this type has a `ListLinks<ID>` field at a fixed offset. > +/// Declares that this type has a [`ListLinks<ID>`] field. I was applying this series the other day, and I noticed these doc-related changes in the patch, which are appreciated (I think you did it to make it consistent with the other lines you were adding with intra-doc links), but I think in general it is better to clean those separately in a patch first. I am mentioning it because the docs do not build due to those -- please check the `rustdoc` target for patches, especially if it is a non-trivial change. I also did another change to make the examples (in the other patch) build with the minimum Rust version. It is good to test that too, since sometimes that can slip, especially as the window of versions grow. Anyway, the examples/series here caught another issue with a previous patch, so that is good news :) Thanks! Cheers, Miguel
On Sat, Jul 19, 2025 at 5:09 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Wed, Jul 9, 2025 at 9:31 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > -/// Declares that this type has a `ListLinks<ID>` field at a fixed offset. > > +/// Declares that this type has a [`ListLinks<ID>`] field. > > I was applying this series the other day, and I noticed these > doc-related changes in the patch, which are appreciated (I think you > did it to make it consistent with the other lines you were adding with > intra-doc links), but I think in general it is better to clean those > separately in a patch first. > > I am mentioning it because the docs do not build due to those -- > please check the `rustdoc` target for patches, especially if it is a > non-trivial change. > > I also did another change to make the examples (in the other patch) > build with the minimum Rust version. It is good to test that too, > since sometimes that can slip, especially as the window of versions > grow. > > Anyway, the examples/series here caught another issue with a previous > patch, so that is good news :) > > Thanks! > > Cheers, > Miguel Thanks Miguel, will do in the future! Tamir
© 2016 - 2025 Red Hat, Inc.