On Wed, Feb 1, 2023 at 8:22 AM Peter Xu <peterx@xxxxxxxxxx> wrote: > > On Wed, Feb 01, 2023 at 07:45:17AM -0800, James Houghton wrote: > > On Tue, Jan 31, 2023 at 5:24 PM Peter Xu <peterx@xxxxxxxxxx> wrote: > > > > > > On Tue, Jan 31, 2023 at 04:24:15PM -0800, James Houghton wrote: > > > > On Mon, Jan 30, 2023 at 1:14 PM Peter Xu <peterx@xxxxxxxxxx> wrote: > > > > > > > > > > On Mon, Jan 30, 2023 at 10:38:41AM -0800, James Houghton wrote: > > > > > > On Mon, Jan 30, 2023 at 9:29 AM Peter Xu <peterx@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > On Fri, Jan 27, 2023 at 01:02:02PM -0800, James Houghton wrote: > > [snip] > > > > > > > Another way to not use thp mapcount, nor break smaps and similar calls to > > > > > > > page_mapcount() on small page, is to only increase the hpage mapcount only > > > > > > > when hstate pXd (in case of 1G it's PUD) entry being populated (no matter > > > > > > > as leaf or a non-leaf), and the mapcount can be decreased when the pXd > > > > > > > entry is removed (for leaf, it's the same as for now; for HGM, it's when > > > > > > > freeing pgtable of the PUD entry). > > > > > > > > > > > > Right, and this is doable. Also it seems like this is pretty close to > > > > > > the direction Matthew Wilcox wants to go with THPs. > > > > > > > > > > I may not be familiar with it, do you mean this one? > > > > > > > > > > https://lore.kernel.org/all/Y9Afwds%2FJl39UjEp@xxxxxxxxxxxxxxxxxxxx/ > > > > > > > > Yep that's it. > > > > > > > > > > > > > > For hugetlb I think it should be easier to maintain rather than any-sized > > > > > folios, because there's the pgtable non-leaf entry to track rmap > > > > > information and the folio size being static to hpage size. > > > > > > > > > > It'll be different to folios where it can be random sized pages chunk, so > > > > > it needs to be managed by batching the ptes when install/zap. > > > > > > > > Agreed. It's probably easier for HugeTLB because they're always > > > > "naturally aligned" and yeah they can't change sizes. > > > > > > > > > > > > > > > > > > > > > Something I noticed though, from the implementation of > > > > > > folio_referenced()/folio_referenced_one(), is that folio_mapcount() > > > > > > ought to report the total number of PTEs that are pointing on the page > > > > > > (or the number of times page_vma_mapped_walk returns true). FWIW, > > > > > > folio_referenced() is never called for hugetlb folios. > > > > > > > > > > FWIU folio_mapcount is the thing it needs for now to do the rmap walks - > > > > > it'll walk every leaf page being mapped, big or small, so IIUC that number > > > > > should match with what it expects to see later, more or less. > > > > > > > > I don't fully understand what you mean here. > > > > > > I meant the rmap_walk pairing with folio_referenced_one() will walk all the > > > leaves for the folio, big or small. I think that will match the number > > > with what got returned from folio_mapcount(). > > > > See below. > > > > > > > > > > > > > > > > > > > But I agree the mapcount/referenced value itself is debatable to me, just > > > > > like what you raised in the other thread on page migration. Meanwhile, I > > > > > am not certain whether the mapcount is accurate either because AFAICT the > > > > > mapcount can be modified if e.g. new page mapping established as long as > > > > > before taking the page lock later in folio_referenced(). > > > > > > > > > > It's just that I don't see any severe issue either due to any of above, as > > > > > long as that information is only used as a hint for next steps, e.g., to > > > > > swap which page out. > > > > > > > > I also don't see a big problem with folio_referenced() (and you're > > > > right that folio_mapcount() can be stale by the time it takes the > > > > folio lock). It still seems like folio_mapcount() should return the > > > > total number of PTEs that map the page though. Are you saying that > > > > breaking this would be ok? > > > > > > I didn't quite follow - isn't that already doing so? > > > > > > folio_mapcount() is total_compound_mapcount() here, IIUC it is an > > > accumulated value of all possible PTEs or PMDs being mapped as long as it's > > > all or part of the folio being mapped. > > > > We've talked about 3 ways of handling mapcount: > > > > 1. The RFC v2 way, which is head-only, and we increment the compound > > mapcount for each PT mapping we have. So a PTE-mapped 2M page, > > compound_mapcount=512, subpage->_mapcount=0 (ignoring the -1 bias). > > 2. The THP-like way. If we are fully mapping the hugetlb page with the > > hstate-level PTE, we increment the compound mapcount, otherwise we > > increment subpage->_mapcount. > > 3. The RFC v1 way (the way you have suggested above), which is > > head-only, and we increment the compound mapcount if the hstate-level > > PTE is made present. > > Oh that's where it come from! It took quite some months going through all > these, I can hardly remember the details. > > > > > With #1 and #2, there is no concern with folio_mapcount(). But with > > #3, folio_mapcount() for a PTE-mapped 2M page mapped in a single VMA > > would yield 1 instead of 512 (right?). That's what I mean. > > > > #1 has problems wrt smaps and migration (though there were other > > problems with those anyway that Mike has fixed), and #2 makes > > MADV_COLLAPSE slow to the point of being unusable for some > > applications. > > Ah so you're talking about after HGM being applied.. while I was only > talking about THPs. > > If to apply the logic here with idea 3), the worst case is we'll need to > have special care of HGM hugetlb in folio_referenced_one(), so the default > page_vma_mapped_walk() may not apply anymore - the resource is always in > hstate sized, so counting small ptes do not help too - we can just walk > until the hstate entry and do referenced++ if it's not none, at the > entrance of folio_referenced_one(). > > But I'm not sure whether that'll be necessary at all, as I'm not sure > whether that path can be triggered at all in any form (where from the top > it should always be shrink_page_list()). In that sense maybe we can also > consider adding a WARN_ON_ONCE() in folio_referenced() where it is a > hugetlb page that got passed in? Meanwhile, adding a TODO comment > explaining that current walk won't work easily for HGM only, so when it > will be applicable to hugetlb we need to rework? > > I confess that's not pretty, though. But that'll make 3) with no major > defect from function-wise. Another potential idea would be to add something like page_vmacount(). For non-HugeTLB pages, page_vmacount() == page_mapcount(). Then for HugeTLB pages, we could keep a separate count (in one of the tail pages, I guess). And then in the places that matter (so smaps, migration, and maybe CoW and hwpoison), potentially change their calls to page_vmacount() instead of page_mapcount(). Then to implement page_vmacount(), we do the RFC v1 mapcount approach (but like.... correctly this time). And then for page_mapcount(), we do the RFC v2 mapcount approach (head-only, once per PTE). Then we fix folio_referenced() without needing to special-case it for HugeTLB. :) Or we could just special-case it. *shrug* Does that sound reasonable? We still have the problem where a series of partially unmaps could leave page_vmacount() incremented, but I don't think that's a big problem. > > Side note: did we finish folio conversion on hugetlb at all? I think at > least we need some helper like folio_test_huge(). It seems still missing. > Maybe it's another clue that hugetlb is not important to folio_referenced() > because it's already fully converted? I'm not sure. A lot of work was done very pretty recently, so I bet there's probably some work left to do.
