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 D8AB0C46467 for ; Thu, 5 Jan 2023 00:04:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 608CD900002; Wed, 4 Jan 2023 19:04:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 591968E0001; Wed, 4 Jan 2023 19:04:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 43266900002; Wed, 4 Jan 2023 19:04:11 -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 32C058E0001 for ; Wed, 4 Jan 2023 19:04:11 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id EF1E91A0335 for ; Thu, 5 Jan 2023 00:04:10 +0000 (UTC) X-FDA: 80318797860.25.5D20503 Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) by imf11.hostedemail.com (Postfix) with ESMTP id 63C4B4000C for ; Thu, 5 Jan 2023 00:04:09 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ECoVNHQ9; spf=pass (imf11.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.178 as permitted sender) smtp.mailfrom=shy828301@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=1672877049; 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=Xzo023694+wb/S1CfmbkiM0eJ0PtYfUNQ8L6gvd/+XM=; b=3oepCMsM43qOs4Pps9npa/3m0vkZZiZdV9tgD5KVXTWkXdkPoFh/3K0UEd651euFdmtdPb QzDbn49y5Z/10tXhLRbOUW386SyUpfrN2ccPPjZEmn4q/514DPYrBBOtxWt9bu57RYI1sL If61k/R9lbiT43qxopWkrJKpbde7oTQ= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ECoVNHQ9; spf=pass (imf11.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.178 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1672877049; a=rsa-sha256; cv=none; b=f/vBykphpHWd8uH/T4CSJzg02ZYuzuFIk5jKI/e75UrnfliPISJ2nTKZ7vWPQJ4NsgDMYi YfpomuulfDkOE3ccjeeJSR4Hkq5SeJURFLm1izqQms6sOHSc9k4QJBLJ5eT1eiXAwPTNC/ Sf23to61+M2r1xVtT7je7fC/qV2JIc0= Received: by mail-pf1-f178.google.com with SMTP id e21so14572084pfl.1 for ; Wed, 04 Jan 2023 16:04:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Xzo023694+wb/S1CfmbkiM0eJ0PtYfUNQ8L6gvd/+XM=; b=ECoVNHQ9tvZ7sQeorEUOdiMYYtFLu8QWelJgUOnooA60Qz4p6taWU3HrqcLqbREAIF STueb232p1T0VLqbkEo3/Fex4BkmArq2ZJ0cBvvOTv5UI+WFMlB8LRcp5SMQ0D5Q+xsC jOdVGno5+5VkcPZuqos5V6+6m6UPKF0XksPm8enhwHqVKbhdBGxIcP8LbHK3T05MwIMt DxCa3rqeU4IX8M4XjfFFzAT+BdP8wp7j7/M06rsaGtA+7Iaajh8FQMUHq+5x7XdL+P2a JWkz8CAJHrWfSGEGNKP5EndFfFGtAuSZS2Wdgas9oor+4kv7dORz3AE2T6UYvzF8n5fA hDLg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Xzo023694+wb/S1CfmbkiM0eJ0PtYfUNQ8L6gvd/+XM=; b=m6Zd/g/mRq86hkMWSoGoueFTttQK7uVjLXDlTp0S3mwWWG7pZ2kfy7NW7bZtHIlqmF ukFtgi6SltdS9RrLuhSozHcUcM2PG7YKFiDpCPb21CWZPPWQIkiVGZtKwtXG84LA0teG pnj8xEg5VfAmNRCTfgCXap07qWD8IlA80mkrfHA0P0zexMOflckzNS5LepWyVo3sABlr UdF5qj5ceJevWRpuogluFpnjWAvw7bHCe0dGzpBbLOY4v7ql3XRIJJbdQYvR1KyKq3nq 5F62wlbaHXlpxLK86k4Wgn8IlMKlEvZN7EsA94DDT4jA4mPwfTK1rn389dNQT0L/SPmV ZYLA== X-Gm-Message-State: AFqh2kp2rv1uJMy37Kq3uSSr2WzuhnQBjPz1qlapUeby4zUOcN27p+8c tJxkF3Na9n8erVwhRDGtNw8+WH+bXdbFx10bReg= X-Google-Smtp-Source: AMrXdXtecdu7jIJp6vSbE9eunxWy6lBtBRMT1gi8Fv4EOrmHa1+mhG4L333Bxqqs5kiqTqJOgzHbNdxMhvEuz+TFG98= X-Received: by 2002:a63:1944:0:b0:492:50ad:d177 with SMTP id 4-20020a631944000000b0049250add177mr3722190pgz.310.1672877048085; Wed, 04 Jan 2023 16:04:08 -0800 (PST) MIME-Version: 1.0 References: <32be06f-f64-6632-4c36-bed7c0695a3b@google.com> <7ff97950-b524-db06-9ad6-e98b80dcfefa@redhat.com> In-Reply-To: <7ff97950-b524-db06-9ad6-e98b80dcfefa@redhat.com> From: Yang Shi Date: Wed, 4 Jan 2023 16:03:56 -0800 Message-ID: Subject: Re: [PATCH] mm/khugepaged: fix collapse_pte_mapped_thp() to allow anon_vma To: David Hildenbrand Cc: Hugh Dickins , Andrew Morton , Jann Horn , "Zach O'Keefe" , Song Liu , linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: 5yaf5uugo6dk37w1mr5b6cxfhwikw4ih X-Rspam-User: X-Rspamd-Queue-Id: 63C4B4000C X-Rspamd-Server: rspam06 X-HE-Tag: 1672877049-984445 X-HE-Meta: U2FsdGVkX185xWuKPtXrKW17VG0g/6gon6PcNAH6NhZkdoKjnvr1pmYjM1+2SOMMwB4FTAsbjW3h9fdZV7k6ZRuWaujeoOLnVe8d6BKMf/RzU7g3X/rZSo0V9osUa8wr5krJRyWEpY6MVIHjUXaVan0BXKPj/5enyg4bqsM/1nu1/CKSuUhLYBlUrKp5iRqH/rp2i75X5vSpzpE1kXs795O7X/TgigiDzDbVyKAr/yWSTSM8buC/wOdAVZ1E1tD9EKMe6R8OpMQeeuVThFA1F9n2GdhO1Bqjo9Wy15ntl0vTa/+wL8ElI1Tn+FTV/06cIsO05601UI+YzTMypcmrdDxj56uZsg6e9JozzbiIn1nmIc5tKcZtKkBy03wIIfNtQ8UU3ZD2aoYKDWCCdIwwExs5AsOkvsbncxa0Qiut1z3f1p0gbNWd/dh0mGK7QUJIM7ZditPe1h3E0eZdK6vyg5SSRy1OPIevkbMP+xQObaOMCihOgStPcoQktTmo5YOClCnhiKLf70wUwEPHyPo8ewSjdVPNIZ71GcbkzevQMSV4qYmxv4xZRo2dD2BONqAR8UmiQy4V439zHKc6jcCRluhL1jlDYn3Z6jSMtZGOEzEwwUP/OktkUvtsnZg0xCevY3fvhJYAYEHQC47zjb9rviaeqDhji8zQJoFShr5svROvFwK8iS8S2AqAt1qVBrVhtNtQXcv6UT3hc3QNFIck2H6ysv0sL6AtM724aIcdKMGbODfZ1Uy9DS5upvmZF8tOiyr2rYK2hePKyvbtrsOcVXe4TPdYPvbRMiCAy+mquOhrdbu2FkcwKhLL7Jvvmus4EuEWEVzsMVHAfSUt+LtgUPEXrV9l0yAkSMEekOoi/S8qcnppjK7naWS5MjBasLKoZweDt02h3z7NcGEXw8EB10fsdLsKu8aVWRG5pHY+qzfq8biyvXruqBlhgqL/BcfibrGooV6qCwqXG9FSVS6 iYNHFybc PYpb3llVp2BFBJtBfXjGHwl0/BnvQhUi1/0+CM4F08qn1W4+wOacFVlcrYGRemt0PPwdUxV5uCH5yONmjMNhXV0FeLPOAiQ4HucMx 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 Wed, Jan 4, 2023 at 1:20 AM David Hildenbrand wrote: > > >> Or am I wrong? > >> > >>> Is anon_vma lock required? Almost not: if any page other than expected > >>> subpage of the non-anon huge page is found in the page table, collapse is > >>> aborted without making any change. However, it is possible that an anon > >>> page was CoWed from this extent in another mm or vma, in which case a > >>> concurrent lookup might look here: so keep it away while clearing pmd > >>> (but perhaps we shall go back to using pmd_lock() there in future). > >>> > >>> Note that collapse_pte_mapped_thp() is exceptional in freeing a page table > >>> without having cleared its ptes: I'm uneasy about that, and had thought > >>> pte_clear()ing appropriate; but exclusive i_mmap lock does fix the problem, > >>> and we would have to move the mmu_notification if clearing those ptes. > >>> > >>> Fixes: 8d3c106e19e8 ("mm/khugepaged: take the right locks for page table > >>> retraction") > >>> Signed-off-by: Hugh Dickins > >>> Cc: Jann Horn > >>> Cc: Yang Shi > >>> Cc: David Hildenbrand > >>> Cc: Zach O'Keefe > >>> Cc: Song Liu > >>> Cc: [5.4+] > >>> --- > >>> What this fixes is not a dangerous instability! But I suggest Cc stable > >>> because uprobes "healing" has regressed in that way, so this should follow > >>> 8d3c106e19e8 into those stable releases where it was backported (and may > >>> want adjustment there - I'll supply backports as needed). > >> > >> If it's really something that doesn't matter in practice (e.g., -1% > >> performance while debugging :) ), I guess no CC is needed. If there are real > >> production workloads that suffer, I guess ccing stable is fine. > > > > It's about recovering performance *after* debugging. It is not something > > that is of any value to me personally, nor (so far as I know) to anyone > > whom I work with. But it is something which Song Liu went to the trouble > > to make possible in his "THP aware uprobe" series three years ago, and it > > is something which Jann unintentionally regressed in his recent commit: > > so I thought it proper to reinstate where regressed. > > Right, although I wonder if that original series fixed a real > performance issue or was more a "this makes sense, let's just optimize > this corner case by some serious complexity". I hope it's not the latter :) > > > > > (What I do have more of an investment in, is for MADV_COLLAPSE to be able > > to collapse some extents in a large vma where some other extent got CoWed, > > so giving the whole vma an anon_vma. But that's not an issue for -stable, > > and I cannot tell you offhand whether undoing this anon_vma exclusion is > > enough to enable that or not - I suspect not, I suspect a result code or > > switch statement needs to be adjusted too.) > > Yeah, having a single COWed page in a large MAP_PRIVATE file/shmem > mapping would disable collapse, so it's the right thing to do. > > Thinking about it some more, and the effective code change, stable > doesn't sound wrong. > > >> > >> > >> Side note: set_huge_pmd() wins the award of "ugliest mm function of early > >> 2023". I was briefly concerned how do_set_pmd() decides whether the PMD can be > >> writable or not. Turns out it's communicated via vm_fault->flags. Just > >> horrible. > > > > I firmly disagree - it's from 2022! and much too small to be ugliest; > > but I haven't thought about the aspect that is bothering you there. > > The ugliest I stumbled over in early 2023 -- until January 2nd :D > > > > > What's bothered me most about it, is the way its name, and the naming of > > the do_set_pmd() it interfaces with, give no hint that they are entirely > > about file (or shmem) vmas, and would not work right on anon vmas > > (I forget whether it's just a matter of which stats updated, or more). > > Yes. I dug very deep into in-place collapse yesterday because I was > briefly concerned about anon THP, and it took me longer to understand > that whole machinery than it should (and that anon THP never ever > collapse in-place). > > Some of that khugepaged stuff needs some *serious* cleanups and > refactoring. do_set_pmd() is not an exception. > > > Some more examples: > > if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) { > ... > hpage_collapse_scan_file() > } else { > hpage_collapse_scan_pmd() > ... > } > > > 1) hpage_collapse_scan_pmd() is only for anon memory. Totally obvious > from the name. But why are we potentially calling it for VMAs that > are not applicable? For maximum David confusion? IIRC the VMAs are checked before, what do you mean about "not applicable"? But anyway khugepaged/MADV_COLLAPSE does release and reacquire mmap_lock multiple times, so there are multiple places to check VMAs validity. > > 2) "IS_ENABLED(CONFIG_SHMEM) && vma->vm_file" is also supposed to cover > ordinary file-thp. Totally obvious from the IS_ENABLED(CONFIG_SHMEM) > ... I probably spent 30minutes understanding what's happening here. > Just misleading and wrong without CONFIG_SHMEM. > > > ... and what's easier to get than this magic set of boolean flags: > > hugepage_vma_check(vma, vma->vm_flags, false, false, true) This is not perfect. I was thinking about changing them to one flag, just like TTU_ flags used by try_to_unmap(). That may make things cleaner. > > ... and obviously > hugepage_vma_revalidate() > is supposed to be a follow up to a previous > hugepage_vma_check() > and totally different from > transhuge_vma_suitable() > > Hard to make it even less consistent. This was after my cleanup, it was much messier before. And I did add comments to make them more understandable, but anyway better naming is definitely welcome. > > Also, it's very clear from the code that SCAN_PTE_MAPPED_HUGEPAGE only > applies to file-thp, right? No. > > -- > Thanks, > > David / dhildenb >