Re: [PATCH v2 2/5] rust: maple_tree: add MapleTree

["Danilo Krummrich" <dakr@kernel.org>]

On Tue Aug 19, 2025 at 2:45 PM CEST, Alice Ryhl wrote:
> On Tue, Aug 19, 2025 at 01:30:30PM +0200, Danilo Krummrich wrote:
>> On Tue Aug 19, 2025 at 12:34 PM CEST, Alice Ryhl wrote:
>> > diff --git a/MAINTAINERS b/MAINTAINERS
>> > index fe168477caa45799dfe07de2f54de6d6a1ce0615..26053163fe5aed2fc4b4e39d47062c93b873ac13 100644
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -16250,7 +16250,9 @@ L:	rust-for-linux@vger.kernel.org
>> >  S:	Maintained
>> >  W:	http://www.linux-mm.org
>> >  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
>> > +F:	rust/helpers/maple_tree.c
>> >  F:	rust/helpers/mm.c
>> > +F:	rust/kernel/maple_tree.rs
>> >  F:	rust/kernel/mm.rs
>> >  F:	rust/kernel/mm/
>> 
>> A later patch adds a separate entry; is this intended?
>
> Ah, no, this isn't intended.
>
>> > +impl<T: ForeignOwnable> MapleTree<T> {
>> > +    /// Create a new maple tree.
>> > +    ///
>> > +    /// The tree will use the regular implementation with a higher branching factor.
>> 
>> What do you mean with "regular implementation" and what is "a higher branching
>> factor" in this context?
>> 
>> Do you mean that the maple tree has a higher branching factor than a regular RB
>> tree, or something else?
>
> This is compared to the alloc variant of the maple tree from the last
> patch in this series.

I think it'd be good to mention this. You could add the corresponding comment
and link when you introduce the type in the subsequent patch.

>> > +    #[inline]
>> > +    pub fn new() -> impl PinInit<Self> {
>> > +        pin_init!(MapleTree {
>> > +            // SAFETY: This initializes a maple tree into a pinned slot. The maple tree will be
>> > +            // destroyed in Drop before the memory location becomes invalid.
>> > +            tree <- Opaque::ffi_init(|slot| unsafe { bindings::mt_init_flags(slot, 0) }),
>> > +            _p: PhantomData,
>> > +        })
>> > +    }
>> > +
>> > +    /// Insert the value at the given index.
>> > +    ///
>> > +    /// If the maple tree already contains a range using the given index, then this call will fail.
>> 
>> Maybe add an error section for this?
>> 
>> > +    ///
>> > +    /// # Examples
>> > +    ///
>> > +    /// ```
>> > +    /// use kernel::maple_tree::{MapleTree, InsertErrorKind};
>> > +    ///
>> > +    /// let tree = KBox::pin_init(MapleTree::<KBox<i32>>::new(), GFP_KERNEL)?;
>> > +    ///
>> > +    /// let ten = KBox::new(10, GFP_KERNEL)?;
>> > +    /// let twenty = KBox::new(20, GFP_KERNEL)?;
>> > +    /// let the_answer = KBox::new(42, GFP_KERNEL)?;
>> > +    ///
>> > +    /// // These calls will succeed.
>> > +    /// tree.insert(100, ten, GFP_KERNEL)?;
>> > +    /// tree.insert(101, twenty, GFP_KERNEL)?;
>> > +    ///
>> > +    /// // This will fail because the index is already in use.
>> > +    /// assert_eq!(
>> > +    ///     tree.insert(100, the_answer, GFP_KERNEL).unwrap_err().cause,
>> 
>> A lot of the examples, including the ones in subsequent patches contain variants
>> of unwrap().
>> 
>> I think we should avoid this and instead handle errors gracefully -- even if it
>> bloats the examples a bit.
>> 
>> My concern is that it otherwise creates the impression that using unwrap() is a
>> reasonable thing to do.
>> 
>> Especially for people new to the kernel or Rust (or both) it might not be
>> obvious that unwrap() is equivalent to
>> 
>> 	if (!ret)
>> 		do_something();
>> 	else
>> 		panic();
>> 
>> or the fact that this is something we should only do as absolute last resort.
>
> How would you write it? The way you write it in normal code is an
> if/else where you handle both cases, but that doesn't map nicely.

I'd just

	assert!(tree.insert(100, the_answer, GFP_KERNEL).is_err());

and if you want to test that the error you'd expect is actually returned, I'd
suggest a regular kunit test, rather than a doc-test.

I think doc-tests should mostly illustrate idiomatic usage, especially now that
we have good and easily accessible kunit support.

I say "mostly" because I think tests to the degree of where they stay within
reasonable bounds of illustrating idiomatic usage are fine of course.