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 DA0FCC7EE30 for ; Thu, 26 Jun 2025 17:58:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4EE6F6B00AD; Thu, 26 Jun 2025 13:58:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 49F6C6B00AE; Thu, 26 Jun 2025 13:58:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3DBB36B00B0; Thu, 26 Jun 2025 13:58:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 2FC616B00AD for ; Thu, 26 Jun 2025 13:58:36 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 70C03140373 for ; Thu, 26 Jun 2025 17:58:35 +0000 (UTC) X-FDA: 83598311790.12.9AEAC28 Received: from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31]) by imf25.hostedemail.com (Postfix) with ESMTP id 8439CA0003 for ; Thu, 26 Jun 2025 17:58:33 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Q6ZuqU0A; spf=pass (imf25.hostedemail.com: domain of dakr@kernel.org designates 172.234.252.31 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1750960713; 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=RGsNtArzckkmZWTogdo2lXiUgLWlrX+HZQN+6jomdHg=; b=ZyZum2iJobA4i4Ay0EOe1by8tfBkDjcor1N70c5kpkzfbQtZoeZsL9tyLEEDeuNy2yjyvC EjkiW8sGDsoQTIdyIJASN3MHRmjc+WuM+ggiJOsc8bPV892d0SZuC+/zSNnTRby0evvikB mN6qtIL6APkHdwbcfyz/1OZ/nj/WsSw= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1750960713; a=rsa-sha256; cv=none; b=CaBtawYFGMHXMkoMOJ8cs2DpAdSJ6bOb9Tk1Jczda627YzYzSSo3PGKR/wSudfKTDqaLUq qWivuR5OLxqwxZMAZcy9EOaH8NcH6PoX9cye3o8rfi9brQ3/p/LHMDQpcjTwbP+8KIR+1B BA5uiMe4P5TglCT25i/W5E2HbVYj/jQ= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Q6ZuqU0A; spf=pass (imf25.hostedemail.com: domain of dakr@kernel.org designates 172.234.252.31 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id 2AE9343BC4; Thu, 26 Jun 2025 17:58:32 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 736EDC4CEEB; Thu, 26 Jun 2025 17:58:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1750960712; bh=9kSgYJslI9669W5ZX/ZizxoETZvyBzl6RRzfWIJiGB8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Q6ZuqU0AhMSVruGouu0fykiZUjYFBqnqPMY7yrwYikDukfBzc6r1l9K4ybVUugEIN 6CvVcyh5euPloItJxGwUVtgghYi17OJYPIIgNrzx/N8tn9j6LNdGbCcLyY+GgEmDdy OC4VCDAOEaYpGxASwwKQSUnVuJXTQo/rjLzqaWWoOkV23OemTpHxO7qLvUH+9s4dhW gntSwkv9pHypP0O8HI6M749L3HKZ5v+mIxFxRnhIdMl5Tf+9MjSnWaMnmOy5sw7F+F UdkwFEyvXl+MKPLGnU6pkFIINh1A6qYQtrtj6mhmapCcOuz0prj3v2AI+l25LFGqJ0 SeG2/nMYA0bnA== Date: Thu, 26 Jun 2025 19:58:27 +0200 From: Danilo Krummrich To: Vitaly Wool Cc: linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Uladzislau Rezki , Alice Ryhl , rust-for-linux@vger.kernel.org Subject: Re: [PATCH v4 3/4] rust: support large alignments in allocations Message-ID: References: <20250626083516.3596197-1-vitaly.wool@konsulko.se> <20250626083642.3596388-1-vitaly.wool@konsulko.se> <72365C10-D984-4BC5-A081-B058C1619D06@konsulko.se> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <72365C10-D984-4BC5-A081-B058C1619D06@konsulko.se> X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 8439CA0003 X-Stat-Signature: udh9cznci5eo6z3kcrcb7akeeaoxe6w4 X-Rspam-User: X-HE-Tag: 1750960713-995331 X-HE-Meta: U2FsdGVkX1+XSWFCsIgm2wN3gdptZcGHIvzKR/zx7NQAS/PivMnfA7SyjrPvTCplowaXeoSRVs8tEafpxDpRKiPOrHJZ3vWmK6ZYoID0ybkLlhDe7TI4G4j09y0eM/PQ+yVKu7wxmx6r5ZXAdAiQfOyw2/gpajrgxFmpm2w65ETSia/ps5rZH0rDKeGHghPTJxL4Zeh+t06YiKDu4Tm9u+Xwnp7f/Xh5oOpJZMGnAE+SF2UUyAAhl0x0nZWTJNJHsnbLioQK+hG8uMU+cO5B8lew36C/qDjlG0QYtYV/qPOKH9H4ln9WrGvF4JmaKOKOskXLseQR5FPkz2TwnqPs0SdiU5Tz0Wuw8x/0X+KbuKeV1BLO6ap5ce95Lce9YHOdN2x4+FPgZPJSxTGXwNJPOMFqe/shMc+tKxVofsHNVI8Ywh47mDs0S5ZrcHx6f49M6M7I/qBPbhHl8ya0/j+ahilgThA7t1+j+8NSsWyp0n9pNWZYjBIws84rNy/mPv4i0uG0Mz18zLYJ2AdpmAWgai6DE1chazq/nMrH5lLAkzOQZFxvL1K4q6rx6lHSttEWxJtWkP7aHlpclRHBcm2yd3yMzw3SXZ5IEvbJdm5FCn8A/AFcbLPRXhQywBYeLacJCr0NQpqEdIFQe49/Bxq+B0csTF6qDZvmtQHf5RrFiPSV3Fgs+u5tRnwFy3ouFH2LsBc/Nd0Q8JduMwjYJqIyfCiC4F+YUjwrfA/HobvM6L/d8VK3GbNuRu1Y1fTV46SeGgi4mAN7qzCEwDIlILtbykCuz9xyk5uIgbqPXygPA0lDaegE6L+GLGxzECoklnTwqBjGP+EhrGGzRNFbGufIGJlJ8U37g0IGrVr/yYOZPm9DnnIE79b+YcnJOmfDqu95OztRY3ttXo+vK0+kreKjw+btjs56m6GLh6vTAZZdYFMOROOgPdGc0Z9zJflybYaFr4ql7YuRlONNMA2Dk0H ENZAYk2D vP5R9bPLRRYiA5SpGctrl8spykUJsINVJeL3RxQ7egEKcLS3/gVL2bA1ctTonuYk4LklMKVpwm/Uz5Oswt6rD4trza1EX8uJG/ZDTH7cCDKCY3u1Y7S6XG688YI7uRl1N1VRhlD7ukSTZFHHDdJDjhUmIvLZEprtTYdxtJ5zh1LBgnknNHBpFoMXPSMv27PHiL028ZkcLsN7QUblDoh8naBd+5VWQBXkukauleCXeKudIq3qpyUJjS3bBKxGJiAt+b9XeQXOrUZrUNKUFIy8dpzL9jwcK+CnqF7VgMDUyQoHJSQI9ynfn5K4/QGFgesC6KOjtsMdDdYg845E= 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 Thu, Jun 26, 2025 at 06:29:24PM +0200, Vitaly Wool wrote: > > > > On Jun 26, 2025, at 2:36 PM, Danilo Krummrich wrote: > > > > On Thu, Jun 26, 2025 at 10:36:42AM +0200, Vitaly Wool wrote: > >> void * __must_check __realloc_size(2) > >> -rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags) > >> +rust_helper_krealloc(const void *objp, size_t new_size, unsigned long align, gfp_t flags) > >> { > >> return krealloc(objp, new_size, flags); > >> } > >> > >> void * __must_check __realloc_size(2) > >> -rust_helper_kvrealloc(const void *p, size_t size, gfp_t flags) > >> +rust_helper_kvrealloc(const void *p, size_t size, unsigned long align, gfp_t flags) > >> { > >> return kvrealloc(p, size, flags); > >> } > > > > I think you forgot to add comments explaining why we have the additional > > discarded align argument. > > > > Also please keep those helpers as they are. You can write an identical inline > > function in Rust that discards the align argument and calls bindings::krealloc, > > etc. > > > > For instance: > > > > unsafe extern "C" fn krealloc_align( > > ptr: *const c_void, > > size: usize, > > _align: c_ulong > > flags: u32, > > ) -> *mut c_void { > > bindings::krealloc(ptr, size, flags) > > } > > > > Ugh. This is indeed a mistake from my side but I don’t quite agree with your variant here too. > The thing is that the new patchset has a patch #2 which adds kvrealloc_node and realloc_node so this chunk IMO should have looked like > > -rust_helper_kvrealloc(const void *p, size_t size, gfp_t flags) > +rust_helper_kvrealloc(const void *p, size_t size, unsigned long align, gfp_t flags) > { > - return kvrealloc(p, size, flags); > + return kvrealloc_node(p, size, align, flags, NUMA_NO_NODE); > > } No for two reasons: 1) Rust helpers are transparent wrappers for C functions / macros slipping through bindgen. We don't add any logic to them, as you do here. 2) This patch is only about supporting large alignments for VMALLOC. There's no need to introduce kvrealloc_node() (yet). The only thing you want here is to keep the signature common between all realloc functions. Hence, you want unsafe extern "C" fn krealloc_align( ptr: *const c_void, size: usize, _align: c_ulong flags: u32, ) -> *mut c_void { bindings::krealloc(ptr, size, flags) } on the Rust side of things. And in the next patch you want unsafe extern "C" fn krealloc_node_align( ptr: *const c_void, size: usize, _align: c_ulong flags: u32, c_int: nid, ) -> *mut c_void { bindings::krealloc_node(ptr, size, flags, nid) } > …exactly like for vmalloc, see also my comment below. > > >> diff --git a/rust/helpers/vmalloc.c b/rust/helpers/vmalloc.c > >> index 80d34501bbc0..4618c0b79283 100644 > >> --- a/rust/helpers/vmalloc.c > >> +++ b/rust/helpers/vmalloc.c > >> @@ -3,7 +3,7 @@ > >> #include > >> > >> void * __must_check __realloc_size(2) > >> -rust_helper_vrealloc(const void *p, size_t size, gfp_t flags) > >> +rust_helper_vrealloc(const void *p, size_t size, unsigned long align, gfp_t flags) > >> { > >> - return vrealloc(p, size, flags); > >> + return vrealloc_node(p, size, align, flags, NUMA_NO_NODE); > >> } > > > > Same here, just make this a "real" helper for vrealloc_node() and create a Rust > > function vrealloc_align() like in the example above. > > Wait, why? What’s the use of vrealloc() if it doesn’t provide the align functionality that we need? That's fine, then this should be void * __must_check __realloc_size(2) rust_helper_vrealloc_node(const void *p, size_t size, unsigned long align, gfp_t flags, int nid) { return vrealloc_node(p, size, align, flags, nid); } and on the Rust side, for this patch, you want: unsafe extern "C" fn vrealloc_align( ptr: *const c_void, size: usize, align: c_ulong flags: u32, c_int: nid, ) -> *mut c_void { bindings::vrealloc_node(ptr, size, align, flags, bindings::NUMA_NO_NODE) } The diff between the patches may come out nicer if you do it the other way around though, i.e. first support node IDs and then support larger alignments than PAGE_SIZE for VMALLOC. > > > >> diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs > >> index aa2dfa9dca4c..a0d78c497974 100644 > >> --- a/rust/kernel/alloc/allocator.rs > >> +++ b/rust/kernel/alloc/allocator.rs > >> @@ -58,7 +58,7 @@ fn aligned_size(new_layout: Layout) -> usize { > >> /// > >> /// One of the following: `krealloc`, `vrealloc`, `kvrealloc`. > >> struct ReallocFunc( > >> - unsafe extern "C" fn(*const crate::ffi::c_void, usize, u32) -> *mut crate::ffi::c_void, > >> + unsafe extern "C" fn(*const crate::ffi::c_void, usize, usize, u32) -> *mut crate::ffi::c_void, > > > > Should be c_ulong instead of usize. > > > > Noted. > > >> ); > >> > >> impl ReallocFunc { > >> @@ -110,7 +110,7 @@ unsafe fn call( > >> // - Those functions provide the guarantees of this function. > >> let raw_ptr = unsafe { > >> // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed. > >> - self.0(ptr.cast(), size, flags.0).cast() > >> + self.0(ptr.cast(), size, layout.align(), flags.0).cast() > >> }; > >> > >> let ptr = if size == 0 { > >> @@ -152,12 +152,6 @@ unsafe fn realloc( > >> old_layout: Layout, > >> flags: Flags, > >> ) -> Result, AllocError> { > >> - // TODO: Support alignments larger than PAGE_SIZE. > >> - if layout.align() > bindings::PAGE_SIZE { > >> - pr_warn!("Vmalloc does not support alignments larger than PAGE_SIZE yet.\n"); > >> - return Err(AllocError); > >> - } > >> - > >> // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously > >> // allocated with this `Allocator`. > >> unsafe { ReallocFunc::VREALLOC.call(ptr, layout, old_layout, flags) } > >> @@ -176,12 +170,6 @@ unsafe fn realloc( > >> old_layout: Layout, > >> flags: Flags, > >> ) -> Result, AllocError> { > >> - // TODO: Support alignments larger than PAGE_SIZE. > >> - if layout.align() > bindings::PAGE_SIZE { > >> - pr_warn!("KVmalloc does not support alignments larger than PAGE_SIZE yet.\n"); > >> - return Err(AllocError); > >> - } > > > > Didn't you propose to use VREALLOC if layout.align() > bindings::PAGE_SIZE? > > > > I did, and this is what happens on the C side now, please see the #2 patch in series. I'm fine doing it on the C side if the C side maintainers agree. However, I don't see you doing it. kvrealloc_node_noprof() does not even have an align argument AFAICS. > I think it’s better this way because of uniformity but I don’t have a strong opinion on this. I agree, but again, I don't think you do it yet. :) > > >> // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously > >> // allocated with this `Allocator`. > >> unsafe { ReallocFunc::KVREALLOC.call(ptr, layout, old_layout, flags) } > > >