On 03.07.24 11:20, ran xiaokai wrote:
On 26.06.24 04:49, ran xiaokai wrote:
From: Ran Xiaokai <ran.xiaokai@xxxxxxxxxx>
KPF_COMPOUND_HEAD and KPF_COMPOUND_TAIL are set on "common" compound
pages, which means of any order, but KPF_THP should only be set
when the folio is a 2M pmd mappable THP. Since commit 19eaf44954df
"should only be set" -- who says that? :)
The documentation only talks about "Contiguous pages which construct
transparent hugepages". Sure, when it was added there were only PMD ones.
("mm: thp: support allocation of anonymous multi-size THP"),
multiple orders of folios can be allocated and mapped to userspace,
so the folio_test_large() check is not sufficient here,
replace it with folio_test_pmd_mappable() to fix this.
A couple of points:
1) If I am not daydreaming, ever since we supported non-PMD THP in the
pagecache (much longer than anon mTHP), we already indicate KPF_THP
for them here. So this is not anon specific? Or am I getting the
PG_lru check all wrong?
2) Anon THP are disabled as default. If some custom tool cannot deal
with that "extension" we did with smaller THP, it shall be updated if
it really has to special-case PMD-mapped THP, before enabled by the
admin.
I think this interface does exactly what we want, as it is right now.
Unless there is *good* reason, we should keep it like that.
So I suggest
a) Extend the documentation to just state "THP of any size and any
mapping granularity" or sth like that.
b) Maybe using folio_test_large_rmappable() instead of "(k & (1 <<
PG_lru)) || is_anon", so even isolated-from-LRU THPs are indicated
properly.
Hi, David,
The "is_anon" check was introduced to also include page vector cache
pages, but now large folios are added to lru list directly, bypassed
the pagevec cache. So the is_anon check seems unnecessary here.
As now pagecache also supports large folios, the is_anon check seems
unsufficient here.
Can i say that for userspace memory,
folio_test_large_rmappable() == folio_test_large()?
if that is true, except the "if ((k & (1 << PG_lru)) || is_anon)"
check, we can also remove the folio_test_large() check,
like this:
else if (folio_test_large_rmappable(folio)) {
u |= 1 << KPF_THP;
else if (is_huge_zero_folio(folio)) {
u |= 1 << KPF_ZERO_PAGE;
u |= 1 << KPF_THP;
}
} else if (is_zero_pfn(page_to_pfn(page)))
This will also include the isolated folios.
You'll have to keep the folio_test_large() check,
folio_test_large_rmappable() wants us to check that ahead of time.
Something like
...
else if (folio_test_large(folio) && folio_test_large_rmappable(folio)) {
/* Note: we indicate any THPs here, not just PMD-sized ones */
u |= 1 << KPF_THP;
} else if (is_huge_zero_folio(folio)) {
u |= 1 << KPF_ZERO_PAGE;
u |= 1 << KPF_THP
} else if (is_zero_pfn(page_to_pfn(page))) {
u |= 1 << KPF_ZERO_PAGE;
}
Would likely work and keep the existing behavior (+ include isolated ones).
--
Cheers,
David / dhildenb
