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 0E62DC3DA63 for ; Tue, 23 Jul 2024 11:55:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 87A176B0089; Tue, 23 Jul 2024 07:55:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 82A376B008A; Tue, 23 Jul 2024 07:55:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 719586B008C; Tue, 23 Jul 2024 07:55:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 556526B0089 for ; Tue, 23 Jul 2024 07:55:59 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 085E11A151A for ; Tue, 23 Jul 2024 11:55:59 +0000 (UTC) X-FDA: 82370863638.06.86C6AD9 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf20.hostedemail.com (Postfix) with ESMTP id 1D4BF1C0018 for ; Tue, 23 Jul 2024 11:55:56 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Vdo+68J3; spf=pass (imf20.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=1721735735; 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=JM7FFkGqS6EUk1uRCYG2f5O4Go5OM2iJCYXMke1ZcXw=; b=iUvYISKgoy7JHr0y56n+sLdMd3m9E5jbdMpCrP+bIZkWbAZ7NR2Qhd3QWxUs0XeNhhpoaE vR2fq+nkrOhU/sH+6TC5BqM6l0BuyRQr8mZJuPGcBHIllBd96FdEXihcx0WdoU5gpW2tMs FYbLXb1E5GlNa0j93jtKVhaDBL5uxcI= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Vdo+68J3; spf=pass (imf20.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=1721735735; a=rsa-sha256; cv=none; b=YHe6jYXqjRrPMZg9Uh3ninZZg5BXB93+vO/JqKB0fy56JRkuu4bLiY8NfWt1OwLBJfKwRd qvL4oFh1KeGam0C8V1thEWDqmklEWzdW7N+bSIL0s4oU6lvH7tUM7l+kVAilNDVnJElVW9 guzZ8ggPCfdialBSA5qgVMpVaNb9rIs= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 0AF1460AC1; Tue, 23 Jul 2024 11:55:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 564EAC4AF09; Tue, 23 Jul 2024 11:55:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1721735755; bh=3TDEs0L944G71NhsWNCAYOPRVeh/85H8RUzJ/eqZfX0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Vdo+68J3DaMhzDgACOCFCQYgb4dH4TUq4so9lG/BIYPd2R01tyAbu1E7uVlKhh+96 h0/vLHzc1rvGJY7EKrASOiEd+ZqDYrKB4nJDFNJj9KzmeENBhuwd2fSP6q71XcHXZb HaN0+4d4ATov849Q2LE/bem+PSweTYDEU/ZqxjBeZKOPGh1BFXm95JmOXLl2Ixj3Bn pk6Q1py99Ytakn/VO+bJVfj0a0vDYjMNEByg+RztYiHLiv5Nds7aF6aGD2S8gND2at 5ajVMYhWYi5L+n0z9suY8H5jvDbDnbdu7vmHZRlWBI8KNQgCrPb8YJeLLq1JaWBpVx NUazqDlkFFu3A== Date: Tue, 23 Jul 2024 13:55:48 +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: 1D4BF1C0018 X-Stat-Signature: e47i3e651s6nm5r6mzba96pstwugznaj X-HE-Tag: 1721735756-191387 X-HE-Meta: U2FsdGVkX1/pUm/LaYFHdFXgLLPxoMaeFV5HR5iCFANkWgNkwIflehT8HNwInZwWk/P8wNJfwC+NvOd226M5Q7waGmDYSf3NoniunydATDjoO5riVqJW1S2IxYlA8k3nhROauhyd16ycF/0QI6MR7mgMA3qO2ivdDkHEWOLYqKVyNnNqsuEsHo+7egebMmtZ+hFJvCZgnX0xVA4aHuR1KwApfVDXOH2ob4CBwSIO0InFLptRQiqgDQT5DbrVYyytM84smm3j0TfEK+wuxJogJxdQkkP951UbIapq6DFIWLecIeR9l3PYAb2IwM3785IiDoxtEvlnBhp5TlgFbAcw/F8jCfZVmzV6K50O5rJU8ecsHWcGPJm2fMdbKfUK+GUtqCxBPoesLEYuTxUn/PMt6WwZvvtcueFHWo6QuY+8SLdqnYP4vOhku400EcEhVRT+pKXJBDWMnHceXzuPBSIoUR/mkB1ni49FfYgMhgYfb8HRvecSUHKEhd5JbF/rRzWrCDqq5VnXD9TwvyWDgaDXUKhCqQL25MiveoE+TmiCvqg3YLIT+FUl2ptDHnFI8ZCbHrFRqKWMQoeg0Sb3ql28x56TI9Uzw4rLkobQ1x94GrQAxM8bFfouGCd6jqHTRKOKQcnpkNc4A8nDauwI5XTsIEK8if0IX7Y/2qyFC4Dw/arkzl7Y2KIw9ilg++hOlCAQF87QC39vfGSMolIy+dMeQl/xomYI9Haza0Qc+9PP/ajR5psa5eI1TTX9vng7ei/JwlenadmeL7GL2Sy4JBZmMSLgFlsF1d1QTywqWh74H7l7CfSRc7b9+NDuCkWemmTOHR/hPMmc3YpqaSxDuyIaaVj+xajuQRfTxWNnIvGgqDEO6JOcrV+Hc02FYNW/vsT15KknhqVQv5SPprhD44mri3v2pchOBrmuWdyTjXWowoDI2qHpfmWEtSeJUazPD/IBKWqsmx/KLQ7+8WV5puB A5Q75Qmd ZOHORi3DhKyDEqniyIFKeAbHlVus7edbBf4m9FxAPtrhBOAe919Xgy7OpEQx/K3W44V0tPb/As9GHVMT9o+sIoeoveosaH/1W/oanLbD4tKWuGQPej1rhxvCazJqy25+CyNbuFzJXi9k/7hP1Ud2Bn8LZJZyut0oRVVh1lcvpjR/CvImKPWSA3zTsAVu7RR4AaarVacl3QVAcqP94w5uNeetAdM/pmyiqNB8hFssrR+akmtHNZxjZnRek9DLht7LnJmZT2WJStIWQZp2cOUVE9/5TbEUxz09ETiJ688l+ZFdEtuIqGpFERv9/yaDDn30zAQdQ49k2khDg9WIRlFeCqP/gOB5ZgEg8/C8rwPyDSSf1U6sOSKxburcU0jTuvMk8iz+r1G0PvYhZMSw= 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 12:55:45PM +0200, Michal Hocko wrote: > On Tue 23-07-24 12:42:17, Danilo Krummrich wrote: > > 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 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. > > No I mean, that the change of the signature and semantic should be done along with > update to callers and the new implementation of the function itself > should be done in its own patch. Sorry, it seems like you lost me a bit. There is one patch that implements vrealloc() and one patch that does the change of krealloc()'s signature, semantics and the corresponding update to the callers. Isn't that already what you ask for? > > [...] > > > > +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. > > I would really prefer to have it there because it makes the follow up > code easier. I don't think it does (see below). Either way, I got notified that Andrew applied the patches to mm-unstable. How to proceed from there for further changes, if any? > > > > > + 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; > > With the early !size && p check we wouldn't right? I think that's unrelated. Your proposed early check checks for size == 0 to free and return early. Whereas this check bails out if we fail kmalloc() with size <= PAGE_SIZE, because a subsequent vmalloc() wouldn't make a lot of sense. > > > > > /* 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; > > } > > I do not see why kvrealloc should have different set of constrains than > vrealloc in this regards. Those constraints come from kvmalloc() and hence should also apply for kvrealloc(). What you seem to question here is whether they should be moved from kvmalloc() to vmalloc() (and hence implicitly to vrealloc()). As for the gfpflags_allow_blocking() check, it seems like this one was suggested by you for kvmalloc() [1]. It seems that some people call kvmalloc() with GPF_ATOMIC (which seems a bit weird at a first glance, but maybe makes sense in some generic code paths). Hence, kvrealloc() must be able to handle it as well. As for the size > INT_MAX check, please see the discussion in commit 0708a0afe291 ("mm: Consider __GFP_NOWARN flag for oversized kvmalloc() calls"). But again, whether those checks should be moved to vmalloc() is probably a different topic. [1] https://lore.kernel.org/all/20220926151650.15293-1-fw@strlen.de/T/#u > > > 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. > > This is effectively open coding part of vrealloc without any good > reason. Please get rid of that. > > -- > Michal Hocko > SUSE Labs >