rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Danilo Krummrich" <dakr@kernel.org>
To: "Hui Zhu" <hui.zhu@linux.dev>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>, <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	<linux-kernel@vger.kernel.org>, <rust-for-linux@vger.kernel.org>,
	"Hui Zhu" <zhuhui@kylinos.cn>,
	"Geliang Tang" <geliang@kernel.org>,
	"Lorenzo Stoakes" <lorenzo.stoakes@oracle.com>,
	"Vlastimil Babka" <vbabka@suse.cz>,
	"Liam R. Howlett" <Liam.Howlett@oracle.com>,
	"Uladzislau Rezki" <urezki@gmail.com>
Subject: Re: [PATCH v2] rust: add a sample alloc usage
Date: Thu, 17 Jul 2025 13:19:05 +0200	[thread overview]
Message-ID: <DBEAFCFHFU35.1IZI3ZSJSIRAY@kernel.org> (raw)
In-Reply-To: <20250717095053.49239-1-hui.zhu@linux.dev>

(Cc: Lorenzo, Vlastimil, Liam, Uladzislau)

On Thu Jul 17, 2025 at 11:50 AM CEST, Hui Zhu wrote:
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index bd2faad63b4f..7c3e68d9ada5 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY)	+= rust_driver_auxiliary.o
>  obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
> +obj-$(CONFIG_SAMPLE_RUST_ALLOC)		+= rust_alloc.o

I think adding an example for large alignment is fine, but let's do this in a
doc-test on VBox in rust/kernel/alloc/kbox.rs. I think adding a separate module
for this is overkill.

Note that doc-tests are executed on boot if CONFIG_RUST_KERNEL_DOCTESTS=y.

> diff --git a/samples/rust/rust_alloc.rs b/samples/rust/rust_alloc.rs
> new file mode 100644
> index 000000000000..61efde37b5f2
> --- /dev/null
> +++ b/samples/rust/rust_alloc.rs
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (c) 2025, Kylin Software
> +
> +//! Rust alloc sample.
> +
> +use kernel::bindings;
> +use kernel::prelude::*;
> +
> +module! {
> +    type: RustAlloc,
> +    name: "rust_alloc",
> +    authors: ["Rust for Linux Contributors"],
> +    description: "Rust alloc sample",
> +    license: "GPL",
> +}
> +
> +const VBOX_SIZE: usize = 1024;
> +const VBOX_LARGE_ALIGN: usize = bindings::PAGE_SIZE * 4;

Please use kernel::page::PAGE_SIZE instead.

> +const KVEC_VAL: [usize; 3] = [10, 20, 30];
> +
> +#[repr(align(128))]
> +struct VboxBlob([u8; VBOX_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 VboxLargeAlignBlob([u8; VBOX_LARGE_ALIGN]);
> +
> +struct RustAlloc {
> +    vbox_blob: VBox<VboxBlob>,
> +    kvec_blob: KVec<usize>,
> +}
> +
> +fn check_align(addr: usize, align: usize) -> bool {
> +    debug_assert!(align.is_power_of_two());
> +    if addr & (align - 1) != 0 {
> +        pr_err!("Address {:#x} is not aligned with {:#x}.\n", addr, align);
> +        false
> +    } else {
> +        true
> +    }
> +}

I think check_align() is unnecessary -- we don't need to test this in this
context.

Instead, I suggest to add some doc-tests to the `Allocator` impls for `Kmalloc`,
`Vmalloc` and `KVmalloc` in rust/kernel/alloc/allocator.rs testing that they
return the expected alignment. This would be a valuable test case for those. If
you're interested in adding them, please feel free to do so. :)

> +impl kernel::Module for RustAlloc {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("Rust allocator sample (init)\n");
> +
> +        let vbox_blob = VBox::<VboxBlob>::new_uninit(GFP_KERNEL)?;
> +        if !check_align(vbox_blob.as_ptr() as usize, 128) {
> +            return Err(EINVAL);
> +        }
> +        let vbox_blob = vbox_blob.write(VboxBlob([0xfeu8; VBOX_SIZE]));

Given that you initialize it with a value anyways, you can just do

	let blob = VBox::new(Blob([0xfeu8; SIZE]), GFP_KERNEL)?;

instead.

> +        if let Ok(_) = VBox::<VboxLargeAlignBlob>::new_uninit(GFP_KERNEL) {
> +            pr_err!("Allocations for VBox with alignments larger than PAGE_SIZE should fail, but here it succeeded.\n");
> +            return Err(EINVAL);
> +        }

This can just be

	assert!(VBox::<VboxLargeAlignBlob>::new_uninit(GFP_KERNEL).is_err());

within a doc-test.

> +        let mut kvec_blob = KVec::new();
> +        kvec_blob.extend_from_slice(&KVEC_VAL, GFP_KERNEL)?;

We already have doc-tests for KVec::new() and KVec::extend_from_slice() in
rust/kernel/alloc/kvec.rs.

> +        Ok(Self {
> +            vbox_blob,
> +            kvec_blob,
> +        })
> +    }
> +}
> +
> +impl Drop for RustAlloc {
> +    fn drop(&mut self) {
> +        pr_info!("Rust allocator sample (exit)\n");
> +
> +        // check the values
> +        for b in self.vbox_blob.0.as_slice().iter() {
> +            if *b != 0xfeu8 {
> +                pr_err!("vbox_blob contains wrong values\n");
> +            }
> +        }
> +
> +        if self.kvec_blob.as_slice() != KVEC_VAL {
> +            pr_err!("kvec_blob contains wrong values\n");
> +        }

Here you're basically testing Vec::as_slice(). We don't have doc-tests for
as_slice() and as_slice_mut() in rust/kernel/alloc/kvec.rs. Please feel free to
add them. :)

> +    }
> +}


  reply	other threads:[~2025-07-17 11:19 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-17  9:50 [PATCH v2] rust: add a sample alloc usage Hui Zhu
2025-07-17 11:19 ` Danilo Krummrich [this message]
2025-07-17 16:14   ` Boqun Feng
2025-07-24  8:06     ` Hui Zhu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DBEAFCFHFU35.1IZI3ZSJSIRAY@kernel.org \
    --to=dakr@kernel.org \
    --cc=Liam.Howlett@oracle.com \
    --cc=a.hindborg@kernel.org \
    --cc=alex.gaynor@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=gary@garyguo.net \
    --cc=geliang@kernel.org \
    --cc=hui.zhu@linux.dev \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lorenzo.stoakes@oracle.com \
    --cc=lossin@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tmgross@umich.edu \
    --cc=urezki@gmail.com \
    --cc=vbabka@suse.cz \
    --cc=zhuhui@kylinos.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).