Message-ID: <aJF9B0sV__t2oG20@sidongui-MacBookPro.local>
Date: Tue, 5 Aug 2025 12:39:51 +0900
From: Sidong Yang <sidong.yang@...iosa.ai>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Caleb Sander Mateos <csander@...estorage.com>,
	Benno Lossin <lossin@...nel.org>, Miguel Ojeda <ojeda@...nel.org>,
	Arnd Bergmann <arnd@...db.de>, Jens Axboe <axboe@...nel.dk>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	io-uring@...r.kernel.org
Subject: Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction
 for io-uring cmd

On Fri, Aug 01, 2025 at 10:48:40AM -0300, Daniel Almeida wrote:

Hi Daniel,

> Hi Sidong,
> 
> > On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@...iosa.ai> wrote:
> > 
> > This patch introduces rust abstraction for io-uring sqe, cmd. IoUringSqe
> > abstracts io_uring_sqe and it has cmd_data(). and IoUringCmd is
> > abstraction for io_uring_cmd. From this, user can get cmd_op, flags,
> > pdu and also sqe.
> 
> IMHO you need to expand this substantially.
> 
> Instead of a very brief discussion of *what* you're doing, you need to explain
> *why* you're doing this and how this patch fits with the overall plan that you
> have in mind.

It seems that it's hard to explain *why* deeply. But I'll try it.

> 
> Also, for the sake of reviewers, try to at least describe the role of all the
> types you've mentioned.

Okay, I'll add some detailed descrption for all types like IoUringCmd.
> 
> > 
> > Signed-off-by: Sidong Yang <sidong.yang@...iosa.ai>
> > ---
> > rust/kernel/io_uring.rs | 183 ++++++++++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs      |   1 +
> > 2 files changed, 184 insertions(+)
> > create mode 100644 rust/kernel/io_uring.rs
> > 
> > diff --git a/rust/kernel/io_uring.rs b/rust/kernel/io_uring.rs
> > new file mode 100644
> > index 000000000000..0acdf3878247
> > --- /dev/null
> > +++ b/rust/kernel/io_uring.rs
> > @@ -0,0 +1,183 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2025 Furiosa AI.
> > +
> 
> Perhaps this instead [0].

Thanks, I'll change it with the format.
> 
> 
> > +//! IoUring command and submission queue entry abstractions.
> 
> Maybe expand this just a little bit as well.

Okay.
> 
> > +//!
> > +//! C headers: [`include/linux/io_uring/cmd.h`](srctree/include/linux/io_uring/cmd.h) and
> > +//! [`include/linux/io_uring/io_uring.h`](srctree/include/linux/io_uring/io_uring.h)
> > +
> > +use core::{mem::MaybeUninit, pin::Pin, ptr::addr_of_mut};
> > +
> > +use crate::{fs::File, types::Opaque};
> > +
> > +/// A Rust abstraction for the Linux kernel's `io_uring_cmd` structure.
> 
> Is there a link for io_uring_cmd that you can use here?

Yes, I'll add a link for the struct.
> 
> > +///
> > +/// This structure is a safe, opaque wrapper around the raw C `io_uring_cmd`
> > +/// binding from the Linux kernel. It represents a command structure used
> > +/// in io_uring operations within the kernel.
> 
> Perhaps backticks on "io_uring" ?

Thanks.
> 
> > +///
> > +/// # Type Safety
> > +///
> > +/// The `#[repr(transparent)]` attribute ensures that this wrapper has
> > +/// the same memory layout as the underlying `io_uring_cmd` structure,
> > +/// allowing it to be safely transmuted between the two representations.
> > +///
> > +/// # Fields
> > +///
> > +/// * `inner` - An opaque wrapper containing the actual `io_uring_cmd` data.
> > +///             The `Opaque` type prevents direct access to the internal
> > +///             structure fields, ensuring memory safety and encapsulation.
> 
> Place this on top of the field itself please. Also, I don´t think you need
> this at all because you don't need to explain the Opaque type, as it's
> extensively used in the kernel crate.

Okay, I'll move this comments to code with the field.
> 
> > +///
> > +/// # Usage
> 
> I don´t think you need this.

Okay.
> 
> > +///
> > +/// This type is used internally by the io_uring subsystem to manage
> > +/// asynchronous I/O commands. It is typically accessed through a pinned
> > +/// mutable reference: `Pin<&mut IoUringCmd>`. The pinning ensures that
> > +/// the structure remains at a fixed memory location, which is required
> > +/// for safe interaction with the kernel's io_uring infrastructure.
> 
> I don´t think you need anything other than:

Thanks, It's too verbose. I'll rewrite this.
> 
> > +/// This type is used internally by the io_uring subsystem to manage
> > +/// asynchronous I/O commands.
> 
> Specifically, you don´t need to say this:
> 
> >  The pinning ensures that
> > +/// the structure remains at a fixed memory location,
> 
> 
> 
> > +///
> > +/// Users typically receive this type as an argument in the `file_operations::uring_cmd()`
> > +/// callback function, where they can access and manipulate the io_uring command
> > +/// data for custom file operations.
> > +///
> > +/// This type should not be constructed or manipulated directly by
> > +/// kernel module developers.
> 
> Well, this is pub, so the reality is that it can be manipulated directly
> through whatever public API it offers.
> 

Agreed, We should provide safe pub fns that it doesn't corrupt the inner state.

> > +#[repr(transparent)]
> > +pub struct IoUringCmd {
> > +    inner: Opaque<bindings::io_uring_cmd>,
> > +}
> > +
> > +impl IoUringCmd {
> > +    /// Returns the cmd_op with associated with the io_uring_cmd.
> 
> Backticks
Thanks.
> 
> > +    #[inline]
> > +    pub fn cmd_op(&self) -> u32 {
> > +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> 
> Not sure I understand what you´re trying to say here. Perhaps add an
> invariant saying that `self.inner` always points to a valid
> `bindings::io_uring_cmd` and mention that here instead.

Thanks, I'll add INVARIANT comment for self.inner and reference it in this comment.
> 
> > +        unsafe { (*self.inner.get()).cmd_op }
> > +    }
> > +
> > +    /// Returns the flags with associated with the io_uring_cmd.
> > +    #[inline]
> > +    pub fn flags(&self) -> u32 {
> > +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> > +        unsafe { (*self.inner.get()).flags }
> > +    }
> > +
> > +    /// Returns the ref pdu for free use.
> 
> I have no idea what "ref pdu" is. You need to describe these acronyms.

Okay. Thanks.
> 
> > +    #[inline]
> > +    pub fn pdu(&mut self) -> &mut MaybeUninit<[u8; 32]> {
> 
> Why MaybeUninit? Also, this is a question for others, but I don´t think
> that `u8`s can ever be uninitialized as all byte values are valid for `u8`.
> 
> > +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> > +        let inner = unsafe { &mut *self.inner.get() };
> > +        let ptr = addr_of_mut!(inner.pdu) as *mut MaybeUninit<[u8; 32]>;
> 
> &raw mut

I didn't know about this. Thanks.
> 
> > +
> > +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> > +        unsafe { &mut *ptr }
> > +    }
> > +
> > +    /// Constructs a new `IoUringCmd` from a raw `io_uring_cmd`
> 
> [`IoUringCmd`]
> 
> By the way, always build the docs and see if they look nice.

Thanks.
> 
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must guarantee that:
> > +    /// - The pointer `ptr` is not null and points to a valid `bindings::io_uring_cmd`.
> > +    /// - The memory pointed to by `ptr` remains valid for the duration of the returned reference's lifetime `'a`.
> > +    /// - The memory will not be moved or freed while the returned `Pin<&mut IoUringCmd>` is alive.
> 
> This returns a wrapper over a mutable reference. You must mention Rust´s aliasing rules here.

Okay. Thanks.
> 
> > +    #[inline]
> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::io_uring_cmd) -> Pin<&'a mut IoUringCmd> {
> > +        // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
> > +        // duration of 'a. The cast is okay because `IoUringCmd` is `repr(transparent)` and has the
> > +        // same memory layout as `bindings::io_uring_cmd`. The returned `Pin` ensures that the object
> > +        // cannot be moved, which is required because the kernel may hold pointers to this memory
> > +        // location and moving it would invalidate those pointers.
> 
> Please break this into multiple paragraphs.

Thanks.
> 
> > +        unsafe { Pin::new_unchecked(&mut *ptr.cast()) }
> > +    }
> > +
> > +    /// Returns the file that referenced by uring cmd self.
> > +    #[inline]
> > +    pub fn file(&self) -> &File {
> > +        // SAFETY: The call guarantees that the `self.inner` is not dangling and stays valid
> > +        let file = unsafe { (*self.inner.get()).file };
> > +        // SAFETY: The call guarantees that `file` points valid file.
> > +        unsafe { File::from_raw_file(file) }
> > +    }
> > +
> > +    /// Returns a reference to the uring cmd's SQE.
> 
> Please define what `SQE` means. Add links if possible.

Okay. Thanks.
> 
> > +    #[inline]
> > +    pub fn sqe(&self) -> &IoUringSqe {
> > +        // SAFETY: The call guarantees that the `self.inner` is not dangling and stays valid
> > +        let sqe = unsafe { (*self.inner.get()).sqe };
> > +        // SAFETY: The call guarantees that the `sqe` points valid io_uring_sqe.
> 
> Backticks

Thanks.
> 
> > +        unsafe { IoUringSqe::from_raw(sqe) }
> > +    }
> > +
> > +    /// Called by consumers of io_uring_cmd, if they originally returned -EIOCBQUEUED upon receiving the command
> 
> Backticks

Thanks.
> 
> > +    #[inline]
> > +    pub fn done(self: Pin<&mut IoUringCmd>, ret: isize, res2: u64, issue_flags: u32) {
> 
> The arguments are cryptic here. Please let us know what they do.

Thanks I'll add description for each arguments.
> 
> > +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> > +        unsafe {
> > +            bindings::io_uring_cmd_done(self.inner.get(), ret, res2, issue_flags);
> > +        }
> > +    }
> > +}
> > +
> > +/// A Rust abstraction for the Linux kernel's `io_uring_sqe` structure.
> > +///
> > +/// This structure is a safe, opaque wrapper around the raw C `io_uring_sqe`
> > +/// binding from the Linux kernel. It represents a Submission Queue Entry
> 
> Ah, SQE == Submission Queue Entry. Is there a link for this?

I'll add a link for this.
> 
> > +/// used in io_uring operations within the kernel.
> > +///
> > +/// # Type Safety
> > +///
> > +/// The `#[repr(transparent)]` attribute ensures that this wrapper has
> > +/// the same memory layout as the underlying `io_uring_sqe` structure,
> > +/// allowing it to be safely transmuted between the two representations.
> > +///
> > +/// # Fields
> > +///
> > +/// * `inner` - An opaque wrapper containing the actual `io_uring_sqe` data.
> > +///             The `Opaque` type prevents direct access to the internal
> > +///             structure fields, ensuring memory safety and encapsulation.
> > +///
> > +/// # Usage
> > +///
> > +/// This type represents a submission queue entry that describes an I/O
> > +/// operation to be executed by the io_uring subsystem. It contains
> > +/// information such as the operation type, file descriptor, buffer
> > +/// pointers, and other operation-specific data.
> 
> This description is very good :)

Thanks. :)
> 
> > +///
> > +/// Users can obtain this type from `IoUringCmd::sqe()` method, which
> > +/// extracts the submission queue entry associated with a command.
> 
> [`IoUringCmd::sqe`]

Thanks.
> 
> > +///
> > +/// This type should not be constructed or manipulated directly by
> > +/// kernel module developers.
> 
> Again, this is pub and can be freely manipulated through whatever
> public API it offers.

Okay.
> 
> > +#[repr(transparent)]
> > +pub struct IoUringSqe {
> > +    inner: Opaque<bindings::io_uring_sqe>,
> > +}
> > +
> > +impl<'a> IoUringSqe {
> 
> Why is this `a here?

It seems that I can delete 'a here. 
> 
> > +    /// Returns the cmd_data with associated with the io_uring_sqe.
> > +    /// This function returns 16 byte array. We don't support IORING_SETUP_SQE128 for now.
> > +    pub fn cmd_data(&'a self) -> &'a [Opaque<u8>] {
> 
> This is automatically placed by the compiler. See the lifetime elision rules
> [1].
> 
Thanks.

> Also why does this return a reference to an array of Opaque<u8>?
> 
> You can return a &[u8] here if you can prove that this reference is legal given
> Rust's aliasing rules. If you can't, then you have to look at what the DMA
> allocator code is doing and use that as an example, i.e.: have a look at the
> dma_read and dma_write macros and mark the function that returns the slice as
> "unsafe".
> 

Okay. It's better to use &[u8].

> > +        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
> > +        let cmd = unsafe { (*self.inner.get()).__bindgen_anon_6.cmd.as_ref() };
> 
> What do you mean by "the call" ? Same in all other places where this sentence is used.
Thanks, it should be dereferecing than call.

> > +
> > +        // SAFETY: The call guarantees that `cmd` is not dangling and stays valid
> > +        unsafe { core::slice::from_raw_parts(cmd.as_ptr() as *const Opaque<u8>, 16) }
> > +    }
> > +
> > +    /// Constructs a new `IoUringSqe` from a raw `io_uring_sqe`
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must guarantee that:
> > +    /// - The pointer `ptr` is not null and points to a valid `bindings::io_uring_sqe`.
> > +    /// - The memory pointed to by `ptr` remains valid for the duration of the returned reference's lifetime `'a`.
> 
> Must mention Rust´s aliasing rules here.

Okay. Thanks.
> 
> > +    #[inline]
> > +    pub unsafe fn from_raw(ptr: *const bindings::io_uring_sqe) -> &'a IoUringSqe {
> > +        // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
> > +        // duration of 'a. The cast is okay because `IoUringSqe` is `repr(transparent)` and has the
> > +        // same memory layout as `bindings::io_uring_sqe`.
> > +        unsafe { &*ptr.cast() }
> > +    }
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 6b4774b2b1c3..fb310e78d51d 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -80,6 +80,7 @@
> > pub mod fs;
> > pub mod init;
> > pub mod io;
> > +pub mod io_uring;
> > pub mod ioctl;
> > pub mod jump_label;
> > #[cfg(CONFIG_KUNIT)]
> > -- 
> > 2.43.0
> > 
> > 
> 
> [0] https://spdx.github.io/spdx-spec/v3.0.1/model/Software/Properties/copyrightText/
> [1] https://doc.rust-lang.org/reference/lifetime-elision.html

Powered by blists - more mailing lists