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 4C981C48BC4 for ; Tue, 20 Feb 2024 22:12:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 990736B0072; Tue, 20 Feb 2024 17:12:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 9407E6B0074; Tue, 20 Feb 2024 17:12:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 82F6C6B0075; Tue, 20 Feb 2024 17:12:34 -0500 (EST) 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 749D86B0072 for ; Tue, 20 Feb 2024 17:12:34 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 01B3BA0BA7 for ; Tue, 20 Feb 2024 22:12:33 +0000 (UTC) X-FDA: 81813582228.17.9F8353D Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by imf13.hostedemail.com (Postfix) with ESMTP id 051822000F for ; Tue, 20 Feb 2024 22:12:31 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=VUfa+A69; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708467152; 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=SNaRiN9Bf5zdH/MAgVRJX5f88QotWSNrDLzJMcP+CQc=; b=gxZkKMsp4C3/RBSleN7YxZhscSbqYZm8h4ymxlI5sk5VFest9xW0cr3xRgb+yiV1NOAQcj lAPjDvY37A1oVPGy1XLjcLaxF43u4RKO3Jcg7g2PDvHCrK0x0NMcCpKIMDULryjAix9sdb jRdNkr7kzNLMVqltvHuFQepscKjRTnk= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=VUfa+A69; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.50 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708467152; a=rsa-sha256; cv=none; b=q2bYfolmam4wJForaQ1pkqc6HevpRdQgeJf5Xc6noXP7M1By4kN1oxjuxtqHBUfPhvhOPB gSXDNzSf4oQkP1NZpH4ijzYAXRRt95FzvsGMAr22+/X03eNU4F8307m5D+XwMf37pPN/xy /r/bTrs+BbJ1nuU+un/vdH7zv/xGWfU= Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-4127109694aso6229075e9.3 for ; Tue, 20 Feb 2024 14:12:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708467150; x=1709071950; darn=kvack.org; 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=SNaRiN9Bf5zdH/MAgVRJX5f88QotWSNrDLzJMcP+CQc=; b=VUfa+A692V78prNQhHoJ6r77/RAH80BHec1JJsoNRbT2g2etg11DJ4NGYfnv3mKeXD wQTGQFNaJcraHciVvhFAnmvPKPW0MKZ6sAv4luoJOB+w6Es3RHn54vrqirNLI1aiKEsE fj8mn6lsJJjs7OsK3TJ9MLKmplCalqOKaRue1P/6cSEfCKfHVAT8thfy2Q1AK5SA+go9 s2/dYWRKRhW+Dmu/5eiBwmlRUMB3s8e02x3XZlOn9s5M5EuqKi+lOmyNHoBGMjwMntuj hI3bygNi8rJaio7AFvPfTpXxQoZSHhHOdBQt7IN0AEDgir53RVjMeYGZX9DTSJiOtFT/ 0BGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708467150; x=1709071950; 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=SNaRiN9Bf5zdH/MAgVRJX5f88QotWSNrDLzJMcP+CQc=; b=Je+NVOGENoFTEDseKFbjlF5u2pOcbC/4Z/dfm6d3vp8dpG4O7GRjPV0HUxzyRvBVGS wx21hGUfVGPz1qv4pdUSGB+yjH8Sy1vhBsKRg3TysIRw/cIlBRw022o19rTEYdIp2fjm GQ8FnB0n+E61JWH2U3nDA7qlYn300sZnwPBUIb6sQ+KvhjcZ/kHd6+7MSGir1VJMAFeX eyX4ecUJqzDl5JyICZHdyFZ6HiZu3DLAZOcM0VAQz8ByMr/7bWElmZHe8PmZSyk4vt37 VuEWh5FCrNJUA4iYjvfPhptbWGXA3hKAtTDr45XpMrJ33EaBn/PgmOPLonZW+ulCOOvy 8EOA== X-Forwarded-Encrypted: i=1; AJvYcCUEl3E0hYYFveheEf1OWtRsE/zORX0Me90vx1XqAEpvRewIfHyjDoqUDQ3b6h7wIpQ3hCclw2Mdm2OQUtVGH760D28= X-Gm-Message-State: AOJu0YxqnTKGyp/+X6TfJCopbAhQ21PbC3VOUE4qNjaK1qsi1G0nkEVW Emt8rWP3fx4iRGPNRecO3pyha45LTdminexZ12Csv3jd515FKA71 X-Google-Smtp-Source: AGHT+IHzTXhHrUUQOhFclGSxI9cVcHGOgBNyNE7dn6pzkooYv3QZnq/3zXnUSQCwdbOg5bewV/mhzQ== X-Received: by 2002:a05:600c:b97:b0:412:7190:ad0f with SMTP id fl23-20020a05600c0b9700b004127190ad0fmr1260528wmb.11.1708467150120; Tue, 20 Feb 2024 14:12:30 -0800 (PST) Received: from localhost (host86-164-109-77.range86-164.btcentralplus.com. [86.164.109.77]) by smtp.gmail.com with ESMTPSA id fs14-20020a05600c3f8e00b00411a595d56bsm15789922wmb.14.2024.02.20.14.12.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Feb 2024 14:12:29 -0800 (PST) Date: Tue, 20 Feb 2024 22:10:13 +0000 From: Lorenzo Stoakes To: Yajun Deng Cc: akpm@linux-foundation.org, Liam.Howlett@oracle.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, vbabka@suse.cz Subject: Re: [PATCH] mm/mmap: Add case 9 in vma_merge() Message-ID: <33023080-9e11-475e-a199-b7ff1e655c57@lucifer.local> References: <20240218085028.3294332-1-yajun.deng@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 051822000F X-Stat-Signature: as9mmpe7ipbu9z3swjuqhipjhnhtx1mj X-HE-Tag: 1708467151-920138 X-HE-Meta: U2FsdGVkX18wAPYoie+oFmPVnT+qHtURfRoR2UjlMdiwxOHfoPHw/Ap1JhTgnBXPIu5yloo9+A1gs03vwEki4TmMaCHibCFc48X4ykb5IEm7rGeS7Tkfe1w68LceEN0N2OgnER1G5Yx9VwHHAEIzWm6vuTFYSpnVriG0MaqdlreKtFmIacgm/20NqgDsYB4Gi4Sq5eh7cadG6CxnehdSNj97MYEPKlWiMN21QKGN6OWKaTXFqxtQ6/OjZ8sX7CJkHMNx2mRLyQh3H6hcCRtEQ5J2+lv0FjHCvBxIZs/VmUMD59uV3b7kqZRbX/ZuBLlYR1ZQaW/L1d5RSMjROmlkgw79mbNhY0uxEvOBafqn/I43Voai+9zxh3ZtgZWJ41kcmgtEKRUo82rbq3npWEETvrjCid1WyY/IqrDGRP9noXvMbrSLyMhvr7T/UAYfPlghlb2WVo1w3zhXDMwXKij7mBEBau+AEb2DcEDeS877sIyrG44AsQLa81wM1P5Aa2v1Vce/BIeFzSlzWKmbyq2pa0oYMF4bF/R534GaEg4W84VGmlIUSytgcbwEqIggqd+apfIZxTyK7lHaDU3FN9Zu+QB8tyd7ZrIHld+YsXTGo2RMINTQCXyFBEw5ztARq9Prl+gOYwH6fwKpFUS4D+MFrgue4uIeji0qxzWlOQOHiyRwNzoua+qYSQ2hkm2gWcils2FZ0qldS4sKVw9c5OwqFCZPr8KO10+jjRDt4d1CUXqtf1/tq1EE4yGGkfVZufISpUPAjdX3C2VyoM/9306GEj3XAYwVLPe6zdprxSL3+yZpPTanL1bYDt/YqncbAiOxF9YSwme50Zdb4jI/SV2fIG1cjv6xOZ4SZY9BgFmsaQCFyyYTDe2ownu5RLoNHB4KHHGotGRBBIHbJ12yHiOR50TW+ZPxC0WeWNsAi3CYI1hmbUtGGwiUWITVeKPRlBjGGO3mKH27mFd3QVAQtO1 oES8bAY3 UtqxO7lVKAx0cIv/BIpMxAcfZlbmHfIqA+J3+35+q39KwzsS/ulLL+AkJFX5GLVcsE+IWrxL3m7vb0Sh5R4v96Yb11D0DLBCkUab+b0nv318dsu3QCNk6DyazQ2ZeKXzHZ9sn7VZ612cARihjIZ/AS8WvM8xhowtThd7gxe1tRWrIpuPx1BgqBfq2Y1FRWJb+9q0rEgpj2QpOxftzl+Nn3NC7IWTHO8c5WaZr+avZlBtOQceyublZf+qyodmVa53iNX6oT8oGf1o0CHHOVNSR0VP/lMDro6EHwPXxCfPJHM2Ju5zp4EGMfh6eb1Bw/pb6ZNM60l7L1qf4FaR7uuOWxvEga36sCqefjv6FFvOkQ7wVYijG8VE74Xr+4+hVpXKzdNRd15Luxk8+Zcfm+h5kq8LesraqmJnPB39x3sU2DmPkdObGwIgTHOV6cQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.043941, 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, Feb 20, 2024 at 09:00:15PM +0000, Lorenzo Stoakes wrote: > On Tue, Feb 20, 2024 at 11:00:30AM +0800, Yajun Deng wrote: > > > > On 2024/2/19 07:03, Lorenzo Stoakes wrote: > [snip] > > > > Yes, it's not a merge case. I label this to make it easier to understand. > > OK, I guess I have to be more explicit + less soft here to avoid confusion > as you seem not to be paying attention to what I have said - We can't have > this in the patch, full stop. > > I (+ Liam) have already explained above as to why, but to emphasise - each > case number refers to a merge case consistently throughout. Arbitrarily > adding a new case label to describe one of the many early exit conditions > proactively HURTS understanding. > > > > > > > * PPNNNNNNNNNN PPPPPPPPPPCC > > > > * mmap, brk or case 4 below case 5 below > > > > * mremap move: > > > > @@ -890,6 +890,9 @@ static struct vm_area_struct > > > > if (vm_flags & VM_SPECIAL) > > > > return NULL; > > > > > > > > + if (prev && end < prev->vm_end) /* case 9 */ > > > > + return NULL; > > > > + > > > I need to get back into vma_merge() head space, but I don't actually think > > > a caller that's behaving correctly should ever do this. I know the ASCII > > > diagram above lists it as a thing that can happen, but I think we > > > implicitly avoid this from the way we invoke callers. Either prev == vma as > > > per vma_merge_extend(), or the loops that invoke vma_merge_new_vma() > > > wouldn't permit this to occur. > > No, it will actually happen. That's why I submitted this patch. > > You aren't explaining any situation where this would happen. As Liam says, > this is something you have to provide. > > I have taken a moment to look into this and I am afraid I don't feel this > patch makes sense. > > Firstly, let's assume you're right and we can reach this function with end > < prev->vm_end: > > 1. curr will be NULL as find_vma_intersection(mm, prev->vm_end, end) will > always find nothing since end < prev->vm_end. > > 2. We discover next by using vma_lookup(mm, end). This will always be NULL > since no VMA starts at end (it is < prev->vm_end so within prev). > > 3. Therefore next will always be NULL. > > 4. Therefore the only situation in which the function would proceed is that > checked in the 'if (prev)' block, but that checks whether addr == > prev->vm_end, but since end < prev->vm_end, it can't [we explicitly > check for addr >= end in a VM_WARN_ON()]. OK I say below about feel fre to embarrass me if I am mistaken, but I can go ahead and do embarrass myself directly :) Apologies - the above is not correct, vma_lookup() doesn't look for an exact match so will assign next = prev in this scenario. However the vm_pgoff check will cause this case to fail also. > > Therefore - we will always abort in this case, and your early check is > really not that useful - it's not something that is likely to come up > (actually I don't think that it can come up, we'll come on to that), and so > being very slightly delayed in exiting is not a great gain. > > You are then also introducing a fairly useless branch for everybody else > for - if it even exists - a very rare scenario. I do not think this is a > good RoI. > > As to whether this can happen - I have dug a bit into callers: > > 1. vma_merge_extend() always specifies vma->vm_end as the start explicitly > to extend the VMA so this scenario isn't possible. > > 2. Both callers of vma_merge_new_vma() are trying to insert a new VMA and > explicitly look for a prev VMA and thus should never trigger this > scenario. > > This leaves vma_modify(), and again I can't see a case where prev would not > actually be the previous VMA, with start/end set accordingly. > > I am happy to be corrected/embarrassed if I'm missed something out here > (vma_merge() is a great function for creating confusion + causing unlikely > scenarios), so please do provide details of such a case if you can find > one. Also, I discovered that I can *ahem* reliably reproduce this scenario, so apologies, you were right to say it can arise (but it would have been useful for you to give details!) I can repro it in mprotect_fixup(), quite reliably, so it might be worth digging into why this scenario is happening. > > TL;DR: > > - The case 9 stuff is completely wrong. > - I do not think this patch is useful even if the scenario you describe > arises. > - I can't see how the scenario you describe could arise. > > So overall, unless you can provide compelling evidence for both this > scenario actually occurring in practice AND the need for an early exit, > this patch is a no-go. > > In addition, if you were to find such, you'd really really need to beef out > the commit message, which is far too short, and frankly incorrect at this > point - if you perform a branch which 99.9999% of the time is not taken, > you are not 'reducing unnecessary operations' you are creating them. > > If you could find compelling evidence to support this patch and send this > as a v2 then I'd consider it, but for the patch in its current form: > > NACK. I'll stick with the NACK (though slightly less assuredly), as the case 9 stuff has to go, that is the real bugbear here for me. It's more review feedback, but the commit message needs beefing up in any case too. I'm still not hugely convinced this is a worthwhile early exit, since we don't treat any other 'can't merge' case specially here. I think the more interesting thing to do here is to figure out why mprotect_fix() (or perhaps others) is even trying the merge under these circumstances. Again as I said in the first reply, thanks for your efforts looking at this! The nacking + such is simply an effort to ensure we are being extremely careful around this highly delicate function. Thanks, Lorenzo