[v2] rust: miscdevice: abstraction for uring-cmd

[RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 2 months, 1 week ago

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.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.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.
+
+//! IoUring command and submission queue entry abstractions.
+//!
+//! 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.
+///
+/// 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.
+///
+/// # 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.
+///
+/// # Usage
+///
+/// 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.
+///
+/// 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.
+#[repr(transparent)]
+pub struct IoUringCmd {
+    inner: Opaque<bindings::io_uring_cmd>,
+}
+
+impl IoUringCmd {
+    /// Returns the cmd_op with associated with the io_uring_cmd.
+    #[inline]
+    pub fn cmd_op(&self) -> u32 {
+        // SAFETY: The call guarantees that `self.inner` is not dangling and stays valid
+        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.
+    #[inline]
+    pub fn pdu(&mut self) -> &mut MaybeUninit<[u8; 32]> {
+        // 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]>;
+
+        // 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`
+    ///
+    /// # 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.
+    #[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.
+        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.
+    #[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.
+        unsafe { IoUringSqe::from_raw(sqe) }
+    }
+
+    /// Called by consumers of io_uring_cmd, if they originally returned -EIOCBQUEUED upon receiving the command
+    #[inline]
+    pub fn done(self: Pin<&mut IoUringCmd>, ret: isize, res2: u64, issue_flags: u32) {
+        // 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
+/// 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.
+///
+/// Users can obtain this type from `IoUringCmd::sqe()` method, which
+/// extracts the submission queue entry associated with a command.
+///
+/// This type should not be constructed or manipulated directly by
+/// kernel module developers.
+#[repr(transparent)]
+pub struct IoUringSqe {
+    inner: Opaque<bindings::io_uring_sqe>,
+}
+
+impl<'a> IoUringSqe {
+    /// 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>] {
+        // 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() };
+
+        // 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`.
+    #[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

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 2 months ago

Hi Sidong,

> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.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.

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

> 
> Signed-off-by: Sidong Yang <sidong.yang@furiosa.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].


> +//! IoUring command and submission queue entry abstractions.

Maybe expand this just a little bit as well.

> +//!
> +//! 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?

> +///
> +/// 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” ?

> +///
> +/// # 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.

> +///
> +/// # Usage

I don’t think you need this.

> +///
> +/// 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:

> +/// 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.

> +#[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

> +    #[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.

> +        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.

> +    #[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

> +
> +        // 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.

> +    ///
> +    /// # 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.

> +    #[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.

> +        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.

> +    #[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

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

Backticks

> +    #[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.

> +        // 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?

> +/// 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 :)

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

[`IoUringCmd::sqe`]

> +///
> +/// 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.

> +#[repr(transparent)]
> +pub struct IoUringSqe {
> +    inner: Opaque<bindings::io_uring_sqe>,
> +}
> +
> +impl<'a> IoUringSqe {

Why is this ‘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].

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".

> +        // 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.
> +
> +        // 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.

> +    #[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

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Benno Lossin 2 months ago

On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> +    #[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`.

`u8` can be uninitialized. Uninitialized doesn't just mean "can take any
bit pattern", but also "is known to the compiler as being
uninitialized". The docs of `MaybeUninit` explain it like this:

    Moreover, uninitialized memory is special in that it does not have a
    fixed value (“fixed” meaning “it won’t change without being written
    to”). Reading the same uninitialized byte multiple times can give
    different results.

But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
instead.

>> +    #[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.

We usually use bullet point lists for this. 

---
Cheers,
Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 2 months ago

Hi Benno,

> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
>>> +    #[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`.
> 
> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
> bit pattern", but also "is known to the compiler as being
> uninitialized". The docs of `MaybeUninit` explain it like this:
> 
>    Moreover, uninitialized memory is special in that it does not have a
>    fixed value (“fixed” meaning “it won’t change without being written
>    to”). Reading the same uninitialized byte multiple times can give
>    different results.
> 
> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
> instead.


Right, but I guess the question then is why would we ever need to use
MaybeUninit here anyways.

It's a reference to a C array. Just treat that as initialized.

— Daniel

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Benno Lossin 2 months ago

On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
> Hi Benno,
>
>> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
>> 
>> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
>>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
>>>> +    #[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`.
>> 
>> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
>> bit pattern", but also "is known to the compiler as being
>> uninitialized". The docs of `MaybeUninit` explain it like this:
>> 
>>    Moreover, uninitialized memory is special in that it does not have a
>>    fixed value (“fixed” meaning “it won’t change without being written
>>    to”). Reading the same uninitialized byte multiple times can give
>>    different results.
>> 
>> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
>> instead.
>
>
> Right, but I guess the question then is why would we ever need to use
> MaybeUninit here anyways.
>
> It's a reference to a C array. Just treat that as initialized.

AFAIK C uninitialized memory also is considered uninitialized in Rust.
So if this array is not properly initialized on the C side, this would
be the correct type. If it is initialized, then just use `&mut [u8; 32]`.

---
Cheers,
Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 4 weeks ago

On Wed, Aug 06, 2025 at 03:38:24PM +0200, Benno Lossin wrote:
> On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
> > Hi Benno,
> >
> >> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
> >> 
> >> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
> >>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >>>> +    #[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`.
> >> 
> >> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
> >> bit pattern", but also "is known to the compiler as being
> >> uninitialized". The docs of `MaybeUninit` explain it like this:
> >> 
> >>    Moreover, uninitialized memory is special in that it does not have a
> >>    fixed value ("fixed" meaning "it won´t change without being written
> >>    to"). Reading the same uninitialized byte multiple times can give
> >>    different results.
> >> 
> >> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
> >> instead.
> >
> >
> > Right, but I guess the question then is why would we ever need to use
> > MaybeUninit here anyways.
> >
> > It's a reference to a C array. Just treat that as initialized.
> 
> AFAIK C uninitialized memory also is considered uninitialized in Rust.
> So if this array is not properly initialized on the C side, this would
> be the correct type. If it is initialized, then just use `&mut [u8; 32]`.

pdu field is memory chunk for driver can use it freely. The driver usually
saves a private data and read or modify it on the other context. using
just `&mut [u8;32]` would be simple and easy to use.

> 
> ---
> Cheers,
> Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Caleb Sander Mateos 1 month, 4 weeks ago

On Fri, Aug 8, 2025 at 2:56 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
>
> On Wed, Aug 06, 2025 at 03:38:24PM +0200, Benno Lossin wrote:
> > On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
> > > Hi Benno,
> > >
> > >> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
> > >>
> > >> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
> > >>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> > >>>> +    #[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`.
> > >>
> > >> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
> > >> bit pattern", but also "is known to the compiler as being
> > >> uninitialized". The docs of `MaybeUninit` explain it like this:
> > >>
> > >>    Moreover, uninitialized memory is special in that it does not have a
> > >>    fixed value ("fixed" meaning "it won´t change without being written
> > >>    to"). Reading the same uninitialized byte multiple times can give
> > >>    different results.
> > >>
> > >> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
> > >> instead.
> > >
> > >
> > > Right, but I guess the question then is why would we ever need to use
> > > MaybeUninit here anyways.
> > >
> > > It's a reference to a C array. Just treat that as initialized.
> >
> > AFAIK C uninitialized memory also is considered uninitialized in Rust.
> > So if this array is not properly initialized on the C side, this would
> > be the correct type. If it is initialized, then just use `&mut [u8; 32]`.
>
> pdu field is memory chunk for driver can use it freely. The driver usually
> saves a private data and read or modify it on the other context. using
> just `&mut [u8;32]` would be simple and easy to use.

MaybeUninit is correct. The io_uring/uring_cmd layer doesn't
initialize the pdu field. struct io_uring_cmd is overlaid with struct
io_kiocb's cmd field. struct io_kiocb's are allocated using
kmem_cache_alloc(_bulk)() in __io_alloc_req_refill(). io_preinit_req()
is called to initialize each struct io_kiocb but doesn't initialize
the cmd field. As Sidong said, it's uninitialized memory for the
->uring_cmd() implementation to use however it wants for the duration
of the command.

Best,
Caleb

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Fri, Aug 08, 2025 at 09:55:22AM -0400, Caleb Sander Mateos wrote:
> On Fri, Aug 8, 2025 at 2:56 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >
> > On Wed, Aug 06, 2025 at 03:38:24PM +0200, Benno Lossin wrote:
> > > On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
> > > > Hi Benno,
> > > >
> > > >> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
> > > >>
> > > >> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
> > > >>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> > > >>>> +    #[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`.
> > > >>
> > > >> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
> > > >> bit pattern", but also "is known to the compiler as being
> > > >> uninitialized". The docs of `MaybeUninit` explain it like this:
> > > >>
> > > >>    Moreover, uninitialized memory is special in that it does not have a
> > > >>    fixed value ("fixed" meaning "it won´t change without being written
> > > >>    to"). Reading the same uninitialized byte multiple times can give
> > > >>    different results.
> > > >>
> > > >> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
> > > >> instead.
> > > >
> > > >
> > > > Right, but I guess the question then is why would we ever need to use
> > > > MaybeUninit here anyways.
> > > >
> > > > It's a reference to a C array. Just treat that as initialized.
> > >
> > > AFAIK C uninitialized memory also is considered uninitialized in Rust.
> > > So if this array is not properly initialized on the C side, this would
> > > be the correct type. If it is initialized, then just use `&mut [u8; 32]`.
> >
> > pdu field is memory chunk for driver can use it freely. The driver usually
> > saves a private data and read or modify it on the other context. using
> > just `&mut [u8;32]` would be simple and easy to use.
> 
> MaybeUninit is correct. The io_uring/uring_cmd layer doesn't
> initialize the pdu field. struct io_uring_cmd is overlaid with struct
> io_kiocb's cmd field. struct io_kiocb's are allocated using
> kmem_cache_alloc(_bulk)() in __io_alloc_req_refill(). io_preinit_req()
> is called to initialize each struct io_kiocb but doesn't initialize
> the cmd field. As Sidong said, it's uninitialized memory for the
> ->uring_cmd() implementation to use however it wants for the duration
> of the command.

Agreed. Thanks.

Thanks,
Sidong
> 
> Best,
> Caleb

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Benno Lossin 1 month, 4 weeks ago

On Fri Aug 8, 2025 at 8:56 AM CEST, Sidong Yang wrote:
> On Wed, Aug 06, 2025 at 03:38:24PM +0200, Benno Lossin wrote:
>> On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
>> > Hi Benno,
>> >
>> >> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
>> >> 
>> >> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
>> >>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> >>>> +    #[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`.
>> >> 
>> >> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
>> >> bit pattern", but also "is known to the compiler as being
>> >> uninitialized". The docs of `MaybeUninit` explain it like this:
>> >> 
>> >>    Moreover, uninitialized memory is special in that it does not have a
>> >>    fixed value ("fixed" meaning "it won´t change without being written
>> >>    to"). Reading the same uninitialized byte multiple times can give
>> >>    different results.
>> >> 
>> >> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
>> >> instead.
>> >
>> >
>> > Right, but I guess the question then is why would we ever need to use
>> > MaybeUninit here anyways.
>> >
>> > It's a reference to a C array. Just treat that as initialized.
>> 
>> AFAIK C uninitialized memory also is considered uninitialized in Rust.
>> So if this array is not properly initialized on the C side, this would
>> be the correct type. If it is initialized, then just use `&mut [u8; 32]`.
>
> pdu field is memory chunk for driver can use it freely. The driver usually
> saves a private data and read or modify it on the other context. using
> just `&mut [u8;32]` would be simple and easy to use.

Private data is usually handled using `ForeignOwnable` in Rust. What
kind of data would be stored there? If it's a pointer, then `&mut [u8;
32]` would not be the correct choice.

---
Cheers,
Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 4 weeks ago

On Fri, Aug 08, 2025 at 10:49:14AM +0200, Benno Lossin wrote:
> On Fri Aug 8, 2025 at 8:56 AM CEST, Sidong Yang wrote:
> > On Wed, Aug 06, 2025 at 03:38:24PM +0200, Benno Lossin wrote:
> >> On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
> >> > Hi Benno,
> >> >
> >> >> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
> >> >> 
> >> >> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
> >> >>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> >>>> +    #[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`.
> >> >> 
> >> >> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
> >> >> bit pattern", but also "is known to the compiler as being
> >> >> uninitialized". The docs of `MaybeUninit` explain it like this:
> >> >> 
> >> >>    Moreover, uninitialized memory is special in that it does not have a
> >> >>    fixed value ("fixed" meaning "it won´t change without being written
> >> >>    to"). Reading the same uninitialized byte multiple times can give
> >> >>    different results.
> >> >> 
> >> >> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
> >> >> instead.
> >> >
> >> >
> >> > Right, but I guess the question then is why would we ever need to use
> >> > MaybeUninit here anyways.
> >> >
> >> > It's a reference to a C array. Just treat that as initialized.
> >> 
> >> AFAIK C uninitialized memory also is considered uninitialized in Rust.
> >> So if this array is not properly initialized on the C side, this would
> >> be the correct type. If it is initialized, then just use `&mut [u8; 32]`.
> >
> > pdu field is memory chunk for driver can use it freely. The driver usually
> > saves a private data and read or modify it on the other context. using
> > just `&mut [u8;32]` would be simple and easy to use.
> 
> Private data is usually handled using `ForeignOwnable` in Rust. What
> kind of data would be stored there? If it's a pointer, then `&mut [u8;
> 32]` would not be the correct choice.

Most driver uses `io_uring_cmd_to_pdu` macro that casts address of pdu to
private data type. It seems that all driver use this macro has it's own
struct type. How about make 2 function for pdu? like store_pdu(), borrow_pdu().

> 
> ---
> Cheers,
> Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Benno Lossin 1 month, 3 weeks ago

On Fri Aug 8, 2025 at 11:43 AM CEST, Sidong Yang wrote:
> On Fri, Aug 08, 2025 at 10:49:14AM +0200, Benno Lossin wrote:
>> On Fri Aug 8, 2025 at 8:56 AM CEST, Sidong Yang wrote:
>> > On Wed, Aug 06, 2025 at 03:38:24PM +0200, Benno Lossin wrote:
>> >> On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
>> >> > Hi Benno,
>> >> >
>> >> >> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
>> >> >> 
>> >> >> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
>> >> >>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> >> >>>> +    #[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`.
>> >> >> 
>> >> >> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
>> >> >> bit pattern", but also "is known to the compiler as being
>> >> >> uninitialized". The docs of `MaybeUninit` explain it like this:
>> >> >> 
>> >> >>    Moreover, uninitialized memory is special in that it does not have a
>> >> >>    fixed value ("fixed" meaning "it won´t change without being written
>> >> >>    to"). Reading the same uninitialized byte multiple times can give
>> >> >>    different results.
>> >> >> 
>> >> >> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
>> >> >> instead.
>> >> >
>> >> >
>> >> > Right, but I guess the question then is why would we ever need to use
>> >> > MaybeUninit here anyways.
>> >> >
>> >> > It's a reference to a C array. Just treat that as initialized.
>> >> 
>> >> AFAIK C uninitialized memory also is considered uninitialized in Rust.
>> >> So if this array is not properly initialized on the C side, this would
>> >> be the correct type. If it is initialized, then just use `&mut [u8; 32]`.
>> >
>> > pdu field is memory chunk for driver can use it freely. The driver usually
>> > saves a private data and read or modify it on the other context. using
>> > just `&mut [u8;32]` would be simple and easy to use.
>> 
>> Private data is usually handled using `ForeignOwnable` in Rust. What
>> kind of data would be stored there? If it's a pointer, then `&mut [u8;
>> 32]` would not be the correct choice.
>
> Most driver uses `io_uring_cmd_to_pdu` macro that casts address of pdu to
> private data type. It seems that all driver use this macro has it's own
> struct type. How about make 2 function for pdu? like store_pdu(), borrow_pdu().

We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
has been called before. Is there any way we can just ensure that pdu is
always initialized? Like a callback that's called once, before the value
is used at all?

---
Cheers,
Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Sat, Aug 09, 2025 at 12:18:49PM +0200, Benno Lossin wrote:
> On Fri Aug 8, 2025 at 11:43 AM CEST, Sidong Yang wrote:
> > On Fri, Aug 08, 2025 at 10:49:14AM +0200, Benno Lossin wrote:
> >> On Fri Aug 8, 2025 at 8:56 AM CEST, Sidong Yang wrote:
> >> > On Wed, Aug 06, 2025 at 03:38:24PM +0200, Benno Lossin wrote:
> >> >> On Wed Aug 6, 2025 at 2:38 PM CEST, Daniel Almeida wrote:
> >> >> > Hi Benno,
> >> >> >
> >> >> >> On 2 Aug 2025, at 07:52, Benno Lossin <lossin@kernel.org> wrote:
> >> >> >> 
> >> >> >> On Fri Aug 1, 2025 at 3:48 PM CEST, Daniel Almeida wrote:
> >> >> >>>> On 27 Jul 2025, at 12:03, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> >> >>>> +    #[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`.
> >> >> >> 
> >> >> >> `u8` can be uninitialized. Uninitialized doesn't just mean "can take any
> >> >> >> bit pattern", but also "is known to the compiler as being
> >> >> >> uninitialized". The docs of `MaybeUninit` explain it like this:
> >> >> >> 
> >> >> >>    Moreover, uninitialized memory is special in that it does not have a
> >> >> >>    fixed value ("fixed" meaning "it won´t change without being written
> >> >> >>    to"). Reading the same uninitialized byte multiple times can give
> >> >> >>    different results.
> >> >> >> 
> >> >> >> But the return type probably should be `&mut [MaybeUninit<u8>; 32]`
> >> >> >> instead.
> >> >> >
> >> >> >
> >> >> > Right, but I guess the question then is why would we ever need to use
> >> >> > MaybeUninit here anyways.
> >> >> >
> >> >> > It's a reference to a C array. Just treat that as initialized.
> >> >> 
> >> >> AFAIK C uninitialized memory also is considered uninitialized in Rust.
> >> >> So if this array is not properly initialized on the C side, this would
> >> >> be the correct type. If it is initialized, then just use `&mut [u8; 32]`.
> >> >
> >> > pdu field is memory chunk for driver can use it freely. The driver usually
> >> > saves a private data and read or modify it on the other context. using
> >> > just `&mut [u8;32]` would be simple and easy to use.
> >> 
> >> Private data is usually handled using `ForeignOwnable` in Rust. What
> >> kind of data would be stored there? If it's a pointer, then `&mut [u8;
> >> 32]` would not be the correct choice.
> >
> > Most driver uses `io_uring_cmd_to_pdu` macro that casts address of pdu to
> > private data type. It seems that all driver use this macro has it's own
> > struct type. How about make 2 function for pdu? like store_pdu(), borrow_pdu().
> 
> We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
> has been called before. Is there any way we can just ensure that pdu is
> always initialized? Like a callback that's called once, before the value
> is used at all?

I've thought about this. As Celab said, returning `&mut MaybeUninit<[u8;32]> is
simple and best. Only driver knows it's initialized. There is no way to
check whether it's initialized with reading the pdu. The best way is to return
`&mut MaybeUninit<[u8;32]>` and driver initializes it in first time. After 
init, driver knows it's guranteed that it's initialized so it could call 
`assume_init_mut()`. And casting to other struct is another problem. The driver
is responsible for determining how to interpret the PDU, whether by using it
directly as a byte array or by performing an unsafe cast to another struct.

Thanks,
Sidong


> 
> ---
> Cheers,
> Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Benno Lossin 1 month, 3 weeks ago

On Sat Aug 9, 2025 at 2:51 PM CEST, Sidong Yang wrote:
> On Sat, Aug 09, 2025 at 12:18:49PM +0200, Benno Lossin wrote:
>> We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
>> has been called before. Is there any way we can just ensure that pdu is
>> always initialized? Like a callback that's called once, before the value
>> is used at all?
>
> I've thought about this. As Celab said, returning `&mut MaybeUninit<[u8;32]> is
> simple and best. Only driver knows it's initialized. There is no way to
> check whether it's initialized with reading the pdu. The best way is to return
> `&mut MaybeUninit<[u8;32]>` and driver initializes it in first time. After 
> init, driver knows it's guranteed that it's initialized so it could call 
> `assume_init_mut()`. And casting to other struct is another problem. The driver
> is responsible for determining how to interpret the PDU, whether by using it
> directly as a byte array or by performing an unsafe cast to another struct.

But then drivers will have to use `unsafe` & possibly cast the slice to
a struct? I think that's bad design since we try to avoid unsafe code in
drivers as much as possible. Couldn't we try to ensure from the
abstraction side that any time you create such an object, the driver
needs to provide the pdu data? Or we could make it implement `Default`
and then set it to that before handing it to the driver.

---
Cheers,
Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Sat, Aug 09, 2025 at 10:22:06PM +0200, Benno Lossin wrote:
> On Sat Aug 9, 2025 at 2:51 PM CEST, Sidong Yang wrote:
> > On Sat, Aug 09, 2025 at 12:18:49PM +0200, Benno Lossin wrote:
> >> We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
> >> has been called before. Is there any way we can just ensure that pdu is
> >> always initialized? Like a callback that's called once, before the value
> >> is used at all?
> >
> > I've thought about this. As Celab said, returning `&mut MaybeUninit<[u8;32]> is
> > simple and best. Only driver knows it's initialized. There is no way to
> > check whether it's initialized with reading the pdu. The best way is to return
> > `&mut MaybeUninit<[u8;32]>` and driver initializes it in first time. After 
> > init, driver knows it's guranteed that it's initialized so it could call 
> > `assume_init_mut()`. And casting to other struct is another problem. The driver
> > is responsible for determining how to interpret the PDU, whether by using it
> > directly as a byte array or by performing an unsafe cast to another struct.
> 
> But then drivers will have to use `unsafe` & possibly cast the slice to
> a struct? I think that's bad design since we try to avoid unsafe code in
> drivers as much as possible. Couldn't we try to ensure from the
> abstraction side that any time you create such an object, the driver
> needs to provide the pdu data? Or we could make it implement `Default`
> and then set it to that before handing it to the driver.

pdu data is [u8; 32] memory space that driver can borrow. this has two kind of
issues. The one is that the array is not initialized and another one is it's
array type that driver should cast it to private data structure unsafely.
The first one could be resolved with returning `&mut MaybeUninit<>`. And the
second one, casting issue, is remaining. 

It seems that we need new unsafe trait like below:

/// Pdu should be... repr C or transparent, sizeof <= 20
unsafe trait Pdu: Sized {}

/// Returning to casted Pdu type T
pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T>

I think it is like bytemuck::Pod trait. Pod meaning plain old data.

Thanks,
Sidong


> 
> ---
> Cheers,
> Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 1 month, 3 weeks ago


> On 10 Aug 2025, at 10:50, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> 
> On Sat, Aug 09, 2025 at 10:22:06PM +0200, Benno Lossin wrote:
>> On Sat Aug 9, 2025 at 2:51 PM CEST, Sidong Yang wrote:
>>> On Sat, Aug 09, 2025 at 12:18:49PM +0200, Benno Lossin wrote:
>>>> We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
>>>> has been called before. Is there any way we can just ensure that pdu is
>>>> always initialized? Like a callback that's called once, before the value
>>>> is used at all?
>>> 
>>> I've thought about this. As Celab said, returning `&mut MaybeUninit<[u8;32]> is
>>> simple and best. Only driver knows it's initialized. There is no way to
>>> check whether it's initialized with reading the pdu. The best way is to return
>>> `&mut MaybeUninit<[u8;32]>` and driver initializes it in first time. After 
>>> init, driver knows it's guranteed that it's initialized so it could call 
>>> `assume_init_mut()`. And casting to other struct is another problem. The driver
>>> is responsible for determining how to interpret the PDU, whether by using it
>>> directly as a byte array or by performing an unsafe cast to another struct.
>> 
>> But then drivers will have to use `unsafe` & possibly cast the slice to
>> a struct? I think that's bad design since we try to avoid unsafe code in
>> drivers as much as possible. Couldn't we try to ensure from the
>> abstraction side that any time you create such an object, the driver
>> needs to provide the pdu data? Or we could make it implement `Default`
>> and then set it to that before handing it to the driver.
> 
> pdu data is [u8; 32] memory space that driver can borrow. this has two kind of
> issues. The one is that the array is not initialized and another one is it's
> array type that driver should cast it to private data structure unsafely.
> The first one could be resolved with returning `&mut MaybeUninit<>`. And the
> second one, casting issue, is remaining. 
> 
> It seems that we need new unsafe trait like below:
> 
> /// Pdu should be... repr C or transparent, sizeof <= 20
> unsafe trait Pdu: Sized {}
> 
> /// Returning to casted Pdu type T
> pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T>

Wait, you receive an uninitialized array, and you’re supposed to cast it to
T, is that correct? Because that does not fit the signature above.

> 
> I think it is like bytemuck::Pod trait. Pod meaning plain old data.
> 
> Thanks,
> Sidong
> 
> 
>> 
>> ---
>> Cheers,
>> Benno


I'm not really sure how this solves the transmute/cast problem. Is the trait
you're referring to supposed to have any member functions? Or is it just a
marker trait?

I wonder if we can fit the existing "kernel::FromBytes" for this problem.

— Daniel

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Sun, Aug 10, 2025 at 11:27:12AM -0300, Daniel Almeida wrote:
> 
> 
> > On 10 Aug 2025, at 10:50, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> > 
> > On Sat, Aug 09, 2025 at 10:22:06PM +0200, Benno Lossin wrote:
> >> On Sat Aug 9, 2025 at 2:51 PM CEST, Sidong Yang wrote:
> >>> On Sat, Aug 09, 2025 at 12:18:49PM +0200, Benno Lossin wrote:
> >>>> We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
> >>>> has been called before. Is there any way we can just ensure that pdu is
> >>>> always initialized? Like a callback that's called once, before the value
> >>>> is used at all?
> >>> 
> >>> I've thought about this. As Celab said, returning `&mut MaybeUninit<[u8;32]> is
> >>> simple and best. Only driver knows it's initialized. There is no way to
> >>> check whether it's initialized with reading the pdu. The best way is to return
> >>> `&mut MaybeUninit<[u8;32]>` and driver initializes it in first time. After 
> >>> init, driver knows it's guranteed that it's initialized so it could call 
> >>> `assume_init_mut()`. And casting to other struct is another problem. The driver
> >>> is responsible for determining how to interpret the PDU, whether by using it
> >>> directly as a byte array or by performing an unsafe cast to another struct.
> >> 
> >> But then drivers will have to use `unsafe` & possibly cast the slice to
> >> a struct? I think that's bad design since we try to avoid unsafe code in
> >> drivers as much as possible. Couldn't we try to ensure from the
> >> abstraction side that any time you create such an object, the driver
> >> needs to provide the pdu data? Or we could make it implement `Default`
> >> and then set it to that before handing it to the driver.
> > 
> > pdu data is [u8; 32] memory space that driver can borrow. this has two kind of
> > issues. The one is that the array is not initialized and another one is it's
> > array type that driver should cast it to private data structure unsafely.
> > The first one could be resolved with returning `&mut MaybeUninit<>`. And the
> > second one, casting issue, is remaining. 
> > 
> > It seems that we need new unsafe trait like below:
> > 
> > /// Pdu should be... repr C or transparent, sizeof <= 20
> > unsafe trait Pdu: Sized {}
> > 
> > /// Returning to casted Pdu type T
> > pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T>
> 
> Wait, you receive an uninitialized array, and you´re supposed to cast it to
> T, is that correct? Because that does not fit the signature above.

Sorry if my intent wasn´t clear. More example below:

// in rust/kernel/io_uring.rs
unsafe trait Pdu: Sized {}
pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T> {
    let inner = unsafe { &mut *self.inner.get() };
    let ptr = &raw mut inner.pdu as *mut MaybeUninit<T>; // the cast here
    unsafe { &mut *ptr }
}

// in driver code
#[repr(C)] struct MyPdu { value: u64 }
unsafe impl Pdu for MyPdu {}

// initialize
ioucmd.pdu().write(MyPdu { value: 1 });

// read or modify
let mypdu = unsafe { ioucmd.pdu().assume_init_mut() };

Thanks,
Sidong
> 
> > 
> > I think it is like bytemuck::Pod trait. Pod meaning plain old data.
> > 
> > Thanks,
> > Sidong
> > 
> > 
> >> 
> >> ---
> >> Cheers,
> >> Benno
> 
> 
> I'm not really sure how this solves the transmute/cast problem. Is the trait
> you're referring to supposed to have any member functions? Or is it just a
> marker trait?
> 
> I wonder if we can fit the existing "kernel::FromBytes" for this problem.
> 
> - Daniel
>

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Benno Lossin 1 month, 3 weeks ago

On Sun Aug 10, 2025 at 4:46 PM CEST, Sidong Yang wrote:
> On Sun, Aug 10, 2025 at 11:27:12AM -0300, Daniel Almeida wrote:
>> > On 10 Aug 2025, at 10:50, Sidong Yang <sidong.yang@furiosa.ai> wrote:
>> > 
>> > On Sat, Aug 09, 2025 at 10:22:06PM +0200, Benno Lossin wrote:
>> >> On Sat Aug 9, 2025 at 2:51 PM CEST, Sidong Yang wrote:
>> >>> On Sat, Aug 09, 2025 at 12:18:49PM +0200, Benno Lossin wrote:
>> >>>> We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
>> >>>> has been called before. Is there any way we can just ensure that pdu is
>> >>>> always initialized? Like a callback that's called once, before the value
>> >>>> is used at all?
>> >>> 
>> >>> I've thought about this. As Celab said, returning `&mut MaybeUninit<[u8;32]> is
>> >>> simple and best. Only driver knows it's initialized. There is no way to
>> >>> check whether it's initialized with reading the pdu. The best way is to return
>> >>> `&mut MaybeUninit<[u8;32]>` and driver initializes it in first time. After 
>> >>> init, driver knows it's guranteed that it's initialized so it could call 
>> >>> `assume_init_mut()`. And casting to other struct is another problem. The driver
>> >>> is responsible for determining how to interpret the PDU, whether by using it
>> >>> directly as a byte array or by performing an unsafe cast to another struct.
>> >> 
>> >> But then drivers will have to use `unsafe` & possibly cast the slice to
>> >> a struct? I think that's bad design since we try to avoid unsafe code in
>> >> drivers as much as possible. Couldn't we try to ensure from the
>> >> abstraction side that any time you create such an object, the driver
>> >> needs to provide the pdu data? Or we could make it implement `Default`
>> >> and then set it to that before handing it to the driver.
>> > 
>> > pdu data is [u8; 32] memory space that driver can borrow. this has two kind of
>> > issues. The one is that the array is not initialized and another one is it's
>> > array type that driver should cast it to private data structure unsafely.
>> > The first one could be resolved with returning `&mut MaybeUninit<>`. And the
>> > second one, casting issue, is remaining. 
>> > 
>> > It seems that we need new unsafe trait like below:
>> > 
>> > /// Pdu should be... repr C or transparent, sizeof <= 20
>> > unsafe trait Pdu: Sized {}
>> > 
>> > /// Returning to casted Pdu type T
>> > pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T>
>> 
>> Wait, you receive an uninitialized array, and you´re supposed to cast it to
>> T, is that correct? Because that does not fit the signature above.
>
> Sorry if my intent wasn´t clear. More example below:
>
> // in rust/kernel/io_uring.rs
> unsafe trait Pdu: Sized {}
> pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T> {
>     let inner = unsafe { &mut *self.inner.get() };
>     let ptr = &raw mut inner.pdu as *mut MaybeUninit<T>; // the cast here
>     unsafe { &mut *ptr }
> }
>
> // in driver code
> #[repr(C)] struct MyPdu { value: u64 }
> unsafe impl Pdu for MyPdu {}
>
> // initialize
> ioucmd.pdu().write(MyPdu { value: 1 });
>
> // read or modify
> let mypdu = unsafe { ioucmd.pdu().assume_init_mut() };

This is the kind of code I'd like to avoid, since it plans to use
`unsafe` in driver code (the `unsafe impl` above is also a problem, but
we can solve that with a derive macro).

Where are the entrypoints for `IoUringCmd` for driver code? I imagine
that there is some kind of a driver callback (like `probe`, `open` etc)
that contains an `Pin<&mut IoUringCmd>` as an argument, right? When is
it created, can we control that & just write some default value to the
pdu field?

---
Cheers,
Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Sun, Aug 10, 2025 at 10:06:21PM +0200, Benno Lossin wrote:
> On Sun Aug 10, 2025 at 4:46 PM CEST, Sidong Yang wrote:
> > On Sun, Aug 10, 2025 at 11:27:12AM -0300, Daniel Almeida wrote:
> >> > On 10 Aug 2025, at 10:50, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> > 
> >> > On Sat, Aug 09, 2025 at 10:22:06PM +0200, Benno Lossin wrote:
> >> >> On Sat Aug 9, 2025 at 2:51 PM CEST, Sidong Yang wrote:
> >> >>> On Sat, Aug 09, 2025 at 12:18:49PM +0200, Benno Lossin wrote:
> >> >>>> We'd need to ensure that `borrow_pdu` can only be called if `store_pdu`
> >> >>>> has been called before. Is there any way we can just ensure that pdu is
> >> >>>> always initialized? Like a callback that's called once, before the value
> >> >>>> is used at all?
> >> >>> 
> >> >>> I've thought about this. As Celab said, returning `&mut MaybeUninit<[u8;32]> is
> >> >>> simple and best. Only driver knows it's initialized. There is no way to
> >> >>> check whether it's initialized with reading the pdu. The best way is to return
> >> >>> `&mut MaybeUninit<[u8;32]>` and driver initializes it in first time. After 
> >> >>> init, driver knows it's guranteed that it's initialized so it could call 
> >> >>> `assume_init_mut()`. And casting to other struct is another problem. The driver
> >> >>> is responsible for determining how to interpret the PDU, whether by using it
> >> >>> directly as a byte array or by performing an unsafe cast to another struct.
> >> >> 
> >> >> But then drivers will have to use `unsafe` & possibly cast the slice to
> >> >> a struct? I think that's bad design since we try to avoid unsafe code in
> >> >> drivers as much as possible. Couldn't we try to ensure from the
> >> >> abstraction side that any time you create such an object, the driver
> >> >> needs to provide the pdu data? Or we could make it implement `Default`
> >> >> and then set it to that before handing it to the driver.
> >> > 
> >> > pdu data is [u8; 32] memory space that driver can borrow. this has two kind of
> >> > issues. The one is that the array is not initialized and another one is it's
> >> > array type that driver should cast it to private data structure unsafely.
> >> > The first one could be resolved with returning `&mut MaybeUninit<>`. And the
> >> > second one, casting issue, is remaining. 
> >> > 
> >> > It seems that we need new unsafe trait like below:
> >> > 
> >> > /// Pdu should be... repr C or transparent, sizeof <= 20
> >> > unsafe trait Pdu: Sized {}
> >> > 
> >> > /// Returning to casted Pdu type T
> >> > pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T>
> >> 
> >> Wait, you receive an uninitialized array, and you´re supposed to cast it to
> >> T, is that correct? Because that does not fit the signature above.
> >
> > Sorry if my intent wasn´t clear. More example below:
> >
> > // in rust/kernel/io_uring.rs
> > unsafe trait Pdu: Sized {}
> > pub fn pdu<T: Pdu>(&mut self) -> &mut MaybeUninit<T> {
> >     let inner = unsafe { &mut *self.inner.get() };
> >     let ptr = &raw mut inner.pdu as *mut MaybeUninit<T>; // the cast here
> >     unsafe { &mut *ptr }
> > }
> >
> > // in driver code
> > #[repr(C)] struct MyPdu { value: u64 }
> > unsafe impl Pdu for MyPdu {}
> >
> > // initialize
> > ioucmd.pdu().write(MyPdu { value: 1 });
> >
> > // read or modify
> > let mypdu = unsafe { ioucmd.pdu().assume_init_mut() };
> 
> This is the kind of code I'd like to avoid, since it plans to use
> `unsafe` in driver code (the `unsafe impl` above is also a problem, but
> we can solve that with a derive macro).
> 
> Where are the entrypoints for `IoUringCmd` for driver code? I imagine
> that there is some kind of a driver callback (like `probe`, `open` etc)
> that contains an `Pin<&mut IoUringCmd>` as an argument, right? When is
> it created, can we control that & just write some default value to the
> pdu field?

There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
would be create in the callback function. But the callback function could be
called repeatedly with same `io_uring_cmd` instance as far as I know.

But in c side, there is initialization step `io_uring_cmd_prep()`.
How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
as flag in pdu for checking initialized also we should provide 31 bytes except
a byte for the flag.

Thanks,
Sidong
> 
> ---
> Cheers,
> Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 1 month, 3 weeks ago

> 
> There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
> would be create in the callback function. But the callback function could be
> called repeatedly with same `io_uring_cmd` instance as far as I know.
> 
> But in c side, there is initialization step `io_uring_cmd_prep()`.
> How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
> as flag in pdu for checking initialized also we should provide 31 bytes except
> a byte for the flag.
> 

That was a follow-up question of mine. Can’t we enforce zero-initialization
in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.

Hopefully this can be done as you've described above, but I don't want to over
extend my opinion on something I know nothing about.

— Daniel

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
> 
> > 
> > There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
> > would be create in the callback function. But the callback function could be
> > called repeatedly with same `io_uring_cmd` instance as far as I know.
> > 
> > But in c side, there is initialization step `io_uring_cmd_prep()`.
> > How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
> > as flag in pdu for checking initialized also we should provide 31 bytes except
> > a byte for the flag.
> > 
> 
> That was a follow-up question of mine. Can´t we enforce zero-initialization
> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
> 
> Hopefully this can be done as you've described above, but I don't want to over
> extend my opinion on something I know nothing about.

I need to add a commit that initialize pdu in prep step in next version. 
I'd like to get a comment from io_uring maintainer Jens. Thanks.

If we could initialize (filling zero) in prep step, How about casting issue?
Driver still needs to cast array to its private struct in unsafe?

Thanks,
Sidong

> 
> - Daniel

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Benno Lossin 1 month, 3 weeks ago

On Mon Aug 11, 2025 at 4:50 PM CEST, Sidong Yang wrote:
> On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
>> > There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
>> > would be create in the callback function. But the callback function could be
>> > called repeatedly with same `io_uring_cmd` instance as far as I know.
>> > 
>> > But in c side, there is initialization step `io_uring_cmd_prep()`.
>> > How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
>> > as flag in pdu for checking initialized also we should provide 31 bytes except
>> > a byte for the flag.
>> > 
>> 
>> That was a follow-up question of mine. Can´t we enforce zero-initialization
>> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
>> 
>> Hopefully this can be done as you've described above, but I don't want to over
>> extend my opinion on something I know nothing about.
>
> I need to add a commit that initialize pdu in prep step in next version. 
> I'd like to get a comment from io_uring maintainer Jens. Thanks.
>
> If we could initialize (filling zero) in prep step, How about casting issue?
> Driver still needs to cast array to its private struct in unsafe?

We still would have the casting issue.

Can't we do the following:

* Add a new associated type to `MiscDevice` called `IoUringPdu` that
  has to implement `Default` and have a size of at most 32 bytes.
* make `IoUringCmd` generic
* make `MiscDevice::uring_cmd` take `Pin<&mut IoUringCmd<Self::IoUringPdu>>`
* initialize the private data to be `IoUringPdu::default()` when we
  create the `IoUringCmd` object.
* provide a `fn pdu(&mut self) -> &mut Pdu` on `IoUringPdu<Pdu>`.

Any thoughts? If we don't want to add a new associated type to
`MiscDevice` (because not everyone has to declare the `IoUringCmd`
data), I have a small trait dance that we can do to avoid that:

    pub trait IoUringMiscDevice: MiscDevice {
        type IoUringPdu: Default; // missing the 32 byte constraint
    }

and then in MiscDevice we still add this function:

        fn uring_cmd(
            _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
            _io_uring_cmd: Pin<&mut IoUringCmd<Self::IoUringPdu>>,
            _issue_flags: u32,
        ) -> Result<i32>
        where
            Self: IoUringMiscDevice,
        {
            build_error!(VTABLE_DEFAULT_ERROR)
        }

It can only be called when the user also implements `IoUringMiscDevice`.

---
Cheers,
Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 1 month, 3 weeks ago


> On 12 Aug 2025, at 05:34, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Mon Aug 11, 2025 at 4:50 PM CEST, Sidong Yang wrote:
>> On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
>>>> There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
>>>> would be create in the callback function. But the callback function could be
>>>> called repeatedly with same `io_uring_cmd` instance as far as I know.
>>>> 
>>>> But in c side, there is initialization step `io_uring_cmd_prep()`.
>>>> How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
>>>> as flag in pdu for checking initialized also we should provide 31 bytes except
>>>> a byte for the flag.
>>>> 
>>> 
>>> That was a follow-up question of mine. Can´t we enforce zero-initialization
>>> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
>>> 
>>> Hopefully this can be done as you've described above, but I don't want to over
>>> extend my opinion on something I know nothing about.
>> 
>> I need to add a commit that initialize pdu in prep step in next version. 
>> I'd like to get a comment from io_uring maintainer Jens. Thanks.
>> 
>> If we could initialize (filling zero) in prep step, How about casting issue?
>> Driver still needs to cast array to its private struct in unsafe?
> 
> We still would have the casting issue.
> 
> Can't we do the following:
> 
> * Add a new associated type to `MiscDevice` called `IoUringPdu` that
>  has to implement `Default` and have a size of at most 32 bytes.
> * make `IoUringCmd` generic
> * make `MiscDevice::uring_cmd` take `Pin<&mut IoUringCmd<Self::IoUringPdu>>`
> * initialize the private data to be `IoUringPdu::default()` when we
>  create the `IoUringCmd` object.
> * provide a `fn pdu(&mut self) -> &mut Pdu` on `IoUringPdu<Pdu>`.
> 
> Any thoughts? If we don't want to add a new associated type to
> `MiscDevice` (because not everyone has to declare the `IoUringCmd`
> data), I have a small trait dance that we can do to avoid that:


Benno,

IIUC, and note that I'm not proficient with io_uring in general:

I think we have to accept that we will need to parse types from and to byte
arrays, and that is inherently unsafe. It is no different from what is going on
in UserSliceReader/UserSliceWriter, and IMHO, we should copy that in as much as
it makes sense.

I think that the only difference is that all uAPI types de-facto satisfy all
the requirements for FromBytes/AsBytes, as we've discussed previously, whereas
here, drivers have to prove that their types can implement the trait.


By the way, Sidong, is this byte array shared with userspace? i.e.: is there
any copy_to/from_user() taking place here?

-- Daniel

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Tue, Aug 12, 2025 at 11:38:51AM -0300, Daniel Almeida wrote:
> 
> 
> > On 12 Aug 2025, at 05:34, Benno Lossin <lossin@kernel.org> wrote:
> > 
> > On Mon Aug 11, 2025 at 4:50 PM CEST, Sidong Yang wrote:
> >> On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
> >>>> There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
> >>>> would be create in the callback function. But the callback function could be
> >>>> called repeatedly with same `io_uring_cmd` instance as far as I know.
> >>>> 
> >>>> But in c side, there is initialization step `io_uring_cmd_prep()`.
> >>>> How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
> >>>> as flag in pdu for checking initialized also we should provide 31 bytes except
> >>>> a byte for the flag.
> >>>> 
> >>> 
> >>> That was a follow-up question of mine. Can´t we enforce zero-initialization
> >>> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
> >>> 
> >>> Hopefully this can be done as you've described above, but I don't want to over
> >>> extend my opinion on something I know nothing about.
> >> 
> >> I need to add a commit that initialize pdu in prep step in next version. 
> >> I'd like to get a comment from io_uring maintainer Jens. Thanks.
> >> 
> >> If we could initialize (filling zero) in prep step, How about casting issue?
> >> Driver still needs to cast array to its private struct in unsafe?
> > 
> > We still would have the casting issue.
> > 
> > Can't we do the following:
> > 
> > * Add a new associated type to `MiscDevice` called `IoUringPdu` that
> >  has to implement `Default` and have a size of at most 32 bytes.
> > * make `IoUringCmd` generic
> > * make `MiscDevice::uring_cmd` take `Pin<&mut IoUringCmd<Self::IoUringPdu>>`
> > * initialize the private data to be `IoUringPdu::default()` when we
> >  create the `IoUringCmd` object.
> > * provide a `fn pdu(&mut self) -> &mut Pdu` on `IoUringPdu<Pdu>`.
> > 
> > Any thoughts? If we don't want to add a new associated type to
> > `MiscDevice` (because not everyone has to declare the `IoUringCmd`
> > data), I have a small trait dance that we can do to avoid that:
> 
> 
> Benno,
> 
> IIUC, and note that I'm not proficient with io_uring in general:
> 
> I think we have to accept that we will need to parse types from and to byte
> arrays, and that is inherently unsafe. It is no different from what is going on
> in UserSliceReader/UserSliceWriter, and IMHO, we should copy that in as much as
> it makes sense.
> 
> I think that the only difference is that all uAPI types de-facto satisfy all
> the requirements for FromBytes/AsBytes, as we've discussed previously, whereas
> here, drivers have to prove that their types can implement the trait.
> 
> 
> By the way, Sidong, is this byte array shared with userspace? i.e.: is there
> any copy_to/from_user() taking place here?

No. pdu array allocated from kernel. I'll use `core::ptr::copy_nonoverlapping`.

Thanks,
Sidong
> 
> -- Daniel

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Tue, Aug 12, 2025 at 10:34:56AM +0200, Benno Lossin wrote:
> On Mon Aug 11, 2025 at 4:50 PM CEST, Sidong Yang wrote:
> > On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
> >> > There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
> >> > would be create in the callback function. But the callback function could be
> >> > called repeatedly with same `io_uring_cmd` instance as far as I know.
> >> > 
> >> > But in c side, there is initialization step `io_uring_cmd_prep()`.
> >> > How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
> >> > as flag in pdu for checking initialized also we should provide 31 bytes except
> >> > a byte for the flag.
> >> > 
> >> 
> >> That was a follow-up question of mine. Can´t we enforce zero-initialization
> >> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
> >> 
> >> Hopefully this can be done as you've described above, but I don't want to over
> >> extend my opinion on something I know nothing about.
> >
> > I need to add a commit that initialize pdu in prep step in next version. 
> > I'd like to get a comment from io_uring maintainer Jens. Thanks.
> >
> > If we could initialize (filling zero) in prep step, How about casting issue?
> > Driver still needs to cast array to its private struct in unsafe?
> 
> We still would have the casting issue.
> 
> Can't we do the following:
> 
> * Add a new associated type to `MiscDevice` called `IoUringPdu` that
>   has to implement `Default` and have a size of at most 32 bytes.
> * make `IoUringCmd` generic
> * make `MiscDevice::uring_cmd` take `Pin<&mut IoUringCmd<Self::IoUringPdu>>`
> * initialize the private data to be `IoUringPdu::default()` when we
>   create the `IoUringCmd` object.

`uring_cmd` could be called multiple times. So we can't initialize
in that time. I don't understand that how can we cast [u8; 32] to
`IoUringPdu` safely. It seems that casting can't help to use unsafe.
I think best way is that just return zerod `&mut [u8; 32]` and
each driver implements safe serde logic for its private data. 

> * provide a `fn pdu(&mut self) -> &mut Pdu` on `IoUringPdu<Pdu>`.
> 
> Any thoughts? If we don't want to add a new associated type to
> `MiscDevice` (because not everyone has to declare the `IoUringCmd`
> data), I have a small trait dance that we can do to avoid that:
> 
>     pub trait IoUringMiscDevice: MiscDevice {
>         type IoUringPdu: Default; // missing the 32 byte constraint
>     }
> 
> and then in MiscDevice we still add this function:
> 
>         fn uring_cmd(
>             _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
>             _io_uring_cmd: Pin<&mut IoUringCmd<Self::IoUringPdu>>,
>             _issue_flags: u32,
>         ) -> Result<i32>
>         where
>             Self: IoUringMiscDevice,
>         {
>             build_error!(VTABLE_DEFAULT_ERROR)
>         }
> 
> It can only be called when the user also implements `IoUringMiscDevice`.
> 
> ---
> Cheers,
> Benno

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 1 month, 3 weeks ago


> On 12 Aug 2025, at 09:19, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> 
> On Tue, Aug 12, 2025 at 10:34:56AM +0200, Benno Lossin wrote:
>> On Mon Aug 11, 2025 at 4:50 PM CEST, Sidong Yang wrote:
>>> On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
>>>>> There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
>>>>> would be create in the callback function. But the callback function could be
>>>>> called repeatedly with same `io_uring_cmd` instance as far as I know.
>>>>> 
>>>>> But in c side, there is initialization step `io_uring_cmd_prep()`.
>>>>> How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
>>>>> as flag in pdu for checking initialized also we should provide 31 bytes except
>>>>> a byte for the flag.
>>>>> 
>>>> 
>>>> That was a follow-up question of mine. Can´t we enforce zero-initialization
>>>> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
>>>> 
>>>> Hopefully this can be done as you've described above, but I don't want to over
>>>> extend my opinion on something I know nothing about.
>>> 
>>> I need to add a commit that initialize pdu in prep step in next version. 
>>> I'd like to get a comment from io_uring maintainer Jens. Thanks.
>>> 
>>> If we could initialize (filling zero) in prep step, How about casting issue?
>>> Driver still needs to cast array to its private struct in unsafe?
>> 
>> We still would have the casting issue.
>> 
>> Can't we do the following:
>> 
>> * Add a new associated type to `MiscDevice` called `IoUringPdu` that
>>  has to implement `Default` and have a size of at most 32 bytes.
>> * make `IoUringCmd` generic
>> * make `MiscDevice::uring_cmd` take `Pin<&mut IoUringCmd<Self::IoUringPdu>>`
>> * initialize the private data to be `IoUringPdu::default()` when we
>>  create the `IoUringCmd` object.
> 
> `uring_cmd` could be called multiple times. So we can't initialize
> in that time. I don't understand that how can we cast [u8; 32] to
> `IoUringPdu` safely. It seems that casting can't help to use unsafe.
> I think best way is that just return zerod `&mut [u8; 32]` and
> each driver implements safe serde logic for its private data. 
> 

Again, can’t we use FromBytes for this?

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 1 month, 3 weeks ago

On Tue, Aug 12, 2025 at 09:43:56AM -0300, Daniel Almeida wrote:
> 
> 
> > On 12 Aug 2025, at 09:19, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> > 
> > On Tue, Aug 12, 2025 at 10:34:56AM +0200, Benno Lossin wrote:
> >> On Mon Aug 11, 2025 at 4:50 PM CEST, Sidong Yang wrote:
> >>> On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
> >>>>> There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
> >>>>> would be create in the callback function. But the callback function could be
> >>>>> called repeatedly with same `io_uring_cmd` instance as far as I know.
> >>>>> 
> >>>>> But in c side, there is initialization step `io_uring_cmd_prep()`.
> >>>>> How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
> >>>>> as flag in pdu for checking initialized also we should provide 31 bytes except
> >>>>> a byte for the flag.
> >>>>> 
> >>>> 
> >>>> That was a follow-up question of mine. Can´t we enforce zero-initialization
> >>>> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
> >>>> 
> >>>> Hopefully this can be done as you've described above, but I don't want to over
> >>>> extend my opinion on something I know nothing about.
> >>> 
> >>> I need to add a commit that initialize pdu in prep step in next version. 
> >>> I'd like to get a comment from io_uring maintainer Jens. Thanks.
> >>> 
> >>> If we could initialize (filling zero) in prep step, How about casting issue?
> >>> Driver still needs to cast array to its private struct in unsafe?
> >> 
> >> We still would have the casting issue.
> >> 
> >> Can't we do the following:
> >> 
> >> * Add a new associated type to `MiscDevice` called `IoUringPdu` that
> >>  has to implement `Default` and have a size of at most 32 bytes.
> >> * make `IoUringCmd` generic
> >> * make `MiscDevice::uring_cmd` take `Pin<&mut IoUringCmd<Self::IoUringPdu>>`
> >> * initialize the private data to be `IoUringPdu::default()` when we
> >>  create the `IoUringCmd` object.
> > 
> > `uring_cmd` could be called multiple times. So we can't initialize
> > in that time. I don't understand that how can we cast [u8; 32] to
> > `IoUringPdu` safely. It seems that casting can't help to use unsafe.
> > I think best way is that just return zerod `&mut [u8; 32]` and
> > each driver implements safe serde logic for its private data. 
> > 
> 
> Again, can´t we use FromBytes for this?

Agreed, we need FromBytes for read_pdu and AsBytes for write_pdu. I'll reference
dma code for next version.

Thanks,
Sidong
> 
>

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 1 month, 3 weeks ago


> On 12 Aug 2025, at 10:56, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> 
> On Tue, Aug 12, 2025 at 09:43:56AM -0300, Daniel Almeida wrote:
>> 
>> 
>>> On 12 Aug 2025, at 09:19, Sidong Yang <sidong.yang@furiosa.ai> wrote:
>>> 
>>> On Tue, Aug 12, 2025 at 10:34:56AM +0200, Benno Lossin wrote:
>>>> On Mon Aug 11, 2025 at 4:50 PM CEST, Sidong Yang wrote:
>>>>> On Mon, Aug 11, 2025 at 09:44:22AM -0300, Daniel Almeida wrote:
>>>>>>> There is `uring_cmd` callback in `file_operation` at c side. `Pin<&mut IoUringCmd>`
>>>>>>> would be create in the callback function. But the callback function could be
>>>>>>> called repeatedly with same `io_uring_cmd` instance as far as I know.
>>>>>>> 
>>>>>>> But in c side, there is initialization step `io_uring_cmd_prep()`.
>>>>>>> How about fill zero pdu in `io_uring_cmd_prep()`? And we could assign a byte
>>>>>>> as flag in pdu for checking initialized also we should provide 31 bytes except
>>>>>>> a byte for the flag.
>>>>>>> 
>>>>>> 
>>>>>> That was a follow-up question of mine. Can´t we enforce zero-initialization
>>>>>> in C to get rid of this MaybeUninit? Uninitialized data is just bad in general.
>>>>>> 
>>>>>> Hopefully this can be done as you've described above, but I don't want to over
>>>>>> extend my opinion on something I know nothing about.
>>>>> 
>>>>> I need to add a commit that initialize pdu in prep step in next version. 
>>>>> I'd like to get a comment from io_uring maintainer Jens. Thanks.
>>>>> 
>>>>> If we could initialize (filling zero) in prep step, How about casting issue?
>>>>> Driver still needs to cast array to its private struct in unsafe?
>>>> 
>>>> We still would have the casting issue.
>>>> 
>>>> Can't we do the following:
>>>> 
>>>> * Add a new associated type to `MiscDevice` called `IoUringPdu` that
>>>> has to implement `Default` and have a size of at most 32 bytes.
>>>> * make `IoUringCmd` generic
>>>> * make `MiscDevice::uring_cmd` take `Pin<&mut IoUringCmd<Self::IoUringPdu>>`
>>>> * initialize the private data to be `IoUringPdu::default()` when we
>>>> create the `IoUringCmd` object.
>>> 
>>> `uring_cmd` could be called multiple times. So we can't initialize
>>> in that time. I don't understand that how can we cast [u8; 32] to
>>> `IoUringPdu` safely. It seems that casting can't help to use unsafe.
>>> I think best way is that just return zerod `&mut [u8; 32]` and

Also, I agree about zeroing out the array, let’s try to not have this
MaybeUninit thing here if possible.

>>> each driver implements safe serde logic for its private data. 
>>> 
>> 
>> Again, can´t we use FromBytes for this?
> 
> Agreed, we need FromBytes for read_pdu and AsBytes for write_pdu. I'll reference
> dma code for next version.
> 
> Thanks,
> Sidong
>> 
>>

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 2 months ago

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@furiosa.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@furiosa.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

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Daniel Almeida 2 months ago

Hi Sidon,

> On 5 Aug 2025, at 00:39, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> 
> 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@furiosa.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.

Just to be clear, you don’t need to go deep enough in the sense that
you’re basically rewriting the documentation that is already available in
C, but you do need to provide an overview of how things fit together, otherwise
we're left to connect the dots.

Have a look at the I2C series [0]. That is all you need to do IMHO.

I’d use that as an example.
 
[0]: https://lore.kernel.org/rust-for-linux/2D1DE1BC-13FB-4563-BE11-232C755B5117@collabora.com/T/#t

— Daniel

Re: [RFC PATCH v2 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

Posted by Sidong Yang 2 months ago

On Tue, Aug 05, 2025 at 10:02:18AM -0300, Daniel Almeida wrote:
> Hi Sidon,
> 
> > On 5 Aug 2025, at 00:39, Sidong Yang <sidong.yang@furiosa.ai> wrote:
> > 
> > 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@furiosa.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.
> 
> Just to be clear, you don´t need to go deep enough in the sense that
> you´re basically rewriting the documentation that is already available in
> C, but you do need to provide an overview of how things fit together, otherwise
> we're left to connect the dots.
> 
> Have a look at the I2C series [0]. That is all you need to do IMHO.
> 
> I´d use that as an example.
>  
> [0]: https://lore.kernel.org/rust-for-linux/2D1DE1BC-13FB-4563-BE11-232C755B5117@collabora.com/T/#t

Thanks, 
I'll rewrite the cover letter and commit messages that reference the example. 

Thanks,
Sidong
> 
> - Daniel