From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id E33A2C87FD4 for ; Thu, 29 Aug 2024 19:14:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5F9A36B00A0; Thu, 29 Aug 2024 15:14:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 581FB6B00A4; Thu, 29 Aug 2024 15:14:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3AEF96B00AD; Thu, 29 Aug 2024 15:14:30 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 1A1846B00A0 for ; Thu, 29 Aug 2024 15:14:30 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id B6125160B25 for ; Thu, 29 Aug 2024 19:14:29 +0000 (UTC) X-FDA: 82506234258.09.992BC4A Received: from mail-4322.protonmail.ch (mail-4322.protonmail.ch [185.70.43.22]) by imf13.hostedemail.com (Postfix) with ESMTP id BA7812001D for ; Thu, 29 Aug 2024 19:14:27 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b="kPZBJh/6"; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf13.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.22 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724958802; a=rsa-sha256; cv=none; b=Xm4h6+mI9075P1siF9SE6GCW0gghfpWYDP5zlUszcIGUkDvUyTzJ9j+lB7oT0LnUDB3sD1 /YFLqwiciu7uAmeefp0UJSTMYxTjvx5O1jtTvbfjNFubkRIgz/BhtUJO1c/Ae88k09bbYG 3PJQ8kTvYOIWU1aDSzLmEIzLiQAuon0= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b="kPZBJh/6"; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf13.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.22 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724958802; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=rSQRReVdO27pYSz1tbezXuKTcJpVLS4AVUyjhTuHuus=; b=hoIbG+KveJ9KjPW0gbonz5LZrr1i/Xy0rvsIvgWONd3qpFa4ovSGJXOn3y+y4apdKPLZKa 3bcN5aA/3NvA5jl4ltK3/ORF9PD6VwInYvubsDMXDKCeGGLQnKeSn9/IXEwBAs+j4tlVKv 3njyvH2AZrmNwUh+L6SBctwOEcLpRb4= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1724958865; x=1725218065; bh=rSQRReVdO27pYSz1tbezXuKTcJpVLS4AVUyjhTuHuus=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=kPZBJh/6H2IHEAQ6MN8+RGFc2MfCDdvbwUwuxa7PvV0eaC7ZqNUwHpDnTF9UntnB3 pu2XgmSH6R193/rjim90MS2PW0Ba3z2rYnaAyvEp9Jl2JQ+5xtqbsguuhrSfrr4sQ8 2aj4gl1DDRV9TqpPT0gdAro2OVAbjeL1yjKRfe9xLm8Sr3ulnAdoS41GhOvDgfRDNV 6hrigPzynqBMHu0Qz3g7G7Qm0LCCJlVDSE8oV7xVdQ+GAVLAIh7u0OAFiVtMFL1FuI G0VchCfDEQXfU+U/2TYFyKXvZ5hLU3mMQ5FaVHbE0b+fwfQEYWIWl6Loru5m+8bCir kbyV1URwnUVoA== Date: Thu, 29 Aug 2024 19:14:18 +0000 To: Danilo Krummrich , ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, a.hindborg@samsung.com, aliceryhl@google.com, akpm@linux-foundation.org From: Benno Lossin Cc: daniel.almeida@collabora.com, faith.ekstrand@collabora.com, boris.brezillon@collabora.com, lina@asahilina.net, mcanal@igalia.com, zhiw@nvidia.com, cjia@nvidia.com, jhubbard@nvidia.com, airlied@redhat.com, ajanulgu@redhat.com, lyude@redhat.com, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v6 22/26] rust: alloc: implement `Cmalloc` in module allocator_test Message-ID: In-Reply-To: <20240816001216.26575-23-dakr@kernel.org> References: <20240816001216.26575-1-dakr@kernel.org> <20240816001216.26575-23-dakr@kernel.org> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: 8e16d31e33690d1f27de194ee7a7143192184df7 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: BA7812001D X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: ru9wmzwpipwhbbu5z6drzdre9bqz34y1 X-HE-Tag: 1724958867-144807 X-HE-Meta: U2FsdGVkX1+EPaV20sJC4yO3RP4gmVJZbOaL5uBtBPzIVgCoClCVRvww05AzVca99mQrp4h0CWWaCYZ5bJNmw/pvxRpoLQAZW+BOLiKHgz5KU8QH5Vuxqk00OVye7Civ09tTvG9XkpquXMjaog5k5nKel65VaWLP+L+qpeRn32eBSXNDVCwfzRmNukFuDUdFgGGzfBd56YJDDFo178XezrW1I/bFA2jkUhvJWaPuWMJMOkoAI3obJT4X/PN3eQA2BUlDUnTgxpI8JCTVm5T4JXLfAlSAU4VXQSYQ3wOMqHIsSRnK3XHYqG6AkvNH0LngSJcwxxiFJ9D4hsh5e80puocTeA+6tJ7SOVRD6LVLhxq04X5NvqSrSh2Kw6b7q/QIndUS2m4EV7x+rVmEc2B+iMlvEc6Px8mcEG51iiDn39Cf8Im8ZM9DTYvpAJLC79fI+k17y4lxiEH3gKBIMhsaDXrg+JkcgG8yk+g8MF0hjfqCQAEIX5EGvTIIdZ48rYjDSZQwP+r6GRQUSVo9WX4wQLh7Hi/y4eYV5MNl04DueLtGf0qL1UNCwznxD3f9cM3t7ILOofYe1GajcbmyZ3qZ0BMzN962o5MiWGB+ySNVueo/5gpP8e4yqkCOGKrD3aXW5/l5xvUu6dxgYs899rnI27JMj0aOruOS9S8oymk7dJWfn0Zb5roIpS8C/m0aaMpNqD91tLXOQcklMaChK/OSgY5hC6R19Js5q7syD7KHEAkNb9YZzIc5H0jNj3cpw8Op+ximxXqHxKBEwUjHzz6eH382sM2k99STKE7iMDBXXSOK9A+fJyfgTaQLm0ITI9voLmflHuahdp/LfiIJkkyVDCDERbbvpBXJLk8E3u43YQKZ7TFtY8I7kHAhvnYn6OmV/Xo+r6lqLw4uiaYfZJWneNpe5wunpUb4Jb8AJG8F9GYKLPVuLE0vEceIJPsNdKHfcLwSCmZTN+tZAj6bT6f 5el7+x8w rWWpb2w9JRMJNvlPEzo9FN7YpvmkEf/MNcaQRPxIWEVSZWOhydggw2eJq/gcxYAdlDM+Dka9RuyG/Z6x8tZR7JiUIIur/vdWLM3h1uE3ieTg/R4IjjPWN2bItIwVxYBaJmUiqfjciONID8uLAcG2Mdm5bfLseCxcw8tPDoaoY+yiXICkxl+JJu6y6YkTaXYKQjFrVegbJBvNqypaIbjTRrsln74T0OXuFjQsz4599/7p7RGjJktm1/oXQD3Yx9aaaQwdDVwnTmQkeRn94cQAXxL0cyn2ZWhWYjdCXl3jFbGtp+NiWTXKEbs25D6sI4uiIUBg8kOvbjV+LjIo87X94ig/ZQxs67vg4MyETkIbwlPzbp7UV7EU1pwJ/BqbR5OX1D4EqmSKuo/ZdvGpG+Vf1hXp8oF6t/Q241qqdfExeBlIm7K7dqGDrbn7XdY89e3J4p1PA87+d6f639+WCwOBPss+7kw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 16.08.24 02:11, Danilo Krummrich wrote: > So far the kernel's `Box` and `Vec` types can't be used by userspace > test cases, since all users of those types (e.g. `CString`) use kernel > allocators for instantiation. >=20 > In order to allow userspace test cases to make use of such types as > well, implement the `Cmalloc` allocator within the allocator_test module > and type alias all kernel allocators to `Cmalloc`. The `Cmalloc` > allocator uses libc's realloc() function as allocator backend. >=20 > Signed-off-by: Danilo Krummrich > --- > rust/kernel/alloc/allocator_test.rs | 178 ++++++++++++++++++++++++++-- > 1 file changed, 171 insertions(+), 7 deletions(-) >=20 > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allo= cator_test.rs > index 1b2642c547ec..7fff308d02dc 100644 > --- a/rust/kernel/alloc/allocator_test.rs > +++ b/rust/kernel/alloc/allocator_test.rs > @@ -2,20 +2,184 @@ >=20 Could add a short paragraph as the module description why this module exists? Would probably be enough to paste the commit message. > #![allow(missing_docs)] >=20 > -use super::{AllocError, Allocator, Flags}; > +use super::{flags::*, AllocError, Allocator, Flags}; > use core::alloc::Layout; > +use core::cmp; > +use core::mem; > +use core::ptr; > use core::ptr::NonNull; >=20 > -pub struct Kmalloc; > +pub struct Cmalloc; > +pub type Kmalloc =3D Cmalloc; > pub type Vmalloc =3D Kmalloc; > pub type KVmalloc =3D Kmalloc; >=20 > -unsafe impl Allocator for Kmalloc { > +extern "C" { > + #[link_name =3D "aligned_alloc"] > + fn libc_aligned_alloc(align: usize, size: usize) -> *mut core::ffi::= c_void; > + > + #[link_name =3D "free"] > + fn libc_free(ptr: *mut core::ffi::c_void); > +} > + > +struct CmallocData { > + // The actual size as requested through `Cmalloc::alloc` or `Cmalloc= ::realloc`. > + size: usize, > + // The offset from the pointer returned to the caller of `Cmalloc::a= lloc` or `Cmalloc::realloc` > + // to the actual base address of the allocation. > + offset: usize, > +} > + > +impl Cmalloc { > + /// Adjust the size and alignment such that we can additionally stor= e `CmallocData` right > + /// before the actual data described by `layout`. > + /// > + /// Example: > + /// > + /// For `CmallocData` assume an alignment of 8 and a size of 16. > + /// For `layout` assume and alignment of 16 and a size of 64. This looks like you want it rendered as bulletpoints (but it won't). > + /// > + /// 0 16 32 = 96 > + /// |----------------|----------------|-----------------------------= -------------------| > + /// empty CmallocData data Can you put this inside of '```'? Then it will render nicely in markdown (don't forget to specify the type 'text') > + /// > + /// For this example the returned `Layout` has an alignment of 32 an= d a size of 96. > + fn layout_adjust(layout: Layout) -> Result { > + let layout =3D layout.pad_to_align(); > + > + // Ensure that `CmallocData` fits into half the alignment. Addit= ionally, this guarantees > + // that advancing a pointer aligned to `align` by `align / 2` we= still satisfy or exceed > + // the alignment requested through `layout`. > + let align =3D cmp::max( > + layout.align(), > + mem::size_of::().next_power_of_two(), > + ) * 2; > + > + // Add the additional space required for `CmallocData`. > + let size =3D layout.size() + mem::size_of::(); > + > + Ok(Layout::from_size_align(size, align) > + .map_err(|_| AllocError)? > + .pad_to_align()) > + } > + > + fn alloc_store_data(layout: Layout) -> Result, AllocErro= r> { > + let requested_size =3D layout.size(); > + > + let layout =3D Self::layout_adjust(layout)?; > + let min_align =3D layout.align() / 2; > + > + // SAFETY: Returns either NULL or a pointer to a memory allocati= on that satisfies or > + // exceeds the given size and alignment requirements. > + let raw_ptr =3D unsafe { libc_aligned_alloc(layout.align(), layo= ut.size()) } as *mut u8; > + > + let priv_ptr =3D NonNull::new(raw_ptr).ok_or(AllocError)?; > + > + // SAFETY: Advance the pointer by `min_align`. The adjustments f= rom `Self::layout_adjust` > + // ensure that after this operation the original size and alignm= ent requirements are still > + // satisfied or exceeded. This SAFETY comment should address why it's OK to call `add`. You justify something different, namely why the allocation still satisfies the requirements of `layout`. That is something that this function should probably guarantee. > + let ptr =3D unsafe { priv_ptr.as_ptr().add(min_align) }; > + > + // SAFETY: `min_align` is greater than or equal to the size of `= CmallocData`, hence we > + // don't exceed the allocation boundaries. > + let data_ptr: *mut CmallocData =3D unsafe { ptr.sub(mem::size_of= ::()) }.cast(); > + > + let data =3D CmallocData { > + size: requested_size, > + offset: min_align, > + }; > + > + // SAFETY: `data_ptr` is properly aligned and within the allocat= ion boundaries reserved for > + // `CmallocData`. > + unsafe { data_ptr.write(data) }; > + > + NonNull::new(ptr).ok_or(AllocError) > + } > + > + /// # Safety > + /// > + /// `ptr` must have been previously allocated with `Self::alloc_stor= e_data`. You additionally need that you have shared access to the pointee. > + unsafe fn data<'a>(ptr: NonNull) -> &'a CmallocData { > + // SAFETY: `Self::alloc_store_data` stores the `CmallocData` rig= ht before the address > + // returned to callers of `Self::alloc_store_data`. > + let data_ptr: *mut CmallocData =3D > + unsafe { ptr.as_ptr().sub(mem::size_of::()) }.c= ast(); > + > + // SAFETY: The `CmallocData` has been previously stored at this = offset with > + // `Self::alloc_store_data`. > + unsafe { &*data_ptr } > + } > + > + /// # Safety > + /// > + /// This function must not be called more than once for the same all= ocation. > + /// > + /// `ptr` must have been previously allocated with `Self::alloc_stor= e_data`. You additionally need that you have exclusive access to the pointee. > + unsafe fn free_read_data(ptr: NonNull) { > + // SAFETY: `ptr` has been created by `Self::alloc_store_data`. > + let data =3D unsafe { Self::data(ptr) }; > + > + // SAFETY: `ptr` has been created by `Self::alloc_store_data`. > + let priv_ptr =3D unsafe { ptr.as_ptr().sub(data.offset) }; > + > + // SAFETY: `priv_ptr` has previously been allocatored with this = `Allocator`. > + unsafe { libc_free(priv_ptr.cast()) }; > + } > +} > + > +unsafe impl Allocator for Cmalloc { > + fn alloc(layout: Layout, flags: Flags) -> Result, Allo= cError> { > + if layout.size() =3D=3D 0 { > + return Ok(NonNull::slice_from_raw_parts(NonNull::dangling(),= 0)); > + } > + > + let ptr =3D Self::alloc_store_data(layout)?; > + > + if flags.contains(__GFP_ZERO) { > + // SAFETY: `Self::alloc_store_data` guarantees that `ptr` po= ints to memory of at least > + // `layout.size()` bytes. > + unsafe { ptr.as_ptr().write_bytes(0, layout.size()) }; > + } This makes me wonder, what other flags should we handle for this allocator? > + > + Ok(NonNull::slice_from_raw_parts(ptr, layout.size())) > + } > + > unsafe fn realloc( > - _ptr: Option>, > - _layout: Layout, > - _flags: Flags, > + ptr: Option>, > + layout: Layout, > + flags: Flags, > ) -> Result, AllocError> { > - panic!(); > + let src: NonNull =3D if let Some(src) =3D ptr { > + src.cast() Why the cast? > + } else { > + return Self::alloc(layout, flags); > + }; You should be able to write this instead: let Some(src) =3D ptr else { return Self::alloc(layout, flags); }; > + > + if layout.size() =3D=3D 0 { > + // SAFETY: `src` has been created by `Self::alloc_store_data= `. This is not true, consider: let ptr =3D alloc(size =3D 0); free(ptr) Alloc will return a dangling pointer due to the first if statement and then this function will pass it to `free_read_data`, even though it wasn't created by `alloc_store_data`. This isn't forbidden by the `Allocator` trait function's safety requirements. > + unsafe { Self::free_read_data(src) }; > + > + return Ok(NonNull::slice_from_raw_parts(NonNull::dangling(),= 0)); > + } > + > + let dst =3D Self::alloc(layout, flags)?; > + > + // SAFETY: `src` has been created by `Self::alloc_store_data`. > + let data =3D unsafe { Self::data(src) }; Same issue here, if the allocation passed in is zero size. I think you have no other choice than to allocate even for zero size requests... Otherwise how would you know that they are zero-sized. --- Cheers, Benno > + > + // SAFETY: `src` has previously been allocated with this `Alloca= tor`; `dst` has just been > + // newly allocated. Copy up to the smaller of both sizes. > + unsafe { > + ptr::copy_nonoverlapping( > + src.as_ptr(), > + dst.as_ptr().cast(), > + cmp::min(layout.size(), data.size), > + ) > + }; > + > + // SAFETY: `src` has been created by `Self::alloc_store_data`. > + unsafe { Self::free_read_data(src) }; > + > + Ok(dst) > } > } > -- > 2.46.0 >=20