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 CB51CC6FD1C for ; Wed, 22 Mar 2023 06:09:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 18F0C6B0075; Wed, 22 Mar 2023 02:09:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 13D6C6B0078; Wed, 22 Mar 2023 02:09:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 004456B007B; Wed, 22 Mar 2023 02:09:50 -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 E4A5A6B0075 for ; Wed, 22 Mar 2023 02:09:50 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id B6063A02E2 for ; Wed, 22 Mar 2023 06:09:50 +0000 (UTC) X-FDA: 80595508140.15.53A8ECC Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by imf14.hostedemail.com (Postfix) with ESMTP id E9DC5100014 for ; Wed, 22 Mar 2023 06:09:48 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ZUDcg4zO; spf=pass (imf14.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.44 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=1679465389; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=HvjMFKouzMGD9RKqt33OnSeBDLHBYhIAGSylF79gu5Q=; b=gij+fVcwVo092iwRystPFUpC6LZ3oChA9KvbbOlh8cfKYr8NlrL2O/jvA1BADWaADyMn1L k18l/hgr10fx1XlmIeeLpQPOkxWYPiaypFlpu2+HwmlNFcWoQ0jHpokB9DJ3PvrywNZihA 0i7Q7/ISpq58b84gPrOEvldRzjEpqZM= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ZUDcg4zO; spf=pass (imf14.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.44 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=1679465389; a=rsa-sha256; cv=none; b=1eibYjBC/IT5MyNUQeuTdTT+Mr2eQn5N45YjzZq8zHCSc6bQssi+AcUhnxwI0MmpXfL57l WYJUwCCpKtPC38CEpLdQ+RLgxUg7skZ/wo9j4sD9ElF3dTnjIYJQDg6uYtB05teTmhXz3E cnBSgTrpHkyUv6OcPSCJyy52iQZo8tA= Received: by mail-wr1-f44.google.com with SMTP id r29so15816543wra.13 for ; Tue, 21 Mar 2023 23:09:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1679465387; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=HvjMFKouzMGD9RKqt33OnSeBDLHBYhIAGSylF79gu5Q=; b=ZUDcg4zOXbE2dv8/jyQLpHSsG7dDHcpPYMGv3MVqVNF5VT7rNnACF8uaThCuwHLsnE r8Bt4X+mNWQJ4t2v7VSUlrW6zlEspRdoPXHD7t1qOZ2b+JN9d/hAoJqDCC1TPGABfJJ4 D5+uc3ZkQSbGDZe/P1DIvk2zDdNkTm4cduJzryhdc00QxYbwnScD8bJ++FnVOyG2CxuK sOQmk6eHRjnP95OYvsNHzCrWsX27nmxbCHOm/iipmp85uscq2Pz6RaUsc3M/VOMCz+dL LfxOinYprIHXRm2UHq0DH0pF1MnopXHl/tbpa1Fc9Q8PvLsjVhLy5d9v9jFErkcDAOmA bjBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679465387; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=HvjMFKouzMGD9RKqt33OnSeBDLHBYhIAGSylF79gu5Q=; b=xBtmXHSd6eJZa462NvuGubdbkI3i01QCrBlgefcX8/Gx8PPaow67kVoTHctayfAihf q9av90Jl9Gz+b3vNYnQrx5rDBWJEFI0laSuITlLj7+utNGf/Lth6Z+A1MMcyMY8mjdHx kGZdd+Q1ncxJAS3Sm0L207OGH8TBnFXTtSF/4Ut0z/mV+rLYGt/ZKtpcm+DT9zwiZdWI SB2AMU1vje/oU+MGE2KEa/CuhlNgM4+RtdHIuVSeYk9fTZY4/7LmEw4MWvb/P3/ZlBwn zlUIKYCPdjAKitI3QPIOOLFVMW1NxTyCXofuh8jlCAVzzPcq5zziXCZJJ4IX5ZTMw9+k 22hg== X-Gm-Message-State: AO0yUKU67T3NxmIe9p+mdLb2hhGM13bxCrnCIlD4OA1YRZCcprN7uwoK KQswiYBs7mKS5gExtMT8JbI= X-Google-Smtp-Source: AK7set8y6o2EV7n6CNBK6TLR8rdVei4DkUckGcIOfXJpfDktQHWW4uq02p4KSucFzmEb4AJUtk7OYA== X-Received: by 2002:a5d:4ec1:0:b0:2ce:a68a:86fe with SMTP id s1-20020a5d4ec1000000b002cea68a86femr4081178wrv.69.1679465387322; Tue, 21 Mar 2023 23:09:47 -0700 (PDT) Received: from localhost (host86-146-209-214.range86-146.btcentralplus.com. [86.146.209.214]) by smtp.gmail.com with ESMTPSA id m20-20020a05600c3b1400b003dc434b39c7sm1301175wms.0.2023.03.21.23.09.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Mar 2023 23:09:46 -0700 (PDT) Date: Wed, 22 Mar 2023 06:07:35 +0000 From: Lorenzo Stoakes To: "Liam R. Howlett" , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , David Hildenbrand , Matthew Wilcox , Vlastimil Babka , maple-tree@lists.infradead.org, Vernon Yang Subject: Re: [PATCH v2 4/4] mm/mmap/vma_merge: init cleanup, be explicit about the non-mergeable case Message-ID: <35e1582a-a64f-44c2-983f-bb1cde6dd98a@lucifer.local> References: <20230322020858.msvpvit63sb437ze@revolver> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230322020858.msvpvit63sb437ze@revolver> X-Rspamd-Queue-Id: E9DC5100014 X-Stat-Signature: aekr9k9i149ctohwuusqxnqufuifptmz X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1679465388-603799 X-HE-Meta: U2FsdGVkX18yC8009z/cwrq662AoYTdcPtnSo8MMSrrMcO0RZP9nUQdfsJqY7nobijS8RSDkronrgH6PLMvDqq8kutN892WlWlBgKYYbxLL7ITsSx7pVJnrMS9ZpXbNtpenwDEOgO9FmL1m3ueOkVfQguk7RFvKStVjsYdoWbP3vtSZfNXNqZ/d1Wn3VNu5cbDJw3VISi6equ+8x2l18YCg9dTBfHZRx9kmsrTtqQt/UvHNnzGmPC9yCXtuDem8U9A3wnAqQdNIc7wkw94F/mQLjCoOBOpq4cgF24WFO0MTF9Xh6r6eRz/gkQAUvuJTAo5/Lr163rqW3FrvciSwh99qKDzbF/yjUPKXabstwEmJX0wazwruVQCRrSIHyUl95pk4L8+PcqQbpptgc7gNSp4XVIDyOxw4qf/wl1mZ/e9AWwORF/bMwNWkzkawN+NnL9DTS0gcsdzI0fGSRbYd9Ua10pLy4VQLaUQyN2NMO8Y3MCZQ5X5UAuGtUXXlZqDZLPXBmATk3ppKUSslH2N+dhoPVleIYKTjC+IEDohLRcAEttxfhoaoIKvVdF3Jj0vRuZ+NuJepGYBgOPkGC7JmlJmc88Go/HNW5Tz9e3Tc4Vij/tvkwkqlycJCycgvTlEDdUf2hqq7ZGxnVcoVkEHPHbxKOT4XrOUxy+LeVCcDVokiXLWjZWxkEWfs76np1sDVCEVYbREgsnfI63YqhcbsReLHftl6oOsCU5k6i0ATGTZJazGbmZJPbhpiHaUAPXeakTvLEpwt9dFoWKCc5nhJnw04orhikEicl57XcGV8SPhumMxGU0V5p37eiqeHfX0crlbUBwXTYHeC9S9SP5mNo8Zm7f5BeNmGKkzH6qXVyo3wjXrOW8fuFG2s8rAF+YgWEkDQQ3dBE6I6IS9422z9pFepWerZyY5ZhK+j12lx60l2S5CRL8aUVmvbmzCP4sfTroM3ImK2dxl55OB2JNtg uVz8SjgT 2pjw9n1I7kYVx1BEixHAtHZdLfgsQwZCZQRSwmYUBdiToz6/oHQGKWcMN2hBwv4yvPfHqPf+z6G/giZ/0MDXpp+tV0DhJFs6AwkgG2gfcnYPrpBvAVtluq9U7/5U7ykAZI6Bq2FFXK+Tf9FTix0NP/mNXmcsB5X4F/acrc24ZasVKMUBxWiDS7QWSOAEwRP9lc8+o73qgDCw4ZBb/vI90epHkB8oanDs8i2DppuJ3IT46ixa5mZDP94lV9VuWWwdlYuOQPiBmag5tJwMAWSOYCMsbb2g+Z3PhHGswzwpiGPfWZYODY9u0An5twMzxCeAn9rZyC7r4Vh6CXJMnanuHA4JFp7DVgVB3p6A09NzovFsGqztt/+9CF4i89fJhF4ZcCc09UC+SVFBxt6DHnAV2+8/T2jTrVvZ6dsjf1oq6bgs4W+/gIMPmWIGAK4hj12sKJLn1HaFl5J7nFH1tQPTPY2tHnW2dBqlx0s69 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 Tue, Mar 21, 2023 at 10:08:58PM -0400, Liam R. Howlett wrote: > * Lorenzo Stoakes [230321 16:46]: > > Reorder the initial variables sensibly and set vma_start and vm_pgoff there > ^vma_pgoff > Indicating it is used for the vm_area_struct *vma > > > rather than later so all initial values are set at the same time meaning we > > don't have to set these later. > > I did these later to reduce the number of times we were checking prev. > With this patch, we check prev 3 times, but before we were checking it > once. The compiler might do something clever here to reduce the > checking? > Apologies for undoing your work! Which obviously wasn't my intention :) I suspect the compiler would, but we probably shouldn't rely on it. > I'm also not sure adding the conditional operator in the init helps your > goal of cleaning it up. The purpose was to group everything together, reduce indentation in the prev case, have it resemble the next case more closely, reduce LoC and to have the prev if block be solely concerned with merging the predecessor rather than both setting these values and then checking to see if we can merge the predecessor. However, on second thoughts I think avoiding repeatedly checking it trumps that so I will revert to the previous approach. > > > > > Rather than setting err = -1 and only resetting if we hit merge cases, > > explicitly check the non-mergeable case to make it abundantly clear that we > > only proceed with the rest if something is mergeable, default err to 0 and > > only update if an error might occur. > > > > Move the merge_prev, merge_next cases closer to the logic determining curr, > > next. > > > > This has no functional impact. > > > > Signed-off-by: Lorenzo Stoakes > > --- > > mm/mmap.c | 55 ++++++++++++++++++++++++++----------------------------- > > 1 file changed, 26 insertions(+), 29 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 7aec49c3bc74..d60cb0b7ae15 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -909,18 +909,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > struct vm_userfaultfd_ctx vm_userfaultfd_ctx, > > struct anon_vma_name *anon_name) > > { > > - pgoff_t pglen = (end - addr) >> PAGE_SHIFT; > > - pgoff_t vma_pgoff; > > struct vm_area_struct *curr, *next, *res; > > struct vm_area_struct *vma, *adjust, *remove, *remove2; > > - int err = -1; > > + struct vma_prepare vp; > > + int err = 0; > > bool merge_prev = false; > > bool merge_next = false; > > bool vma_expanded = false; > > - struct vma_prepare vp; > > + unsigned long vma_start = prev ? prev->vm_start : addr; > > unsigned long vma_end = end; > > + pgoff_t vma_pgoff = prev ? prev->vm_pgoff : 0; > > + pgoff_t pglen = (end - addr) >> PAGE_SHIFT; > > long adj_start = 0; > > - unsigned long vma_start = addr; > > > > validate_mm(mm); > > /* > > @@ -940,6 +940,23 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > /* Is there a VMA next to a hole (case 1 - 3) or prev (4)? */ > > next = vma_lookup(mm, end); > > > > + /* Can we merge the predecessor? */ > > + if (prev && addr == prev->vm_end && mpol_equal(vma_policy(prev), policy) > > + && can_vma_merge_after(prev, vm_flags, anon_vma, file, > > + pgoff, vm_userfaultfd_ctx, anon_name)) { > > + merge_prev = true; > > + vma_prev(vmi); > > + } > > + > > + /* Can we merge the successor? */ > > + merge_next = next && mpol_equal(policy, vma_policy(next)) && > > + can_vma_merge_before(next, vm_flags, > > + anon_vma, file, pgoff+pglen, > > + vm_userfaultfd_ctx, anon_name); > > + > > + if (!merge_prev && !merge_next) > > + return NULL; /* Not mergeable. */ > > + > > /* > > * 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. > > @@ -951,26 +968,6 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > VM_WARN_ON(curr && (addr != curr->vm_start || end > curr->vm_end)); > > VM_WARN_ON(addr >= end); > > > > - if (prev) { > > - vma_start = prev->vm_start; > > - vma_pgoff = prev->vm_pgoff; > > - /* Can we merge the predecessor? */ > > - if (prev->vm_end == addr && mpol_equal(vma_policy(prev), policy) > > - && can_vma_merge_after(prev, vm_flags, anon_vma, file, > > - pgoff, vm_userfaultfd_ctx, anon_name)) { > > - merge_prev = true; > > - vma_prev(vmi); > > - } > > - } > > - > > - /* Can we merge the successor? */ > > - 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; > > - } > > - > > remove = remove2 = adjust = NULL; > > /* Can we merge both the predecessor and the successor? */ > > if (merge_prev && merge_next && > > @@ -985,7 +982,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > err = dup_anon_vma(prev, curr); > > } > > } else if (merge_prev) { > > - err = 0; /* case 2 */ > > + /* case 2 */ > > if (curr) { > > err = dup_anon_vma(prev, curr); > > if (end == curr->vm_end) { /* case 7 */ > > @@ -995,7 +992,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > adj_start = (end - curr->vm_start); > > } > > } > > - } else if (merge_next) { > > + } else { /* merge_next */ > > res = next; > > if (prev && addr < prev->vm_end) { /* case 4 */ > > vma_end = addr; > > @@ -1011,7 +1008,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > vma_start = addr; > > vma_end = next->vm_end; > > vma_pgoff = next->vm_pgoff; > > - err = 0; > > + > > Was this blank line intentional? I assume so, to give a gap for the > comment below? It's probably worth having. > > > if (curr) { /* case 8 */ > > vma_pgoff = curr->vm_pgoff; > > remove = curr; > > @@ -1020,7 +1017,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm, > > } > > } > > > > - /* Cannot merge or error in anon_vma clone */ > > + /* Error in anon_vma clone. */ > > if (err) > > return NULL; > > > > -- > > 2.39.2 > > > >