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 E844BC76195 for ; Mon, 20 Mar 2023 14:58:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 575B86B0074; Mon, 20 Mar 2023 10:58:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 526286B0078; Mon, 20 Mar 2023 10:58:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3C6EB6B007B; Mon, 20 Mar 2023 10:58:29 -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 29A356B0074 for ; Mon, 20 Mar 2023 10:58:29 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id E7F1EC0AC2 for ; Mon, 20 Mar 2023 14:58:28 +0000 (UTC) X-FDA: 80589582696.06.FBEDC99 Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) by imf27.hostedemail.com (Postfix) with ESMTP id 0336840012 for ; Mon, 20 Mar 2023 14:58:26 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=PGMwQ1yw; spf=pass (imf27.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.41 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679324307; a=rsa-sha256; cv=none; b=4oNlM7k4+38Nwmo3V1gDnLuCVnxIMVvYLmfGPjgSPBaEFR2HAWdVG6VUVuAfnar0PEs7Af 3edWeH4PsyCA38KmXB/tLrw8l9qBRpv358GRlAz9YYIaiNQQqp2wYrRw0mAD5bzNi5k2zd 6AB6wJA7299HYb4HPSI4TMzMc6r86tA= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=PGMwQ1yw; spf=pass (imf27.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.41 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1679324307; 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=1yGS1YiNMYe/NdaTEd2bd8nkPGKEIMSPxe8CGVin02c=; b=XpzM9nbW/u+k7hke1rm0plIr6cgQDpjeBjgWH12qbV/uH39gjbgiFddrb3PIoz07D0m7FW YE5sPs0Va9oMXgSXJbvNeLXGWprxF4JY15RddMJmj4+2Tg/fdzLIN617e8jJ7DUAQbp6oJ +XDc2gpOKk2EUiXcDeTvj7Dd0l4sc10= Received: by mail-wr1-f41.google.com with SMTP id o7so10650968wrg.5 for ; Mon, 20 Mar 2023 07:58:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1679324305; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=1yGS1YiNMYe/NdaTEd2bd8nkPGKEIMSPxe8CGVin02c=; b=PGMwQ1ywFQ/NK+0AX3IbIsbgW0fk0RZfu2mlDh9yk+Kr4vQ+QIzD1HEaJDNi3ibMgl HDx3peBG8Cg+oMIV7LWGXfuxcvWJqmWjCxCuZ2kNlcHKkdappyGbARGH2+sOEGRv5xrt hF8XIkuiDoqdeXwtesotF0z0IeIGlxYHl6+Ks+tZu4FhRGk+6Pvl6Ivwr9qGK/Gu5EeH RVINS5Rol3i5lqzvCFMEey8JErR8YH4x/TR9p/udrzsh68wJyHRJxlaErK3dovJcrMSV lJozmusk0Sp1Mr1Kf0cfAV7TsaFh02aLBq6wx+d/qXW3pRjgJXoxYUwD2VX+LsMQWsqF 4YiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679324305; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=1yGS1YiNMYe/NdaTEd2bd8nkPGKEIMSPxe8CGVin02c=; b=IEMi+rkRErS4396tzTGiPk7bme0UNbm9kGIdDCGkZJupk+voEWbv6QGuEllB5oU9hp asgmQAk+Qhu4FgWJ1FL5irNHltstmGJxeRvwc106QzypMkxSfcRsru8yfZKfT5mnv3GV xdrdwg57dv37y13EzgXM7GCsk8qJcy4miNTFN+dUtcPHkIQd7Jc6r2lqaeQjQhNo5jLK yBvtS72Oncz9AwUINhSxbovHJwwMaJU5Ve7LsTsGXq9acOrEDafo/n4mqrOnLHNHYeRb R/D7eDJmPtQ+rqfbDX4MKrW6FYqqD6AQfPx0j2Y9Vu24Wo95D4GUccwdaUwKTTviJuMC DQAw== X-Gm-Message-State: AO0yUKUnEvnYbi482XE4ePBIkkRiiecc6x7CYePBjYY7nk8N9TDtFHY1 qr4W0gFfIKqB+PVabTp04WU= X-Google-Smtp-Source: AK7set8mG8h9++ybZp7nZllv5rbNabzYUIZZFk4oZSjlzkjFUbzUK1cdkCVHrFDfcv4RkXh4Q0ogEA== X-Received: by 2002:adf:f607:0:b0:2cb:c474:7597 with SMTP id t7-20020adff607000000b002cbc4747597mr13382636wrp.66.1679324305181; Mon, 20 Mar 2023 07:58:25 -0700 (PDT) Received: from localhost ([2a00:23ee:1938:1bcd:b99a:5dee:d658:4b02]) by smtp.gmail.com with ESMTPSA id p17-20020adfcc91000000b002c71dd1109fsm9152912wrj.47.2023.03.20.07.58.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Mar 2023 07:58:24 -0700 (PDT) Date: Mon, 20 Mar 2023 14:58:22 +0000 From: Lorenzo Stoakes To: Vernon Yang Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , David Hildenbrand , Matthew Wilcox , Vlastimil Babka , "Liam R . Howlett" , maple-tree@lists.infradead.org Subject: Re: [PATCH 2/4] mm/mmap/vma_merge: set next to NULL if not applicable Message-ID: References: <4d717269303d8a6fe1d837968c252eeb6ff1d7e5.1679137163.git.lstoakes@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Queue-Id: 0336840012 X-Rspamd-Server: rspam01 X-Stat-Signature: 4qsinp1dehbixw1kh6di13udcrkucnxt X-HE-Tag: 1679324306-784279 X-HE-Meta: U2FsdGVkX1+Ou5OJTElMZSgfhz6bNIXmAKwO8x6DnCQM0Pk03oYBxhtMchgvrSYT666+CQ0Q8tt0UWiCRkodWSi247SnKg+KVS1UQI11N0QtBMKz1bcGqxmr8WPqTs417tABPyKeVa+OhknkRc1U71BqVV6u/Wy+ykiQiBgKy4P3X3tNQy176LkAF8x5mae1SQ3n9w0yJjfPIXYxrYU749OUF45xPweaZZHTZwb8gOCY97b8XjAuwrid/uwgzDE1bswa+nnghrzgvNRC9leMlMZJSKvII/zvx1AwXY60ubxHinIKSlPHKNHfSMYMuJr8k/+UYwBP8eYJ3qkF8YA0oNSmWC4LifAiRsYdMP/Fh7nLkQKw9MIUR90EjRbhQGern2tv6gMoEiTiq1lrIg8Xn/3j6cDyF6ZDCMMClAJpsJZjNRcFJ2bgUYf9rnQnsd6XxgROiNU481Z+zFBwmx95KdDFZqy+QN+IgCTcOK6O7jAlVcMJhFERZJ5rH/CcDRrr6gwPfDmQX/5IJ4thnFiwI9vSdPkmfeFEN+/29428JwBQNR3QpTAAQ6BvpY1B9CB77jTp+1RVpSEjJcXXvSp+LdaS78qf1Jo9F1Z2EgotckFBV8l74rn7RvQoCi96Qk43jkOVVIosUJpgl8t24ilpRIVrBc8Mbj3EEHe5pjgMiSF+JGQ8rBHCeTU8Gs7W+YOiRyGsDtR6a6LZ3gs87XsFNx8T8RSIZCtjqA+q7tlc37F7LZHAwzxrZwQtoUxkP7JJ9Rd9XTbuvY6CKP3x0mZznzO+JgdHsAYHPRDmitTvYLJDz7QZB0U6WHpPJHJg0d/1QJ2lGzxPwG0FOF5lEAd9X4eI7TUquUs1P7JYtI4lLeYPQHIa4hC8kOX+OT3wmeiSDHCOnmSwizC9ksORO0nJq+IZ4Gdx4JdgMNg4zb1qxRhkxkpo9AsDEldoQw+nkUbUnDyjMCvcbbRpINtTPed 9ZifqDjW OdJUs/ZTK8luXSrceXLz7vGj05eozmMnziLw+fovrdbBxlVkOxuNzgLWndZP7+4eruKU1EMARJdPge+NKa7wM1VdTHXCH0T35Wx9FZ1HEJAShZBzp6L8jfzUx3vvFxifHkgvZwz3wY71dCz+LM/iK6k7SM4UjE15gifnEn5/owRdIwgZvU7OhVzysiFakcFNCDeXPsRh4qLohKivhe2hot2biRlBWYLOssDrCO1kI6XO6MW86dthM4VEpMABSgKcngmzcdkRiJaZmvbT6oIRROCzg75SbqpcI7una6rmpYlOwCcLUfytJwmePHrDGOTbztPrz7dNJdoaLUc2WXgSHOBkr7QBbeuqf0pza5ldg07jkTyNCGuT7nHiP7QFduni8qDfcd72f+B5grV+Cc/xaxYGQCGCqSiUfK2hUwo9S2U6TGvGT/1SHESX7qO15PTX/ppmRTTkmjOp9KmskOovOfWAr3ZD86BPRmYMTDWHymuYWcVrJSH74UDgX1SNE/s7cpluzqlP5wfve1GVd5nDepJOQaSYbFhwBSpISIDkWPZJCg3ow5Z6kdMTpJQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000005, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Mar 20, 2023 at 10:25:11PM +0800, Vernon Yang wrote: > On Sat, Mar 18, 2023 at 11:13:19AM +0000, Lorenzo Stoakes wrote: > > We are only interested in next if end == next->vm_start (in which case we > > check to see if we can set merge_next), so perform this check alongside > > checking whether curr should be set. > > > > This groups all of the simple range checks together and establishes the > > invariant that, if prev, curr or next are non-NULL then their positions are > > as expected. > > > > Additionally, use the abstract 'vma' object to look up the possible curr or > > next VMA in order to avoid any confusion as to what these variables > > represent - now curr and next are assigned once and only once. > > Hi Lorenzo, > > Due to the "vma" variable is used as an intermediate member, I feels a bit > confusing, so cleanup this patch as below. Hi Vernon, if you read the commit message you'll see what you're undoing here is exactly what I have done on purpose. The point is to assign each of curr and next once and only once after we've determined which of the two we are assigning to. You also delete some of the explanation which I added explicitly to make the logic clearer and adjust _punctionation_ neither of which I feel are positive changes. The point is that existing logic treated either mid or curr as temporary variables that might get reassigned. By using a temporary value and explaining what we're doing, you can see _why_ we are assigning it. > > If this cleanup patch is issue, please let me know, and then ignore it > directly, thanks. So I'm afraid I am not in favour of your change, though thank you for your interest! I am happy to get feedback on the change, though I'd suggest commenting on anything you have issues with rather than attempting to rework my change as if we start getting patches within patches it's going to end like inception :) Cheers, Lorenzo > > ---- > From 7dac3ed8c1b747c2edf2a6c867c4e6342c7447c3 Mon Sep 17 00:00:00 2001 > From: Vernon Yang > Date: Mon, 20 Mar 2023 21:38:09 +0800 > Subject: [PATCH] mm/mmap/vma_merge: set next to NULL if not applicable-cleanup > > Make code logic simpler and more readable. > > Signed-off-by: Vernon Yang > --- > mm/mmap.c | 23 ++++++----------------- > 1 file changed, 6 insertions(+), 17 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 45bd43225013..78cb96774602 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -921,7 +921,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > * If there is a previous VMA specified, find the next, otherwise find > * the first. > */ > - vma = find_vma(mm, prev ? prev->vm_end : 0); > + curr = find_vma(mm, prev ? prev->vm_end : 0); > > /* > * Does the input range span an existing VMA? If so, we designate this > @@ -929,21 +929,19 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > * > * Cases 5 - 8. > */ > - if (vma && end > vma->vm_start) { > - curr = vma; > - > + if (curr && end > curr->vm_start) { > /* > * If the addr - end range spans this VMA entirely, then we > * check to see if another VMA follows it. > * > - * If it is _immediately_ adjacent (checked below), then we > + * If it is immediately adjacent (checked below), then we > * designate it 'next' (cases 6 - 8). > */ > if (curr->vm_end == end) > - vma = find_vma(mm, curr->vm_end); > + next = find_vma(mm, curr->vm_end); > else > /* Case 5. */ > - vma = NULL; > + next = NULL; > } else { > /* > * The addr - end range either spans the end of prev or spans no > @@ -952,19 +950,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > * > * Cases 1 - 4. > */ > + next = curr; > curr = NULL; > } > > - /* > - * We only actually examine the next VMA if it is immediately adjacent > - * to end which sits either at the end of a hole (cases 1 - 3), PPPP > - * (case 4) or CCCC (cases 6 - 8). > - */ > - if (vma && end == vma->vm_start) > - next = vma; > - else > - next = NULL; > - > /* > * By default, we return prev. Cases 3, 4, 8 will instead return next > * and cases 3, 8 will also update vma to point at next. > -- > 2.34.1 > > > > > > This has no functional impact. > > > > Signed-off-by: Lorenzo Stoakes > > --- > > mm/mmap.c | 61 ++++++++++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 49 insertions(+), 12 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index c9834364ac98..66893fc72e03 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -930,15 +930,53 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > if (vm_flags & VM_SPECIAL) > > return NULL; > > > > - curr = find_vma(mm, prev ? prev->vm_end : 0); > > - if (curr && curr->vm_end == end) /* cases 6, 7, 8 */ > > - next = find_vma(mm, curr->vm_end); > > - else > > - next = curr; > > + /* > > + * If there is a previous VMA specified, find the next, otherwise find > > + * the first. > > + */ > > + vma = find_vma(mm, prev ? prev->vm_end : 0); > > + > > + /* > > + * Does the input range span an existing VMA? If so, we designate this > > + * VMA 'curr'. The caller will have ensured that curr->vm_start == addr. > > + * > > + * Cases 5 - 8. > > + */ > > + if (vma && end > vma->vm_start) { > > + curr = vma; > > > > - /* In cases 1 - 4 there's no CCCC vma */ > > - if (curr && end <= curr->vm_start) > > + /* > > + * If the addr - end range spans this VMA entirely, then we > > + * check to see if another VMA follows it. > > + * > > + * If it is _immediately_ adjacent (checked below), then we > > + * designate it 'next' (cases 6 - 8). > > + */ > > + if (curr->vm_end == end) > > + vma = find_vma(mm, curr->vm_end); > > + else > > + /* Case 5. */ > > + vma = NULL; > > + } else { > > + /* > > + * The addr - end range either spans the end of prev or spans no > > + * VMA at all - in either case we dispense with 'curr' and > > + * maintain only 'prev' and (possibly) 'next'. > > + * > > + * Cases 1 - 4. > > + */ > > curr = NULL; > > + } > > + > > + /* > > + * We only actually examine the next VMA if it is immediately adjacent > > + * to end which sits either at the end of a hole (cases 1 - 3), PPPP > > + * (case 4) or CCCC (cases 6 - 8). > > + */ > > + if (vma && end == vma->vm_start) > > + next = vma; > > + else > > + next = NULL; > > > > /* verify some invariant that must be enforced by the caller */ > > VM_WARN_ON(prev && addr <= prev->vm_start); > > @@ -959,11 +997,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > } > > } > > /* Can we merge the successor? */ > > - if (next && end == next->vm_start && > > - mpol_equal(policy, vma_policy(next)) && > > - can_vma_merge_before(next, vm_flags, > > - anon_vma, file, pgoff+pglen, > > - vm_userfaultfd_ctx, anon_name)) { > > + if (next && mpol_equal(policy, vma_policy(next)) && > > + can_vma_merge_before(next, vm_flags, > > + anon_vma, file, pgoff+pglen, > > + vm_userfaultfd_ctx, anon_name)) { > > merge_next = true; > > } > > > > > > -- > > 2.39.2 > >