Re: [PATCH v4 1/2] rust: allocator: add unit tests of kmalloc, vmalloc and kvmalloc

[Hui Zhu <hui.zhu@linux.dev>]

Hi Danilo,

On Thu, Jul 24, 2025 at 05:59:39PM +0200, Danilo Krummrich wrote:
> On Thu Jul 24, 2025 at 11:25 AM CEST, Hui Zhu wrote:
> > From: Hui Zhu <zhuhui@kylinos.cn>
> >
> > Add KUnit test cases to validate the functionality of Rust allocation
> > wrappers (kmalloc, vmalloc, kvmalloc).
> >
> > The tests include:
> > Basic allocation tests for each allocator using a 1024-byte Blob
> > structure initialized with a 0xfe pattern.
> > Large alignment (> PAGE_SIZE) allocation testing using an 8192-byte
> > aligned LargeAlignBlob structure.
> > Verification of allocation constraints:
> > - kmalloc successfully handles large alignments.
> > - vmalloc and kvmalloc correctly fail for unsupported large alignments.
> > Content verification through byte-by-byte pattern checking.
> >
> > Co-developed-by: Geliang Tang <geliang@kernel.org>
> > Signed-off-by: Geliang Tang <geliang@kernel.org>
> > Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> 
> Thanks for the patch, additional test are always welcome! :)

Great!

> 
> > ---
> >  rust/kernel/alloc/allocator.rs | 57 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >
> > diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
> > index aa2dfa9dca4c..430d1f664fdf 100644
> > --- a/rust/kernel/alloc/allocator.rs
> > +++ b/rust/kernel/alloc/allocator.rs
> > @@ -187,3 +187,60 @@ unsafe fn realloc(
> >          unsafe { ReallocFunc::KVREALLOC.call(ptr, layout, old_layout, flags) }
> >      }
> >  }
> > +
> > +#[macros::kunit_tests(rust_allocator_kunit)]
> > +mod tests {
> > +    use super::*;
> > +    use kernel::prelude::*;
> > +
> > +    const TEST_SIZE: usize = 1024;
> > +    const LARGE_ALIGN_TEST_SIZE: usize = kernel::page::PAGE_SIZE * 4;
> > +    #[repr(align(128))]
> > +    struct Blob([u8; TEST_SIZE]);
> > +    // This structure is used to test the allocation of alignments larger
> > +    // than PAGE_SIZE.
> > +    // Since this is not yet supported, avoid accessing the contents of
> > +    // the structure for now.
> > +    #[allow(dead_code)]
> > +    #[repr(align(8192))]
> > +    struct LargeAlignBlob([u8; LARGE_ALIGN_TEST_SIZE]);
> > +
> > +    #[test]
> > +    fn test_kmalloc() -> Result<(), AllocError> {
> > +        let blob = KBox::new(Blob([0xfeu8; TEST_SIZE]), GFP_KERNEL)?;
> 
> Since those are now actual unit tests on the Allocator implementations, it would
> be fine to use them directly. However, for the case you are testing here, i.e.
> alignment using Box is perfectly fine.
> 
> Having that said, I wouldn't call those tests test_*malloc(), since they're not
> really testing all aspects of a certain allocator, but only the success to
> allocate with certain alignment arguments.
> 
> Instead, I propose to write just a single test, test_alignment(), with a few
> helper functions.
> 
> For instance, your Blob test structure could have a constructor that is generic
> over A: Allocator.
> 
> However, given that you really only want to check alignment, you probably want a
> structure like this instead.
> 
> 	struct TestAlign<A: Allocator>(Box<MaybeUninit<[u8; TEST_SIZE]>, A>);
> 
> 	impl<A: Allocator> TestAlign<A> {
> 	   fn new() -> Result<Self> {
> 	      Box::<_, A>::new_uninit(GFP_KERNEL)
> 	   }
> 
> 	   fn alignment_valid(&self) -> bool {
> 	      ...
> 	   }
> 	}
> 
> and then test_alignment() can just do
> 
> 	let test = TestAlign::<Kmalloc>::new()?;
> 	assert!(test.alignment_valid());
> 
> 	...
> 
> Given that this is now a unit test I also think that actually validating the
> alignment of the pointer Box wraps makes sense, similar to what you had in v2.
> 
> > +        for b in blob.0.as_slice().iter() {
> > +            assert_eq!(*b, 0xfeu8);
> > +        }
> 
> I don't think that this has any valid in the context of testing alignment.
> 
> > +        let blob = KBox::new(LargeAlignBlob([0xfdu8; LARGE_ALIGN_TEST_SIZE]), GFP_KERNEL)?;
> 
> For the large alignment case, you can consider to let TestAlign take a const
> generic, additional to A: Allocator, e.g.
> 
> 	struct TestAlign<A: Allocator, const SIZE: usize>(Box<MaybeUninit<[u8; SIZE]>, A>);
> 
> This way you can keep test_alignment() function very compact.
>

I sent new patches according to your comments.
 
> - Danilo

Best,
Hui