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 D5F7DC61DA4 for ; Wed, 15 Mar 2023 21:34:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4A3406B0075; Wed, 15 Mar 2023 17:34:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 452F56B0078; Wed, 15 Mar 2023 17:34:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 31A426B007B; Wed, 15 Mar 2023 17:34:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 1F34D6B0075 for ; Wed, 15 Mar 2023 17:34:31 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id E37A51211FD for ; Wed, 15 Mar 2023 21:34:30 +0000 (UTC) X-FDA: 80572436700.04.882A17F Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by imf02.hostedemail.com (Postfix) with ESMTP id ED4488000C for ; Wed, 15 Mar 2023 21:34:28 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=Q64A6ZlD; spf=pass (imf02.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.43 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=1678916069; 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=YLcjo2B8r8ivBq3i6ffImqlDDisi96ITVWAL0NlihTo=; b=3nfAP9TEZwFj3GC2dG8cA/1F3nwpUOlvIQ9qV/eEr+NF2Bw2vK+1mvZXsMFeqbbRlHnrYR SE1XLvJh+rF3nb/J+GrCHenkJukCVrpsU4Xg6+BYwqEHnzfG1pXP4e8DUds8+LgFOswy/S UAVfPPpkLapCeOFnISVWCPD2aYTlQUI= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=Q64A6ZlD; spf=pass (imf02.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.43 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=1678916069; a=rsa-sha256; cv=none; b=q5I96XpgQhjtU2R4vVB0n2FvxmQJjfdwzEbwckCR9EoPaIGcDzIkv6fBUTA5xbMHMyMsWI UmYUGHxg4RpkeZ25H/Z9rJrRyTGFXC3YO0p7o7RYqlm1HS5GRr72u2yBUWY3YiOq8xkPcH AYZu/6opmEXTPzqzuB+Ik0c70KIy4/s= Received: by mail-wr1-f43.google.com with SMTP id h17so4545866wrt.8 for ; Wed, 15 Mar 2023 14:34:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678916067; 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=YLcjo2B8r8ivBq3i6ffImqlDDisi96ITVWAL0NlihTo=; b=Q64A6ZlDe5txZwdIUjX+vFEyMP6wKCYqWXROUtY4u6Fzsu5wQ9rLiTGSaJ0Rr6jt94 tgMo/mBSJw4LF/KTiq+sl/bTusFDK3VNr1dk0sy3aAUC2Sz9c5WnXSRRylLgECuCQF8A 5IiGAKV+d/4ZHvfHrp/qAWGGhlMj0F1f3nLjzqz4IjZ6O5wg1U9chV4DSgHemsTNIRpF 7LZKkk6H769QEmaeN0FrgvRc3OWATWb7vTXtc6CIm1AZ4OhL4Pm+zEcUxG6pWnL/tX9d tWi+svkYEdpkR2Pod1Ik+eHLTqEm7wXUJjgxchr7uqyDhEutb7XGC5MIGrSXottX+Apr gUoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678916067; 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=YLcjo2B8r8ivBq3i6ffImqlDDisi96ITVWAL0NlihTo=; b=It0xnmBhXs6ah7hECGKg7wiPyKZDhnRcRahREUy62V2BN7zVtNoyVj4C9vXAA1pfbv z+4Fwg82NQHEBq5F3jLPX5clJFx5ZjsXOtC7VrGyQtAny/cdoaZHiZpmKsrJAheekwgO MbqAEebD+ADtoOKp7aCyd96vMjmkIfV0QwY/dPC+r56P9/XEn2i1OFPW5E5goHVhROSc DntTa0v6ZigNeKC3mWyYVWN16j3QrLDENVyi3r+/v/AidfwXtDU2JGGU1bgMsNV5AywV MgmOwdbgf6dRrmYPAcmS/gIJfiTNnDs0qUu+rtq8tv8AzJ1XAoff2pfqpbcRSWfaDB9L lkyw== X-Gm-Message-State: AO0yUKWIeSaI13WU1S32l/n3n/ol1wbBPpRECMAGj6QNA2dquaos5GSI uvnmWm2VAnA2qcPFg3X4Gkc= X-Google-Smtp-Source: AK7set8I+as2cu3fa3E4ag8Ty+sXQ8g/o8JmFF1c5BVKl1eVfu8Jzv/xGyDQJiKEbVuprWssmI7beg== X-Received: by 2002:a5d:5003:0:b0:2cf:3396:9126 with SMTP id e3-20020a5d5003000000b002cf33969126mr3277124wrt.10.1678916067184; Wed, 15 Mar 2023 14:34:27 -0700 (PDT) Received: from localhost (host86-146-209-214.range86-146.btcentralplus.com. [86.146.209.214]) by smtp.gmail.com with ESMTPSA id o13-20020a5d4a8d000000b002c5534db60bsm5588826wrq.71.2023.03.15.14.34.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Mar 2023 14:34:26 -0700 (PDT) Date: Wed, 15 Mar 2023 21:34:24 +0000 From: Lorenzo Stoakes To: Vlastimil Babka Cc: Andrew Morton , "Liam R. Howlett" , Matthew Wilcox , linux-mm@kvack.org, linux-kernel@vger.kernel.org, patches@lists.linux.dev, maple-tree@lists.infradead.org Subject: Re: [PATCH 06/10] mm/mmap/vma_merge: set mid to NULL if not applicable Message-ID: <388605ee-261d-4aa9-8d75-4afbee87adbc@lucifer.local> References: <20230309111258.24079-1-vbabka@suse.cz> <20230309111258.24079-7-vbabka@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230309111258.24079-7-vbabka@suse.cz> X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: ED4488000C X-Stat-Signature: 4rucjxdpappc3jtdjkd43z8kmjaz3tsg X-HE-Tag: 1678916068-559117 X-HE-Meta: U2FsdGVkX19S08G8OsKv20kNMGn5ozStQzkJXqxyOK0MpwNe79/0BpVm8kMwhd5/ZgN71HvL8hOs3W4M75laCA3fJ6HEB27U7MZzCtZ4srbHm7alBgGgTUUYcrQ97qA1cbblHBBEfSvO8LTr1dlGDn6padK1fVo/N6WLZhus0CrRBU0nRxhd6mEtAQo5e0mczvQuJu5nOU+UQm2/4Ty4ZB9AwHBL/aLH6EJXglo3++5gFDg2TYrRu+QjyvNRgEgdjpxmXZzNdfyo6SohEyF/5vaAjb2+peB0wJiI7/A2P5lNdMOrxPTiTnsPrc7fAacwmsZqyjq1F85KUCOlI3mjVROQG7UWW7v6KUoezjaP36wD9zWv5/X7Jq1e2XvAYYX7N1zZnoiqWT0any3LX2kgjqBpkJL9RcGeHQqqA2r7h7TWex+7fhxPhMiQmgY7ukR9fi+UM5ohLejFFcEucFFcE4chxcNY1Ad0bqYQNPUk+K6XHilyTGoBUSghX4SpeTtSbsy7sIfs1+koM/iiSNhzJ4O+TB8SHmPzcnJO50g1HuwEL/VKxX4peoilSTZzF8HaZU6lGvcsrZSJ+0k9UXmH1pOlvgf5FenNTlwRHR8RiKXzwAgceYmTMDKM4g+rH1qfrFuUD7pPshc2+tc+P/nVYpG1dmxxB+vftvU+3KzoUYAVGZBmMgvf69p2mYPSgcnBzzleBQag2e80eeafsChHu7rPCPxDblbDK9EZGEp7Roj9//4TlIpAq4+3SwRLseaIEahc9eRbTs1LgtkhMIKjRi1gQ0WW5+qjBCFMzCExxA+zfyA/KJXQF0p9spqCoIx7ebSD6pUM/spLtEJAhRJCgx9MPCSsYn/qHxSphqPq0RCmBX84iZEEYi/m0VgevSo2g6tWzRdPtkvIYR4O4WrwBm98lYl8KzyO0z3z1/TW/nJhICiUqqWNfPZ8dmCJgbHYc7bbSmY5f7dLDBCMTgY lGPaEt0n Ut8/Llf1ZrFIGSv1sHxitxRGrISRG1vsgXBU33DOVtPaCx/Naxa6M7erZgB2AprU44EdoOtk8ICfFSNNCIYI6UcRW3q3hbn4YITaXpz8y5FpuUDuYG4GgLNbSVM1a1j1oo1bXXlz957idxIUgKt78czj76z8GdwRDub1+kmm5KJGs4hSnK0GY6PVcaWPkEZ/4x8KRHHx1ADx13SkUribhzONs16Nsb1ndUNjNyfYjzdYQyqsARNuwECoN6hZDvI2bmRs/YWiJQ9Tn1yu1ViFnxzaZ8ddqLRRU5mK+dKeJ4qZCJ3WOXrvcHsL7+CVRfxwsPHO+9He1SbFTqk2lZuatU8ggQLGU3VipJQvfHOCkgXjcPJ/QdrtKdgcMGt5MwJKtZ9hDwBuAbCNZBx/xLpTn2TmRPAyfPzlsRzNYmnXi1/O1h9gHBNnOMIz7l1VNIW63NMC+6dxg0kU7V1xX8iMO8ypVvsp8nuxE+0N2 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: On Thu, Mar 09, 2023 at 12:12:54PM +0100, Vlastimil Babka wrote: > There are several places where we test if 'mid' is really the area NNNN > in the diagram and the tests have two variants and are non-obvious to > follow. Instead, set 'mid' to NULL up-front if it's not the NNNN area, > and simplify the tests. > > Also update the description in comment accordingly. > > Signed-off-by: Vlastimil Babka > --- > mm/mmap.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index be60b344e4b1..3396c9b13f1c 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -848,10 +848,11 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags, > * > * The following mprotect cases have to be considered, where AAAA is > * the area passed down from mprotect_fixup, never extending beyond one > - * vma, PPPPPP is the prev vma specified, and NNNNNN the next vma after: > + * vma, PPPPPP is the prev vma specified, NNNN is a vma that overlaps > + * the area AAAA and XXXXXX the next vma after AAAA: I think this is worded in a bit of a confusing way + can be read as 'NNNN is a vma that overlaps the area AAAA and XXXXXX' whereas you mean to say 'NNNN is a VMA that overlaps the area AAAA, and XXXXXX is the next vma after AAAA'. This therefore might be better worded as:- 'PPPP is the previous VMA, NNNN is a VMA which overlaps AAAA and XXXX is the next VMA after AAAA.' Also - nit, but there's also inconsistency here between the number of letters in each block, e.g. 6 P's 4 N's 4 A's and 6 X's. 'N' and 'X' are starting to be horrifically misleading here imo, I feel as if 'N' moving to 'O' (for overlapping) and 'X' to 'N' would make a big difference here. > * > * AAAA AAAA AAAA > - * PPPPPPNNNNNN PPPPPPXXXXXX PPPPPPNNNNNN > + * PPPPPPXXXXXX PPPPPPXXXXXX PPPPPPNNNNNN > * cannot merge might become might become > * PPXXXXXXXXXX PPPPPPPPPPNN > * mmap, brk or case 4 below case 5 below > @@ -879,9 +880,10 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags, > * > * In the code below: > * PPPP is represented by *prev > - * NNNN is represented by *mid (and possibly equal to *next) > - * XXXX is represented by *next or not represented at all. > - * AAAA is not represented - it will be merged or the function will return NULL > + * NNNN is represented by *mid or not represented at all (NULL) > + * XXXX is represented by *next or not represented at all (NULL) > + * AAAA is not represented - it will be merged and the vma containing the > + * area is returned, or the function will return NULL > */ > struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > struct vm_area_struct *prev, unsigned long addr, > @@ -918,6 +920,9 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > else > next = mid; > > + if (mid && end <= mid->vm_start) > + mid = NULL; > + Might be worth putting a comment with the cases where this will happen, 1 - 4 right? And also something like 'does AAAA overlap with mid?' And I really think renaming this to 'overlapping' or 'overlaps' or similar would make a big readability difference. However we do have the thorny issue of case 4 where A overlaps P... But probably the fact that we treat this as a separate VMA from prev is enough to make it clear it being called 'overlaps' means 'separate from prev, also overlaps' so I think that's fine. Adding this actually makes me think twice about the previous 'natural order' patch, because the intuition which that promotes is:- mid = VMA after prev next = VMA after mid [ prev ] [ mid ] [ next ] But in reality that else branch means that next could be be equal to mid and now if there isn't overlap we rename mid to next effectively, e.g.:- mid = VMA after prev next = mid delete mid Which feels like the 'natural' intuition is suddenly broken. Maybe this needs reworking to be super explicit about this? Such as:- struct vm_area_struct tmp; ... /* If there is a previous VMA, find the next, otherwise find the first. */ tmp = find_vma(mm, prev ? prev->vm_end : 0); /* * If the address range overlaps with the input range (which can cover only a * single VMA at most), then we are only interested in next if we span right up * to its end. * * Otherwise we are simply left with prev and next. */ overlaps = tmp && end > tmp->vm_start ? tmp : NULL; if (overlaps) next = overlaps->vm_end == end ? overlaps->vm_next : NULL; else next = tmp; Of course I haven't read the rest of the patches in this series so you may address aspects of this already :) > /* verify some invariant that must be enforced by the caller */ > VM_WARN_ON(prev && addr <= prev->vm_start); > VM_WARN_ON(mid && end > mid->vm_end); > @@ -952,7 +957,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > remove = next; /* case 1 */ > vma_end = next->vm_end; > err = dup_anon_vma(prev, next); > - if (mid != next) { /* case 6 */ > + if (mid) { /* case 6 */ > remove = mid; > remove2 = next; > if (!next->anon_vma) > @@ -960,7 +965,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > } > } else if (merge_prev) { > err = 0; /* case 2 */ > - if (mid && end > mid->vm_start) { > + if (mid) { > err = dup_anon_vma(prev, mid); > if (end == mid->vm_end) { /* case 7 */ > remove = mid; > @@ -982,7 +987,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > vma_end = next->vm_end; > vma_pgoff = next->vm_pgoff; > err = 0; > - if (mid != next) { /* case 8 */ > + if (mid) { /* case 8 */ > vma_pgoff = mid->vm_pgoff; > remove = mid; > err = dup_anon_vma(next, mid); > -- > 2.39.2 > Other than the nitty comment notes and the conceptual discussion, this LGTM so:- Reviewed-By: Lorenzo Stoakes