rbtree v4 #1081

matthewtgilbride · 2024-05-16T14:17:22Z

Responding to Benno's comments here: https://lore.kernel.org/all/20240418-b4-rbtree-v3-0-323e134390ce@google.com/

This patchset contains the red-black tree abstractions needed by the Rust implementation of the Binder driver. Binder driver benefits from O(log n) search/insertion/deletion of key/value mappings in various places, including `process.rs` and `range_alloc.rs`. In `range_alloc.rs`, the ability to store and search by a generic key type is also useful. Please see the Rust Binder RFC for usage examples [1]. Note that the `container_of` macro is currently used only by `rbtree` itself. Users of "rust: rbtree: add red-black tree implementation backed by the C version" [PATCH RFC 03/20] rust_binder: add threading support [PATCH RFC 05/20] rust_binder: add nodes and context managers [PATCH RFC 06/20] rust_binder: add oneway transactions Users of "rust: rbtree: add `RBTreeIterator`" [PATCH RFC 17/20] rust_binder: add oneway spam detection Users of "rust: rbtree: add `RBTreeIteratorMut`" [PATCH RFC 06/20] rust_binder: add oneway transactions Users of "rust: rbtree: add `RBTreeCursor`" [PATCH RFC 06/20] rust_binder: add oneway transactions Users of "rust: rbtree: add RBTree::entry" Not used in the original RFC, but introduced after further code review. See: https://r.android.com/2849906 The Rust Binder RFC addresses the upstream deprecation of red-black tree. Quoted here for convenience: "This RFC uses the kernel's red-black tree for key/value mappings, but we are aware that the red-black tree is deprecated. We did this to make the performance comparison more fair, since C binder also uses rbtree for this. We intend to replace these with XArrays instead. That said, we don't think that XArray is a good fit for the range allocator, and we propose to continue using the red-black tree for the range allocator." Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-0-08ba9197f637@google.com/ [1] To: Miguel Ojeda <ojeda@kernel.org> To: Alex Gaynor <alex.gaynor@gmail.com> To: Wedson Almeida Filho <wedsonaf@gmail.com> To: Boqun Feng <boqun.feng@gmail.com> To: Gary Guo <gary@garyguo.net> To: Björn Roy Baron <bjorn3_gh@protonmail.com> To: Benno Lossin <benno.lossin@proton.me> To: Andreas Hindborg <a.hindborg@samsung.com> To: Alice Ryhl <aliceryhl@google.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> To: Arve Hjønnevåg <arve@android.com> To: Todd Kjos <tkjos@android.com> To: Martijn Coenen <maco@android.com> To: Joel Fernandes <joel@joelfernandes.org> To: Christian Brauner <christian@brauner.io> To: Carlos Llamas <cmllamas@google.com> To: Suren Baghdasaryan <surenb@google.com> Cc: Rob Landley <rob@landley.net> Cc: Davidlohr Bueso <dave@stgolabs.net> Cc: Michel Lespinasse <michel@lespinasse.org> Cc: rust-for-linux@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Matt Gilbride <mattgilbride@google.com> --- Changes in v4: - EDITME: describe what is new in this series revision. - EDITME: use bulletpoints and terse descriptions. - Link to v3: https://lore.kernel.org/r/20240418-b4-rbtree-v3-0-323e134390ce@google.com Changes in v3: - Address various feedback re: SAFETY and INVARIANT comments from v2. - Update variable naming and add detailed comments for the `RBTree::insert` (later moved to `RBTree::raw_entry`) implementation. - Link to v2: https://lore.kernel.org/r/20240219-b4-rbtree-v2-0-0b113aab330d@google.com Changes in v2: - Update documentation link to the C header file - Use `core::convert::Infallible` in try_reserve_node - Link to v1: https://lore.kernel.org/r/20240205-b4-rbtree-v1-0-995e3eee38c0@google.com --- b4-submit-tracking --- # This section is used internally by b4 prep for tracking purposes. { "series": { "revision": 4, "change-id": "20231205-b4-rbtree-abb1a016f0a0", "prefixes": [], "history": { "v1": [ "20240205-b4-rbtree-v1-0-995e3eee38c0@google.com" ], "v2": [ "20240219-b4-rbtree-v2-0-0b113aab330d@google.com" ], "v3": [ "20240418-b4-rbtree-v3-0-323e134390ce@google.com" ] } } }

Sometimes (see [1]) it is necessary to drop the value inside of a `Box<T>`, but retain the allocation. For example to reuse the allocation in the future. Introduce a new function `drop_contents` that turns a `Box<T>` into `Box<MaybeUninit<T>>` by dropping the value. Signed-off-by: Benno Lossin <benno.lossin@proton.me> Link: https://lore.kernel.org/rust-for-linux/20240418-b4-rbtree-v3-5-323e134390ce@google.com/ [1]

The rust rbtree exposes a map-like interface over keys and values, backed by the kernel red-black tree implementation. Values can be inserted, deleted, and retrieved from a `RBTree` by key. This base abstraction is used by binder to store key/value pairs and perform lookups, for example the patch "[PATCH RFC 03/20] rust_binder: add threading support" in the binder RFC [1]. Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-3-08ba9197f637@google.com/ [1] Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> Tested-by: Alice Ryhl <aliceryhl@google.com> Signed-off-by: Matt Gilbride <mattgilbride@google.com>

- Add Iterator implementation (`RBTreeIterator`) for `RBTree`, allowing iteration over (key, value) pairs in key order. - Add individual `keys()` and `values()` functions to iterate over keys or values alone. - Update doctests to use iteration instead of explicitly getting items. Iteration is needed by the binder driver to enumerate all values in a tree for oneway spam detection [1]. Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-17-08ba9197f637@google.com/ [1] Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> Tested-by: Alice Ryhl <aliceryhl@google.com> Signed-off-by: Matt Gilbride <mattgilbride@google.com>

Add mutable Iterator implementation (`RBTreeIteratorMut`) for `RBTree`, allowing iteration over (key, value) pairs in key order. Only values are mutable, as mutating keys implies modifying a node's position in the tree. Mutable iteration is used by the binder driver during shutdown to clean up the tree maintained by the "range allocator" [1]. Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-6-08ba9197f637@google.com/ [1] Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> Signed-off-by: Matt Gilbride <mattgilbride@google.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> Tested-by: Alice Ryhl <aliceryhl@google.com>

Add a cursor interface to `RBTree`, supporting the following use cases: - Inspect the current node pointed to by the cursor, inspect/move to it's neighbors in sort order (bidirectionally). - Mutate the tree itself by removing the current node pointed to by the cursor, or one of its neighbors. Add functions to obtain a cursor to the tree by key: - The node with the smallest key - The node with the largest key - The node matching the given key, or the one with the next larger key The cursor abstraction is needed by the binder driver to efficiently search for nodes and (conditionally) modify them, as well as their neighbors [1]. Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-6-08ba9197f637@google.com/ [1] Co-developed-by: Alice Ryhl <aliceryhl@google.com> Signed-off-by: Alice Ryhl <aliceryhl@google.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> Tested-by: Alice Ryhl <aliceryhl@google.com> Signed-off-by: Matt Gilbride <mattgilbride@google.com>

This mirrors the entry API [1] from the Rust standard library on `RBTree`. This API can be used to access the entry at a specific key and make modifications depending on whether the key is vacant or occupied. This API is useful because it can often be used to avoid traversing the tree multiple times. This is used by binder to look up and conditionally access or insert a value, depending on whether it is there or not [2]. Link: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html [1] Link: https://android-review.googlesource.com/c/kernel/common/+/2849906 [2] Signed-off-by: Alice Ryhl <aliceryhl@google.com> Tested-by: Alice Ryhl <aliceryhl@google.com> Signed-off-by: Matt Gilbride <mattgilbride@google.com>

matthewtgilbride · 2024-05-16T14:18:41Z

rust/kernel/alloc/box_ext.rs

+    /// use kernel::alloc::flags;
+    ///
+    /// let value = Box::new([0; 32], flags::GFP_KERNEL);
+    /// let value = value.unwrap().drop_contents();


This change is ever-so-slightly different than Benno's - mainly just fixing the doctests (IIRC an unwrap() call was missing somewhere, possibly here)

matthewtgilbride · 2024-05-16T14:20:38Z

rust/kernel/rbtree.rs

+// [`RBTreeNode`] without synchronization.
+unsafe impl<K: Sync, V: Sync> Sync for RBTreeNode<K, V> {}
+
+struct Node<K, V> {


Personal preference: I prefer putting items that give a high-level
overview of the module to the top. I don't feel like I gain anything
from seeing the definition of the Node type this early.

Done.

matthewtgilbride · 2024-05-16T14:50:58Z

rust/kernel/rbtree.rs

+impl<K, V> RBTreeNodeReservation<K, V> {
+    /// Allocates memory for a node to be eventually initialised and inserted into the tree via a
+    /// call to [`RBTree::insert`].
+    pub fn new(flags: Flags) -> Result<RBTreeNodeReservation<K, V>> {


This function creates a RBTreeNodeReservation, I think it would make
sense to move it to that type and just name this function new.'

Done

matthewtgilbride · 2024-05-16T14:51:33Z

rust/kernel/rbtree.rs

+    /// call to [`RBTree::insert`].
+    pub fn new(flags: Flags) -> Result<RBTreeNodeReservation<K, V>> {
+        Ok(RBTreeNodeReservation {
+            node: Box::new_uninit(flags)?,


Box::new_uninit() probably makes more sense here. (what you did is not
wrong, but I think the intent is better captured by new_uninit)

Done.

matthewtgilbride · 2024-05-16T14:52:29Z

rust/kernel/rbtree.rs

+impl<K, V> RBTreeNode<K, V> {
+    /// Allocates and initialises a node that can be inserted into the tree via
+    /// [`RBTree::insert`].
+    pub fn new(key: K, value: V, flags: Flags) -> Result<RBTreeNode<K, V>> {


Same with this function, I would move it to RBTreeNode and call it
new.

Done

matthewtgilbride · 2024-05-16T15:18:43Z

rust/kernel/rbtree.rs

+
+/// A view into a single entry in a map, which may either be vacant or occupied.
+///
+/// This enum is constructed from the [`RBTree::entry`].


You could just write [RBTree::entry].

Done.

matthewtgilbride · 2024-05-16T15:19:48Z

rust/kernel/rbtree.rs

+
+        // INVARIANT: We are linking in a new node, which is valid. It remains valid because we
+        // "forgot" it with `Box::into_raw`.
+        // SAFETY: The type invariants of `RawVacantEntry` are exactly the safety requirements of `rb_link_node`.


I don't like the formulation "valid in an appropriate way", since if you
don't know what the appropriate way is, this doesn't help you.

Done. LMK WYT.

matthewtgilbride · 2024-05-16T15:20:29Z

rust/kernel/rbtree.rs

+/// A view into an occupied entry in a [`RBTree`]. It is part of the [`Entry`] enum.
+///
+/// # Invariants
+/// - `node_links` is a valid, non-null pointer to a tree node in `self.rbtree`


It should be the same tree as self.rbtree, right? (I see you calling
rb_replace_node below with the rbtree root used)

Done.

matthewtgilbride · 2024-05-16T15:21:40Z

rust/kernel/rbtree.rs

@@ -330,61 +336,52 @@ where
        // we find an empty subtree, we can insert the new node using `rb_link_node`.
        let mut parent = core::ptr::null_mut();


Nit: why are you moving this line below child_field_of_parent? Just an
artifact of rebasing?

I think so - I think what is currently there is the ordering we want.

matthewtgilbride · 2024-05-16T15:24:07Z

rust/kernel/rbtree.rs

+            // SAFETY: `node` is a non-null node so it is valid by the type invariants.
+            let left_child = unsafe { (*node).rb_left };
+            // SAFETY: `node` is a non-null node so it is valid by the type invariants.
+            let right_child = unsafe { (*node).rb_right };


Since you have this pattern multiple times, I think you could have a
single function that walks the tree and takes care of most of the
unsafe stuff. A good starting point might be this:a

unsafe fn walk<F, R>(node: *mut bindings::rb_node, dir: F) -> R where /* this, key */ F: FnMut(*mut bindings::rb_node, &K) -> Either<Direction, R>;

Acknowledged - I think the subsequent entry patch takes care of this duplication, so I'm not sure it's worth addressing here. Let me know if you disagree and/or I misunderstood.

yeah the entry patch removes the duplication. I have run a couple times into thinking that the duplication should be removed, only to later see the entry patch.
How do you feel about moving it more to the start of the patch series? Or is that too much rebasing work (it probably is a lot of work, so maybe keep it this way...)?

y86-dev

I went through the patches, I hope I found everything, but when you send it to the list, I might find some more things. let me know if/when I should take another look

y86-dev · 2024-05-28T13:33:06Z

rust/kernel/rbtree.rs

+
+impl<K, V> RBTree<K, V>
+where
+    K: Ord,


Sounds good, I just wanted to know if you thought about this potential issue.

y86-dev · 2024-05-28T13:44:31Z

rust/kernel/rbtree.rs

+/// # Invariants
+///
+/// Non-null parent/children pointers stored in instances of the `rb_node` C struct are always
+/// valid, and pointing to a field of our internal representation of a node.


I think it would be a good idea to explicitly name the type of the internal representation.

y86-dev · 2024-05-28T13:50:12Z

rust/kernel/rbtree.rs

+        // SAFETY: All pointers are non-null and valid (`*next_child` is null, but `next_child` is a
+        // mutable reference).


there is no next_child.

y86-dev · 2024-05-28T13:55:36Z

rust/kernel/rbtree.rs

+    pub fn iter(&self) -> Iter<'_, K, V> {
+        Iter {
+            _tree: PhantomData,
+            // INVARIANT:


the INVARIANT comment normally sits outside of the struct that you are creating. The SAFETY comment needs that self.root is valid and the invariant comment should only mention that rb_first returns a valid pointer. Additionally you should mention that that pointer points into a valid RBTree.

y86-dev · 2024-05-28T13:58:57Z

rust/kernel/rbtree.rs

@@ -436,21 +455,76 @@ unsafe impl<'a, K: Sync, V: Sync> Sync for Iter<'a, K, V> {}
 impl<'a, K, V> Iterator for Iter<'a, K, V> {
    type Item = (&'a K, &'a V);

+    fn next(&mut self) -> Option<Self::Item> {
+        self.iter_raw.next().map(|(k, v)|
+            // SAFETY: Due to `&mut self`, we have exclusive access to `k` and `v`, for the lifetime of `'a`.


This seems wrong, since the Iter only borrows the tree immutably.

y86-dev · 2024-05-28T14:02:48Z

rust/kernel/rbtree.rs

+            // SAFETY: `node` is a non-null node so it is valid by the type invariants.
+            let left_child = unsafe { (*node).rb_left };
+            // SAFETY: `node` is a non-null node so it is valid by the type invariants.
+            let right_child = unsafe { (*node).rb_right };


yeah the entry patch removes the duplication. I have run a couple times into thinking that the duplication should be removed, only to later see the entry patch.
How do you feel about moving it more to the start of the patch series? Or is that too much rebasing work (it probably is a lot of work, so maybe keep it this way...)?

y86-dev · 2024-05-28T14:08:12Z

rust/kernel/rbtree.rs

+/// - `current` points to a node that is in the same [`RBTree`] that `root` is pointing to.
+/// - A cursor must borrow the [`RBTree`] containing `root` and `current` mutably.
+pub struct RBTreeCursor<'a, K, V> {
+    _tree: PhantomData<&'a mut RBTree<K, V>>,


Since you are storing the root in the cursor, why not remove the PhantomData and literally have a &mut RBTree<K, V> field that points to the tree?

y86-dev · 2024-05-28T14:10:30Z

rust/kernel/rbtree.rs

+        self.mv(Direction::Next)
+    }
+
+    fn mv(self, direction: Direction) -> Option<Self> {


Ahhh I forgot about that... then keep mv (the workaround is ugly and shouldn't be used for this purpose [just to let you know, you can use r#move]) or you could also use move_to.

matthewtgilbride and others added 7 commits May 3, 2024 12:49

matthewtgilbride commented May 16, 2024

View reviewed changes

y86-dev reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbtree v4 #1081

rbtree v4 #1081

matthewtgilbride commented May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

matthewtgilbride May 16, 2024

y86-dev May 28, 2024

y86-dev left a comment

y86-dev May 28, 2024

y86-dev May 28, 2024

y86-dev May 28, 2024

y86-dev May 28, 2024

y86-dev May 28, 2024

y86-dev May 28, 2024

y86-dev May 28, 2024

y86-dev May 28, 2024

		@@ -330,61 +336,52 @@ where
		// we find an empty subtree, we can insert the new node using `rb_link_node`.
		let mut parent = core::ptr::null_mut();

		// SAFETY: All pointers are non-null and valid (`*next_child` is null, but `next_child` is a
		// mutable reference).

rbtree v4 #1081

Are you sure you want to change the base?

rbtree v4 #1081

Conversation

matthewtgilbride commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y86-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment