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 9557CC3DA7F for ; Thu, 15 Aug 2024 14:23:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E3A386B010F; Thu, 15 Aug 2024 10:23:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DEA136B0110; Thu, 15 Aug 2024 10:23:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CB1E06B0111; Thu, 15 Aug 2024 10:23:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id ADEB46B010F for ; Thu, 15 Aug 2024 10:23:18 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 6B61781613 for ; Thu, 15 Aug 2024 14:23:18 +0000 (UTC) X-FDA: 82454697276.24.9D8E833 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf28.hostedemail.com (Postfix) with ESMTP id 8C48AC000A for ; Thu, 15 Aug 2024 14:23:15 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Hrokymxa; spf=pass (imf28.hostedemail.com: domain of dakr@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723731722; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=gBdCDBGE7O4hlaQLCSDVE9sadVAGvn/39R2GdsLGg9U=; b=qmodLcGx877sgul32IhgpVdbzQnjZYeQ657P9TAozQ/HAH348ydGRp/bHVrpuOw+hXWW4t DyZS54r2k7XX+hkW8NfYxVMF7CkFWZfTmugZMtp4SR60KEvkgzcTeF6IE1FasVpkNYy/PE dPDyTXG3iiF+7oTOWI8miDZOyiftT8Q= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723731722; a=rsa-sha256; cv=none; b=g2bV1zTmewaNZOotN9NTHcu5oDhYUmKu2lI24rNhQHuYVq9DDxWn4CEWlqjpapOJQZgXUt yVCkfusLlX2WGyTUK0yk2JJUOxRFYktV/UpLnHWHBtOiu64p891XhAoZv4OueSsuBs+b5M HiOVFnACyMusFDQCHYqMEg77Oz05lyY= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Hrokymxa; spf=pass (imf28.hostedemail.com: domain of dakr@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=dakr@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 9EB9B61EC4; Thu, 15 Aug 2024 14:23:14 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 96176C32786; Thu, 15 Aug 2024 14:23:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1723731794; bh=ZICUfqW3ibLnp1tG3YkER08ff+Zhm0tKGhwrShQiyvg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HrokymxamxMNOr/rIg/O/ihcm9ydAtLZNCV8EtB4nD6/GRac43Zn9YWH1wmI4m/Up dv/SjEjMNXUGY5g7k1L50DYUbSfvs4oCT+DOSykiL2NAT7eElnEgV+wTo0U8GjaXEk Wb5gEcxdNVytXkLc7z5L0lDnxPH4B31H1l2+KT6531S1RfNeo6jOP/pVkmmAk9hMZS GfU6IoTl3nN9Jedr83EPgcIbv4pP0OMq5ZNDw2sgBmLkY9hjok6nSp6U1nmx4fwO0c loF1iJtBrdFXkvQASm0qOwQTohZRTg4x7wiLn6xCePFkugZjR/kYVPnOJ3OfQwGu1V HeMEHkWXO+G1A== Date: Thu, 15 Aug 2024 16:23:06 +0200 From: Danilo Krummrich To: Benno Lossin Cc: 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, 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 v5 06/26] rust: alloc: implement `Vmalloc` allocator Message-ID: References: <20240812182355.11641-1-dakr@kernel.org> <20240812182355.11641-7-dakr@kernel.org> <5dfe8bae-2c1e-47d4-9fb4-373b7d714c4f@proton.me> <01a46c6d-0107-4455-8c87-af43426752ff@proton.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <01a46c6d-0107-4455-8c87-af43426752ff@proton.me> X-Stat-Signature: 714rbfzjeotozw4djzfy1yof7o9gu184 X-Rspamd-Queue-Id: 8C48AC000A X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1723731795-347705 X-HE-Meta: U2FsdGVkX18TH+SklSx/Gysz+49cqA7+inMYa0N/q1NiufLIWNvkNfpDBJX04fwgJFEg+iS/9OaRAXdciNbNNyhyPuW9VJE12nf5qLBHnPHQIrT0P0yFSXo2rlxGy7EFaaY0o7Z5QhpMvFD+gKOQrHZkcL4fELVntd9UqWm4DxYwPJETZxnUotY0Bff39ILy6u0t1zbWZ+24EL3+OuEglk8gsf7qpgyNsdaXIJMfrcnPWaq3jDOZO5X/6FtfYX3PZ8nABGQ04P0kYCOQJN3R3pGxx/7EDIvhQ9faGAokVcxDIeJ2zw2huwxvr1w/RWqPlYOiMKj6Nk1+XkNDPDZYogy3E7oIIwQcigOi/D9sbYjYlZwic1KXgarJaujSRLVwdlVj9FqCLxNuL/+SvBhCK6Jm1hZXGEWSKe4D104/VpQMezf0yf0XtopucXq7M0C+9RFHqAxd4pMCpuyECTJnuKJiuPGdVHUpmHGGlrgfT4lF76Ozhzf9Wk9EspQ8Uqz902c8eHIwWH18rsy5iG1fp22q2BBOr+HNxInPmrJ4u2rXi8rozvlxQ3Tt9sBffJMzIoiFq/bntXPt3wSb1kTC6/7Yzb8u8MDnS0BwVqucPYH3wKsWc+yjgvVd9CnRa/fTwT37xErMouGnbBpym+2Mx9+8ZBhEFOtIL/K/gTYE4pV82VOsX8VNZxpaR/QunoBO5XYzWtFm4/digrwGtiKCmYTAWJgg4Yxpsr4sOOceLp9Nm7KRwcoxVhG1fJs+BdxYf8ocFrqL35JNd1b6Q7YNC3SOhyjaRGDHzxXKD692vZRGJXe/8ikulqA/qS0lkQ/dbksGaVqhluVaJaHKGSvi+tbFPTuVwHPsF/dttEg6n1x8sEgkTHYqGoqFDErxcDeRAgJe4OnNOGRtHaeD+eeEiMrnTWQ4LHw41MDhNn3ksG7m4R/9G5VChnyRFebqYmEyMkijGhJuKvxMQcAhK1R Shppjhj1 XbiPkZ7vVmvtFV1YeALd3Cs+RpZq2Wa6zBZKeU+Jg+18UGrtUS3qOhalfN6+qiz0y2NduG/P3ERpPyD9EhXhQPEnI/5FJd2knhQNgVgXqFbqh13BtGj5EGaaATofmFFHNd/AlaG5o5OsE+jmaRLO5FP7OlsgxhyJuszumamXn6a7AaSs5ZREciUgLd90UVcw7W+wU0uqbQhWGwVP9ms2k2e+SNRJFWQ3nquHjnIwNIl2mD6BrMfTQk0xqqf4txeHhJFNtvm7bNGO0dstH34yexnR4i1aFIbnfQYpGFpryTLPadFT4ighX1++4FCG77ynYxghRfdPOfL5hcNdczFRxgQjxWa9udnCbCPFr 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, Aug 15, 2024 at 01:44:27PM +0000, Benno Lossin wrote: > On 15.08.24 14:29, Danilo Krummrich wrote: > > On Thu, Aug 15, 2024 at 06:48:19AM +0000, Benno Lossin wrote: > >> On 15.08.24 01:20, Danilo Krummrich wrote: > >>> On Thu, Aug 15, 2024 at 12:13:06AM +0200, Danilo Krummrich wrote: > >>>> > >>>>> > >>>>>> + ptr: Option>, > >>>>>> + 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); > >>>>> > >>>>> I think here we should first try to use `build_error!`, most often the > >>>>> alignment will be specified statically, so it should get optimized away. > >>>> > >>>> Sure, we can try that first. > >>> > >>> I think I spoke too soon here. I don't think `build_error!` or `build_assert!` > >>> can work here, it would also fail the build when the compiler doesn't know the > >>> value of the alignment, wouldn't it? I remember that I wasn't overly happy about > >>> failing this on runtime either when I first thought about this case, but I also > >>> couldn't think of something better. > >> > >> Yes, it might fail even though the alignment at runtime will be fine. > >> But that's why I suggested trying `build_error!`(or `build_assert!`) > >> first, if nobody hits the case where the compiler cannot figure it out, > >> then we can keep it. If there are instances, where it fails, but the > >> alignment would be fine at runtime, then we can change it to the above. > >> (I would add such a comment above the assert). > > > > Unfortunately, it already does fail with just the test cases. > > Aw that's sad. > > > Anyway, even if it would have been fine, I don't think it would have been nice > > for a future user to run into a build error even though the alignment is > > perfectlly within bounds. > > I think it would have been better compared to failing with a warning at > runtime. Generally, yes. But I think it's not acceptable to make calls fail that should actually succeed. > > >>> In the end it's rather unlikely to ever hit this case, and probably even more > >>> unlikely to hit it for a sane reason. > >> > >> Yeah, but I still prefer the build to fail, rather than emitting a warn > >> message that can be overlooked at runtime. > >> > >>>>> How difficult will it be to support this? (it is a weird requirement, > >>>>> but I dislike just returning an error...) > >>>> > >>>> It's not difficult to support at all. But it requires a C API taking an > >>>> alignment argument (same for `KVmalloc`). > >> > >> I see, that's good to know. > >> > >>>> Coming up with a vrealloc_aligned() is rather trivial. kvrealloc_aligned() would > >>>> be a bit weird though, because the alignment argument could only be really > >>>> honored if we run into the vrealloc() case. For the krealloc() case it'd still > >>>> depend on the bucket size that is selected for the requested size. > >> > >> Yeah... Maybe some more logic on the Rust side can help with that. > > > > Only if we reimplement `KVmalloc` in Rust, However, there are quite some special > > cases in __kvmalloc_node_noprof(), i.e. fixup page flags, sanity check the size > > on kmalloc failure, fail on certain page flags, etc. > > > > I don't really want to duplicate this code, unless we absolutely have to. > > I am under the (probably wrong) impression that kvmalloc has some size > check and selects vmalloc or kmalloc depending on that. Basically, yes. But as mentioned above, there are quite some corner cases [1]. > I think that we > could check the size and if it is going to allocate via kmalloc, then we > adjust the size for alignment as usual We don't need this adjustment any longer, see commit ad59baa31695 ("slab, rust: extend kmalloc() alignment guarantees to remove Rust padding"). > and if it is going to select > vmalloc, then we can just pass the alignment (if the vmalloc alignment > patch is done first). Yeah, but as mentioned, I'd prefer to do this in C, such that we don't need to open code everything the C code already does. [1] https://elixir.bootlin.com/linux/v6.11-rc3/source/mm/util.c#L628 > > >>>> Adding the C API, I'm also pretty sure someone's gonna ask what we need an > >>>> alignment larger than PAGE_SIZE for and if we have a real use case for that. > >>>> I'm not entirely sure we have a reasonable answer for that. > >> > >> We could argue that we can remove an "ugly hack" (when we don't have the > >> build assert, if we do have that, I don't mind not supporting it), but I > >> agree that finding a user will be difficult. > > > > I'd argue it's not really a hack to fail on something that's not supported > > (yet). Allocations can (almost) always fail, this is just another case. > > I guess since this is a deterministic failure, it's better than other > failures. But I would still say this is hacky. > > --- > Cheers, > Benno >