Re: [EXTERNAL] [PATCH] mm/thp: fix "mm: thp: kill __transhuge_page_enabled()"

[Zach O'Keefe]

On Thu, Aug 17, 2023 at 5:18 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Wed, Aug 16, 2023 at 02:31:06PM -0700, Zach O'Keefe wrote:
> > On Mon, Aug 14, 2023 at 7:24 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > > So if we find a large folio that is PMD mappable, and there's nothing
> > > at vmf->pmd, we install a PMD-sized mapping at that spot.  If that
> > > fails, we install the preallocated PTE table at vmf->pmd and continue to
> > > trying set one or more PTEs to satisfy this page fault.
> >
> > Aha! I see. I did not expect ->fault() to have this logic, as I had
> > incorrectly thought (aka assumed) the pmd vs pte-mapping logic split
> > at create_huge_pmd(); i.e. do_huge_pmd_anonymous_page(), or
> > ->huge_fault(), or fallback to pte-mapping. It seems very weird to me
> > that hugepage_vma_check() "artificially" says "no" to file and shmem
> > along the fault path, so they can go and do their own thing in
> > ->fault().
>
> Wow, hugepage_vma_check() is a very complicated function.  I'm glad I
> ignored it!

Ya it's a tangly area. Far better now though, then before Yang
centralized everything. But yes, now I need to figure out what to do
with it..

> > IIUC then, there is a bug in smaps THPeligible code when
> > CONFIG_READ_ONLY_THP_FOR_FS is not set. Not obvious, but apparently
> > this config is (according to it's Kconfig desc) khugepaged-only, so it
> > should be fine for it to be disabled, yet allow
> > do_sync_mmap_readahead() to install a pmd for file-backed memory.
> > hugepage_vma_check() will need to be patched to fix this.
>
> I guess so ...

The easiest and most satisfying way to handle this -- and I think we
talked about this before -- is relaxing that complicated
file_thp_enabled() check when the file's mapping supports large
folios. I think that makes sense to me, though I don't know all the
details fs-side. Will we need any hook to give fs the chance to update
any internal state on collapse?

> > But I have a larger question for you: should we care about
> > /sys/kernel/mm/transparent_hugepage/enabled for file-fault? We
> > currently don't. Seems weird that we can transparently get a hugepage
> > when THP="never". Also, if THP="always", we might as well skip the
> > VM_HUGEPAGE check, and try the final pmd install (and save khugepaged
> > the trouble of attempting it later).
>
> I deliberately ignored the humungous complexity of the THP options.
> They're overgrown and make my brain hurt. [..]

Same

> [..] Instead, large folios are
> adaptive; they observe the behaviour of the user program and choose based
> on history what to do.  This is far superior to having a sysadmin tell
> us what to do!

I had written a bunch on this, but I arrived to the conclusion that
(a) pmd-mapping here is ~ a free win, and (b) I'm not the best  person
to argue for these knobs, given MADV_COLLAPSE ignores them entirely :P

..But (sorry) what about MMF_DISABLE_THP?