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 13A52C7EE30 for ; Tue, 1 Jul 2025 08:31:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ACB336B00AE; Tue, 1 Jul 2025 04:31:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AA2D46B00B0; Tue, 1 Jul 2025 04:31:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9B8B66B00B1; Tue, 1 Jul 2025 04:31:02 -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 891406B00AE for ; Tue, 1 Jul 2025 04:31:02 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 37D171D54E0 for ; Tue, 1 Jul 2025 08:31:02 +0000 (UTC) X-FDA: 83615025564.30.D128E1E Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf16.hostedemail.com (Postfix) with ESMTP id 4023218000F for ; Tue, 1 Jul 2025 08:31:00 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf16.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1751358660; 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; bh=82LbpV9DaMDVWHw9jhZbXMaoLgqeZK9pqo50tUUL8o8=; b=vdahfW2ZnigE8ZLuMXm8Z/Xh32R3korlruoAYsiHeaKEE41u+kidmqQ58VLlOz8kzswPTT zBMIy3QRz97BSnLO32ouMQHNiQdOX8jSsVJRV4Z5yC4dkR8zwqdIo1D8sO0IVd1ls9z8eA rOL4BPjRRBLWXhMFa1r9pu4IAsOtWxo= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf16.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1751358660; a=rsa-sha256; cv=none; b=izqrmhC4HG4wU36OmzmMuESTk6+fprOv0ceP4cFIEYOH/X/kshgQQTPGcidlVi9iA6/mgY E5lIH7ZcyWZ05msSWX1uEsrVU0ptrR7pYQiI2OzH15T5i5QPBYTre8Wo362b5YQBhUT2FB odc93O5gJjgr75YWNyCiQ5t8RN0RSLU= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9D7A51595; Tue, 1 Jul 2025 01:30:43 -0700 (PDT) Received: from [10.57.84.129] (unknown [10.57.84.129]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EF9E63F58B; Tue, 1 Jul 2025 01:30:53 -0700 (PDT) Message-ID: <3776ac82-4cd5-41e7-9b96-137a1ae6f473@arm.com> Date: Tue, 1 Jul 2025 09:30:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 3/4] mm: Optimize mprotect() by PTE-batching Content-Language: en-GB To: Lorenzo Stoakes Cc: Dev Jain , akpm@linux-foundation.org, david@redhat.com, willy@infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org, Liam.Howlett@oracle.com, vbabka@suse.cz, jannh@google.com, anshuman.khandual@arm.com, peterx@redhat.com, joey.gouly@arm.com, ioworker0@gmail.com, baohua@kernel.org, kevin.brodsky@arm.com, quic_zhenhuah@quicinc.com, christophe.leroy@csgroup.eu, yangyicong@hisilicon.com, linux-arm-kernel@lists.infradead.org, hughd@google.com, yang@os.amperecomputing.com, ziy@nvidia.com References: <20250628113435.46678-1-dev.jain@arm.com> <20250628113435.46678-4-dev.jain@arm.com> <0315c016-d707-42b9-8038-7a2d5e4387f6@lucifer.local> <5900e978-6349-4a3d-a384-758889b678a0@lucifer.local> From: Ryan Roberts In-Reply-To: <5900e978-6349-4a3d-a384-758889b678a0@lucifer.local> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 4023218000F X-Stat-Signature: 75caiz1k1suf6myq5rjbzg4pr8hb7ut1 X-HE-Tag: 1751358660-36182 X-HE-Meta: U2FsdGVkX1+N8SBfKsOO2k6Jv/oE/H438TolOsxc9HDJVPfBBVgkHgDS+qvaQ2fOSNzY+PBxemusjHZnBfuT5VfICthHkwC+SYCmHgngUFeAgN14m+m4Bd+bwRJCds335ionyT4m88bd35bRIqc416nTpwjWw9jlpbMqu7ImI9qdhAsv8Hd9JvqednTG9WjLoBcLuw2g6TrTaoeoIqIJK/WVPAPLyNQmYhyg7oBwHghKtoJb0vrExXRhid9GiKN8GjbIrUoys5tc6WiiLbg8TEWQS6SvOF3Mh8R8ebB9JX1nNtBd1LNzZLRnXSLYtOPS1B7hNOAV6I+OfU+ritQP9IcWMN6pTGI4rVIlhI6Q0Rsa1A5/VFDu/BrapPseCQeTv1UIEBNIkoR3vM5NrBXWCSwNYKYcsCipY17YexqZY6Wt8A1S+el1VuL3jh+nEhKEbq7/n9LdsPr79v3mXjy64Xx5qaKCwC4Y6dJnBB/v9HZ8yU0cQ38FxK80EdxENv9O1MK8aOawZ+bgXC5E1zm1gL3MbmKQ6gViFtdebn+JkuNYwq2D8JfwMEolpLsfYRs8g073CxzIiCL6SXwSqSH+A2JDZTYdEhpPs+BCjO3eB1uXA/+dg1cktzvUA/GfwdLHv+vpeXei82i154W+ZM+MWPCqjJim59tFvZBuR5gOlJFimEWV15gjZVn0EES+hdZdm7HJBb4G4v4W8Amh4ASPw35KzItw4JxCdUoM9tDlwSR+iTqCQkpNGB/8MuQV9tKto5iA+mgwx8T9YZYQdkRzk5KgsnrTRHW1MLwdWivgi8V23lC4Tzqisj1ASs0VpOrV+EsJ8fTeAzmTeizyJrwg1OkmElKMEBzbX5oy+fPzFqXtu8HE53ZdWr3mTEL90qENL/PFlNz5loA5YLxjCUJMwdvKOv+UowENswevYrOwvu0ep96M1JT8TXnePH15dxvcJ8G65SI8cIR5UbLs0+t qq72kzbs HIXBsHDSHu2rIB5GY6ERYonFXCmd2xcUcxlmUahVFWOac6k0OpSE1HelBHYoicGRcZPsZLGLR5+DhX2ir+VGsmmqQP9efyTxvtE74wIJIbD9Q7Do= 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: List-Subscribe: List-Unsubscribe: On 01/07/2025 09:15, Lorenzo Stoakes wrote: > On Tue, Jul 01, 2025 at 09:03:27AM +0100, Ryan Roberts wrote: >>> >>> ////// end of Lorenzo's suggestion ////// >>> >>> You can obviously modify this to change other stuff like whether you feed back >>> the PAE or not in private case for use in your code. >> >> This sugestion for this part of the problem looks much cleaner! > > Thanks :) > >> >> Sorry; this whole struct tristate thing was my idea. I never really liked it but >> I was more focussed on trying to illustrate the big picture flow that I thought >> would work well with a batch and sub-batches (which it seems below that you >> hate... but let's talk about that down there). > > Yeah, this is fiddly stuff so I get it as a sort of psuedocode, but as code > obviously I've made my feelings known haha. > > It seems that we can apply the fundamental underlying idea without needing to do > it this way at any rate so we should be good. > >>>> >>>> static int mprotect_folio_pte_batch(struct folio *folio, unsigned long addr, >>>> - pte_t *ptep, pte_t pte, int max_nr_ptes) >>>> + pte_t *ptep, pte_t pte, int max_nr_ptes, fpb_t switch_off_flags) >>> >>> This last parameter is pretty horrible. It's a negative mask so now you're >>> passing 'ignore soft dirty' to the function meaning 'don't ignore it'. This is >>> just planting land mines. >>> >>> Obviously David's flag changes will also alter all this. >>> >>> Just add a boolean re: soft dirty. >> >> Dev had a boolean for this in the last round. I've seen various functions expand >> over time with increasing numbers of bool flags. So I asked to convert to a >> flags parameter and just pass in the flags we need. Then it's a bit more future >> proof and self documenting. (For the record I dislike the "switch_off_flags" >> approach taken here). > > Yeah, but we can change this when it needs to be changed. When it comes to > internal non-uAPI stuff I don't think we need to be too worried about > future-proofing like this at least just yet. > > Do not fear the future churn... ;) > > I mean I guess David's new flags will make this less egregious anyway. > >>>> oldpte = modify_prot_start_ptes(vma, addr, pte, nr_ptes); >>>> ptent = pte_modify(oldpte, newprot); >>>> >>>> @@ -227,15 +294,39 @@ static long change_pte_range(struct mmu_gather *tlb, >>>> * example, if a PTE is already dirty and no other >>>> * COW or special handling is required. >>>> */ >>>> - if ((cp_flags & MM_CP_TRY_CHANGE_WRITABLE) && >>>> - !pte_write(ptent) && >>>> - can_change_pte_writable(vma, addr, ptent)) >>>> - ptent = pte_mkwrite(ptent, vma); >>>> - >>>> - modify_prot_commit_ptes(vma, addr, pte, oldpte, ptent, nr_ptes); >>>> - if (pte_needs_flush(oldpte, ptent)) >>>> - tlb_flush_pte_range(tlb, addr, PAGE_SIZE); >>>> - pages++; >>>> + set_write = (cp_flags & MM_CP_TRY_CHANGE_WRITABLE) && >>>> + !pte_write(ptent); >>>> + if (set_write) >>>> + set_write = maybe_change_pte_writable(vma, addr, ptent, folio); >>>> + >>>> + while (nr_ptes) { >>>> + if (set_write == TRI_MAYBE) { >>>> + sub_nr_ptes = anon_exclusive_batch(folio, >>>> + pgidx, nr_ptes, &sub_set_write); >>>> + } else { >>>> + sub_nr_ptes = nr_ptes; >>>> + sub_set_write = (set_write == TRI_TRUE); >>>> + } >>>> + >>>> + if (sub_set_write) >>>> + newpte = pte_mkwrite(ptent, vma); >>>> + else >>>> + newpte = ptent; >>>> + >>>> + modify_prot_commit_ptes(vma, addr, pte, oldpte, >>>> + newpte, sub_nr_ptes); >>>> + if (pte_needs_flush(oldpte, newpte)) >>>> + tlb_flush_pte_range(tlb, addr, >>>> + sub_nr_ptes * PAGE_SIZE); >>>> + >>>> + addr += sub_nr_ptes * PAGE_SIZE; >>>> + pte += sub_nr_ptes; >>>> + oldpte = pte_advance_pfn(oldpte, sub_nr_ptes); >>>> + ptent = pte_advance_pfn(ptent, sub_nr_ptes); >>>> + nr_ptes -= sub_nr_ptes; >>>> + pages += sub_nr_ptes; >>>> + pgidx += sub_nr_ptes; >>>> + } >>> >>> I hate hate hate having this loop here, let's abstract this please. >>> >>> I mean I think we can just use mprotect_folio_pte_batch() no? It's not >>> abstracting much here, and we can just do all this handling there. Maybe have to >>> pass in a bunch more params, but it saves us having to do all this. >> >> In an ideal world we would flatten and just have mprotect_folio_pte_batch() >> return the batch size considering all the relevant PTE bits AND the >> AnonExclusive bit on the pages. IIRC one of Dev's earlier versions modified the >> core folio_pte_batch() function to also look at the AnonExclusive bit, but I >> really disliked changing that core function (I think others did too?). > > Yeah let's not change the core function. > > My suggestion is to have mprotect_folio_pte_batch() do this. > >> >> So barring that approach, we are really only left with the batch and sub-batch >> approach - although, yes, it could be abstracted more. We could maintain a >> context struct that persists across all calls to mprotect_folio_pte_batch() and >> it can use that to keep it's state to remember if we are in the middle of a >> sub-batch and decide either to call folio_pte_batch() to get a new batch, or >> call anon_exclusive_batch() to get the next sub-batch within the current batch. >> But that started to feel overly abstracted to me. > > Having this nested batch/sub-batch loop really feels worse. You just get lost in > the complexity here very easily. > > But i"m also not sure we need to maintain _that_ much state? > > We're already looping over all of the PTEs here, so abstracting _the entire > loop_ and all the sub-batch stuff to another function, that is > mprotect_folio_pte_batch() I think sensibly, so it handles this for you makes a > ton of sense. So effectively turn mprotect_folio_pte_batch() into an iterator; have a struct and a funtion to init the struct for the the number of ptes we want to iterate over, then a per iteration function that progressively returns batches? Then we just have a simple loop here that gets the next batch and processes it? > >> >> This loop approach, as written, felt more straightforward for the reader to >> understand (i.e. the least-worst option). Is the context approach what you are >> suggesting or do you have something else in mind? >> > > See above. > >>> >>> Alternatively, we could add a new wrapper function, but yeah definitely not >>> this. >>> >>> Also the C programming language asks... etc etc. ;) >>> >>> Overall since you always end up processing folio_nr_pages(folio) you can just >>> have the batch function or a wrapper return this and do updates as necessary >>> here on that basis, and leave the 'sub' batching to that function. >> >> Sorry I don't understand this statement - could you clarify? Especially the bit >> about "always ... processing folio_nr_pages(folio)"; I don't think we do. In >> various corner cases the size of the folio has no relationship to the way the >> PTEs are mapped. > > Right yeah I put this badly. Obviously you can have all sorts of fun with large > folios partially mapped and page-table split and etc. etc. > > I should have said 'always process nr_ptes'. > > The idea is to abstract this sub-batch stuff to another function, fundamentally.