Sorry, but this code is getting quite confusing, especially with such
misleading "large folio" comments.
Even without MADV_HUGEPAGE we will be allocating large folios, as
emphasized by Willy [1]. So the only thing MADV_HUGEPAGE controls is
*which* large folios we allocate. .. as Willy says [2]: "We were only
intending to breach the 'max' for the MADV_HUGE case, not for all cases."
I have no idea how *anybody* should derive from the code here that we
treat MADV_HUGEPAGE in a special way.
Simply completely confusing.
My interpretation of "I don't know if we should try to defend a stupid
sysadmin against the consequences of their misconfiguration like this"
means" would be "drop this patch and don't change anything".
Without this change, large folios won’t function as expected.
Currently, to support MADV_HUGEPAGE, you’d need to set readahead_kb to
2MB, 4MB, or more. However, many applications run without
> MADV_HUGEPAGE, and a larger readahead_kb might not be optimal for> them.
Someone configured: "Don't readahead more than 128KiB"
And then we complain why we "don't readahead more than 128KiB".
:)
"mm/filemap: Support VM_HUGEPAGE for file mappings" talks about "even if
we have no history of readahead being successful".
So not about exceeding the configured limit, but exceeding the
"readahead history".
So I consider VM_HUGEPAGE the sign here to "ignore readahead history"
and not to "violate the config".
But that's just my opinion.
No changes to API, no confusing code.
New features like large folios can often create confusion with
existing rules or APIs, correct?
We should not try making it even more confusing, if possible.
Maybe pr_info_once() when someone uses MADV_HUGEPAGE with such backends
to tell the sysadmin that something stupid is happening ...
It's not a flawed setup; it's just that this new feature doesn’t work
well with the existing settings, and updating those settings to
accommodate it isn't always feasible.
I don't agree. But it really is Willy's take.
The code, as it stands is confusing and nobody will be able to figure
out how MADV_HUGEPAGE comes into play here and why we suddenly exceed
"max/config" simply because "cur" is larger than max.
For example, in the code
ra->size = start - index; /* old async_size */
ra->size += req_count;
ra->size = get_next_ra_size(ra, max_pages);
What happens if ra->size was at max, then we add "req_count" and
suddenly we exceed "max" and say "well, sure that's fine now". Even if
MADV_HUGEPAGE was never involved? Maybe it cannot happen, but it sure is
confusing.
Not to mention that "It's worth noting that if read_ahead_kb is set to a
larger value that isn't aligned with huge page sizes (e.g., 4MB +
128KB), it may still fail to map to hugepages." sounds very odd :(
--
Cheers,
David / dhildenb
