Add the actual linked list itself.
The linked list uses the following design: The List type itself just has
a single pointer to the first element of the list. And the actual list
items then form a cycle. So the last item is `first->prev`.
This is slightly different from the usual kernel linked list. Matching
that exactly would amount to giving List two pointers, and having it be
part of the cycle of items. This alternate design has the advantage that
the cycle is never completely empty, which can reduce the number of
branches in some cases. However, it also has the disadvantage that List
must be pinned, which this design is trying to avoid.
Having the list items form a cycle rather than having null pointers at
the beginning/end is convenient for several reasons. For one, it lets us
store only one pointer in List, and it simplifies the implementation of
several functions.
Unfortunately, the `remove` function that removes an arbitrary element
from the list has to be unsafe. This is needed because there is no way
to handle the case where you pass an element from the wrong list. For
example, if it is the first element of some other list, then that other
list's `first` pointer would not be updated. Similarly, it could be a
data race if you try to remove it from two different lists in parallel.
(There's no problem with passing `remove` an item that's not in any
list. Additionally, other removal methods such as `pop_front` need not
be unsafe, as they can't be used to remove items from another list.)
A future patch in this series will introduce support for cursors that
can be used to remove arbitrary items without unsafe code.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/list.rs | 331 +++++++++++++++++++++++++++++++++++++++++++++++-
rust/kernel/list/arc.rs | 6 +-
2 files changed, 332 insertions(+), 5 deletions(-)
diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index 8fb7d2c13580..d3f8e38d9ff4 100644
--- a/rust/kernel/list.rs
+++ b/rust/kernel/list.rs
@@ -6,6 +6,7 @@
use crate::init::PinInit;
use crate::types::Opaque;
+use core::marker::PhantomData;
use core::ptr;
mod impl_list_item_mod;
@@ -16,7 +17,42 @@
impl_list_arc_safe, AtomicListArcTracker, ListArc, ListArcSafe, TryNewListArc,
};
-/// Implemented by types where a [`ListArc<Self>`] can be inserted into a `List`.
+/// A linked list.
+///
+/// All elements in this linked list will be [`ListArc`] references to the value. Since a value can
+/// only have one `ListArc` (for each pair of prev/next pointers), this ensures that the same
+/// prev/next pointers are not used for several linked lists.
+///
+/// # Invariants
+///
+/// * If the list is empty, then `first` is null. Otherwise, `first` points at the `ListLinks`
+/// field of the first element in the list.
+/// * All prev/next pointers in `ListLinks` fields of items in the list are valid and form a cycle.
+/// * For every item in the list, the list owns the associated [`ListArc`] reference and has
+/// exclusive access to the `ListLinks` field.
+pub struct List<T: ?Sized + ListItem<ID>, const ID: u64 = 0> {
+ first: *mut ListLinksFields,
+ _ty: PhantomData<ListArc<T, ID>>,
+}
+
+// SAFETY: This is a container of `ListArc<T, ID>`, and access to the container allows the same
+// type of access to the `ListArc<T, ID>` elements.
+unsafe impl<T, const ID: u64> Send for List<T, ID>
+where
+ ListArc<T, ID>: Send,
+ T: ?Sized + ListItem<ID>,
+{
+}
+// SAFETY: This is a container of `ListArc<T, ID>`, and access to the container allows the same
+// type of access to the `ListArc<T, ID>` elements.
+unsafe impl<T, const ID: u64> Sync for List<T, ID>
+where
+ ListArc<T, ID>: Sync,
+ T: ?Sized + ListItem<ID>,
+{
+}
+
+/// Implemented by types where a [`ListArc<Self>`] can be inserted into a [`List`].
///
/// # Safety
///
@@ -58,7 +94,7 @@ pub unsafe trait ListItem<const ID: u64 = 0>: ListArcSafe<ID> {
/// been called.
unsafe fn view_value(me: *mut ListLinks<ID>) -> *const Self;
- /// This is called when an item is inserted into a `List`.
+ /// This is called when an item is inserted into a [`List`].
///
/// # Guarantees
///
@@ -103,7 +139,6 @@ struct ListLinksFields {
/// The fields are null if and only if this item is not in a list.
#[repr(transparent)]
pub struct ListLinks<const ID: u64 = 0> {
- #[allow(dead_code)]
inner: Opaque<ListLinksFields>,
}
@@ -125,4 +160,294 @@ pub fn new() -> impl PinInit<Self> {
}),
}
}
+
+ /// # Safety
+ ///
+ /// `me` must be dereferencable.
+ #[inline]
+ unsafe fn fields(me: *mut Self) -> *mut ListLinksFields {
+ // SAFETY: The caller promises that the pointer is valid.
+ unsafe { Opaque::raw_get(ptr::addr_of!((*me).inner)) }
+ }
+
+ /// # Safety
+ ///
+ /// `me` must be dereferencable.
+ #[inline]
+ unsafe fn from_fields(me: *mut ListLinksFields) -> *mut Self {
+ me.cast()
+ }
+}
+
+impl<T: ?Sized + ListItem<ID>, const ID: u64> List<T, ID> {
+ /// Creates a new empty list.
+ pub const fn new() -> Self {
+ Self {
+ first: ptr::null_mut(),
+ _ty: PhantomData,
+ }
+ }
+
+ /// Returns whether this list is empty.
+ pub fn is_empty(&self) -> bool {
+ self.first.is_null()
+ }
+
+ /// Add the provided item to the back of the list.
+ pub fn push_back(&mut self, item: ListArc<T, ID>) {
+ let raw_item = ListArc::into_raw(item);
+ // SAFETY:
+ // * We just got `raw_item` from a `ListArc`, so it's in an `Arc`.
+ // * If this requirement is violated, then the previous caller of `prepare_to_insert`
+ // violated the safety requirement that they can't give up ownership of the `ListArc`
+ // until they call `post_remove`.
+ // * We own the `ListArc`.
+ // * Removing items from this list is always done using `remove_internal_inner`, which
+ // calls `post_remove` before giving up ownership.
+ let list_links = unsafe { T::prepare_to_insert(raw_item) };
+ // SAFETY: We have not yet called `post_remove`, so `list_links` is still valid.
+ let item = unsafe { ListLinks::fields(list_links) };
+
+ if self.first.is_null() {
+ self.first = item;
+ // SAFETY: The caller just gave us ownership of these fields.
+ // INVARIANT: A linked list with one item should be cyclic.
+ unsafe {
+ (*item).next = item;
+ (*item).prev = item;
+ }
+ } else {
+ let next = self.first;
+ // SAFETY: By the type invariant, this pointer is valid or null. We just checked that
+ // it's not null, so it must be valid.
+ let prev = unsafe { (*next).prev };
+ // SAFETY: Pointers in a linked list are never dangling, and the caller just gave us
+ // ownership of the fields on `item`.
+ // INVARIANT: This correctly inserts `item` between `prev` and `next`.
+ unsafe {
+ (*item).next = next;
+ (*item).prev = prev;
+ (*prev).next = item;
+ (*next).prev = item;
+ }
+ }
+ }
+
+ /// Add the provided item to the front of the list.
+ pub fn push_front(&mut self, item: ListArc<T, ID>) {
+ let raw_item = ListArc::into_raw(item);
+ // SAFETY:
+ // * We just got `raw_item` from a `ListArc`, so it's in an `Arc`.
+ // * If this requirement is violated, then the previous caller of `prepare_to_insert`
+ // violated the safety requirement that they can't give up ownership of the `ListArc`
+ // until they call `post_remove`.
+ // * We own the `ListArc`.
+ // * Removing items] from this list is always done using `remove_internal_inner`, which
+ // calls `post_remove` before giving up ownership.
+ let list_links = unsafe { T::prepare_to_insert(raw_item) };
+ // SAFETY: We have not yet called `post_remove`, so `list_links` is still valid.
+ let item = unsafe { ListLinks::fields(list_links) };
+
+ if self.first.is_null() {
+ // SAFETY: The caller just gave us ownership of these fields.
+ // INVARIANT: A linked list with one item should be cyclic.
+ unsafe {
+ (*item).next = item;
+ (*item).prev = item;
+ }
+ } else {
+ let next = self.first;
+ // SAFETY: We just checked that `next` is non-null.
+ let prev = unsafe { (*next).prev };
+ // SAFETY: Pointers in a linked list are never dangling, and the caller just gave us
+ // ownership of the fields on `item`.
+ // INVARIANT: This correctly inserts `item` between `prev` and `next`.
+ unsafe {
+ (*item).next = next;
+ (*item).prev = prev;
+ (*prev).next = item;
+ (*next).prev = item;
+ }
+ }
+ self.first = item;
+ }
+
+ /// Removes the last item from this list.
+ pub fn pop_back(&mut self) -> Option<ListArc<T, ID>> {
+ if self.first.is_null() {
+ return None;
+ }
+
+ // SAFETY: We just checked that the list is not empty.
+ let last = unsafe { (*self.first).prev };
+ // SAFETY: The last item of this list is in this list.
+ Some(unsafe { self.remove_internal(last) })
+ }
+
+ /// Removes the first item from this list.
+ pub fn pop_front(&mut self) -> Option<ListArc<T, ID>> {
+ if self.first.is_null() {
+ return None;
+ }
+
+ // SAFETY: The first item of this list is in this list.
+ Some(unsafe { self.remove_internal(self.first) })
+ }
+
+ /// Removes the provided item from this list and returns it.
+ ///
+ /// This returns `None` if the item is not in the list. (Note that by the safety requirements,
+ /// this means that the item is not in any list.)
+ ///
+ /// # Safety
+ ///
+ /// `item` must not be in a different linked list (with the same id).
+ pub unsafe fn remove(&mut self, item: &T) -> Option<ListArc<T, ID>> {
+ let mut item = unsafe { ListLinks::fields(T::view_links(item)) };
+ // SAFETY: The user provided a reference, and reference are never dangling.
+ //
+ // As for why this is not a data race, there are two cases:
+ //
+ // * If `item` is not in any list, then these fields are read-only and null.
+ // * If `item` is in this list, then we have exclusive access to these fields since we
+ // have a mutable reference to the list.
+ //
+ // In either case, there's no race.
+ let ListLinksFields { next, prev } = unsafe { *item };
+
+ debug_assert_eq!(next.is_null(), prev.is_null());
+ if !next.is_null() {
+ // This is really a no-op, but this ensures that `item` is a raw pointer that was
+ // obtained without going through a pointer->reference->pointer conversion rountrip.
+ // This ensures that the list is valid under the more restrictive strict provenance
+ // ruleset.
+ //
+ // SAFETY: We just checked that `next` is not null, and it's not dangling by the
+ // list invariants.
+ unsafe {
+ debug_assert_eq!(item, (*next).prev);
+ item = (*next).prev;
+ }
+
+ // SAFETY: We just checked that `item` is in a list, so the caller guarantees that it
+ // is in this list. The pointers are in the right order.
+ Some(unsafe { self.remove_internal_inner(item, next, prev) })
+ } else {
+ None
+ }
+ }
+
+ /// Removes the provided item from the list.
+ ///
+ /// # Safety
+ ///
+ /// `item` must point at an item in this list.
+ unsafe fn remove_internal(&mut self, item: *mut ListLinksFields) -> ListArc<T, ID> {
+ // SAFETY: The caller promises that this pointer is not dangling, and there's no data race
+ // since we have a mutable reference to the list containing `item`.
+ let ListLinksFields { next, prev } = unsafe { *item };
+ // SAFETY: The pointers are ok and in the right order.
+ unsafe { self.remove_internal_inner(item, next, prev) }
+ }
+
+ /// Removes the provided item from the list.
+ ///
+ /// # Safety
+ ///
+ /// The `item` pointer must point at an item in this list, and we must have `(*item).next ==
+ /// next` and `(*item).prev == prev`.
+ unsafe fn remove_internal_inner(
+ &mut self,
+ item: *mut ListLinksFields,
+ next: *mut ListLinksFields,
+ prev: *mut ListLinksFields,
+ ) -> ListArc<T, ID> {
+ // SAFETY: We have exclusive access to the pointers of items in the list, and the prev/next
+ // pointers are always valid for items in a list.
+ //
+ // INVARIANT: There are three cases:
+ // * If the list has at least three items, then after removing the item, `prev` and `next`
+ // will be next to each other.
+ // * If the list has two items, then the remaining item will point at itself.
+ // * If the list has one item, then `next == prev == item`, so these writes have no
+ // effect. The list remains unchanged and `item` is still in the list for now.
+ unsafe {
+ (*next).prev = prev;
+ (*prev).next = next;
+ }
+ // SAFETY: We have exclusive access to items in the list.
+ // INVARIANT: `item` is being removed, so the pointers should be null.
+ unsafe {
+ (*item).prev = ptr::null_mut();
+ (*item).next = ptr::null_mut();
+ }
+ // INVARIANT: There are three cases:
+ // * If `item` was not the first item, then `self.first` should remain unchanged.
+ // * If `item` was the first item and there is another item, then we just updated
+ // `prev->next` to `next`, which is the new first item, and setting `item->next` to null
+ // did not modify `prev->next`.
+ // * If `item` was the only item in the list, then `prev == item`, and we just set
+ // `item->next` to null, so this correctly sets `first` to null now that the list is
+ // empty.
+ if self.first == item {
+ // SAFETY: The `prev` pointer is the value that `item->prev` had when it was in this
+ // list, so it must be valid. There is no race since `prev` is still in the list and we
+ // still have exclusive access to the list.
+ self.first = unsafe { (*prev).next };
+ }
+
+ // SAFETY: `item` used to be in the list, so it is dereferencable by the type invariants
+ // of `List`.
+ let list_links = unsafe { ListLinks::from_fields(item) };
+ // SAFETY: Any pointer in the list originates from a `prepare_to_insert` call.
+ let raw_item = unsafe { T::post_remove(list_links) };
+ // SAFETY: The above call to `post_remove` guarantees that we can recreate the `ListArc`.
+ unsafe { ListArc::from_raw(raw_item) }
+ }
+
+ /// Moves all items from `other` into `self`.
+ ///
+ /// The items of `other` are added to the back of `self`, so the last item of `other` becomes
+ /// the last item of `self`.
+ pub fn push_all_back(&mut self, other: &mut List<T, ID>) {
+ // First, we insert the elements into `self`. At the end, we make `other` empty.
+ if self.is_empty() {
+ // INVARIANT: All of the elements in `other` become elements of `self`.
+ self.first = other.first;
+ } else if !other.is_empty() {
+ let other_first = other.first;
+ // SAFETY: The other list is not empty, so this pointer is valid.
+ let other_last = unsafe { (*other_first).prev };
+ let self_first = self.first;
+ // SAFETY: The self list is not empty, so this pointer is valid.
+ let self_last = unsafe { (*self_first).prev };
+
+ // SAFETY: We have exclusive access to both lists, so we can update the pointers.
+ // INVARIANT: This correctly sets the pointers to merge both lists. We do not need to
+ // update `self.first` because the first element of `self` does not change.
+ unsafe {
+ (*self_first).prev = other_last;
+ (*other_last).next = self_first;
+ (*self_last).next = other_first;
+ (*other_first).prev = self_last;
+ }
+ }
+
+ // INVARIANT: The other list is now empty, so update its pointer.
+ other.first = ptr::null_mut();
+ }
+}
+
+impl<T: ?Sized + ListItem<ID>, const ID: u64> Default for List<T, ID> {
+ fn default() -> Self {
+ List::new()
+ }
+}
+
+impl<T: ?Sized + ListItem<ID>, const ID: u64> Drop for List<T, ID> {
+ fn drop(&mut self) {
+ while let Some(item) = self.pop_front() {
+ drop(item);
+ }
+ }
}
diff --git a/rust/kernel/list/arc.rs b/rust/kernel/list/arc.rs
index 97bd9d52b5cf..a29bd26e49cf 100644
--- a/rust/kernel/list/arc.rs
+++ b/rust/kernel/list/arc.rs
@@ -124,8 +124,8 @@ fn try_new_list_arc(&self) -> bool {
/// The `ListArc` type can be thought of as a special reference to a refcounted object that owns the
/// permission to manipulate the `next`/`prev` pointers stored in the refcounted object. By ensuring
/// that each object has only one `ListArc` reference, the owner of that reference is assured
-/// exclusive access to the `next`/`prev` pointers. When a `ListArc` is inserted into a `List`, the
-/// `List` takes ownership of the `ListArc` reference.
+/// exclusive access to the `next`/`prev` pointers. When a `ListArc` is inserted into a [`List`],
+/// the [`List`] takes ownership of the `ListArc` reference.
///
/// There are various strategies to ensuring that a value has only one `ListArc` reference. The
/// simplest is to convert a [`UniqueArc`] into a `ListArc`. However, the refcounted object could
@@ -144,6 +144,8 @@ fn try_new_list_arc(&self) -> bool {
///
/// * Each reference counted object has at most one `ListArc` for each value of `ID`.
/// * The tracking inside `T` is aware that a `ListArc` reference exists.
+///
+/// [`List`]: crate::list::List
#[repr(transparent)]
pub struct ListArc<T, const ID: u64 = 0>
where
--
2.45.2.1089.g2a221341d9-googOn 23.07.24 10:22, Alice Ryhl wrote:
> + /// Add the provided item to the back of the list.
> + pub fn push_back(&mut self, item: ListArc<T, ID>) {
> + let raw_item = ListArc::into_raw(item);
> + // SAFETY:
> + // * We just got `raw_item` from a `ListArc`, so it's in an `Arc`.
> + // * If this requirement is violated, then the previous caller of `prepare_to_insert`
> + // violated the safety requirement that they can't give up ownership of the `ListArc`
> + // until they call `post_remove`.
I don't like this negative phrasing, what about "Since we have ownership
of the `ListArc`, `post_remove` must have been called after each
previous call to `prepare_to_insert`."?
> + // * We own the `ListArc`.
> + // * Removing items from this list is always done using `remove_internal_inner`, which
> + // calls `post_remove` before giving up ownership.
> + let list_links = unsafe { T::prepare_to_insert(raw_item) };
> + // SAFETY: We have not yet called `post_remove`, so `list_links` is still valid.
> + let item = unsafe { ListLinks::fields(list_links) };
> +
> + if self.first.is_null() {
> + self.first = item;
> + // SAFETY: The caller just gave us ownership of these fields.
> + // INVARIANT: A linked list with one item should be cyclic.
> + unsafe {
> + (*item).next = item;
> + (*item).prev = item;
> + }
> + } else {
> + let next = self.first;
> + // SAFETY: By the type invariant, this pointer is valid or null. We just checked that
> + // it's not null, so it must be valid.
> + let prev = unsafe { (*next).prev };
> + // SAFETY: Pointers in a linked list are never dangling, and the caller just gave us
> + // ownership of the fields on `item`.
> + // INVARIANT: This correctly inserts `item` between `prev` and `next`.
> + unsafe {
> + (*item).next = next;
> + (*item).prev = prev;
> + (*prev).next = item;
> + (*next).prev = item;
> + }
You have this pattern several times, maybe make a function for this?
> + }
> + }
> +
> + /// Add the provided item to the front of the list.
> + pub fn push_front(&mut self, item: ListArc<T, ID>) {
> + let raw_item = ListArc::into_raw(item);
> + // SAFETY:
> + // * We just got `raw_item` from a `ListArc`, so it's in an `Arc`.
> + // * If this requirement is violated, then the previous caller of `prepare_to_insert`
> + // violated the safety requirement that they can't give up ownership of the `ListArc`
> + // until they call `post_remove`.
> + // * We own the `ListArc`.
> + // * Removing items] from this list is always done using `remove_internal_inner`, which
Typo: "]".
> + // calls `post_remove` before giving up ownership.
> + let list_links = unsafe { T::prepare_to_insert(raw_item) };
> + // SAFETY: We have not yet called `post_remove`, so `list_links` is still valid.
> + let item = unsafe { ListLinks::fields(list_links) };
> +
> + if self.first.is_null() {
> + // SAFETY: The caller just gave us ownership of these fields.
> + // INVARIANT: A linked list with one item should be cyclic.
> + unsafe {
> + (*item).next = item;
> + (*item).prev = item;
> + }
> + } else {
> + let next = self.first;
> + // SAFETY: We just checked that `next` is non-null.
> + let prev = unsafe { (*next).prev };
> + // SAFETY: Pointers in a linked list are never dangling, and the caller just gave us
> + // ownership of the fields on `item`.
> + // INVARIANT: This correctly inserts `item` between `prev` and `next`.
> + unsafe {
> + (*item).next = next;
> + (*item).prev = prev;
> + (*prev).next = item;
> + (*next).prev = item;
> + }
> + }
> + self.first = item;
> + }
> +
> + /// Removes the last item from this list.
> + pub fn pop_back(&mut self) -> Option<ListArc<T, ID>> {
> + if self.first.is_null() {
> + return None;
> + }
> +
> + // SAFETY: We just checked that the list is not empty.
Additionally you need the type invariant that pointers in linked lists
are valid... This is a bit annoying, maybe in the future, we can have a
`ValidPtr` type that we could use here instead to avoid these
comments...
> + let last = unsafe { (*self.first).prev };
> + // SAFETY: The last item of this list is in this list.
> + Some(unsafe { self.remove_internal(last) })
> + }
> +
> + /// Removes the first item from this list.
> + pub fn pop_front(&mut self) -> Option<ListArc<T, ID>> {
> + if self.first.is_null() {
> + return None;
> + }
> +
> + // SAFETY: The first item of this list is in this list.
> + Some(unsafe { self.remove_internal(self.first) })
> + }
> +
> + /// Removes the provided item from this list and returns it.
> + ///
> + /// This returns `None` if the item is not in the list. (Note that by the safety requirements,
> + /// this means that the item is not in any list.)
> + ///
> + /// # Safety
> + ///
> + /// `item` must not be in a different linked list (with the same id).
> + pub unsafe fn remove(&mut self, item: &T) -> Option<ListArc<T, ID>> {
> + let mut item = unsafe { ListLinks::fields(T::view_links(item)) };
> + // SAFETY: The user provided a reference, and reference are never dangling.
> + //
> + // As for why this is not a data race, there are two cases:
> + //
> + // * If `item` is not in any list, then these fields are read-only and null.
> + // * If `item` is in this list, then we have exclusive access to these fields since we
> + // have a mutable reference to the list.
> + //
> + // In either case, there's no race.
> + let ListLinksFields { next, prev } = unsafe { *item };
> +
> + debug_assert_eq!(next.is_null(), prev.is_null());
> + if !next.is_null() {
> + // This is really a no-op, but this ensures that `item` is a raw pointer that was
> + // obtained without going through a pointer->reference->pointer conversion rountrip.
> + // This ensures that the list is valid under the more restrictive strict provenance
> + // ruleset.
> + //
> + // SAFETY: We just checked that `next` is not null, and it's not dangling by the
> + // list invariants.
> + unsafe {
> + debug_assert_eq!(item, (*next).prev);
> + item = (*next).prev;
> + }
How bad do you reckon is this for performance?
---
Cheers,
Benno
> +
> + // SAFETY: We just checked that `item` is in a list, so the caller guarantees that it
> + // is in this list. The pointers are in the right order.
> + Some(unsafe { self.remove_internal_inner(item, next, prev) })
> + } else {
> + None
> + }
> + }
On Thu, Aug 1, 2024 at 11:11 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 23.07.24 10:22, Alice Ryhl wrote:
> > + /// Add the provided item to the back of the list.
> > + pub fn push_back(&mut self, item: ListArc<T, ID>) {
> > + let raw_item = ListArc::into_raw(item);
> > + // SAFETY:
> > + // * We just got `raw_item` from a `ListArc`, so it's in an `Arc`.
> > + // * If this requirement is violated, then the previous caller of `prepare_to_insert`
> > + // violated the safety requirement that they can't give up ownership of the `ListArc`
> > + // until they call `post_remove`.
>
> I don't like this negative phrasing, what about "Since we have ownership
> of the `ListArc`, `post_remove` must have been called after each
> previous call to `prepare_to_insert`."?
I think we just need to argue about the most recent call to
prepare_to_insert but ok.
> > + // * We own the `ListArc`.
> > + // * Removing items from this list is always done using `remove_internal_inner`, which
> > + // calls `post_remove` before giving up ownership.
> > + let list_links = unsafe { T::prepare_to_insert(raw_item) };
> > + // SAFETY: We have not yet called `post_remove`, so `list_links` is still valid.
> > + let item = unsafe { ListLinks::fields(list_links) };
> > +
> > + if self.first.is_null() {
> > + self.first = item;
> > + // SAFETY: The caller just gave us ownership of these fields.
> > + // INVARIANT: A linked list with one item should be cyclic.
> > + unsafe {
> > + (*item).next = item;
> > + (*item).prev = item;
> > + }
> > + } else {
> > + let next = self.first;
> > + // SAFETY: By the type invariant, this pointer is valid or null. We just checked that
> > + // it's not null, so it must be valid.
> > + let prev = unsafe { (*next).prev };
> > + // SAFETY: Pointers in a linked list are never dangling, and the caller just gave us
> > + // ownership of the fields on `item`.
> > + // INVARIANT: This correctly inserts `item` between `prev` and `next`.
> > + unsafe {
> > + (*item).next = next;
> > + (*item).prev = prev;
> > + (*prev).next = item;
> > + (*next).prev = item;
> > + }
>
> You have this pattern several times, maybe make a function for this?
It's just two times. I think it's fine.
> > + if !next.is_null() {
> > + // This is really a no-op, but this ensures that `item` is a raw pointer that was
> > + // obtained without going through a pointer->reference->pointer conversion rountrip.
> > + // This ensures that the list is valid under the more restrictive strict provenance
> > + // ruleset.
> > + //
> > + // SAFETY: We just checked that `next` is not null, and it's not dangling by the
> > + // list invariants.
> > + unsafe {
> > + debug_assert_eq!(item, (*next).prev);
> > + item = (*next).prev;
> > + }
>
> How bad do you reckon is this for performance?
I don't think it's a problem at all.
Alice
On 01.08.24 11:40, Alice Ryhl wrote:
> On Thu, Aug 1, 2024 at 11:11 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On 23.07.24 10:22, Alice Ryhl wrote:
>>> + /// Add the provided item to the back of the list.
>>> + pub fn push_back(&mut self, item: ListArc<T, ID>) {
>>> + let raw_item = ListArc::into_raw(item);
>>> + // SAFETY:
>>> + // * We just got `raw_item` from a `ListArc`, so it's in an `Arc`.
>>> + // * If this requirement is violated, then the previous caller of `prepare_to_insert`
>>> + // violated the safety requirement that they can't give up ownership of the `ListArc`
>>> + // until they call `post_remove`.
>>
>> I don't like this negative phrasing, what about "Since we have ownership
>> of the `ListArc`, `post_remove` must have been called after each
>> previous call to `prepare_to_insert`."?
>
> I think we just need to argue about the most recent call to
> prepare_to_insert but ok.
I would argue that's exactly what my version does. Maybe "Since we have
ownership of the `ListArc`, the most recent call to `prepare_to_insert`
must have had a matching `post_remove` call afterwards."
But I liked the above version more.
>>> + // * We own the `ListArc`.
>>> + // * Removing items from this list is always done using `remove_internal_inner`, which
>>> + // calls `post_remove` before giving up ownership.
>>> + let list_links = unsafe { T::prepare_to_insert(raw_item) };
>>> + // SAFETY: We have not yet called `post_remove`, so `list_links` is still valid.
>>> + let item = unsafe { ListLinks::fields(list_links) };
>>> +
>>> + if self.first.is_null() {
>>> + self.first = item;
>>> + // SAFETY: The caller just gave us ownership of these fields.
>>> + // INVARIANT: A linked list with one item should be cyclic.
>>> + unsafe {
>>> + (*item).next = item;
>>> + (*item).prev = item;
>>> + }
>>> + } else {
>>> + let next = self.first;
>>> + // SAFETY: By the type invariant, this pointer is valid or null. We just checked that
>>> + // it's not null, so it must be valid.
>>> + let prev = unsafe { (*next).prev };
>>> + // SAFETY: Pointers in a linked list are never dangling, and the caller just gave us
>>> + // ownership of the fields on `item`.
>>> + // INVARIANT: This correctly inserts `item` between `prev` and `next`.
>>> + unsafe {
>>> + (*item).next = next;
>>> + (*item).prev = prev;
>>> + (*prev).next = item;
>>> + (*next).prev = item;
>>> + }
>>
>> You have this pattern several times, maybe make a function for this?
>
> It's just two times. I think it's fine.
Sure, it seemed more in my mind.
>>> + if !next.is_null() {
>>> + // This is really a no-op, but this ensures that `item` is a raw pointer that was
>>> + // obtained without going through a pointer->reference->pointer conversion rountrip.
>>> + // This ensures that the list is valid under the more restrictive strict provenance
>>> + // ruleset.
>>> + //
>>> + // SAFETY: We just checked that `next` is not null, and it's not dangling by the
>>> + // list invariants.
>>> + unsafe {
>>> + debug_assert_eq!(item, (*next).prev);
>>> + item = (*next).prev;
>>> + }
>>
>> How bad do you reckon is this for performance?
>
> I don't think it's a problem at all.
Sounds good.
---
Cheers,
Benno
© 2016 - 2024 Red Hat, Inc.