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 88643C3DA63 for ; Tue, 23 Jul 2024 10:42:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 053586B0083; Tue, 23 Jul 2024 06:42:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F1F0F6B0088; Tue, 23 Jul 2024 06:42:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DE5F26B0089; Tue, 23 Jul 2024 06:42:28 -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 BA7366B0083 for ; Tue, 23 Jul 2024 06:42:28 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 34459141DBB for ; Tue, 23 Jul 2024 10:42:28 +0000 (UTC) X-FDA: 82370678376.23.A8B2A61 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf30.hostedemail.com (Postfix) with ESMTP id 7F7FC8002A for ; Tue, 23 Jul 2024 10:42:26 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="uz/5htZm"; spf=pass (imf30.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=1721731324; 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=8QffQryXZYjcoAqVleW0oIH64OO5jGJzP2sJrM5Zdrk=; b=q7q5X+TDHlEGEjFtrJjSkiCkBwArNQHGIjriw04PP+eUiWg7DvC+gaQmYo1935Rw0RdTQa XuoJXgMp643VmsXFvaEIUVFTCu2B84zViWvncIQ5H4PgCOWuO0IR8PrbCins07rqMdJ5rG ezfPRIWj7SXuApB5yYn/kZauShx75UA= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="uz/5htZm"; spf=pass (imf30.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-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721731324; a=rsa-sha256; cv=none; b=InSrIIxjpuWCkdukjiaC7N7ijDPUsY3B/kpeLZDYHLb3GZtLLW/fIDYAaYTzj7Qaj9HUzT cSFF2vmquBXOeh/EMCEKKqcpEXyJgoPBuk7mvp3VE+MzPj9Kj/XFFLvFpS5G943g4DQhZE aujq7nG7xKuUCsdUo5B53bd11R3ub+8= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 2EFE760DC1; Tue, 23 Jul 2024 10:42:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 791ACC4AF0A; Tue, 23 Jul 2024 10:42:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1721731344; bh=4QoKm4L4N0dQCpl1EJWKuRlJU5EU8KSEXc0sJOpWP3c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=uz/5htZmkRUJMT3OYtj0uhQSZ056sGgfASjVPhAm7EvpWMQkDFDuwx2ioWpabM0gm kcdEzvQtAvROOqLWqGtrPcaNzCRnzY6u/9yHSQTt1JRovkAgITxLB2Xok1ppc5XbMp ZspuumCY/cyfqebaUz/AXFkhKTHCxhvZb7DR9HjSFa2/h0qHuYJXpfhAKw5P+rZlVo +2x6pKsv93pJVSatHTgQm8md0hJJ+o7udYZAhJqyWV6tI4OUtiA6MQWCpnYVZbWyDh lC5h6pntLHo2nvDSwH0MNMMBSbGZWPzy1WBYprEqIpjdx/XqJABfiRRkfMl/x7k3wp 0U+Nh/Ky35iJw== Date: Tue, 23 Jul 2024 12:42:17 +0200 From: Danilo Krummrich To: Michal Hocko Cc: cl@linux.com, penberg@kernel.org, rientjes@google.com, iamjoonsoo.kim@lge.com, akpm@linux-foundation.org, vbabka@suse.cz, roman.gushchin@linux.dev, 42.hyeyoo@gmail.com, urezki@gmail.com, hch@infradead.org, kees@kernel.org, ojeda@kernel.org, wedsonaf@gmail.com, mpe@ellerman.id.au, chandan.babu@oracle.com, christian.koenig@amd.com, maz@kernel.org, oliver.upton@linux.dev, linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH v2 2/2] mm: kvmalloc: align kvrealloc() with krealloc() Message-ID: References: <20240722163111.4766-1-dakr@kernel.org> <20240722163111.4766-3-dakr@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 7F7FC8002A X-Stat-Signature: 1z1jpp5u1cfoobo1ybnxcgo45hqq3mwp X-HE-Tag: 1721731346-797091 X-HE-Meta: U2FsdGVkX1/I3sCX1c0Qfay6+zIR96mGHCKRP/8a8oWJovms3Yi1Qdb+Mut+N4UBb9QtSQHU0LgVCQJjNrZlIFVLekhNR/xI2mJmoXBDhneWJU35deK3UyDsUOrD6F+RbNTOl+Y8mfn8HQqI6EvxDKp81czPI+Of+rBhYxZCh3N6g6lYPRBX9a/JkK156glUDxzA5jEVHf3fiqaJy/WOWUnQizu6WCZzevWLQP0KE7rJWTBbIU14t/CdLjTBNzNJwVdFQXphuhERskpmb9ZG4ZabcrhIYTxXIy6eIIC1scji7zHpLUP5N05NvZ3691f/sZbpLNEQmw1az7T3GBZFjHBCQaj7F7fRNh/8NnsZNz7o3520yzo3OX7xYNPfqxiINTNy8Tp3f/sd/FENIhpAnHczaSVya7KaVWPl4zj9e7zFcjN4ZCuBGfTucNnhGGDRgp9bp8q39gmIVuLs4bZfwsGGoWSdQ3ofrVOOvxVF7Jmpspov2gQnl2osFXwAqPQFXiGHYmxG0wM/2Ja0zdmpCd9Yd4iVXMHnSOGwCfKeJdbNEknpQWAE7ZuWvu7nhuBA2oQOa3XFZcP+YmkXvRlMpOiY5+S90wDUt2ipy7eHc4LdGhywINftNkKLfcqAzIbXeoHt2805asxypAw1FHmsTcOqEcDMLNFoDe5EgHfdI94i4TJUAhNXMmuSO/64JnVNCU2G8eZb4ZZIh1D2TjgQMbq9VZCBnkqNq0MsqxZC8uzkaLIhT25weAyWKI7rXigeRYfce1/lFaFatz/yBlbedFT0RPXmFQBjS+NuRmDHf02ARd7+eyNWS268DM6lDxUAWcaUA6+CL8d4nLZpQJ0aio4zpSrP8MaHSaNSD6oqE0YjQOpazwq04NyHVOyTrc3L3WZOuVr3EmiSxlO0U5wkHn0zVooNbvgM48ni60NsPwKrmL7+lBS1A7qoJsB77iX8EnIPpAQgZ7VWdGftwQz roW/pr1r 8o0Zgd9OhBfnYucUPbCaQDsQs9eFnPF93pKACTGigzkKc8l0hZNYKYMYv/Tj4n7/RxXupi0SDXW7XrcH62YnobKBnqdQr9w7xkTenshzG6rTpATFozDFaZpjLOC8h6zzXE1drOFyFiIJChBOYBI4toAc9ln9mJdafTsvPGhTrPIMKsrA2drKUyru2nLla93PUyjMwRAzQJl3SLdHXlpneCvqX7ledyjZfu7sXi7fwcHbE3Ii3zSy7qDvslnKkJ582fJOLYRvfQ5mdzHVhqubu9G5Q8QImyuenGfD7EBG9EMQ1D24= 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 Tue, Jul 23, 2024 at 09:50:13AM +0200, Michal Hocko wrote: > On Mon 22-07-24 18:29:24, Danilo Krummrich wrote: > > Besides the obvious (and desired) difference between krealloc() and > > kvrealloc(), there is some inconsistency in their function signatures > > and behavior: > > > > - krealloc() frees the memory when the requested size is zero, whereas > > kvrealloc() simply returns a pointer to the existing allocation. > > > > - krealloc() behaves like kmalloc() if a NULL pointer is passed, whereas > > kvrealloc() does not accept a NULL pointer at all and, if passed, > > would fault instead. > > > > - krealloc() is self-contained, whereas kvrealloc() relies on the caller > > to provide the size of the previous allocation. > > > > Inconsistent behavior throughout allocation APIs is error prone, hence make > > kvrealloc() behave like krealloc(), which seems superior in all mentioned > > aspects. > > I completely agree with this. Fortunately the number of existing callers > is small and none of them really seem to depend on the current behavior > in that aspect. > > > Besides that, implementing kvrealloc() by making use of krealloc() and > > vrealloc() provides oppertunities to grow (and shrink) allocations more > > efficiently. For instance, vrealloc() can be optimized to allocate and > > map additional pages to grow the allocation or unmap and free unused > > pages to shrink the allocation. > > This seems like a change that is independent on the above and should be > a patch on its own. The optimizations you mean? Yes, I intend to do this in a separate series. For now, I put TODOs in vrealloc. > > [...] > > > diff --git a/mm/util.c b/mm/util.c > > index bc488f0121a7..0ff5898cc6de 100644 > > --- a/mm/util.c > > +++ b/mm/util.c > > @@ -608,6 +608,28 @@ unsigned long vm_mmap(struct file *file, unsigned long addr, > > } > > EXPORT_SYMBOL(vm_mmap); > > > > +static gfp_t kmalloc_gfp_adjust(gfp_t flags, size_t size) > > This seems like a generally useful helper which it is not. I would call > it something like __kvmalloc_gfp_adjust or something similar so that it is > clear that this is just a helper to adjust gfp flag for slab allocator > path Christoph proposed this name, I think he wanted to encode the target of the flags, whereas you want to encode where the function is intended to be called from. When I originally named this thing, I had the same conflict - encoding both turns out clumsy - and came up with to_kmalloc_flags(). Personally, I'd be fine with __kvmalloc_gfp_adjust() too. > > [...] > > -void *kvrealloc_noprof(const void *p, size_t oldsize, size_t newsize, gfp_t flags) > > +/** > > + * kvrealloc - reallocate memory; contents remain unchanged > > + * @p: object to reallocate memory for > > + * @size: the size to reallocate > > + * @flags: the flags for the page level allocator > > + * > > + * The contents of the object pointed to are preserved up to the lesser of the > > + * new and old size (__GFP_ZERO flag is effectively ignored). > > + * > > + * If @p is %NULL, kvrealloc() behaves exactly like kvmalloc(). If @size is 0 > > + * and @p is not a %NULL pointer, the object pointed to is freed. > > + * > > + * Return: pointer to the allocated memory or %NULL in case of error > > + */ > > +void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags) > > { > > - void *newp; > > + void *n; > > + > > if (!size && p) { > kvfree(p); > return NULL; > } > > would make this code flow slightly easier to read because the freeing > path would be shared for all compbinations IMO. Personally, I like it without. For me the simplicity comes from directing things to either krealloc() or vrealloc(). But I'd be open to change it however. > > > + if (is_vmalloc_addr(p)) > > + return vrealloc_noprof(p, size, flags); > > + > > + n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size)); > > + if (!n) { > > + /* We failed to krealloc(), fall back to kvmalloc(). */ > > + n = kvmalloc_noprof(size, flags); > > Why don't you simply use vrealloc_noprof here? We could do that, but we'd also need to do the same checks kvmalloc() does, i.e. /* * It doesn't really make sense to fallback to vmalloc for sub page * requests */ if (ret || size <= PAGE_SIZE) return ret; /* non-sleeping allocations are not supported by vmalloc */ if (!gfpflags_allow_blocking(flags)) return NULL; /* Don't even allow crazy sizes */ if (unlikely(size > INT_MAX)) { WARN_ON_ONCE(!(flags & __GFP_NOWARN)); return NULL; } Does the kmalloc() retry through kvmalloc() hurt us enough to do that? This should only ever happen when we switch from a kmalloc buffer to a vmalloc buffer, which we only do once, we never switch back. > > > + if (!n) > > + return NULL; > > + > > + if (p) { > > + /* We already know that `p` is not a vmalloc address. */ > > + memcpy(n, p, ksize(p)); > > + kfree(p); > > + } > > + } > > > > - if (oldsize >= newsize) > > - return (void *)p; > > - newp = kvmalloc_noprof(newsize, flags); > > - if (!newp) > > - return NULL; > > - memcpy(newp, p, oldsize); > > - kvfree(p); > > - return newp; > > + return n; > > } > > EXPORT_SYMBOL(kvrealloc_noprof); > > > > -- > > 2.45.2 > > -- > Michal Hocko > SUSE Labs >