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 CE223C83F12 for ; Mon, 28 Aug 2023 19:00:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 10469280024; Mon, 28 Aug 2023 15:00:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0B53B8E001E; Mon, 28 Aug 2023 15:00:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E9837280024; Mon, 28 Aug 2023 15:00:24 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id D914F8E001E for ; Mon, 28 Aug 2023 15:00:24 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 7E4F01C96AE for ; Mon, 28 Aug 2023 19:00:24 +0000 (UTC) X-FDA: 81174429168.23.5752567 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) by imf28.hostedemail.com (Postfix) with ESMTP id 84AC8C002A for ; Mon, 28 Aug 2023 19:00:22 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=A0HvRBvL; spf=pass (imf28.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.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=1693249222; 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=nWVaklve78TY8Ndro9SxAEjWhaM4lUPv6QRMiokVKjk=; b=cLa+kHV1ZVxHkU7MsLRSfyQRDJrZGpcvCNSd5OAn2R35qtWTqD2lhHEGLlACjPGM/6AkdS uwaQGYjcmXVMmeMKL+HwBeiBk/svVd26eEvoYwmmeKTAmCloa9feuT7qGnvXl6wYOvmV5E pOxGPCHVvqjqZUI7lY2mcmHk5kpPHl4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1693249222; a=rsa-sha256; cv=none; b=qlnJci7jmKiR8tQV+2f7n78ZDY5zopCzEgeZSnsDNg1exTNwg0tnkpZvr7v1x4Ri72hAvw Jz8Px8MW+bvYMybf2vrTVdzf7izRlZnZptp+RIA34WNQj/pOGfmRhnQC7v4sdCx3w+8hxU suhJd62nb3XJeA5/xNxOrtMjwDMn6LE= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=A0HvRBvL; spf=pass (imf28.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.43 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-3fee5ddc23eso33639965e9.1 for ; Mon, 28 Aug 2023 12:00:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693249221; x=1693854021; 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=nWVaklve78TY8Ndro9SxAEjWhaM4lUPv6QRMiokVKjk=; b=A0HvRBvLv+ZUbs5yBJuawXPTe5T27ocbRvkAiVZCtL9aYA7Serw7K8Pd0SmR7bmDIw fgARXAfql8f4I5PQg8MvRrYZo0xxBJ34E/J/Bt9KSqsVxTPk4wuBNvpDXIllYAgY684X KJI9N1MhnDT37N8B3srSSWc5hWiwSVMBp33f3ghQCN29lJZurGCOorLkfx9KHQ5vjyLT LndP90pd3pEMda0uNE8eTHiWiRhLsunQti7oW4gGcaTy5Hue/MWXjRm1o/97zpNTg/aW wa1katQl0yOGTcsh/yZnbPC0QCmcZYrlDdyjAShClx5tKzIiGVS4rjyFLSZhkKhHJbId Xzlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693249221; x=1693854021; 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=nWVaklve78TY8Ndro9SxAEjWhaM4lUPv6QRMiokVKjk=; b=RyW+glVopxmPP0QXu58eP/KN3tscDSiShcNs6nIPfQCmdEXGdFZal2T2JAs3te8D6L cYJUUwDSNdZb0hSEbP4zmQrpgDtzfQzUcDd+J1TVRdjsH8dsj/Xni083cl/T4cLMXbyt y8qBuiyrNGoaLAXopAJV2giQmEfNKTxwhUYYWoChgfoJwo5gAaYUur1lWeBEmN4tP8K6 tGOXvMa4mgIxqoZILG1n+8No5N6BS5B2VXPCu//z/rvnleXadv6JwTRgIT81rgO09btB o88LltwHInk3HD9vXZRD1ZWh712Z2Fj5Cn6HJttkVfJo/bpIJ0/IbYwrIKYGDBokM0g5 9JjA== X-Gm-Message-State: AOJu0YwdBLEYUZ+bL2ynGkD9bx7QDVnLL9UcA9f5KpqL/xWqw8uv0p1r K2CtyqXhURcxAx8RaofW7cU= X-Google-Smtp-Source: AGHT+IE1B2gPj/pPtcZ4/vI/oz1diEErn2Krz1vr8bos9oo17Wv8vTp0dxrT8KTKied+xZLC+qSgRw== X-Received: by 2002:a7b:cc8e:0:b0:401:b3a5:ec03 with SMTP id p14-20020a7bcc8e000000b00401b3a5ec03mr8009357wma.1.1693249220534; Mon, 28 Aug 2023 12:00:20 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id 10-20020a05600c234a00b003feeb082a9fsm11662696wmq.3.2023.08.28.12.00.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Aug 2023 12:00:19 -0700 (PDT) Date: Mon, 28 Aug 2023 20:00:18 +0100 From: Lorenzo Stoakes To: Joel Fernandes Cc: linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, Shuah Khan , Vlastimil Babka , Michal Hocko , Linus Torvalds , Kirill A Shutemov , "Liam R. Howlett" , "Paul E. McKenney" , Suren Baghdasaryan , Kalesh Singh , Lokesh Gidra Subject: Re: [PATCH v5 2/7] mm/mremap: Allow moves within the same VMA Message-ID: <8891681e-532c-4d7b-bc28-b4ad3e017331@lucifer.local> References: <20230822015501.791637-1-joel@joelfernandes.org> <20230822015501.791637-3-joel@joelfernandes.org> <46196ba1-c54d-4c1d-954f-a0006602af99@lucifer.local> <20230828183240.GA1621761@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230828183240.GA1621761@google.com> X-Rspamd-Queue-Id: 84AC8C002A X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 7js1gcz1zucgy4uotcd5r88o4gw73q8o X-HE-Tag: 1693249222-845203 X-HE-Meta: U2FsdGVkX187rof4Fx66uwlGSwYuBMyOdGmjXPHD4g5+S6Js0z3dnHstuWF8YMZ7fv1xI195iiBB8NXJnQs2QZQwZfMLsg7YZZJTaA6LvQ5Fx4pGJyvpsNqp4pPCl1iu4Ajte/FszRlNPIkx/FjHQjdZrYqqPTySlsQfn34yNjjNM4vy7n/+TXGPx3MG0ycAgJV1ynYKpFC5dLW7+WpTYOcpl4tM19pLlZgjMudpalEDVH4rtsf71RQmcv/nwafKpiMmiVxEcYdGaKubNLiB771vL+vZJvpizCVxSDzZbhu4/FtwU6ivy1OsvOctIRvtQ8LrI+qhUrJA5ahLgaFRcUfgecWINtJjF6L4nB0JsTUdpq0vWJ9kOfaj5YW5x46OqsMSfBK9T2+qJdCXTWNWRxzt5VfOyoEPicCibhbgDrbKxtj+akuQ26+qLsDxFHxtiE5y7D/wPFIBDua0GeqfYgDexXONhZwX89NypYhxDWAsWb78wfrIme04a+SFNRasDArjMnfrUQS9pQJIyXowf+K0uWYWRjbAn23G/Qac8/IeKbeZUDQy0XSEK8j5KWtFOp9FAcC+d9TFvKSIW4memaP4h9MNK3f5OXx1AxnfZ/PZNZGNBoxKpLHJu5K3Qn+525V4hBUfeQFLifayKvoah6t7CefshKGVuP1qOoGwPoRIcr9ALrzhX0uOO8GJYX6mSy61qmv5YABDY4PDNj/YcA1mI6Nq6OarESK8ok0xp68bAe1kThyJsogE0ah5pe08twVMWwwHbTJWWD9lWLp2NFhZ+CwgDNOdS7GXfgj7NY5BwAaKApgyWU9/i9sY26vlfBOt3qDBJLcTURmP/Cqxq/GE+Vkd9IMJM6xNFRD7qW8aJ7iJYFdFydCx8Wpg8ye7kZ3p7AI3zGbOZjgJU3H9nOiHy+rYVhybBnCir57VeY7wSgUrYLyj3sbV6s6sL5mB7mAggzGdVsq3QJr+9u+ 8zyCdPCP NQQT1mLgHU1yw+14rEylT/xzFQapvn2Rbm3uphT1SHer813vExzV/GJ0jGh+bkBmCNyg0/JBOCk9aKG/ZlRvh0u+AX6T8lVKJVRNG9PUlrWOZv4vGO3juQ0LpF+hZAVXlf3Bxs6eIJKy8EIv/DsaF+Ek1Ry1GSWwdxDEUR+WADntpca2qHZjuTii0qquwl0KXl+luPZ/ohT39d8B+84UeGJ2quYIVkvEhirExmrSzYerpbQkbAjM11pi3Qtdj1HUpe19SCHxYEGJm18gb/NputGcE3i5JwFS1bXOoybmxQe9esXUJ64DjC5p6nz597XZrs+8Ud+HUA8DP2PECNdjQ1UrlSPBfdmi1Crh2AytwaM6waozokG1C4umATb4MakSWt2HrvfwmbefeboaaYAJ7xRaf7A== 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 Mon, Aug 28, 2023 at 06:32:40PM +0000, Joel Fernandes wrote: > On Sun, Aug 27, 2023 at 10:21:14AM +0100, Lorenzo Stoakes wrote: > [..] > > > > > > /* > > > * Flags used by change_protection(). For now we make it a bitmap so > > > diff --git a/mm/mremap.c b/mm/mremap.c > > > index 035fbf542a8f..06baa13bd2c8 100644 > > > --- a/mm/mremap.c > > > +++ b/mm/mremap.c > > > @@ -490,12 +490,13 @@ static bool move_pgt_entry(enum pgt_entry entry, struct vm_area_struct *vma, > > > } > > > > > > /* > > > - * A helper to check if a previous mapping exists. Required for > > > - * move_page_tables() and realign_addr() to determine if a previous mapping > > > - * exists before we can do realignment optimizations. > > > + * A helper to check if aligning down is OK. The aligned address should fall > > > + * on *no mapping*. For the stack moving down, that's a special move within > > > + * the VMA that is created to span the source and destination of the move, > > > + * so we make an exception for it. > > > */ > > > static bool can_align_down(struct vm_area_struct *vma, unsigned long addr_to_align, > > > - unsigned long mask) > > > + unsigned long mask, bool for_stack) > > > { > > > unsigned long addr_masked = addr_to_align & mask; > > > > > > @@ -504,7 +505,7 @@ static bool can_align_down(struct vm_area_struct *vma, unsigned long addr_to_ali > > > * of the corresponding VMA, we can't align down or we will destroy part > > > * of the current mapping. > > > */ > > > - if (vma->vm_start != addr_to_align) > > > + if (!for_stack && vma->vm_start != addr_to_align) > > > return false; > > > > I'm a little confused by this exception, is it very specifically for the > > shift_arg_pages() case where can assume we are safe to just discard the > > lower portion of the stack? > > > > Wouldn't the find_vma_intersection() line below fail in this case? I may be > > missing something here :) > > I think you are right. In v4, this was not an issue as we did this: > > > + if (!for_stack && vma->vm_start != addr_to_align) > + return false; > + > + cur = find_vma_prev(vma->vm_mm, vma->vm_start, &prev); > + if (WARN_ON_ONCE(cur != vma)) > + return false; > > Which essentially means this patch is a NOOP in v5 for the stack case. > > So what we really want is the VMA previous to @vma and whether than subsumes > the masked address. > > Should I just change it back to the v4 version then as above for both patch 1 > and 2 and carry your review tags? You will not be surprised to hear that I'd rather not :) I think if we did revert to that approach it'd need rework anyway, so I'd ask for a respin w/o tag if we were to go down that road. HOWEVER let's first clarify what we want to check. My understand (please correct me if mistaken) is that there are two acceptable cases:- 1. !for_stack addr_masked addr_to_align | | v v . |-----| . <-must be empty-> | vma | . |-----| 2. for_stack addr_masked addr_to_align | | v v |----.-------------------.-----| | . vma . | |----.-------------------.-----| Meaning that there are only two cases that we should care about:- 1. !for_stack: addr_to_align == vma->vm_start and no other VMA exists between this and addr_masked 2. for_stack: addr_masked is in the same VMA as addr_to_align. In this case, the check can surely be:- return find_vma_intersection(vma->vm_mm, addr_masked, addr_to_align) == (for_stack ? vma : NULL); (maybe would be less ugly to actually assign the intersection value to a local var and check that) > > This is also hard to test as it requires triggering the execve stack move > case. Though it is not a bug (as it is essentially a NOOP), it still would be > nice to test it. This is complicated by also the fact that mremap(2) itself > does not allow overlapping moves. I could try to hardcode the unfavorable > situation as I have done in the past to force that mremap warning. I find this exception a bit confusing, why are we so adamant on performing the optimisation in this case when it makes the code uglier and is rather hard to understand? Does it really matter that much? I wonder whether it wouldn't be better to just drop that (unless you really felt strongly about it) for the patch set and then perhaps address it in a follow up? This may entirely be a product of my simply not entirely understanding this case so do forgive the probing, I just want to make sure we handle it correctly! > > thanks, > > - Joel >