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 F3297C48BEB for ; Thu, 22 Feb 2024 08:33:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 676F76B0074; Thu, 22 Feb 2024 03:33:48 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 626E86B0075; Thu, 22 Feb 2024 03:33:48 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4EEE46B0078; Thu, 22 Feb 2024 03:33:48 -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 3D31F6B0074 for ; Thu, 22 Feb 2024 03:33:48 -0500 (EST) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 02F1E408E5 for ; Thu, 22 Feb 2024 08:33:47 +0000 (UTC) X-FDA: 81818776536.19.410AC30 Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by imf12.hostedemail.com (Postfix) with ESMTP id 820FB4000E for ; Thu, 22 Feb 2024 08:33:42 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=S0bwQQYC; spf=pass (imf12.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.52 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=1708590822; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=bH5i/1tcGhrzuGuRkdGry1qY4fQR5qkGcCI1YHWBx9Q=; b=lAz2Ams6Io2186z6N+RMC7yACvRWxhYbeAzIQam8ZpRv96ioE94VMUTwcSYaEIO2DMhDTC cjDu/eQDOPFi0TkfARAsjJFQe4AqQEXSeewJw5xyMPRRE8KMzvOAdD9+QB9aPy7Mp6p41/ VEuO6r7srAuF2gnyChVzOT5hcwLMF5U= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708590822; a=rsa-sha256; cv=none; b=4c/8bLzQvw3kRmuMjaB73yYYeVYBceLBBoFitbdb+SP1VeKvBwbohjfRfXaJKY7W3x5H12 yi1OGH9V2oZX7Qw4UIvr5AICFgxqOmbgYu6ttDUubxvLVSahBv4YEpvUqvclrO6qrUw63m 51RpjqZPcSR8wim5nKyf2i9J8+d+DZc= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=S0bwQQYC; spf=pass (imf12.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.52 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wr1-f52.google.com with SMTP id ffacd0b85a97d-33d6bd39470so728611f8f.3 for ; Thu, 22 Feb 2024 00:33:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708590821; x=1709195621; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=bH5i/1tcGhrzuGuRkdGry1qY4fQR5qkGcCI1YHWBx9Q=; b=S0bwQQYC5HXPbWNMqSQ2Jd5E9/7eR31kqSCVoAR4tsFgZtaK+k2scu4x/+SiB68pNc FFfN29uo4RTILa4bA/Q6wRj55dET8eixEPNh1epdc5uIwqxIeSq3Kcj4JYVGQlOyPeY3 mgCUglxnn2OoR5+RIImhcDCnc+fGsYe1CKWp8N/ul5VUTwINfa6BGg5LbmJHGpPb7jR3 aNzvPOuQqH/dOBcooF49OqxMEwW9swpMnkcXQF8Xq5r+/xlM1EEkh35/eJh+3qCm3/J5 DEA/qJxtjR0v4teWwoDwVoJsnEBoYt2wTTzuOK6sfdqId9xvtTo6sma95XwBuPzzD5W/ d6QQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708590821; x=1709195621; h=in-reply-to:content-transfer-encoding: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=bH5i/1tcGhrzuGuRkdGry1qY4fQR5qkGcCI1YHWBx9Q=; b=AJ36OVQBV3S41b6soBzkvBxMchg5xvqNLhhNk0ABbWsZC88IA5Rg46EpXAIyn924Fc z81v27s3TMvjjHNg/5CoHiF710WwGiG5yTMHfEMNKC167BlCkK8uehXPbHpe3mw62QKV Ov8ik8H0VKDPF7wFPnq9gnWc8YZL2ONUAkH4VjiGE4qtVXW+/H/2p8dFDwpUkWMs+myb HxFT9V5t8OgnY5VihF3zOXLcWbw4gASrFqgw21rSz3leE6Hj/z26XkMdpuZmTQ/fctjN xP0XC8HmyFXFk/xZDDxKmgGrcE7LRmdoBXbgKjePaGayZ4AzdZsdQ6V0bT6CKRi40lb0 tqsg== X-Forwarded-Encrypted: i=1; AJvYcCVOqe+TG6DWqFzmSjz6auxm1sIY9RZCdgeGl5MUmsaDhPtI1o4K00OW+93EpaopvDNB6sDMFcmeNeM0SNVxl4mNTnc= X-Gm-Message-State: AOJu0Yzi1WXhrR1Jm8uMziAkRZlQPDRmGBesgD+CByVt0lnBewwtUe5u M+UQ4qFYZTUBV9U9rmxiLa3V5H4MoynYpibp1AyIBDFYZFdciipd X-Google-Smtp-Source: AGHT+IGA36t4KzNPr+jWh8q57ab/UYxkN2JIqsVq8CaWM+itVn6VwkV3YGGQiMlqFlcrWxF1qrr0Fw== X-Received: by 2002:adf:f4d0:0:b0:33b:3f43:e007 with SMTP id h16-20020adff4d0000000b0033b3f43e007mr15015688wrp.57.1708590820527; Thu, 22 Feb 2024 00:33:40 -0800 (PST) Received: from localhost (host86-164-109-77.range86-164.btcentralplus.com. [86.164.109.77]) by smtp.gmail.com with ESMTPSA id s8-20020adf9788000000b0033ae7d768b2sm19664667wrb.117.2024.02.22.00.33.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Feb 2024 00:33:38 -0800 (PST) Date: Thu, 22 Feb 2024 08:31:23 +0000 From: Lorenzo Stoakes To: Yajun Deng Cc: "Liam R. Howlett" , akpm@linux-foundation.org, vbabka@suse.cz, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] mm/mmap: return early if it can't merge in vma_merge() Message-ID: References: <20240221091453.1785076-1-yajun.deng@linux.dev> <20240221153827.wkmjnnwsf6lyxatc@revolver> <082fed0a-8489-37d1-f990-067976260659@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <082fed0a-8489-37d1-f990-067976260659@linux.dev> X-Rspamd-Queue-Id: 820FB4000E X-Rspam-User: X-Stat-Signature: mnf3e178kgxfop9b1uk1qcofgkux6ip5 X-Rspamd-Server: rspam03 X-HE-Tag: 1708590822-802409 X-HE-Meta: U2FsdGVkX1/QjxUKOAzC0cluw7mbrxvX/CVaV/ScjKfGXVt+HlYL8CMX9HchFpzClv4XfZ40ipmG/trV28B5S9wZX3AHGTUylVh/8LIeUplgnou4LGS0KciML5E+L46qcJ7C/WSQ5UmLbly9JYevaM/YcNgiGcDUBO09sNjxKcwzB5zyyx0rHj295JjudvDWnVmyoZllAxWAQrr9vVHeVJTW8/cBII2In/05Y0Iup99MW61oJK4mxegw+KXb2IzJG6ovFkF54e65cujUUBofzN9M51bxd0a/CL5RGCKnSLNtsUngFK7X7Vd6N8Nh1H0IDrCGwvVlomVx1ebCR7wPGCnm3NYG2d13gVw3+nwFSq3DhYxmJJH+KUGePh3Fnx811+JkMxdxKLRPpjc/9mq84CwN4GPSNwEO5F54FLJN5ctb2cWpu/GLrUv2RKrizpkfgXjITp1bYSapa9p1WoCZjMod+if+sgdvqryFsC1u8aMeR8BfCMbyZdPxiVLWOSjnRuToovQElZXPULD7S3GDZoOLWDd7iIIPdtxysctHr1Noj6vknzV2LRRRg+y3gI+emnZPVw7FHm+6e7q8ckf2TIKXRRjwrfrohVQN5GfoCpF/J8gILhCcl+nMI68pm4NZceIHpW8FIQcGvVTY7RaJAReyGh6sNAftSIzuyzJSlakaahFCmTuNEr67PIRR4YuYIJrYFukubsLETxHBgcGAB5t6aXztohpHKeZ9AnDtEMhM4nFtYsrZPxMqFqyQtrym+hrTNI1XGA54nDXv14SzbIfd27rwmz6Xvs/oP1BLZEb0R3mC7jptuLbxyIB0A/PCuG6iS6VXrVAisdqdiBsK+tCaAKjB8NOY4RWFmFDM1JfYyI0vew3QsxAWrDRk1z1JKvOWcCk8f3v0c95UysTMve6+WCP5umAYlVbSNRPLIDdo8AyH07ymte84sGtQU4kC/XKsrTX56X0JTX+fAO3 ji0LQV8H p3BQuEAzPMt3IVOdmq8zU5Hb4yrJ9+HgGzzYxuR+7SdR4B2oJFJVUSAIBwM/XZ6Z6RJhuBx6GDZ3ssm8= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000002, 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 Thu, Feb 22, 2024 at 03:47:04PM +0800, Yajun Deng wrote: > > On 2024/2/22 04:41, Lorenzo Stoakes wrote: > > On Wed, Feb 21, 2024 at 10:38:27AM -0500, Liam R. Howlett wrote: > > > * Yajun Deng [240221 04:15]: > > > > In most cases, the range of the area is valid. But in do_mprotect_pkey(), > > > > the minimum value of end and vma->vm_end is passed to mprotect_fixup(). > > > > This will lead to the end is less than the end of prev. > > > > > > > > In this case, the curr will be NULL, but the next will be equal to the > > > > prev. So it will attempt to merge before, the vm_pgoff check will cause > > > > this case to fail. > > > > > > > > To avoid the process described above and reduce unnecessary operations. > > > > Add a check to immediately return NULL if the end is less than the end of > > > > prev. > > > If it's only one caller, could we stop that caller instead of checking > > > an almost never case for all callers? Would this better fit in > > > vma_modify()? Although that's not just for this caller at this point. > > > Maybe there isn't a good place? > > I definitely agree with Liam that this should not be in vma_merge(), as > > it's not going to be relevant to _most_ callers. > > > > I am not sure vma_modify() is much better, this would be the only early > > exit check in that function and makes what is very simple and > > straightforward now more confusing. > > > There are two paths that will cause this case. One is in mprotect_fixup(), > the other is in > > madvise_update_vma(). > > > One way is to separate out the split_vma() from vma_modify(). And create a > new helper function. Absolutely not. I wrote the vma_modify() patch series explicitly to expose _less_ not more. > > We can call it directly at source, but we need to do this in both paths. > It's more complicated. > > > The other way is to add a check in vma_modify(). Like the following: As I said above, I really don't think this is a good idea, you're just special casing one non-merge case but not any others + adding an unnecessary branch. > > diff --git a/mm/mmap.c b/mm/mmap.c > index 0fccd23f056e..f93f1d3939f2 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -2431,11 +2431,15 @@ struct vm_area_struct *vma_modify(struct > vma_iterator *vmi, >         pgoff_t pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> > PAGE_SHIFT); >         struct vm_area_struct *merged; > > +       if (prev && end < prev->vm_end) > +               goto cannot_merge; > + >         merged = vma_merge(vmi, prev, vma, start, end, vm_flags, >                            pgoff, policy, uffd_ctx, anon_name); >         if (merged) >                 return merged; > > +cannot_merge: >         if (vma->vm_start < start) { >                 int err = split_vma(vmi, vma, start, 1); > > > > And I think this is the crux of it - it's confusing that we special case > > this one particular non-merge scenario, but no others (all of which we then > > deem ok to be caught by the usual rules). > > > > I think it's simpler (and more efficient) to just keep things the way they > > are. > > > > > Or are there other reasons this may happen and is better done in this > > > function? > > > > > > Often, this is called the "punch a hole" scenario; where an operation > > > creates two entries from the old data and either leaves an empty space > > > or fills the space with a new VMA. > > > > > > > Signed-off-by: Yajun Deng > > > > --- > > > > v2: remove the case label. > > > > v1: https://lore.kernel.org/all/20240218085028.3294332-1-yajun.deng@linux.dev/ > > > > --- > > > > mm/mmap.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > > > index 0fccd23f056e..7668854d2246 100644 > > > > --- a/mm/mmap.c > > > > +++ b/mm/mmap.c > > > > @@ -890,6 +890,9 @@ static struct vm_area_struct > > > > if (vm_flags & VM_SPECIAL) > > > > return NULL; > > > > > > > > + if (prev && end < prev->vm_end) > > > > + return NULL; > > > > + > > > > /* Does the input range span an existing VMA? (cases 5 - 8) */ > > > > curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end); > > > > > > > > -- > > > > 2.25.1 > > > > > > So overall I don't think this check makes much sense anywhere. > > > > I think a better solution would be to prevent it happening _at source_ if > > you can find a logical way of doing so. > > > > I do plan to do some cleanup passes over this stuff once again so maybe I > > can figure something out that better handles non-merge scenarios. > > > > This is a great find though overall even if a patch doesn't make sense > > Yajun, thanks for this, it's really made me think about this case (+ others > > like it) :) I guess maybe again I've not been clear enough on this, so unless compelling reasoning can otherwise be provided, I feel this check should not be added _anywhere_. Therefore, NACK.