Message ID | 20230925200110.1979606-1-zokeefe@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp1459175vqu; Mon, 25 Sep 2023 13:17:13 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG7BsmuPWm8tNS26TYou5EbYtvWx6TRYmIn081yb2wtB/j59FCJ8lZ60R61d5W13UfYLFXb X-Received: by 2002:a05:6830:20d3:b0:6bc:f6d0:87d4 with SMTP id z19-20020a05683020d300b006bcf6d087d4mr9508953otq.29.1695673033459; Mon, 25 Sep 2023 13:17:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695673033; cv=none; d=google.com; s=arc-20160816; b=MmlC7Qzma6xmUGOPsnDb7oZn7zXrDsAtaQyc2tNUJQTcgs8qZzeIu42ZBgfMxWysnh 6LtzwXU6VyHCoS4EqXgA4BMvF+v5cm1O06XOBCRy+XmOLdbxDcgkoJrc3xPFC2C7oW2u H0mJDoWzXJfLGdz+zwExTMApWqYtFCbbzgJpir32XttkIRGWU5co1QXDgJcy99wGUrIv NamRDG/azQi3SX6WxDWBLarei7ixU0DwmmVBvG1gKOUl1quicSe2b3AcHDJOYY7n3qSX mh97Dbkc/+EJsgMvoSomKWmCXZ+LXksiG7yBed/gSm3xcH01ux/lnQZ/qoIS/1+6fnqg INUg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :dkim-signature; bh=zmn8UWXrJoykttF2WL0ItLwCoiyMGCPwIkseCGByc8Y=; fh=DWC6fWv2CqL3wI7WAPu7bJm4SC9JGPPCdWngas7hxd4=; b=KFsHB/WvTBw6FdX2nhrKfY4AH2577W89OT0co0XGEYeQK3+1pFnsiN+9QeBM/jra6h PEmzBtAtqbu0D0WFKrjIKVRXJ/TBwyEX1cxorHzqjjhK9uAPvM0dQ5rugZFmcyFW1k9T AShxw4LvVoxejwMr751HREYk69ud989M0AJsGIrmfR4jmMlscIQxp3LZEQ5i6JvXDDxu ITr3gTnuE6KNQrsqqyPinCb2ZjZfNpdoV4v3mSLhNW/NxBHWZ9Eq5URb0ikbMlUAAx+b /uk3PeCVw86eiqc7M9/G9agautUeVmkamCArSyH8vQ7hTonK1HLbQYB7oWkfYNFDRWzR cUtQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=qQHVJZOe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from howler.vger.email (howler.vger.email. [23.128.96.34]) by mx.google.com with ESMTPS id bx6-20020a056a00428600b00690bd5a0bccsi10821040pfb.363.2023.09.25.13.17.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Sep 2023 13:17:13 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=qQHVJZOe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id 8FB0F82CFA65; Mon, 25 Sep 2023 13:01:34 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233132AbjIYUB0 (ORCPT <rfc822;pusanteemu@gmail.com> + 29 others); Mon, 25 Sep 2023 16:01:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43456 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229481AbjIYUBZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 25 Sep 2023 16:01:25 -0400 Received: from mail-yw1-x114a.google.com (mail-yw1-x114a.google.com [IPv6:2607:f8b0:4864:20::114a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B573110C for <linux-kernel@vger.kernel.org>; Mon, 25 Sep 2023 13:01:18 -0700 (PDT) Received: by mail-yw1-x114a.google.com with SMTP id 00721157ae682-59c2ca3bcf9so134649387b3.3 for <linux-kernel@vger.kernel.org>; Mon, 25 Sep 2023 13:01:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1695672078; x=1696276878; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=zmn8UWXrJoykttF2WL0ItLwCoiyMGCPwIkseCGByc8Y=; b=qQHVJZOeERvwGglS6nJtg3QgMycP+eC9XYQXnqFKA+1TjQBXsNvepv8NEB70lpglXe FyMriTsIOp7ul47hpTVdhcvNto8A7K78vLXlYx2wFoZPs7W8OeM+qURNgMhqMKX2jGTt +X/Iat2n3k/XyqqBH5EZRn45fuK7nkhgkbdYzABloB6qzcoBzvIWgk2TFbR7uDi6URXz er5bK8AaNf3IOZWc9tPEYmQnPGWCyEhqdiHVkDC9Z22qkli962KPYXwo7j04r1acDoGM oI3hN8/n1RHwbs7e6ADJ2J1ryuABxNjVHuJHi5l/SAHuGREHS1MbDMNx4LcchEopmp2U gU6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695672078; x=1696276878; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=zmn8UWXrJoykttF2WL0ItLwCoiyMGCPwIkseCGByc8Y=; b=ghqrVFknaQ6af7oPFDJZw39Sjb5dicjhxMN00c0oY3F/0vzUivSBl0Smxzlc/OdCrX wVm+/UcAxu6vl4TE1ZJtAW+tJTX8ML2JKrd/gLGVfAuHBjk2c9Kzd8P5dcFeXZFxKBLR R5PojUedh+R9ES7wWyRBrgcWJ4qB+28DQO2NqZO++E9TG4+3LU1dmcQ0RfDUevCCyANN EgMC5x8FjXmuEMiJbaP5NKa9xdHghZ8gkV6aIIhfnEPyTD6SlgL0cmwH+iixq3ABSKWs jL38Riut7Ej9lEl7wc0n4GmSaXGOpQ7hJFZCCrbVPMBUa8u/sRjaiFZxtVTlvwF25mzb Cv3Q== X-Gm-Message-State: AOJu0YxOVrIgQfcg2uNf4GGgk2NT41rJmMgmSwrgbjDiRFYICrL9lo4W v/sM1z2AZmLmxZuJwxuJqo9lTht0Gf6G X-Received: from zokeefe3.c.googlers.com ([fda3:e722:ac3:cc00:20:ed76:c0a8:2971]) (user=zokeefe job=sendgmr) by 2002:a81:e503:0:b0:59b:e81f:62ab with SMTP id s3-20020a81e503000000b0059be81f62abmr92301ywl.7.1695672077978; Mon, 25 Sep 2023 13:01:17 -0700 (PDT) Date: Mon, 25 Sep 2023 13:01:10 -0700 Mime-Version: 1.0 X-Mailer: git-send-email 2.42.0.515.g380fc7ccd1-goog Message-ID: <20230925200110.1979606-1-zokeefe@google.com> Subject: [PATCH v4] mm/thp: fix "mm: thp: kill __transhuge_page_enabled()" From: "Zach O'Keefe" <zokeefe@google.com> To: linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org, "Zach O'Keefe" <zokeefe@google.com>, Saurabh Singh Sengar <ssengar@microsoft.com>, Yang Shi <shy828301@gmail.com>, Matthew Wilcox <willy@infradead.org>, David Hildenbrand <david@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Mon, 25 Sep 2023 13:01:34 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778042046769523371 X-GMAIL-MSGID: 1778042046769523371 |
Series |
[v4] mm/thp: fix "mm: thp: kill __transhuge_page_enabled()"
|
|
Commit Message
Zach O'Keefe
Sept. 25, 2023, 8:01 p.m. UTC
The 6.0 commits:
commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()")
commit 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()")
merged "can we have THPs in this VMA?" logic that was previously done
separately by fault-path, khugepaged, and smaps "THPeligible" checks.
During the process, the semantics of the fault path check changed in two
ways:
1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps path).
2) We no longer checked if non-anonymous memory had a vm_ops->huge_fault
handler that could satisfy the fault. Previously, this check had been
done in create_huge_pud() and create_huge_pmd() routines, but after
the changes, we never reach those routines.
During the review of the above commits, it was determined that in-tree
users weren't affected by the change; most notably, since the only relevant
user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is
explicitly approved early in approval logic. However, this was a bad
assumption to make as it assumes the only reason to support ->huge_fault
was for DAX (which is not true in general).
Remove the VM_NO_KHUGEPAGED check when not in collapse path and give
any ->huge_fault handler a chance to handle the fault. Note that we
don't validate the file mode or mapping alignment, which is consistent
with the behavior before the aforementioned commits.
Fixes: 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()")
Reported-by: Saurabh Singh Sengar <ssengar@microsoft.com>
Signed-off-by: Zach O'Keefe <zokeefe@google.com>
Cc: Yang Shi <shy828301@gmail.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
---
I've updated the changelog to reflect discussions in [1] -- leaving
ack to David / Matthew on whether to take the patch.
Changed from v3[1]:
- [akpm / David H. / M. Wilcox] Updated log to capture email discussion
Changed from v2[2]:
- Fixed false negative in smaps check when !dax && ->huge_fault
Changed from v1[3]:
- [Saurabhi] Allow ->huge_fault handler to handle fault, if it exists
[1] https://lore.kernel.org/linux-mm/20230821234844.699818-1-zokeefe@google.com/
[2] https://lore.kernel.org/linux-mm/20230818211533.2523697-1-zokeefe@google.com/
[3] https://lore.kernel.org/linux-mm/CAAa6QmQw+F=o6htOn=6ADD6mwvMO=Ow_67f3ifBv3GpXx9Xg_g@mail.gmail.com/
---
mm/huge_memory.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
Comments
On Mon, Sep 25, 2023 at 1:01 PM Zach O'Keefe <zokeefe@google.com> wrote: > > The 6.0 commits: > > commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()") > commit 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > > merged "can we have THPs in this VMA?" logic that was previously done > separately by fault-path, khugepaged, and smaps "THPeligible" checks. > > During the process, the semantics of the fault path check changed in two > ways: > > 1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps path). > 2) We no longer checked if non-anonymous memory had a vm_ops->huge_fault > handler that could satisfy the fault. Previously, this check had been > done in create_huge_pud() and create_huge_pmd() routines, but after > the changes, we never reach those routines. > > During the review of the above commits, it was determined that in-tree > users weren't affected by the change; most notably, since the only relevant > user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is > explicitly approved early in approval logic. However, this was a bad > assumption to make as it assumes the only reason to support ->huge_fault > was for DAX (which is not true in general). > > Remove the VM_NO_KHUGEPAGED check when not in collapse path and give > any ->huge_fault handler a chance to handle the fault. Note that we > don't validate the file mode or mapping alignment, which is consistent > with the behavior before the aforementioned commits. Looks good to me. Reviewed-by: Yang Shi <shy828301@gmail.com> > > Fixes: 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > Reported-by: Saurabh Singh Sengar <ssengar@microsoft.com> > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > Cc: Yang Shi <shy828301@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: David Hildenbrand <david@redhat.com> > --- > I've updated the changelog to reflect discussions in [1] -- leaving > ack to David / Matthew on whether to take the patch. > > Changed from v3[1]: > - [akpm / David H. / M. Wilcox] Updated log to capture email discussion > Changed from v2[2]: > - Fixed false negative in smaps check when !dax && ->huge_fault > Changed from v1[3]: > - [Saurabhi] Allow ->huge_fault handler to handle fault, if it exists > > [1] https://lore.kernel.org/linux-mm/20230821234844.699818-1-zokeefe@google.com/ > [2] https://lore.kernel.org/linux-mm/20230818211533.2523697-1-zokeefe@google.com/ > [3] https://lore.kernel.org/linux-mm/CAAa6QmQw+F=o6htOn=6ADD6mwvMO=Ow_67f3ifBv3GpXx9Xg_g@mail.gmail.com/ > > --- > mm/huge_memory.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 0f93a73115f73..797fe617e51ab 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -100,11 +100,11 @@ bool hugepage_vma_check(struct vm_area_struct *vma, unsigned long vm_flags, > return in_pf; > > /* > - * Special VMA and hugetlb VMA. > + * khugepaged special VMA and hugetlb VMA. > * Must be checked after dax since some dax mappings may have > * VM_MIXEDMAP set. > */ > - if (vm_flags & VM_NO_KHUGEPAGED) > + if (!in_pf && !smaps && (vm_flags & VM_NO_KHUGEPAGED)) > return false; > > /* > @@ -132,12 +132,18 @@ bool hugepage_vma_check(struct vm_area_struct *vma, unsigned long vm_flags, > !hugepage_flags_always()))) > return false; > > - /* Only regular file is valid */ > - if (!in_pf && file_thp_enabled(vma)) > - return true; > - > - if (!vma_is_anonymous(vma)) > + if (!vma_is_anonymous(vma)) { > + /* > + * Trust that ->huge_fault() handlers know what they are doing > + * in fault path. > + */ > + if (((in_pf || smaps)) && vma->vm_ops->huge_fault) > + return true; > + /* Only regular file is valid in collapse path */ > + if (((!in_pf || smaps)) && file_thp_enabled(vma)) > + return true; > return false; > + } > > if (vma_is_temporary_stack(vma)) > return false; > -- > 2.42.0.515.g380fc7ccd1-goog >
On Mon, 25 Sep 2023 13:01:10 -0700 "Zach O'Keefe" <zokeefe@google.com> wrote: > The 6.0 commits: > > commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()") > commit 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > > merged "can we have THPs in this VMA?" logic that was previously done > separately by fault-path, khugepaged, and smaps "THPeligible" checks. > > During the process, the semantics of the fault path check changed in two > ways: > > 1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps path). > 2) We no longer checked if non-anonymous memory had a vm_ops->huge_fault > handler that could satisfy the fault. Previously, this check had been > done in create_huge_pud() and create_huge_pmd() routines, but after > the changes, we never reach those routines. > > During the review of the above commits, it was determined that in-tree > users weren't affected by the change; most notably, since the only relevant > user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is > explicitly approved early in approval logic. However, this was a bad > assumption to make as it assumes the only reason to support ->huge_fault > was for DAX (which is not true in general). > > Remove the VM_NO_KHUGEPAGED check when not in collapse path and give > any ->huge_fault handler a chance to handle the fault. Note that we > don't validate the file mode or mapping alignment, which is consistent > with the behavior before the aforementioned commits. It's unclear what are the userspace visible impacts of this change. Which makes it hard for others to determine whether -stable kernels should be patched. > Fixes: 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > Reported-by: Saurabh Singh Sengar <ssengar@microsoft.com> It's nice to include a Closes: link after a Reported-by:. Then readers are better able to answer the above question. > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > Cc: Yang Shi <shy828301@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: David Hildenbrand <david@redhat.com>
On Mon, 25 Sep 2023 13:01:10 -0700 "Zach O'Keefe" <zokeefe@google.com> wrote: > The 6.0 commits: > > commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()") > commit 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > > merged "can we have THPs in this VMA?" logic that was previously done > separately by fault-path, khugepaged, and smaps "THPeligible" checks. > > During the process, the semantics of the fault path check changed in two > ways: > > 1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps path). > 2) We no longer checked if non-anonymous memory had a vm_ops->huge_fault > handler that could satisfy the fault. Previously, this check had been > done in create_huge_pud() and create_huge_pmd() routines, but after > the changes, we never reach those routines. > > During the review of the above commits, it was determined that in-tree > users weren't affected by the change; most notably, since the only relevant > user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is > explicitly approved early in approval logic. However, this was a bad > assumption to make as it assumes the only reason to support ->huge_fault > was for DAX (which is not true in general). > > Remove the VM_NO_KHUGEPAGED check when not in collapse path and give > any ->huge_fault handler a chance to handle the fault. Note that we > don't validate the file mode or mapping alignment, which is consistent > with the behavior before the aforementioned commits. > > ... > > @@ -100,11 +100,11 @@ bool hugepage_vma_check(struct vm_area_struct *vma, unsigned long vm_flags, > return in_pf; > Ryan's "mm: thp: introduce anon_orders and anon_always_mask sysfs files" changes hugepage_vma_check() to return an unsigned int, so this patch will need some rework to fit in after that. However Ryan's overall series "variable-order, large folios for anonymous memory" is in early days and might not make it. And as I don't know what is the urgency of this patch ("mm/thp: fix "mm: thp: kill __transhuge_page_enabled()"), I'm unable to decide which patch needs to come first (thus requiring rework of the other patch). Please discuss!
On 06.10.23 19:58, Andrew Morton wrote: > On Mon, 25 Sep 2023 13:01:10 -0700 "Zach O'Keefe" <zokeefe@google.com> wrote: > >> The 6.0 commits: >> >> commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()") >> commit 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") >> >> merged "can we have THPs in this VMA?" logic that was previously done >> separately by fault-path, khugepaged, and smaps "THPeligible" checks. >> >> During the process, the semantics of the fault path check changed in two >> ways: >> >> 1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps path). >> 2) We no longer checked if non-anonymous memory had a vm_ops->huge_fault >> handler that could satisfy the fault. Previously, this check had been >> done in create_huge_pud() and create_huge_pmd() routines, but after >> the changes, we never reach those routines. >> >> During the review of the above commits, it was determined that in-tree >> users weren't affected by the change; most notably, since the only relevant >> user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is >> explicitly approved early in approval logic. However, this was a bad >> assumption to make as it assumes the only reason to support ->huge_fault >> was for DAX (which is not true in general). >> >> Remove the VM_NO_KHUGEPAGED check when not in collapse path and give >> any ->huge_fault handler a chance to handle the fault. Note that we >> don't validate the file mode or mapping alignment, which is consistent >> with the behavior before the aforementioned commits. >> >> ... >> >> @@ -100,11 +100,11 @@ bool hugepage_vma_check(struct vm_area_struct *vma, unsigned long vm_flags, >> return in_pf; >> > > Ryan's "mm: thp: introduce anon_orders and anon_always_mask sysfs > files" changes hugepage_vma_check() to return an unsigned int, so this > patch will need some rework to fit in after that. > > However Ryan's overall series "variable-order, large folios for > anonymous memory" is in early days and might not make it. > > And as I don't know what is the urgency of this patch ("mm/thp: fix > "mm: thp: kill __transhuge_page_enabled()"), I'm unable to decide which > patch needs to come first (thus requiring rework of the other patch). > > Please discuss! IMHO clearly this one.
On 25.09.23 22:01, Zach O'Keefe wrote: > The 6.0 commits: > > commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()") > commit 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > > merged "can we have THPs in this VMA?" logic that was previously done > separately by fault-path, khugepaged, and smaps "THPeligible" checks. > > During the process, the semantics of the fault path check changed in two > ways: > > 1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps path). > 2) We no longer checked if non-anonymous memory had a vm_ops->huge_fault > handler that could satisfy the fault. Previously, this check had been > done in create_huge_pud() and create_huge_pmd() routines, but after > the changes, we never reach those routines. > > During the review of the above commits, it was determined that in-tree > users weren't affected by the change; most notably, since the only relevant > user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is > explicitly approved early in approval logic. However, this was a bad > assumption to make as it assumes the only reason to support ->huge_fault > was for DAX (which is not true in general). > > Remove the VM_NO_KHUGEPAGED check when not in collapse path and give > any ->huge_fault handler a chance to handle the fault. Note that we > don't validate the file mode or mapping alignment, which is consistent > with the behavior before the aforementioned commits. > > Fixes: 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > Reported-by: Saurabh Singh Sengar <ssengar@microsoft.com> > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > Cc: Yang Shi <shy828301@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: David Hildenbrand <david@redhat.com> > --- > I've updated the changelog to reflect discussions in [1] -- leaving > ack to David / Matthew on whether to take the patch. Works for me. Acked-by: David Hildenbrand <david@redhat.com>
On Fri, 6 Oct 2023 20:52:30 +0200 David Hildenbrand <david@redhat.com> wrote: > > And as I don't know what is the urgency of this patch ("mm/thp: fix > > "mm: thp: kill __transhuge_page_enabled()"), I'm unable to decide which > > patch needs to come first (thus requiring rework of the other patch). > > > > Please discuss! > > IMHO clearly this one. OK. I'll drop the "variable-order, large folios for anonymous memory" v6 series for now.
On 06/10/2023 20:11, Andrew Morton wrote: > On Fri, 6 Oct 2023 20:52:30 +0200 David Hildenbrand <david@redhat.com> wrote: > >>> And as I don't know what is the urgency of this patch ("mm/thp: fix >>> "mm: thp: kill __transhuge_page_enabled()"), I'm unable to decide which >>> patch needs to come first (thus requiring rework of the other patch). >>> >>> Please discuss! >> >> IMHO clearly this one. > > OK. I'll drop the "variable-order, large folios for anonymous memory" v6 > series for now. Yep, agreed!
On Fri, Oct 6, 2023 at 10:50 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 25 Sep 2023 13:01:10 -0700 "Zach O'Keefe" <zokeefe@google.com> wrote: > > > The 6.0 commits: > > > > commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()") > > commit 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > > > > merged "can we have THPs in this VMA?" logic that was previously done > > separately by fault-path, khugepaged, and smaps "THPeligible" checks. > > > > During the process, the semantics of the fault path check changed in two > > ways: > > > > 1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps path). > > 2) We no longer checked if non-anonymous memory had a vm_ops->huge_fault > > handler that could satisfy the fault. Previously, this check had been > > done in create_huge_pud() and create_huge_pmd() routines, but after > > the changes, we never reach those routines. > > > > During the review of the above commits, it was determined that in-tree > > users weren't affected by the change; most notably, since the only relevant > > user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is > > explicitly approved early in approval logic. However, this was a bad > > assumption to make as it assumes the only reason to support ->huge_fault > > was for DAX (which is not true in general). > > > > Remove the VM_NO_KHUGEPAGED check when not in collapse path and give > > any ->huge_fault handler a chance to handle the fault. Note that we > > don't validate the file mode or mapping alignment, which is consistent > > with the behavior before the aforementioned commits. > > It's unclear what are the userspace visible impacts of this change. > Which makes it hard for others to determine whether -stable kernels > should be patched. IMO, I don't think this change is suitable for -stable; the only users that would have been affected are those that maintain out-of-tree drivers / code that hooked into ->huge_fault() or used VM_MIXEDMAP + THP. No users of the in-tree kernel would have been affected. It's still a good "fix" to make going forward (and certainly happy to be able to help Saurabh out). + greg k-h for vis / to confirm. > > Fixes: 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > > Reported-by: Saurabh Singh Sengar <ssengar@microsoft.com> > > It's nice to include a Closes: link after a Reported-by:. Then readers > are better able to answer the above question. Ah, apologies, Andrew; I didn't know such a tag existed -- I'll be sure to include it in the future. > > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > > Cc: Yang Shi <shy828301@gmail.com> > > Cc: Matthew Wilcox <willy@infradead.org> > > Cc: David Hildenbrand <david@redhat.com> >
On Fri, Oct 6, 2023 at 2:28 PM Ryan Roberts <ryan.roberts@arm.com> wrote: > > On 06/10/2023 20:11, Andrew Morton wrote: > > On Fri, 6 Oct 2023 20:52:30 +0200 David Hildenbrand <david@redhat.com> wrote: > > > >>> And as I don't know what is the urgency of this patch ("mm/thp: fix > >>> "mm: thp: kill __transhuge_page_enabled()"), I'm unable to decide which > >>> patch needs to come first (thus requiring rework of the other patch). > >>> > >>> Please discuss! > >> > >> IMHO clearly this one. > > > > OK. I'll drop the "variable-order, large folios for anonymous memory" v6 > > series for now. > > Yep, agreed! Thank all and sorry for the late response ; have been buried as of late. Best, Zach
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 0f93a73115f73..797fe617e51ab 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -100,11 +100,11 @@ bool hugepage_vma_check(struct vm_area_struct *vma, unsigned long vm_flags, return in_pf; /* - * Special VMA and hugetlb VMA. + * khugepaged special VMA and hugetlb VMA. * Must be checked after dax since some dax mappings may have * VM_MIXEDMAP set. */ - if (vm_flags & VM_NO_KHUGEPAGED) + if (!in_pf && !smaps && (vm_flags & VM_NO_KHUGEPAGED)) return false; /* @@ -132,12 +132,18 @@ bool hugepage_vma_check(struct vm_area_struct *vma, unsigned long vm_flags, !hugepage_flags_always()))) return false; - /* Only regular file is valid */ - if (!in_pf && file_thp_enabled(vma)) - return true; - - if (!vma_is_anonymous(vma)) + if (!vma_is_anonymous(vma)) { + /* + * Trust that ->huge_fault() handlers know what they are doing + * in fault path. + */ + if (((in_pf || smaps)) && vma->vm_ops->huge_fault) + return true; + /* Only regular file is valid in collapse path */ + if (((!in_pf || smaps)) && file_thp_enabled(vma)) + return true; return false; + } if (vma_is_temporary_stack(vma)) return false;