Re: [PATCH 21/46] hugetlb: use struct hugetlb_pte for walk_hugetlb_range

From: David Hildenbrand
Date: Wed Jan 18 2023 - 12:09:18 EST

On 18.01.23 16:35, Peter Xu wrote:
On Wed, Jan 18, 2023 at 10:43:47AM +0100, David Hildenbrand wrote:
On 18.01.23 00:11, James Houghton wrote:
On Mon, Jan 16, 2023 at 2:17 AM David Hildenbrand <david@xxxxxxxxxx> wrote:

On 12.01.23 22:33, Peter Xu wrote:
On Thu, Jan 12, 2023 at 04:17:52PM -0500, James Houghton wrote:
I'll look into it, but doing it this way will use _mapcount, so we
won't be able to use the vmemmap optimization. I think even if we do
use Hugh's approach, refcount is still being kept on the head page, so
there's still an overflow risk there (but maybe I am
misunderstanding).

Could you remind me what's the issue if using refcount on the small pages
rather than the head (assuming vmemmap still can be disabled)?

The THP-way of doing things is refcounting on the head page. All folios
use a single refcount on the head.

There has to be a pretty good reason to do it differently.

Peter and I have discussed this a lot offline. There are two main problems here:

1. Refcount overflow

Refcount is always kept on the head page (before and after this
series). IIUC, this means that if THPs could be 1G in size, they too
would be susceptible to the same potential overflow. How easy is the
overflow? [1]

Right. You'd need 8k VMAs. With 2 MiB THP you'd need 4096k VMAs. So ~64
processes with 64k VMAs. Not impossible to achieve if one really wants to
break the system ...

Side note: a long long time ago, we used to have sub-page refcounts for THP.
IIRC, that was even before we had sub-page mapcounts and was used to make
COW decisions.


To deal with this, the best solution we've been able to come up with
is to check if refcount is > INT_MAX/2 (similar to try_get_page()),
and if it is, stop the operation (UFFDIO_CONTINUE or a page fault)
from proceeding. In the UFFDIO_CONTINUE case, return ENOMEM. In the
page fault cause, return VM_FAULT_SIGBUS (not VM_FAULT_OOM; we don't
want to kill a random process).

You'd have to also make sure that fork() won't do the same. At least with
uffd-wp, Peter also added page table copying during fork() for MAP_SHARED
mappings, which would have to be handled.

If we want such a check to make a real difference, IIUC we may want to
consider having similar check in:

page_ref_add
page_ref_inc
page_ref_inc_return
page_ref_add_unless

But it's unfortunate that mostly all the callers to these functions
(especially the first two) do not have a retval yet at all. Considering
the low possibility so far on having it overflow, maybe it can also be done
for later (and I think checking negative as try_get_page would suffice too).


Of course, one can just disallow fork() with any HGM right from the start
and keep it all simpler to not open up a can of worms there.

Is it reasonable, to have more than one (or a handful) of VMAs mapping a
huge page via a HGM? Restricting it to a single one, would make handling
much easier.

If there is ever demand for more HGM mappings, that whole problem (and
complexity) could be dealt with later. ... but I assume it will already be a
requirement for VMs (e.g., under QEMU) that share memory with other
processes (virtiofsd and friends?)

Yes, any form of multi-proc QEMU will need that for supporting HGM
postcopy.




(So David, I think this answers your question. Refcount should be
handled just like THPs.)

2. page_mapcount() API differences

In this series, page_mapcount() returns the total number of page table
references for the compound page. For example, if you have a
PTE-mapped 2M page (with no other mappings), page_mapcount() for each
4K page will be 512. This is not the same as a THP: page_mapcount()
would return 1 for each page. Because of the difference in
page_mapcount(), we have 4 problems:

IMHO, it would actually be great to just be able to remove the sub-page
mapcounts for THP and make it all simpler.

Right now, the sub-page mapcount is mostly required for making COW
decisions, but only for accounting purposes IIRC (NR_ANON_THPS,
NR_SHMEM_PMDMAPPED, NR_FILE_PMDMAPPED) and mlock handling IIRC. See
page_remove_rmap().

If we can avoid that complexity right from the start for hugetlb, great, ..


i. Smaps uses page_mapcount() >= 2 to determine if hugetlb memory is
"private_hugetlb" or "shared_hugetlb".
ii. Migration with MPOL_MF_MOVE will check page_mapcount() to see if
the hugepage is shared or not. Pages that would otherwise be migrated
now require MPOL_MF_MOVE_ALL to be migrated.
[Really both of the above are checking how many VMAs are mapping our hugepage.]
iii. CoW. This isn't a problem right now because CoW is only possible
with MAP_PRIVATE VMAs and HGM can only be enabled for MAP_SHARED VMAs.
iv. The hwpoison handling code will check if it successfully unmapped
the poisoned page. This isn't a problem right now, as hwpoison will
unmap all the mappings for the hugepage, not just the 4K where the
poison was found.

Doing it this way allows HGM to remain compatible with the hugetlb
vmemmap optimization. None of the above problems strike me as
particularly major, but it's unclear to me how important it is to have
page_mapcount() have a consistent meaning for hugetlb vs non-hugetlb.

See below, maybe we should tackle HGM from a different direction.


The other way page_mapcount() (let's say the "THP-like way") could be
done is like this: increment compound mapcount if we're mapping a
hugetlb page normally (e.g., 1G page with a PUD). If we're mapping at
high-granularity, increment the mapcount for each 4K page that is
getting mapped (e.g., PMD within a 1G page: increment the mapcount for
the 512 pages that are now mapped). This yields the same
page_mapcount() API we had before, but we lose the hugetlb vmemmap
optimization.

We could introduce an API like hugetlb_vma_mapcount() that would, for
hugetlb, give us the number of VMAs that map a hugepage, but I don't
think people would like this.

I'm curious what others think (Mike, Matthew?). I'm guessing the
THP-like way is probably what most people would want, though it would
be a real shame to lose the vmemmap optimization.

Heh, not me :) Having a single mapcount is certainly much cleaner. ... and
if we're dealing with refcount overflows already, mapcount overflows are not
an issue.


I wonder if the following crazy idea has already been discussed: treat the
whole mapping as a single large logical mapping. One reference and one
mapping, no matter how the individual parts are mapped into the assigned
page table sub-tree.

Because for hugetlb with MAP_SHARED, we know that the complete assigned
sub-tree of page tables can only map the given hugetlb page, no fragments of
something else. That's very different to THP in private mappings ...

So as soon as the first piece gets mapped, we increment refcount+mapcount.
Other pieces in the same subtree don't do that.

Once the last piece is unmapped (or simpler: once the complete subtree of
page tables is gone), we decrement refcount+mapcount. Might require some
brain power to do this tracking, but I wouldn't call it impossible right
from the start.

Would such a design violate other design aspects that are important?

The question is how to maintaining above information.

Right.


It needs to be per-map (so one page mapped multiple times can be accounted
differently), and per-page (so one mapping/vma can contain multiple pages).
So far I think that's exactly the pgtable. If we can squeeze information
into the pgtable it'll work out, but definitely not trivial. Or we can
maintain seperate allocates for such information, but that can be extra
overheads too.

If there is no sub-pgtable level, there is certainly no HGM. If there is a sub-pgtable, we can store that information in that pgtable memmap most probably. Maybe simply a pointer to the hugetlb page. As long the pointer is there, we increment the mapcount/refcount.


Either directly, or via some additional metadata. Metadata should be small and most probably "noting relevant in size" compared to the actual 1 GiB page or the 2 MiB+ of page tables to cover 1 GiB.

We could even teach most pgtable walkers to just assume that "logically" there is simply a hugtlb page mapped, without traversing the actual sub-pgtables. IIUC, only pgtable walkers that actually want to access page content (page faults, pinning) or change PTEs (mprotect, uffd) would really care. Maybe stuff like smaps could just say "well, there is a hugetlb page mapped" and continue. Just a thought.





So far I'd still consider going with reusing thp mapcounts, which will
mostly be what James mentioned above. The only difference is I'm not sure
whether we should allow mapping e.g. 2M ranges for 1G pages. THP mapcount
doesn't have intermediate layer to maintain mapcount information like 2M,
so to me it's easier we start with only mapping either the hpage size or
PAGE_SIZE, not any intermediate size allowed.

Having intermediate size mapping allowed can at least be error prone to
me. One example is if some pgtable walker found a 2M page, it may easily
fetch the PFN out of it, assuming it's a compound page and it should
satisfy PageHead(pfn)==true but it'll start to break here, because the 2M
PFN will only be a small page pfn for the 1G huge page in this case.

To me, intermediate sized mappings are good to have but not required to
resolve HGM problems, at least so far. Said that, I'm fine with looking at
what it'll look like if James would like to keep persuing that direction.

Yeah, was just an idea from my side to avoid most of the refcount and mapcount issues -- in theory :)

Let me think about that all a bit more ...

--
Thanks,

David / dhildenb