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

["Zach O'Keefe" <zokeefe@google.com>]

On Thu, Aug 17, 2023 at 12:01 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> 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.

Ok, no free lunch here then. I'll give myself a reminder to come back
here then and dig a little deeper. Thanks Matthew

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

What should be done to be consistent with the collapse side here, for
file/shmem if at all? Answering the question, "can this memory be
backed by THPs" is becoming really complex, and that THPelligble smaps
field is becoming increasingly more difficult to use.