[PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

ISO C's `aligned_alloc` is partially implementation-defined; on some
systems it inherits stricter requirements from POSIX's `posix_memalign`.

This causes the call added in commit dd09538fb409 ("rust: alloc:
implement `Cmalloc` in module allocator_test") to fail on macOS because
it doesn't meet the requirements of `posix_memalign`.

Adjust the call to meet the POSIX requirement and add a comment. This fixes
failures in `make rusttest` on macOS.

Fixes: dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")

Acked-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Changes in v4:
- Revert to `aligned_alloc` and correct rationale. (Miguel Ojeda)
- Apply Danilo's Acked-by from v2.
- Rebase on v6.14-rc2.
- Link to v3: https://lore.kernel.org/r/20250206-aligned-alloc-v3-1-0cbc0ab0306d@gmail.com

Changes in v3:
- Replace `aligned_alloc` with `posix_memalign` for portability.
- Link to v2: https://lore.kernel.org/r/20250202-aligned-alloc-v2-1-5af0b5fdd46f@gmail.com

Changes in v2:
- Shorten some variable names. (Danilo Krummrich)
- Replace shadowing alignment variable with a second call to
  Layout::align. (Danilo Krummrich)
- Link to v1: https://lore.kernel.org/r/20250201-aligned-alloc-v1-1-c99a73f3cbd4@gmail.com
---
 rust/kernel/alloc/allocator_test.rs | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
index e3240d16040b..1c881ed73d79 100644
--- a/rust/kernel/alloc/allocator_test.rs
+++ b/rust/kernel/alloc/allocator_test.rs
@@ -62,9 +62,30 @@ unsafe fn realloc(
             ));
         }
 
+        // ISO C (ISO/IEC 9899:2011) defines `aligned_alloc`:
+        //
+        // > The value of alignment shall be a valid alignment supported by the implementation
+        // [...].
+        //
+        // As an example of the "supported by the implementation" requirement, POSIX.1-2001 (IEEE
+        // 1003.1-2001) defines `posix_memalign`:
+        //
+        // > The value of alignment shall be a power of two multiple of sizeof (void *).
+        //
+        // and POSIX-based implementations of `aligned_alloc` inherit this requirement. At the time
+        // of writing, this is known to be the case on macOS (but not in glibc).
+        //
+        // Satisfy the stricter requirement to avoid spurious test failures on some platforms.
+        let min_align = core::mem::size_of::<*const crate::ffi::c_void>();
+        let (align, size) = if layout.align() < min_align {
+            (min_align, layout.size().div_ceil(min_align) * min_align)
+        } else {
+            (layout.align(), layout.size())
+        };
+
         // SAFETY: Returns either NULL or a pointer to a memory allocation that satisfies or
         // exceeds the given size and alignment requirements.
-        let dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;
+        let dst = unsafe { libc_aligned_alloc(align, size) } as *mut u8;
         let dst = NonNull::new(dst).ok_or(AllocError)?;
 
         if flags.contains(__GFP_ZERO) {

---
base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20250201-aligned-alloc-b52cb2353c82

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Miguel Ojeda]

On Mon, Feb 10, 2025 at 3:55 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Fixes: dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
>

Hmm... The newline issue is still present in newer series you are
sending. Maintainers need to fix this manually, so please fix it on
your side.

> Acked-by: Danilo Krummrich <dakr@kernel.org>

I understand that you re-picked the v2 ack, since it is (at least the
code) similar again now, right?

Thanks!

Cheers,
Miguel

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Tue, Feb 11, 2025 at 10:18 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Feb 10, 2025 at 3:55 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Fixes: dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
> >
>
> Hmm... The newline issue is still present in newer series you are
> sending. Maintainers need to fix this manually, so please fix it on
> your side.

Drat, will do.

> > Acked-by: Danilo Krummrich <dakr@kernel.org>
>
> I understand that you re-picked the v2 ack, since it is (at least the
> code) similar again now, right?

Yep, I mentioned it under "Changes in v4".

>
> Thanks!
>
> Cheers,
> Miguel

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Gary Guo]

On Mon, 10 Feb 2025 09:55:19 -0500
Tamir Duberstein <tamird@gmail.com> wrote:

> ISO C's `aligned_alloc` is partially implementation-defined; on some
> systems it inherits stricter requirements from POSIX's `posix_memalign`.
> 
> This causes the call added in commit dd09538fb409 ("rust: alloc:
> implement `Cmalloc` in module allocator_test") to fail on macOS because
> it doesn't meet the requirements of `posix_memalign`.
> 
> Adjust the call to meet the POSIX requirement and add a comment. This fixes
> failures in `make rusttest` on macOS.
> 
> Fixes: dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
> Changes in v4:
> - Revert to `aligned_alloc` and correct rationale. (Miguel Ojeda)
> - Apply Danilo's Acked-by from v2.
> - Rebase on v6.14-rc2.
> - Link to v3: https://lore.kernel.org/r/20250206-aligned-alloc-v3-1-0cbc0ab0306d@gmail.com
> 
> Changes in v3:
> - Replace `aligned_alloc` with `posix_memalign` for portability.
> - Link to v2: https://lore.kernel.org/r/20250202-aligned-alloc-v2-1-5af0b5fdd46f@gmail.com
> 
> Changes in v2:
> - Shorten some variable names. (Danilo Krummrich)
> - Replace shadowing alignment variable with a second call to
>   Layout::align. (Danilo Krummrich)
> - Link to v1: https://lore.kernel.org/r/20250201-aligned-alloc-v1-1-c99a73f3cbd4@gmail.com
> ---
>  rust/kernel/alloc/allocator_test.rs | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> index e3240d16040b..1c881ed73d79 100644
> --- a/rust/kernel/alloc/allocator_test.rs
> +++ b/rust/kernel/alloc/allocator_test.rs
> @@ -62,9 +62,30 @@ unsafe fn realloc(
>              ));
>          }
>  
> +        // ISO C (ISO/IEC 9899:2011) defines `aligned_alloc`:
> +        //
> +        // > The value of alignment shall be a valid alignment supported by the implementation
> +        // [...].
> +        //
> +        // As an example of the "supported by the implementation" requirement, POSIX.1-2001 (IEEE
> +        // 1003.1-2001) defines `posix_memalign`:
> +        //
> +        // > The value of alignment shall be a power of two multiple of sizeof (void *).
> +        //
> +        // and POSIX-based implementations of `aligned_alloc` inherit this requirement. At the time
> +        // of writing, this is known to be the case on macOS (but not in glibc).
> +        //
> +        // Satisfy the stricter requirement to avoid spurious test failures on some platforms.
> +        let min_align = core::mem::size_of::<*const crate::ffi::c_void>();
> +        let (align, size) = if layout.align() < min_align {
> +            (min_align, layout.size().div_ceil(min_align) * min_align)
> +        } else {
> +            (layout.align(), layout.size())
> +        };

I think this can be more concisely expressed as

	let layout = layout.align_to(min_align)?.pad_to_align();

Best,
Gary

> +
>          // SAFETY: Returns either NULL or a pointer to a memory allocation that satisfies or
>          // exceeds the given size and alignment requirements.
> -        let dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;
> +        let dst = unsafe { libc_aligned_alloc(align, size) } as *mut u8;
>          let dst = NonNull::new(dst).ok_or(AllocError)?;
>  
>          if flags.contains(__GFP_ZERO) {
> 
> ---
> base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
> change-id: 20250201-aligned-alloc-b52cb2353c82
> 
> Best regards,

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Tue, Feb 11, 2025 at 11:52 AM Gary Guo <gary@garyguo.net> wrote:
>
> On Mon, 10 Feb 2025 09:55:19 -0500
> Tamir Duberstein <tamird@gmail.com> wrote:
>
> > ISO C's `aligned_alloc` is partially implementation-defined; on some
> > systems it inherits stricter requirements from POSIX's `posix_memalign`.
> >
> > This causes the call added in commit dd09538fb409 ("rust: alloc:
> > implement `Cmalloc` in module allocator_test") to fail on macOS because
> > it doesn't meet the requirements of `posix_memalign`.
> >
> > Adjust the call to meet the POSIX requirement and add a comment. This fixes
> > failures in `make rusttest` on macOS.
> >
> > Fixes: dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
> >
> > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> > Changes in v4:
> > - Revert to `aligned_alloc` and correct rationale. (Miguel Ojeda)
> > - Apply Danilo's Acked-by from v2.
> > - Rebase on v6.14-rc2.
> > - Link to v3: https://lore.kernel.org/r/20250206-aligned-alloc-v3-1-0cbc0ab0306d@gmail.com
> >
> > Changes in v3:
> > - Replace `aligned_alloc` with `posix_memalign` for portability.
> > - Link to v2: https://lore.kernel.org/r/20250202-aligned-alloc-v2-1-5af0b5fdd46f@gmail.com
> >
> > Changes in v2:
> > - Shorten some variable names. (Danilo Krummrich)
> > - Replace shadowing alignment variable with a second call to
> >   Layout::align. (Danilo Krummrich)
> > - Link to v1: https://lore.kernel.org/r/20250201-aligned-alloc-v1-1-c99a73f3cbd4@gmail.com
> > ---
> >  rust/kernel/alloc/allocator_test.rs | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > index e3240d16040b..1c881ed73d79 100644
> > --- a/rust/kernel/alloc/allocator_test.rs
> > +++ b/rust/kernel/alloc/allocator_test.rs
> > @@ -62,9 +62,30 @@ unsafe fn realloc(
> >              ));
> >          }
> >
> > +        // ISO C (ISO/IEC 9899:2011) defines `aligned_alloc`:
> > +        //
> > +        // > The value of alignment shall be a valid alignment supported by the implementation
> > +        // [...].
> > +        //
> > +        // As an example of the "supported by the implementation" requirement, POSIX.1-2001 (IEEE
> > +        // 1003.1-2001) defines `posix_memalign`:
> > +        //
> > +        // > The value of alignment shall be a power of two multiple of sizeof (void *).
> > +        //
> > +        // and POSIX-based implementations of `aligned_alloc` inherit this requirement. At the time
> > +        // of writing, this is known to be the case on macOS (but not in glibc).
> > +        //
> > +        // Satisfy the stricter requirement to avoid spurious test failures on some platforms.
> > +        let min_align = core::mem::size_of::<*const crate::ffi::c_void>();
> > +        let (align, size) = if layout.align() < min_align {
> > +            (min_align, layout.size().div_ceil(min_align) * min_align)
> > +        } else {
> > +            (layout.align(), layout.size())
> > +        };
>
> I think this can be more concisely expressed as
>
>         let layout = layout.align_to(min_align)?.pad_to_align();

Yep this works. Thanks Gary.

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Miguel Ojeda]

On Tue, Feb 11, 2025 at 4:21 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Drat, will do.

Thanks!

> Yep, I mentioned it under "Changes in v4".

I meant to confirm the reasoning -- it is all good, thanks!
(personally I would probably have dropped it in a case like this,
since the change in comments is substantial and Danilo was waiting for
the clarification from Alejandro etc.).

Cheers,
Miguel

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Tue, Feb 11, 2025 at 4:46 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 4:21 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Drat, will do.
>
> Thanks!
>
> > Yep, I mentioned it under "Changes in v4".
>
> I meant to confirm the reasoning -- it is all good, thanks!
> (personally I would probably have dropped it in a case like this,
> since the change in comments is substantial and Danilo was waiting for
> the clarification from Alejandro etc.).
>
> Cheers,
> Miguel

Got it! I've dropped it from v5 in which I've used Gary's pithier
formulation. I'd like to get an acknowledgement that the commit
message and code comment make sense before sending it.

It'd be great to get this into 6.14 so that local builds are clean for
the next cycle.

Cheers.

Tamir

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Danilo Krummrich]

On Tue, Feb 11, 2025 at 10:46:14PM +0100, Miguel Ojeda wrote:
> On Tue, Feb 11, 2025 at 4:21 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Drat, will do.
> 
> Thanks!
> 
> > Yep, I mentioned it under "Changes in v4".
> 
> I meant to confirm the reasoning -- it is all good, thanks!
> (personally I would probably have dropped it in a case like this,
> since the change in comments is substantial and Danilo was waiting for
> the clarification from Alejandro etc.).

Agree with Miguel, better to drop it in such cases.

But no worries, Tamir. It was still valid in this case, which is why I did not
complain. :)

Also feel free to keep it for v5, moving to Gary's simplification.

- Danilo

Re: [PATCH v4] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Tue, Feb 11, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Agree with Miguel, better to drop it in such cases.
>
> But no worries, Tamir. It was still valid in this case, which is why I did not
> complain. :)
>
> Also feel free to keep it for v5, moving to Gary's simplification.
>
> - Danilo

🫡

Thanks!