[PATCH v3] rust: optimize error type to use nonzero

[PATCH v3] rust: optimize error type to use nonzero

[Filipe Xavier]

Optimize `Result<(), Error>` size by changing `Error` type to 
`NonZero*` for niche optimization.

This reduces the space used by the `Result` type, as the `NonZero*` 
type enables the compiler to apply more efficient memory layout.
For example, the `Result<(), Error>` changes size from 8 to 4 bytes.

Link: https://github.com/Rust-for-Linux/linux/issues/1120
Signed-off-by: Filipe Xavier <felipe_life@live.com>
---
 rust/kernel/error.rs | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index d7965ded2973..9e2910a6c1ea 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -9,6 +9,7 @@
 use alloc::alloc::LayoutError;
 
 use core::fmt;
+use core::num::NonZeroI32;
 use core::num::TryFromIntError;
 use core::str::Utf8Error;
 
@@ -20,7 +21,18 @@ macro_rules! declare_err {
             $(
             #[doc = $doc]
             )*
-            pub const $err: super::Error = super::Error(-(crate::bindings::$err as i32));
+            pub const $err: super::Error = {
+                let err_code: i32 = -(crate::bindings::$err as i32);
+                crate::build_assert!(
+                    err_code >= -(bindings::MAX_ERRNO as i32) && err_code < 0,
+                    "`err` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`)"
+                );
+
+                // SAFETY: `err_code` is checked above to be in a valid range.
+                unsafe {
+                    super::Error::from_errno_unchecked(err_code)
+                }
+            };
         };
     }
 
@@ -88,7 +100,7 @@ macro_rules! declare_err {
 ///
 /// The value is a valid `errno` (i.e. `>= -MAX_ERRNO && < 0`).
 #[derive(Clone, Copy, PartialEq, Eq)]
-pub struct Error(core::ffi::c_int);
+pub struct Error(NonZeroI32);
 
 impl Error {
     /// Creates an [`Error`] from a kernel error code.
@@ -107,7 +119,8 @@ pub fn from_errno(errno: core::ffi::c_int) -> Error {
 
         // INVARIANT: The check above ensures the type invariant
         // will hold.
-        Error(errno)
+        // SAFETY: `errno` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`).
+        unsafe { Error::from_errno_unchecked(errno) }
     }
 
     /// Creates an [`Error`] from a kernel error code.
@@ -115,21 +128,22 @@ pub fn from_errno(errno: core::ffi::c_int) -> Error {
     /// # Safety
     ///
     /// `errno` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`).
-    unsafe fn from_errno_unchecked(errno: core::ffi::c_int) -> Error {
+    const unsafe fn from_errno_unchecked(errno: core::ffi::c_int) -> Error {
         // INVARIANT: The contract ensures the type invariant
         // will hold.
-        Error(errno)
+        // SAFETY: The caller guarantees `errno` is non-zero.
+        Error(unsafe { NonZeroI32::new_unchecked(errno) })
     }
 
     /// Returns the kernel error code.
     pub fn to_errno(self) -> core::ffi::c_int {
-        self.0
+        self.0.get()
     }
 
     #[cfg(CONFIG_BLOCK)]
     pub(crate) fn to_blk_status(self) -> bindings::blk_status_t {
         // SAFETY: `self.0` is a valid error due to its invariant.
-        unsafe { bindings::errno_to_blk_status(self.0) }
+        unsafe { bindings::errno_to_blk_status(self.0.get()) }
     }
 
     /// Returns the error encoded as a pointer.
@@ -137,7 +151,7 @@ pub fn to_ptr<T>(self) -> *mut T {
         #[cfg_attr(target_pointer_width = "32", allow(clippy::useless_conversion))]
         // SAFETY: `self.0` is a valid error due to its invariant.
         unsafe {
-            bindings::ERR_PTR(self.0.into()) as *mut _
+            bindings::ERR_PTR(self.0.get().into()) as *mut _
         }
     }
 
@@ -145,7 +159,7 @@ pub fn to_ptr<T>(self) -> *mut T {
     #[cfg(not(testlib))]
     pub fn name(&self) -> Option<&'static CStr> {
         // SAFETY: Just an FFI call, there are no extra safety requirements.
-        let ptr = unsafe { bindings::errname(-self.0) };
+        let ptr = unsafe { bindings::errname(-self.0.get()) };
         if ptr.is_null() {
             None
         } else {
-- 
2.46.0

Re: [PATCH v3] rust: optimize error type to use nonzero

[Miguel Ojeda]

On Fri, Oct 4, 2024 at 1:16 AM Filipe Xavier <felipe_life@live.com> wrote:
>
> Optimize `Result<(), Error>` size by changing `Error` type to
> `NonZero*` for niche optimization.
>
> This reduces the space used by the `Result` type, as the `NonZero*`
> type enables the compiler to apply more efficient memory layout.
> For example, the `Result<(), Error>` changes size from 8 to 4 bytes.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1120
> Signed-off-by: Filipe Xavier <felipe_life@live.com>

Please note that, for new versions of a series, it is typically
expected a changelog below the `---` line, even if small. Otherwise
reviewers need to scan between the two versions to see what changed.

In addition, you have sent the new version, but you didn't reply or
have a comment here on the changes you did w.r.t. the feedback, so it
is hard to know why you changed/did in the new version.

For instance, you are using here a `build_assert!`, and that may be a
good idea, but I mentioned in v2 whether perhaps this is guaranteed by
const evaluation (or not). So given the `build_assert!`, I assume you
found a case where it is required, so it would be nice to mention
which one, for instance.

Thanks!

Cheers,
Miguel

Re: [PATCH v3] rust: optimize error type to use nonzero

[Filipe Xavier]

On Fri, Oct 3, 2024 at 11:48 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> Please note that, for new versions of a series, it is typically
> expected a changelog below the `---` line, even if small. Otherwise
> reviewers need to scan between the two versions to see what changed.
>
> In addition, you have sent the new version, but you didn't reply or
> have a comment here on the changes you did w.r.t. the feedback, so it
> is hard to know why you changed/did in the new version.
>
> For instance, you are using here a `build_assert!`, and that may be a
> good idea, but I mentioned in v2 whether perhaps this is guaranteed by
> const evaluation (or not). So given the `build_assert!`, I assume you
> found a case where it is required, so it would be nice to mention
> which one, for instance.

I'm sorry, actually it makes sense, I didn't know about the changelog, 
i'll send it next time.

About build_assert on macro, could assume that the C header file defines
these constants with non-zero values, but since the from_errno function 
checks this, I thought it would be interesting to validate it too. 
It makes sense?

---
Changes in v3:
- Adjusted safety comment of function from_errno.
- Adjusted macro declare_err to check if error code is valid and 
  its safety comment fixed.
Link to v2: https://lore.kernel.org/rust-for-linux/BL0PR02MB49141A172569679932EBA20CE9712@BL0PR02MB4914.namprd02.prod.outlook.com/

Changes in v2:
- Adjusted on commit message.
- Added safety comment on macro.
- Reverted signature of function from_errno.
- Removed explicit type on function to_ptr.
Link to v1: https://lore.kernel.org/rust-for-linux/BL0PR02MB49142BDF011175DADD6E79D0E9702@BL0PR02MB4914.namprd02.prod.outlook.com/
---

Thanks!

Filipe

Re: [PATCH v3] rust: optimize error type to use nonzero

[Miguel Ojeda]

On Fri, Oct 4, 2024 at 1:50 PM Filipe Xavier <felipe_life@live.com> wrote:
>
> I'm sorry, actually it makes sense, I didn't know about the changelog,
> i'll send it next time.

No worries at all! It is part of learning how things are usually done.

> About build_assert on macro, could assume that the C header file defines
> these constants with non-zero values, but since the from_errno function
> checks this, I thought it would be interesting to validate it too.
> It makes sense?

Sorry, I am not exactly sure what you mean. If you need to trust the C
header file, then the macro would be unsafe and its callers would need
to explain that they indeed have to trust the C headers.

Instead, if it is possible to have a safe `const` constructor, then we
should just do that. Now, can `from_errno` be `const` to do so? If
not, can we refactor things to make it so somehow (perhaps a new
`const` private fallible constructor)?

Answering these questions should guide you a bit on a solution --
and the answers are typically what one would include in the commit
message, i.e. explaining why you picked the solution (i.e. the design
choices) you did.

Thanks!

Cheers,
Miguel

Re: [PATCH v3] rust: optimize error type to use nonzero

[Filipe Xavier]

> For instance, you are using here a `build_assert!`, and that may be a
> good idea, but I mentioned in v2 whether perhaps this is guaranteed
> by const evaluation (or not). So given the `build_assert!`, I assume you
> found a case where it is required, so it would be nice to mention
> which one, for instance.

Before the macro doesn't use unsafe anywhere, but now i need call a 
`NonZeroI32::new_unchecked`, i added the `build_assert!` to ensure
that the macro is safe, and improve the safety comment.

> Instead, if it is possible to have a safe `const` constructor, then we
> should just do that. Now, can `from_errno` be `const` to do so? If
> not, can we refactor things to make it so somehow (perhaps a new
> `const` private fallible constructor)?

It's possible, but i don't like how it ends up with EINVAL instead of a
compilation failure when passed an incorrect value.

`
macro_rules! declare_err {
        ($err:tt $(,)? $($doc:expr),+) => {
            $(
            #[doc = $doc]
            )*
            pub const $err: super::Error = {
                super::Error::from_errno_const(-(crate::bindings::$err as i32))
            };
        };
    }

impl Error {
    /// Creates an [`Error`] from a kernel error code.
    ///
    /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
    /// be returned in such a case.
    pub fn from_errno(errno: core::ffi::c_int) -> Error {
        let result: Error = Error::from_errno_const(errno);

        if result == code::EINVAL {
            // TODO: Make it a `WARN_ONCE` once available.
            crate::pr_warn!(
                "attempted to create `Error` with out of range `errno`: {}",
                errno
            );
        }

        result
    }

    /// Creates an [`Error`] from a kernel error code.
    ///
    /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
    /// be returned in such a case.
    const fn from_errno_const(errno: core::ffi::c_int) -> Error {
        if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
            return unsafe { Error::from_errno_unchecked(bindings::EINVAL as i32) };
        }
        
        // INVARIANT: The check above ensures the type invariant
        // will hold.
        // SAFETY: `errno` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`).
        unsafe { Error::from_errno_unchecked(errno) }
    }
}`

Thanks!

Filipe

Re: [PATCH v3] rust: optimize error type to use nonzero

[Miguel Ojeda]

On Sat, Oct 5, 2024 at 7:14 PM Filipe Xavier <felipe_life@live.com> wrote:
>
> Before the macro doesn't use unsafe anywhere, but now i need call a
> `NonZeroI32::new_unchecked`, i added the `build_assert!` to ensure
> that the macro is safe, and improve the safety comment.

Yeah, that is good, what I was saying is that the compiler should be
able to catch that case during CTFE (although I think it does not
guarantee it in the general case, but I don't know if it does in a
case like this). So I was curious.

> It's possible, but i don't like how it ends up with EINVAL instead of a
> compilation failure when passed an incorrect value.

Not necessarily -- you can return an `Option<Error>`, for instance,
from that new constructor.

(You may find that you want `feature(const_option)`; but you could do
it manually in a stable way too).

Cheers,
Miguel

Re: [PATCH v3] rust: optimize error type to use nonzero

[Filipe Xavier]

> Not necessarily -- you can return an `Option<Error>`, for instance,
> from that new constructor.

Nice, it worked here, what do you think about it?

---
`macro_rules! declare_err {
        ($err:tt $(,)? $($doc:expr),+) => {
            $(
            #[doc = $doc]
            )*
            pub const $err: super::Error = {
                match super::Error::from_errno_const(-(crate::bindings::$err as i32)) {
                    Some(err) => err,
                    None => panic!("Invalid errno in declare_err!"),
                }
            };
        };
    }

impl Error { ...

/// Creates an [`Error`] from a kernel error code.
    ///
    /// It is a bug to pass an out-of-range `errno`. `None` would
    /// be returned in such a case.
    const fn from_errno_const(errno: core::ffi::c_int) -> Option<Error> {
        if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
            return None;
        }

        // SAFETY: `errno` must be within error code range (i.e. `>= -MAX_ERRNO && < 0`).
        Some(unsafe { Error::from_errno_unchecked(errno) })
    }
`
---

Thanks!
Filipe

Re: [PATCH v3] rust: optimize error type to use nonzero

[Miguel Ojeda]

On Sat, Oct 5, 2024 at 8:52 PM Filipe Xavier <felipe_life@live.com> wrote:
>
> Nice, it worked here, what do you think about it?

Yeah, something like that is what I was thinking, though let's see
what others say.

Since this returns a case that callers can distinguish thanks to
`Option`, I would perhaps avoid saying it is a bug to call it with an
invalid `errno`, and I would call it simply `try_from_errno`.

`// SAFETY` comments should explain why a precondition is true, not
just re-state the precondition, by the way.

Thanks!

Cheers,
Miguel