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.
