2024-05-06 09:56:46

by Alice Ryhl

[permalink] [raw]
Subject: [PATCH v2 8/9] rust: list: support heterogeneous lists

Support linked lists that can have many different structs at once. This
is generally done using trait objects. The main challenge is figuring
what the struct is given only a pointer to the ListLinks.

We do this by storing a pointer to the struct next to the ListLinks
field. The container_of operation will then just read that pointer. When
the type is a trait object, that pointer will be a fat pointer whose
metadata is a vtable that tells you what kind of struct it is.

Heterogeneous lists are heavily used by Rust Binder. There are a lot of
so-called todo lists containing various events that need to be delivered
to userspace next time userspace calls into the driver. And there are
quite a few different todo item types: incoming transaction, changes to
refcounts, death notifications, and more.

Signed-off-by: Alice Ryhl <[email protected]>
---
rust/kernel/list.rs | 47 ++++++++++++++-
rust/kernel/list/impl_list_item_mod.rs | 105 +++++++++++++++++++++++++++++++++
2 files changed, 151 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index 641b434e3841..3e687401c6d3 100644
--- a/rust/kernel/list.rs
+++ b/rust/kernel/list.rs
@@ -7,12 +7,16 @@
use crate::init::PinInit;
use crate::sync::ArcBorrow;
use crate::types::Opaque;
+use core::cell::UnsafeCell;
use core::iter::{DoubleEndedIterator, FusedIterator};
use core::marker::PhantomData;
+use core::mem::MaybeUninit;
use core::ptr;

mod impl_list_item_mod;
-pub use self::impl_list_item_mod::{impl_has_list_links, impl_list_item, HasListLinks};
+pub use self::impl_list_item_mod::{
+ impl_has_list_links, impl_has_list_links_self_ptr, impl_list_item, HasListLinks, HasSelfPtr,
+};

mod arc;
pub use self::arc::{
@@ -180,6 +184,47 @@ unsafe fn from_fields(me: *mut ListLinksFields) -> *mut Self {
}
}

+/// Similar to [`ListLinks`], but also contains a pointer to the full value.
+///
+/// This type can be used instead of [`ListLinks`] to support lists with trait objects.
+#[repr(C)]
+pub struct ListLinksSelfPtr<T: ?Sized, const ID: u64 = 0> {
+ /// The `ListLinks` field inside this value.
+ ///
+ /// This is public so that it can be used with `impl_has_list_links!`.
+ pub inner: ListLinks<ID>,
+ self_ptr: UnsafeCell<MaybeUninit<*const T>>,
+}
+
+// SAFETY: The fields of a ListLinksSelfPtr can be moved across thread boundaries.
+unsafe impl<T: ?Sized + Send, const ID: u64> Send for ListLinksSelfPtr<T, ID> {}
+// SAFETY: The type is opaque so immutable references to a ListLinksSelfPtr are useless. Therefore,
+// it's okay to have immutable access to a ListLinks from several threads at once.
+//
+// Note that `inner` being a public field does not prevent this type from being opaque, since
+// `inner` is a opaque type.
+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
+ // not be constructed in an `Arc` that already has a `ListArc`.
+ Self {
+ inner: ListLinks {
+ inner: Opaque::new(ListLinksFields {
+ prev: ptr::null_mut(),
+ next: ptr::null_mut(),
+ }),
+ },
+ self_ptr: UnsafeCell::new(MaybeUninit::zeroed()),
+ }
+ }
+}
+
impl<T: ?Sized + ListItem<ID>, const ID: u64> List<T, ID> {
/// Creates a new empty list.
pub const fn new() -> Self {
diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs
index 3ff483be89d1..96e90c0ec587 100644
--- a/rust/kernel/list/impl_list_item_mod.rs
+++ b/rust/kernel/list/impl_list_item_mod.rs
@@ -62,6 +62,49 @@ 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>`.
+///
+/// # Safety
+///
+/// The `ListLinks<ID>` field of this struct at the offset `HasListLinks<ID>::OFFSET` must be
+/// inside a `ListLinksSelfPtr<T, ID>`.
+pub unsafe trait HasSelfPtr<T: ?Sized, const ID: u64 = 0>
+where
+ Self: HasListLinks<ID>,
+{
+}
+
+/// Implements the [`HasListLinks`] and [`HasSelfPtr`] traits for the given type.
+#[macro_export]
+macro_rules! impl_has_list_links_self_ptr {
+ ($(impl$({$($implarg:tt)*})?
+ HasSelfPtr<$item_type:ty $(, $id:tt)?>
+ for $self:ident $(<$($selfarg:ty),*>)?
+ { self.$field:ident }
+ )*) => {$(
+ // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has the
+ // right type.
+ unsafe impl$(<$($implarg)*>)? $crate::list::HasSelfPtr<$item_type $(, $id)?> for
+ $self $(<$($selfarg),*>)?
+ {}
+
+ unsafe impl$(<$($implarg)*>)? $crate::list::HasListLinks$(<$id>)? for
+ $self $(<$($selfarg),*>)?
+ {
+ 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.
+ let ptr: *mut $crate::list::ListLinksSelfPtr<$item_type $(, $id)?> =
+ unsafe { ::core::ptr::addr_of_mut!((*ptr).$field) };
+ ptr.cast()
+ }
+ }
+ )*};
+}
+pub use impl_has_list_links_self_ptr;
+
/// Implements the [`ListItem`] trait for the given type.
///
/// Assumes that the type implements [`HasListLinks`].
@@ -95,5 +138,67 @@ unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
}
}
};
+
+ (
+ impl$({$($generics:tt)*})? ListItem<$num:tt> for $t:ty {
+ using ListLinksSelfPtr;
+ } $($rest:tt)*
+ ) => {
+ unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $t {
+ unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
+ // 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;
+ // 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);
+
+ // SAFETY: This value is not accessed in any other places than `prepare_to_insert`,
+ // `post_remove`, or `view_value`. By the safety requirements of those methods,
+ // none of these three methods may be called in parallel with this call to
+ // `prepare_to_insert`, so this write will not race with any other access to the
+ // value.
+ unsafe { ::core::ptr::write(cell_inner, me) };
+
+ links_field
+ }
+
+ unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
+ // SAFETY: The caller promises that `me` points at a valid value of type `Self`.
+ unsafe { <Self as HasListLinks<$num>>::raw_get_list_links(me.cast_mut()) }
+ }
+
+ // This function is also used as the implementation of `post_remove`, so the caller
+ // may choose to satisfy the safety requirements of `post_remove` instead of the safety
+ // requirements for `view_value`.
+ 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);
+ // This returns the same pointer as the one passes to the previous call to
+ // `prepare_to_insert` since that previous call wrote that pointer to this
+ // location, and the value has not been modified since.
+ //
+ // 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
+ // `post_remove`.
+ unsafe { ::core::ptr::read(cell_inner) }
+ }
+
+ unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
+ // SAFETY: This specific implementation of `view_value` allows the caller to
+ // promise the safety requirements of `post_remove` instead of the safety
+ // requirements for `view_value`.
+ unsafe { <Self as $crate::list::ListItem<$num>>::view_value(me) }
+ }
+ }
+ };
}
pub use impl_list_item;

--
2.45.0.rc1.225.g2a3ae87e7f-goog



2024-05-27 10:46:26

by Benno Lossin

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] rust: list: support heterogeneous lists

On 06.05.24 11:53, Alice Ryhl wrote:
> Support linked lists that can have many different structs at once. This

have -> hold

> is generally done using trait objects. The main challenge is figuring
> what the struct is given only a pointer to the ListLinks.
>
> We do this by storing a pointer to the struct next to the ListLinks
> field. The container_of operation will then just read that pointer. When
> the type is a trait object, that pointer will be a fat pointer whose
> metadata is a vtable that tells you what kind of struct it is.
>
> Heterogeneous lists are heavily used by Rust Binder. There are a lot of
> so-called todo lists containing various events that need to be delivered
> to userspace next time userspace calls into the driver. And there are
> quite a few different todo item types: incoming transaction, changes to
> refcounts, death notifications, and more.
>
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> rust/kernel/list.rs | 47 ++++++++++++++-
> rust/kernel/list/impl_list_item_mod.rs | 105 +++++++++++++++++++++++++++++++++
> 2 files changed, 151 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
> index 641b434e3841..3e687401c6d3 100644
> --- a/rust/kernel/list.rs
> +++ b/rust/kernel/list.rs
> @@ -7,12 +7,16 @@
> use crate::init::PinInit;
> use crate::sync::ArcBorrow;
> use crate::types::Opaque;
> +use core::cell::UnsafeCell;
> use core::iter::{DoubleEndedIterator, FusedIterator};
> use core::marker::PhantomData;
> +use core::mem::MaybeUninit;
> use core::ptr;
>
> mod impl_list_item_mod;
> -pub use self::impl_list_item_mod::{impl_has_list_links, impl_list_item, HasListLinks};
> +pub use self::impl_list_item_mod::{
> + impl_has_list_links, impl_has_list_links_self_ptr, impl_list_item, HasListLinks, HasSelfPtr,
> +};
>
> mod arc;
> pub use self::arc::{
> @@ -180,6 +184,47 @@ unsafe fn from_fields(me: *mut ListLinksFields) -> *mut Self {
> }
> }
>
> +/// Similar to [`ListLinks`], but also contains a pointer to the full value.
> +///
> +/// This type can be used instead of [`ListLinks`] to support lists with trait objects.
> +#[repr(C)]
> +pub struct ListLinksSelfPtr<T: ?Sized, const ID: u64 = 0> {
> + /// The `ListLinks` field inside this value.
> + ///
> + /// This is public so that it can be used with `impl_has_list_links!`.
> + pub inner: ListLinks<ID>,
> + self_ptr: UnsafeCell<MaybeUninit<*const T>>,

Why don't you use `Opaque` instead?

> +}
> +
> +// SAFETY: The fields of a ListLinksSelfPtr can be moved across thread boundaries.
> +unsafe impl<T: ?Sized + Send, const ID: u64> Send for ListLinksSelfPtr<T, ID> {}
> +// SAFETY: The type is opaque so immutable references to a ListLinksSelfPtr are useless. Therefore,
> +// it's okay to have immutable access to a ListLinks from several threads at once.
> +//
> +// Note that `inner` being a public field does not prevent this type from being opaque, since
> +// `inner` is a opaque type.
> +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
> + // not be constructed in an `Arc` that already has a `ListArc`.
> + Self {
> + inner: ListLinks {
> + inner: Opaque::new(ListLinksFields {
> + prev: ptr::null_mut(),
> + next: ptr::null_mut(),
> + }),
> + },
> + self_ptr: UnsafeCell::new(MaybeUninit::zeroed()),
> + }
> + }
> +}
> +
> impl<T: ?Sized + ListItem<ID>, const ID: u64> List<T, ID> {
> /// Creates a new empty list.
> pub const fn new() -> Self {
> diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs
> index 3ff483be89d1..96e90c0ec587 100644
> --- a/rust/kernel/list/impl_list_item_mod.rs
> +++ b/rust/kernel/list/impl_list_item_mod.rs
> @@ -62,6 +62,49 @@ 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>`.
> +///
> +/// # Safety
> +///
> +/// The `ListLinks<ID>` field of this struct at the offset `HasListLinks<ID>::OFFSET` must be
> +/// inside a `ListLinksSelfPtr<T, ID>`.
> +pub unsafe trait HasSelfPtr<T: ?Sized, const ID: u64 = 0>
> +where
> + Self: HasListLinks<ID>,
> +{
> +}
> +
> +/// Implements the [`HasListLinks`] and [`HasSelfPtr`] traits for the given type.
> +#[macro_export]
> +macro_rules! impl_has_list_links_self_ptr {
> + ($(impl$({$($implarg:tt)*})?
> + HasSelfPtr<$item_type:ty $(, $id:tt)?>
> + for $self:ident $(<$($selfarg:ty),*>)?
> + { self.$field:ident }
> + )*) => {$(
> + // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has the
> + // right type.
> + unsafe impl$(<$($implarg)*>)? $crate::list::HasSelfPtr<$item_type $(, $id)?> for
> + $self $(<$($selfarg),*>)?
> + {}
> +
> + unsafe impl$(<$($implarg)*>)? $crate::list::HasListLinks$(<$id>)? for

Missing SAFETY comment.

> + $self $(<$($selfarg),*>)?
> + {
> + 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.
> + let ptr: *mut $crate::list::ListLinksSelfPtr<$item_type $(, $id)?> =
> + unsafe { ::core::ptr::addr_of_mut!((*ptr).$field) };
> + ptr.cast()
> + }
> + }
> + )*};
> +}
> +pub use impl_has_list_links_self_ptr;
> +
> /// Implements the [`ListItem`] trait for the given type.
> ///
> /// Assumes that the type implements [`HasListLinks`].
> @@ -95,5 +138,67 @@ unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
> }
> }
> };
> +
> + (
> + impl$({$($generics:tt)*})? ListItem<$num:tt> for $t:ty {
> + using ListLinksSelfPtr;
> + } $($rest:tt)*
> + ) => {
> + unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $t {

Missing SAFETY comment.

---
Cheers,
Benno

> + unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
> + // 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;
> + // 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);
> +
> + // SAFETY: This value is not accessed in any other places than `prepare_to_insert`,
> + // `post_remove`, or `view_value`. By the safety requirements of those methods,
> + // none of these three methods may be called in parallel with this call to
> + // `prepare_to_insert`, so this write will not race with any other access to the
> + // value.
> + unsafe { ::core::ptr::write(cell_inner, me) };
> +
> + links_field
> + }
> +
> + unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
> + // SAFETY: The caller promises that `me` points at a valid value of type `Self`.
> + unsafe { <Self as HasListLinks<$num>>::raw_get_list_links(me.cast_mut()) }
> + }
> +
> + // This function is also used as the implementation of `post_remove`, so the caller
> + // may choose to satisfy the safety requirements of `post_remove` instead of the safety
> + // requirements for `view_value`.
> + 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);
> + // This returns the same pointer as the one passes to the previous call to
> + // `prepare_to_insert` since that previous call wrote that pointer to this
> + // location, and the value has not been modified since.
> + //
> + // 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
> + // `post_remove`.
> + unsafe { ::core::ptr::read(cell_inner) }
> + }
> +
> + unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
> + // SAFETY: This specific implementation of `view_value` allows the caller to
> + // promise the safety requirements of `post_remove` instead of the safety
> + // requirements for `view_value`.
> + unsafe { <Self as $crate::list::ListItem<$num>>::view_value(me) }
> + }
> + }
> + };
> }
> pub use impl_list_item;
>
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>