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

[Matthew Wilcox <willy@infradead.org>]

On Thu, Aug 17, 2023 at 11:13:36AM -0700, Zach O'Keefe wrote:
> > > 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?

If the filesystem has per-folio metadata, we need to give the filesystem
the chance to set that up.  I've vaguaely been wondering about using the
->migrate_folio callback for it.  At the moment, I think it just refuses
to work if the folio isn't order-0.

> > > 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?

Yeah, we ignore that too.  My rationale is -- as you said -- using the
PMDs is actually free, and it's really none of the app's business how
the page cache chooses to cache things.