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 EF612C49EA1 for ; Tue, 6 Aug 2024 14:39:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8AE306B0089; Tue, 6 Aug 2024 10:39:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 85CFD6B008C; Tue, 6 Aug 2024 10:39:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 724F26B0092; Tue, 6 Aug 2024 10:39:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 5566B6B0089 for ; Tue, 6 Aug 2024 10:39:19 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 0472D1A01DB for ; Tue, 6 Aug 2024 14:39:18 +0000 (UTC) X-FDA: 82422078438.18.69007AB Received: from bee.tesarici.cz (bee.tesarici.cz [37.205.15.56]) by imf02.hostedemail.com (Postfix) with ESMTP id 2896F8001F for ; Tue, 6 Aug 2024 14:39:16 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=tesarici.cz header.s=mail header.b=GMlT1K4Q; dmarc=pass (policy=quarantine) header.from=tesarici.cz; spf=pass (imf02.hostedemail.com: domain of petr@tesarici.cz designates 37.205.15.56 as permitted sender) smtp.mailfrom=petr@tesarici.cz ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722955106; a=rsa-sha256; cv=none; b=XNDghbccyjsHT0BqkN5FUTQ3B7Vx5N4tWmghkAq4y0N0eyAzt8JyWk3Tx1XMz716dgqJTx 6rfNlbb3e4s0aoLVYQwXTHQilVlko8+AiJKzPTqy8M0p3KvqobKqu2m92+5qVzxtdcEr3l 8mQ5Y3dDVE/qNh3nujR3vKFWYZuk0+U= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=tesarici.cz header.s=mail header.b=GMlT1K4Q; dmarc=pass (policy=quarantine) header.from=tesarici.cz; spf=pass (imf02.hostedemail.com: domain of petr@tesarici.cz designates 37.205.15.56 as permitted sender) smtp.mailfrom=petr@tesarici.cz ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722955106; 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=1PTYclq5sGfpv3NAMIKIUDYiBHvA3Eo1GiGmYnQbzU4=; b=zbxjAMqLCv9WdqBSgKh+lm7Cn8+BrsmJTBRIvE2nZeeo5rc5p+dnShI8f9g6LdRiQNHFag hD6tBwpcf6yfHfEBUCgi+ajfWRUUSxB6KDYRmWr5RkRp6I5GL8M8mmDMUGT3LI23vKcNjn TMfm0ZZq8oiZQeMT/fLilE30oaejzQk= Received: from mordecai.tesarici.cz (unknown [193.86.92.181]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by bee.tesarici.cz (Postfix) with ESMTPSA id 3F6FE1D1A64; Tue, 6 Aug 2024 16:39:15 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tesarici.cz; s=mail; t=1722955155; bh=1PTYclq5sGfpv3NAMIKIUDYiBHvA3Eo1GiGmYnQbzU4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=GMlT1K4Q+3fZxVtkXabhN48mW3V4XmbryxkuKMT2KlQasS4GnS7dtESErsxEhv61A PkIs8uvElHRyHTEa5BUfz5LZnUKyFq5ajJsfvF6BflXDqKHJXSFGwGkZxiTahYkSY6 nPY9iSvYF8rLPHUlJhxFrgQUanq5zEYYzsdGZCxJZBFUds2+4bpTgv38iXdFht95dg 4QfExY9hq13m7cQz0Ukw4dVZtWQMi7IQAWegKM37O23o0CAq/MhNpIw0SVssabv24f pagnKQ0E3QhcmmozjpFGOJztF6iv2WyV5ZTl0bFOApyEqAdS+PWskuqMZ17bGYe+mi cqqxmuI/Sv8xg== Date: Tue, 6 Aug 2024 16:39:11 +0200 From: Petr =?UTF-8?B?VGVzYcWZw61r?= To: Lorenzo Stoakes Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , "Liam R . Howlett" , Vlastimil Babka Subject: Re: [PATCH 08/10] mm: introduce commit_merge(), abstracting merge operation Message-ID: <20240806163911.638cfbb3@mordecai.tesarici.cz> In-Reply-To: References: <3b04eb13b499df3ebf50ae3cde9a7ed5e76237fd.1722849860.git.lorenzo.stoakes@oracle.com> <20240806154116.015e329a@mordecai.tesarici.cz> <415d9d9c-7b63-47f0-9091-678f0d8d1268@lucifer.local> <20240806161321.376f0a55@mordecai.tesarici.cz> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 2896F8001F X-Stat-Signature: 3su5xochz7pmd35ryuj3j56yuues3ebw X-Rspam-User: X-HE-Tag: 1722955156-910074 X-HE-Meta: U2FsdGVkX18XPPG0acGqyl76YwqcEK511VVqHHRD/SsYwg0sSIUgQFfiHu2RWt6lPnCudf2T45zaPrZmfnEXl0oerzYrivgOoaSryXmb75ZBGj5FEWBI9HoQdKMK9oG0v1qmdMOuqJ7OFPoe1jxgadJEtW7vs4klQgXhtSi8uASCEqllbm0SD+FMQh6amr7x4RJ913YJYyfdGLcRv5LYqCUTk+DzNANsQTIEzn4+UroSnPNXD3cM2dJH9P3FFW4QGYwGr3fjsgV9nHcpQ8Hes6wo2YzZnJnfIOsBrkCyA9L7TkcO3yTk3DBcX+0y0mbZGvAR1RJq3YZHH22yED4WqfZWEJwFMxIX7zlsKA/ev5dWj0cK2hymJAca7gvIhz6519N9a1ssXNttK3x+nopCfcoQww0NAcK6XW7p3KWsKS4qYlr4fqP24JeKjKj17LJPoTV9DeV+RLY99kz96VFcwUGyTDRVFTpRfF/dCmgr6xY2VH5OFs1/igOAah/A8Y2ELW04LfKnuivLLMG2BsER92PywaqAYVt0n+aWQQQrgeLelxFuPEOX3Bh7waTG3JTQw6uBDnmS3si88dAFrMnuJZa9w92lAXTRf1YWaX1RraOyUIk8kBW5+4YpDWYlqTT1QZmBdZAWlrW/s1EWQeN5h7qC+9F40i0HzYJhp7M7ASxv/M1qsZBeybltcyfoQe1tmQcqmdUcE+TJlPe1JsLWEmMZvh5gZdaooHZhprb1j6la95zd5DkW7l1r3eNtdU8exNwJaByCUZjtEJ48bk57FcQ3ndiHCbqtkonE9EDAlSdKOdToSt554ch82+p8YrGtrnADw87lil+vmg78BaG78tjCgz/CXkNllExxNveVt/DCLfDxIbHwHuGFLEglDvjZXr3TAaHyyAa/Go5ZMnSgiKZSS4fJ/UQa/Us0xrkop90WlBmAv32E7zbPAcWVEAeFVVstFEIoR5PfRIFRMKm zolVXZ1e KQ7cZisjSYJci3lIOPZYTTKuXB221cQeWEGqm0u3Dfqg43mwtZeLGHiQc0742t9fdw0i47bnOoSTeyw2f6SpFXMAGyQaXzgg0gS+wFYnMYw19WfDDAC/TIFjGXxLy/z/7j/mpnjOUVpjmC3H8+k2FzS3Kg85AQaeLBjVsRrGl/7UNhxZoU+dC61U62wQMZCL+eAwCHJ4lyt8RxH7dbpCdhZp9+FZbiYnR8qU7d6b0PpVp33L+MqLArAQB2gpmiAzJLIAHNd6ti+Bg6tHAU9S4s2jGKf1qQkZm1P2v2beqcI4LWWk9AaA7l5ff3Srj9ovBOMcIW3i/ENMUgYh7Ja+L/F7efoZ9hR2FEKRf 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, 6 Aug 2024 15:30:49 +0100 Lorenzo Stoakes wrote: > On Tue, Aug 06, 2024 at 04:13:21PM GMT, Petr Tesa=C5=99=C3=ADk wrote: > > On Tue, 6 Aug 2024 14:48:33 +0100 > > Lorenzo Stoakes wrote: > > =20 > > > On Tue, Aug 06, 2024 at 03:41:16PM GMT, Petr Tesa=C5=99=C3=ADk wrote:= =20 > > > > On Mon, 5 Aug 2024 13:13:55 +0100 > > > > Lorenzo Stoakes wrote: > > > > =20 > > > > > Pull this operation into its own function and have vma_expand() c= all > > > > > commit_merge() instead. > > > > > > > > > > This lays the groundwork for a subsequent patch which replaces vm= a_merge() > > > > > with a simpler function which can share the same code. > > > > > > > > > > Signed-off-by: Lorenzo Stoakes > > > > > --- > > > > > mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------= ------ > > > > > 1 file changed, 45 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/mm/vma.c b/mm/vma.c > > > > > index a404cf718f9e..b7e3c64d5d68 100644 > > > > > --- a/mm/vma.c > > > > > +++ b/mm/vma.c > > > > > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm) > > > > > } > > > > > #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */ > > > > > > > > > > +/* Actually perform the VMA merge operation. */ > > > > > +static int commit_merge(struct vma_merge_struct *vmg, > > > > > + struct vm_area_struct *adjust, > > > > > + struct vm_area_struct *remove, > > > > > + struct vm_area_struct *remove2, > > > > > + long adj_start, > > > > > + bool expanded) > > > > > +{ > > > > > + struct vma_prepare vp; > > > > > + > > > > > + init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > > > > + > > > > > + if (expanded) { > > > > > + vma_iter_config(vmg->vmi, vmg->start, vmg->end); > > > > > + } else { > > > > > + vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, > > > > > + adjust->vm_end); > > > > > + } =20 > > > > > > > > It's hard to follow the logic if you the "expanded" parameter is al= ways > > > > true. I have to look at PATCH 09/10 first to see how it is expected= to > > > > be used. Is there no other way? > > > > > > > > Note that this is not needed for adjust and adj_start, because they= are > > > > merely moved here from vma_expand() and passed down as parameters to > > > > other functions. =20 > > > > > > See the next patch to understand how these are used, as the commit me= ssage > > > says, this lays the groundwork for the next patch which actually uses= both > > > of these. > > > > > > I have tried hard to clarify how these are used, however there is some > > > unavoidable and inherent complexity in this logic. If you don't belie= ve me, > > > I suggest trying to follow the logic of the existing code :) > > > > > > And if you want to _really_ have fun, I suggest you try to understand= the > > > logic around v6.0 prior to Liam's interventions. > > > > > > We might be able to try to improve the logic flow further, but it's o= ne > > > step at a time with this. =20 > > > > What I mean is: Is there no way to arrange the patch series so that I > > don't have to look at PATH 09/10 before I can understand code in patch > > 08/10? =20 >=20 > No. >=20 > > > > This PATCH 08/10 adds only one call to commit_merge() and that one > > always sets expanded to true. Maybe you could introduce commit_merge() > > here without the parameter and add it in PATCH 09/10? =20 >=20 > No, I won't do that, you haven't made a case for it. >=20 > > > > Petr T =20 >=20 > I appreciate you are doing a drive-by review on code you aren't familiar > with, but it's worth appreciating that there is some context here - this = is > intentionally isolating _existing_ logic from vma_expand() and vma_merge() > in such a way that we have a _generic_ function we can use for this > operation. The history you make today becomes the learning material for the next generation of kernel hackers (who will also lack a lot of context). > I think it'd be _more_ confusing and (surprising given your rather pedant= ic > interpretation of churn elsewhere) churny to rewrite this again with a > bunch of added logic in the next commit. >=20 > I think this is highly subjective, and I'm not sure it's a great use of > either of our time to get too stuck in the weeds on this kind of thing. Yep. We can all agre this is the best way to convey the idea behind the changes. Don't get me wrong; this whole series does a lot of good in terms of code readability, even for a bystander like myself. Petr T