1. Patchew
  2. linux
  3. [RFC PATCH 0/2] rust: Zeroable: allow struct update syntax outside init macros
  4. View patch

[PATCH 2/2] rust: block/mq: replace mem::zeroed() with Zeroable trait

Paolo Bonzini posted 2 patches 1 year, 2 months ago
[PATCH 2/2] rust: block/mq: replace mem::zeroed() with Zeroable trait
Posted by Paolo Bonzini 1 year, 2 months ago 
Isolate the unsafety in the declaration of the Zeroable trait, instead of having
to use "unsafe" just to declare a struct.  This is more similar to how you would
use "..Default::default()" (which is also a possibility here, but arguably
less efficient).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/kernel/block/mq/gen_disk.rs |  8 +++++---
 rust/kernel/block/mq/tag_set.rs  | 10 ++++++----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index 708125dce96a..65342d065296 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -6,7 +6,7 @@
 //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
 
 use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
-use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
+use crate::{bindings, error::from_err_ptr, error::Result, init::Zeroable, sync::Arc};
 use crate::{error, static_lock_class};
 use core::fmt::{self, Write};
 
@@ -31,6 +31,9 @@ fn default() -> Self {
     }
 }
 
+// SAFETY: `bindings::queue_limits` contains only fields that are valid when zeroed.
+unsafe impl Zeroable for bindings::queue_limits {}
+
 impl GenDiskBuilder {
     /// Create a new instance.
     pub fn new() -> Self {
@@ -93,8 +96,7 @@ pub fn build<T: Operations>(
         name: fmt::Arguments<'_>,
         tagset: Arc<TagSet<T>>,
     ) -> Result<GenDisk<T>> {
-        // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
-        let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
+        let mut lim: bindings::queue_limits = Zeroable::ZERO;
 
         lim.logical_block_size = self.logical_block_size;
         lim.physical_block_size = self.physical_block_size;
diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
index f9a1ca655a35..1ff7366ca549 100644
--- a/rust/kernel/block/mq/tag_set.rs
+++ b/rust/kernel/block/mq/tag_set.rs
@@ -10,6 +10,7 @@
     bindings,
     block::mq::{operations::OperationsVTable, request::RequestDataWrapper, Operations},
     error,
+    init::Zeroable,
     prelude::PinInit,
     try_pin_init,
     types::Opaque,
@@ -32,6 +33,10 @@ pub struct TagSet<T: Operations> {
     _p: PhantomData<T>,
 }
 
+// SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
+// all are allowed to be 0.
+unsafe impl Zeroable for bindings::blk_mq_tag_set {}
+
 impl<T: Operations> TagSet<T> {
     /// Try to create a new tag set
     pub fn new(
@@ -39,9 +44,6 @@ pub fn new(
         num_tags: u32,
         num_maps: u32,
     ) -> impl PinInit<Self, error::Error> {
-        // SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
-        // all are allowed to be 0.
-        let tag_set: bindings::blk_mq_tag_set = unsafe { core::mem::zeroed() };
         let tag_set = core::mem::size_of::<RequestDataWrapper>()
             .try_into()
             .map(|cmd_size| {
@@ -55,7 +57,7 @@ pub fn new(
                     flags: bindings::BLK_MQ_F_SHOULD_MERGE,
                     driver_data: core::ptr::null_mut::<core::ffi::c_void>(),
                     nr_maps: num_maps,
-                    ..tag_set
+                    ..Zeroable::ZERO
                 }
             });
 
-- 
2.47.0
Re: [PATCH 2/2] rust: block/mq: replace mem::zeroed() with Zeroable trait
Posted by Alice Ryhl 1 year, 2 months ago 
On Thu, Nov 28, 2024 at 3:13 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Isolate the unsafety in the declaration of the Zeroable trait, instead of having
> to use "unsafe" just to declare a struct.  This is more similar to how you would
> use "..Default::default()" (which is also a possibility here, but arguably
> less efficient).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/kernel/block/mq/gen_disk.rs |  8 +++++---
>  rust/kernel/block/mq/tag_set.rs  | 10 ++++++----
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index 708125dce96a..65342d065296 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -6,7 +6,7 @@
>  //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
>
>  use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
> -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
> +use crate::{bindings, error::from_err_ptr, error::Result, init::Zeroable, sync::Arc};
>  use crate::{error, static_lock_class};
>  use core::fmt::{self, Write};
>
> @@ -31,6 +31,9 @@ fn default() -> Self {
>      }
>  }
>
> +// SAFETY: `bindings::queue_limits` contains only fields that are valid when zeroed.
> +unsafe impl Zeroable for bindings::queue_limits {}
> +
>  impl GenDiskBuilder {
>      /// Create a new instance.
>      pub fn new() -> Self {
> @@ -93,8 +96,7 @@ pub fn build<T: Operations>(
>          name: fmt::Arguments<'_>,
>          tagset: Arc<TagSet<T>>,
>      ) -> Result<GenDisk<T>> {
> -        // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
> -        let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
> +        let mut lim: bindings::queue_limits = Zeroable::ZERO;
>
>          lim.logical_block_size = self.logical_block_size;
>          lim.physical_block_size = self.physical_block_size;
> diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
> index f9a1ca655a35..1ff7366ca549 100644
> --- a/rust/kernel/block/mq/tag_set.rs
> +++ b/rust/kernel/block/mq/tag_set.rs
> @@ -10,6 +10,7 @@
>      bindings,
>      block::mq::{operations::OperationsVTable, request::RequestDataWrapper, Operations},
>      error,
> +    init::Zeroable,
>      prelude::PinInit,
>      try_pin_init,
>      types::Opaque,
> @@ -32,6 +33,10 @@ pub struct TagSet<T: Operations> {
>      _p: PhantomData<T>,
>  }
>
> +// SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
> +// all are allowed to be 0.
> +unsafe impl Zeroable for bindings::blk_mq_tag_set {}

This will have to be reverted if we want to split up the kernel crate
due to the orphan rule.

Alice
Re: [PATCH 2/2] rust: block/mq: replace mem::zeroed() with Zeroable trait
Posted by Paolo Bonzini 1 year, 2 months ago 
On 11/29/24 10:41, Alice Ryhl wrote:
> On Thu, Nov 28, 2024 at 3:13 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Isolate the unsafety in the declaration of the Zeroable trait, instead of having
>> to use "unsafe" just to declare a struct.  This is more similar to how you would
>> use "..Default::default()" (which is also a possibility here, but arguably
>> less efficient).
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   rust/kernel/block/mq/gen_disk.rs |  8 +++++---
>>   rust/kernel/block/mq/tag_set.rs  | 10 ++++++----
>>   2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>> index 708125dce96a..65342d065296 100644
>> --- a/rust/kernel/block/mq/gen_disk.rs
>> +++ b/rust/kernel/block/mq/gen_disk.rs
>> @@ -6,7 +6,7 @@
>>   //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
>>
>>   use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
>> -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
>> +use crate::{bindings, error::from_err_ptr, error::Result, init::Zeroable, sync::Arc};
>>   use crate::{error, static_lock_class};
>>   use core::fmt::{self, Write};
>>
>> @@ -31,6 +31,9 @@ fn default() -> Self {
>>       }
>>   }
>>
>> +// SAFETY: `bindings::queue_limits` contains only fields that are valid when zeroed.
>> +unsafe impl Zeroable for bindings::queue_limits {}
>> +
>>   impl GenDiskBuilder {
>>       /// Create a new instance.
>>       pub fn new() -> Self {
>> @@ -93,8 +96,7 @@ pub fn build<T: Operations>(
>>           name: fmt::Arguments<'_>,
>>           tagset: Arc<TagSet<T>>,
>>       ) -> Result<GenDisk<T>> {
>> -        // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
>> -        let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>> +        let mut lim: bindings::queue_limits = Zeroable::ZERO;
>>
>>           lim.logical_block_size = self.logical_block_size;
>>           lim.physical_block_size = self.physical_block_size;
>> diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
>> index f9a1ca655a35..1ff7366ca549 100644
>> --- a/rust/kernel/block/mq/tag_set.rs
>> +++ b/rust/kernel/block/mq/tag_set.rs
>> @@ -10,6 +10,7 @@
>>       bindings,
>>       block::mq::{operations::OperationsVTable, request::RequestDataWrapper, Operations},
>>       error,
>> +    init::Zeroable,
>>       prelude::PinInit,
>>       try_pin_init,
>>       types::Opaque,
>> @@ -32,6 +33,10 @@ pub struct TagSet<T: Operations> {
>>       _p: PhantomData<T>,
>>   }
>>
>> +// SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
>> +// all are allowed to be 0.
>> +unsafe impl Zeroable for bindings::blk_mq_tag_set {}
> 
> This will have to be reverted if we want to split up the kernel crate
> due to the orphan rule.

Ok, I will look into using the bindgen doc comments then.  It looks like 
the idea is not dead on arrival at least!  Thanks for the review.

Paolo

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.