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 0A70FC48BC4 for ; Tue, 20 Feb 2024 21:02:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7C7476B0081; Tue, 20 Feb 2024 16:02:36 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 777A66B0082; Tue, 20 Feb 2024 16:02:36 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 617D36B0083; Tue, 20 Feb 2024 16:02:36 -0500 (EST) 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 4EF436B0081 for ; Tue, 20 Feb 2024 16:02:36 -0500 (EST) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id F32CAA0B92 for ; Tue, 20 Feb 2024 21:02:35 +0000 (UTC) X-FDA: 81813405870.02.9E4A5E9 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by imf02.hostedemail.com (Postfix) with ESMTP id D974C80021 for ; Tue, 20 Feb 2024 21:02:33 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=R4y8Bnsd; spf=pass (imf02.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.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=1708462953; 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=vHk8B1G9ngPFWZyvzcJjpFnWjSLSm2alsXCknI8ZS8Y=; b=d5FsLtm3lQgsgF2sJIbCrEhfUD4OTuV2UGzV46Z9Gh+J912hBrXSD+lsXvkrDQdyePuBGQ tM/txjvwQ6AZ0mxJKlTgTIiY4sOy3gJQey599mCO4QX6HCx/o5VEv33PjG2cPz5WP0kF2E d304yTrO/2Gcv04Vrv472szeUPlglNw= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=R4y8Bnsd; spf=pass (imf02.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.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=1708462954; a=rsa-sha256; cv=none; b=3Xz8a6vfpckp6cxL4NsLlEdwAcZX5Ptb9P9xuQhIip4aKrF2SlFBd2012eC+3wgSyNNzPu /N15V4MEWQYyDw2yMZ6pKRwL7Jg+wTfDZOt74wWwGOwzRSpyrtVNpIXqF8k/tXEBGDFXWI PvIquRniVyu3ZcNUoKrgGgV9dcjl6Bw= Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-412718a8ea7so5610855e9.3 for ; Tue, 20 Feb 2024 13:02:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708462952; x=1709067752; 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=vHk8B1G9ngPFWZyvzcJjpFnWjSLSm2alsXCknI8ZS8Y=; b=R4y8Bnsd6FEnPHVCtlEjgwubobDQ7r9ot9XDG/mw1UdvfdC6nK+P35TBTRoFtTLh85 MVcrF68YUElBM7jga+w4xp0e8sr9w+wTnZkiID6s4wLmKpMDpXpPprv43ZMsmQ49nFr8 BkDHtXUc+7bDHyhmrlCtrmReh1eSi2pJ2VZdfKUjXc7ezwz/UYrJSx72xSaA6XbjKmmC dBmMc7OjlYvKHAsjEUy2rXq73g/emaK2Y8u5HMo42cvO6byKvku7PHU7tHplHI7iHBDF 0x37unME2SgFQGsTRuhb7jSu/7XoL+TDudKLaIBEfWeT7eD3U44mwOKbPWXIbzVGhEoy TbQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708462952; x=1709067752; 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=vHk8B1G9ngPFWZyvzcJjpFnWjSLSm2alsXCknI8ZS8Y=; b=dGE5UbIG1FVO3XzdRylganKXZGxAJXkChRgcI4qyP0hO07vn/FjoTVrhZvh7tAKCaA dYZ0nTNYVfOMtexlM3g7NKBV3cuctFErlPswI3BAb7VbASLLk51f1Yh+jzav3t/mlehx eb0sqSWLe/CHqjFMqaYWtPSy2tZvqtuqs3PU1ML8HRhZW6GS2cpxEDOAZtWYlndByppi 4ekZOX12B4sFB7Jpu6zHq1AfMS0s1hI9YPM1/ltj6yeDXcWALKeukKYK19AoFgutJmmu xHUJU8IBquLXiMjPspx9ELuiEXD3UZBxqnAipMECIddOHJBEgRiSHVkj0oMW5wZQWak4 mPDg== X-Forwarded-Encrypted: i=1; AJvYcCXoKMQzoFQ1ShEZp2CLL4LucHmhHC0R2dDDPNJqVfxJS9sNKChS1Gc9JQBzdbxMiDb3GXqetjfY7OzcQpW9Kklxp5w= X-Gm-Message-State: AOJu0YzRhZEQfPLtihMk67FJGoc1F/+8cz7bVRoAgixGDIHJeKJ8MSxU jmEInQnYld9jJ4zS3vwTWGb1+WSZtt6uUC7t5yuCQWhsmMwHiQB1 X-Google-Smtp-Source: AGHT+IGNd4EhKrOF2qguezkMCdiri+YzoOSXxtGqLSGpEMpRRkdkkVB+APiJgoZ4zrZJv+9oUTVssg== X-Received: by 2002:a05:600c:190c:b0:411:cb30:8e00 with SMTP id j12-20020a05600c190c00b00411cb308e00mr12141209wmq.3.1708462951917; Tue, 20 Feb 2024 13:02:31 -0800 (PST) Received: from localhost (host86-164-109-77.range86-164.btcentralplus.com. [86.164.109.77]) by smtp.gmail.com with ESMTPSA id t18-20020a05600c451200b0040fd1629443sm16087456wmo.18.2024.02.20.13.02.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Feb 2024 13:02:30 -0800 (PST) Date: Tue, 20 Feb 2024 21:00:15 +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: 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-Rspamd-Queue-Id: D974C80021 X-Rspam-User: X-Stat-Signature: 7y77e34acxos3sox754wif4d3ipqumow X-Rspamd-Server: rspam01 X-HE-Tag: 1708462953-334763 X-HE-Meta: U2FsdGVkX1+7bK6O4+4TsCuuc1+sg/8eebwkuvGWddiZNvHH0WpIgokpIZK7SI1oTu5VN1G8VV+us/cI3ebqCapVkUjoKSYtpICSHYpvGHXJIxzSbnChOPSIYadTNJf3CktItRiLqDj8U1w2nARHqKRboXiXKMj10s1Vs1Pd5pUIDYTUTiPPqGIPh9KY25wOOG69956vSFFplbFt82voKKjro/OShBbLUbiA4YcZnnisHvYmLO78jvVEXRls1RqYtj3ENJkxk+KmCNPbAhi2geZ/qaoymTyb45QNWQ94VEGDRlbbw1sCp0Lr+QnVXX/j0MR84bbeqHCO3vhqqwbTHMDY/EXiUnf9AYkQO3h5zLbzS6OuYeaSr5Hn+jmKUFtUuks6BM/8pz9K+SfudeU2QeMoaaeUg6QKEk1r1ytTFj3KW7qJk3PUmI0Bk+e9NresUWon1sTiROSrESNB8VU+QJ5OsqDx0XLRK0oiK80gBdFj04xRT7p5/mKjB8E/UkbKecJv8w6CvHJ+hhX3KLJZXpV+P+MJCxD5chr/fVZDEHn3m4d6KQI6kkkdChT4nItDRM/kbq7wT4YjAt1/gJyQQaJsJ2I5dkdLsxjNwMU3BZ7Hd6DnZpqk8hCJAXF6EbtQhKJjKWbox23r6sCaz7lsTQ+p2ZDFmKBT0lsfYxlfnPU4LLZkXI1l5IrvxIQJ8E9AjU9RKD7x8QBcdgTOKp+WHmd/N7IZ8kVkkZsxEiNv2lzCW4Pit8UkHN77d4JANjjgen3MyTIDUQSrR5PwD/2t0pJDFwNgikj6KxBQtFdSNw0Bdo+eEz/LFL3K0MnTCr+R3/JTUwWM7oJiXVotxwG/Ez00JUUd5CMXqRoD7T7j9Mx4rufOpQ5DmcxwGUPVEdrxwLoKiWaW+Cvylkm5SYFWsashTi5Ej5w+Rp9Vib46kEdXsZ1rfELPndBYQUjagHAS0fjmvL3002MHFZm+9vx ymg3HSWO W1dBhPWR2KmWSNqd3PVSMvae/FC4DbLoswBhQNHQqZQO2ROftppkh0eV1riXPHtjQ0VANILy90mNK9WDdzlWdcmnxPASUX/k8ZTFD3tmbltGJtR8VN6MhZbN6VfurBZE5YQSRNiIwLKbOYoPcuOvfhLy8guLtZbQeckLUaExr7PnGZsV5/M3HzeIL8Lu44pPx709b3CP1B026dmYFZY/h7CiKyC8k8ap/rLd92Cs+r/GOSpZ3At9Zs8R85JNT1SeZaIWe9Q5I7h4Wd7nRGjImBVi1BDyznkB+oa6kctBs5wnvKiF/7wBmuXzUtlSO0u7B91BTjkakCJOrUSmjkGVpIzLr4zT8JrL97IerVGEH/fOp0miO3jol0zeuS1U1Mx0sQr5DwEAmiE9dvTTquTTcGCnRhCbY+9wGyv8aQ4SEHsDoLhZdQnaAj+STmw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000037, 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 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()]. 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. 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.