Drivers: hv: mshv_vtl: restore 2M VTL0 low mappings to bound PageTables - #159
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a regression in /dev/mshv_vtl_low where sustained VTL0 I/O caused excessive page-table growth (4K PTEs per touched page) by restoring raw-PFN huge mappings (2M/1G) and adding safeguards around GUP behavior and memmap-less ranges.
Changes:
- Restore huge VTL0 fault handling using
vmf_insert_pfn_{pmd,pud}()(raw PFN) to avoid per-page PTE installation and rmap/RSS drift. - Track ranges where
devm_memremap_pages()fails and force those ranges down a 4Kpte_specialfallback to prevent GUP from oopsing on memmap-less huge PMDs. - Make
add_vtl0_mem()idempotent for already-registered ranges and pin pgmap folios to avoid GUP refcount warnings with raw-PFN huge mappings.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8050075 to
4126a31
Compare
4126a31 to
ec6650a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
drivers/hv/mshv_vtl_main.c:3991
- This comment refers to unmap_mapping_range(), but add_vtl0_mem() uses unmap_mapping_pages(). Using the wrong function name here is misleading when reasoning about how the fallback PTEs are invalidated.
* The unmap_mapping_pages() in add_vtl0_mem() invalidates this
drivers/hv/mshv_vtl_main.c:1330
- mshv_vtl_ioctl_add_vtl0_mem() is not serialized: the idempotency check is an RCU read without any writer-side exclusion. Two concurrent MSHV_ADD_VTL0_MEMORY ioctls for the same range can both pass mshv_vtl_low_range_registered(), then both call devm_memremap_pages(), folio_get() the same PFNs (permanent pin), and list_add_rcu() duplicate ranges. This can permanently inflate refcounts and grow mshv_vtl_low_ranges unexpectedly.
Please serialize add_vtl0_mem registrations (e.g., a dedicated mutex around the entire registration path, including the range_registered check, devm_memremap_pages(), folio_get() loop, and list_add_rcu()).
/*
* Idempotent: a range already registered (e.g. re-registered across a
* servicing save/restore) keeps its existing mapping and pin; don't
* re-memremap or report a spurious failure.
*/
if (mshv_vtl_low_range_registered(vtl0_mem.start_pfn, vtl0_mem.last_pfn))
return 0;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
drivers/hv/mshv_vtl_main.c:1300
- mshv_vtl_low_failed_clear() only removes failed-range entries that are fully covered by the newly registered [start_pfn,last_pfn) range. If a prior devm_memremap_pages() failure recorded a larger range that partially overlaps this successful registration, the stale entry remains and mshv_vtl_low_pfn_failed() will keep forcing 4K fallback for PFNs that now do have a memmap.
Consider shrinking/splitting any overlapping failed-range entry to exclude the newly registered subrange (and only delete entries that are fully covered), so successful sub-registrations can resume 2M/1G mappings without losing tracking for the still-memmap-less portions.
list_for_each_entry_safe(r, tmp, &mshv_vtl_low_failed_ranges, list) {
if (r->start_pfn >= start_pfn && r->end_pfn <= last_pfn) {
list_del_rcu(&r->list);
kfree_rcu(r, rcu);
}
drivers/hv/mshv_vtl_main.c:1330
- The new "idempotent" early-return check is not race-safe: mshv_vtl_ioctl_add_vtl0_mem() is called without any higher-level mutex (see mshv_vtl_ioctl()), so two concurrent MSHV_ADD_VTL0_MEMORY calls for the same/overlapping range can both miss mshv_vtl_low_range_registered(). One thread may succeed while the other hits a transient failure (or -EBUSY) and incorrectly records the range as "failed" and/or unmaps existing mappings.
Consider serializing range registration attempts (e.g., a dedicated mutex around the add/failed_add/failed_clear + memremap_pages + list_add sequence), so idempotency and failed-range tracking remain correct under concurrent ioctls.
if (mshv_vtl_low_range_registered(vtl0_mem.start_pfn, vtl0_mem.last_pfn))
return 0;
|
=== [1/1] Drivers: hv: mshv_vtl: restore 2M VTL0 low mappings to bound PageTables ===
: Why it doesn't fire in practice: GUP/[pin_user_pages] into VTL0 only happens for DMA, and OpenHCL calls add_vtl0_mem (registration) before exposing a range for DMA (expose_va). The CPU-relay path that touches unregistered memory uses memcpy, not GUP. So unregistered PFNs are never GUP'd. Registrations cannot be done for everything considering lazy memory registration design, and if in such cases, we fallback to smaller pages, it would also lead to problem of memory bloat.
Same practical mitigation as #1: the range's registration failed, so OpenHCL won't DMA/GUP it. Narrow window, no crash in practice. Could be hardened but low priority.
: Trying out the fix for this
|
|
Fixed most of the relevant AI agent reviews, tried it again and repeated this cycle 4 times. Uploading the change now. |
Under sustained VTL0 I/O the paravisor's /proc/meminfo PageTables grew
from ~660 kB into the tens of MB, steadily eating into VTL2 memory. The
low VTL0 mapping (/dev/mshv_vtl_low) was installing a 4K PTE for every
page it touched instead of a 2M PMD, so each touched 2M region cost a
full extra page table.
The regression came from gating the huge-fault path on a registered
struct page. Underhill registers lower-VTL memory with the kernel
lazily - only for ranges handed to a device for DMA (expose_va) - because
a small VTL2 cannot afford struct pages for all of guest RAM. The CPU
relay path, however, touches far more memory than is ever DMA'd, so most
huge faults hit not-yet-registered pfns, failed the gate, and fell back
to 4K.
Fix it by mapping huge VTL0 faults by raw pfn again, via
vmf_insert_pfn_pmd() with no registration gate (the pre-v6.15 behaviour).
vmf_insert_pfn_pmd() dereferences no struct page, so a 2M map is valid
even for an unregistered pfn and PageTables stays flat.
Going back to the raw-pfn path re-exposes issues that the recent
folio-based rework had addressed; handle each without giving up 2M:
- rmap/RSS drift: vmf_insert_folio_pmd() adds a file rmap and RSS that
zap_huge_pmd() never reverses on this VM_MIXEDMAP, non-DAX VMA
(vma_is_special_huge() true, vma_is_dax() false), leaking a folio
reference and tripping a "Bad rss-counter state" BUG. The pfn
inserter carries no such state, so this drift simply goes away.
- GUP refcount race: a huge pfn PMD holds no folio reference, so a
zap racing pin_user_pages() could drop the refcount to 0 and warn
in try_grab_folio(). Take a permanent reference on each pgmap folio
in add_vtl0_mem() instead; VTL0 memory lives for the partition's
lifetime, so the count never reaches 0.
- GUP over smaller folios: add_vtl0_mem() derives the folio order
from the range's alignment, so a sub-2M-aligned edge yields folios
smaller than a PMD. Slow GUP would then batch a whole 2M span's
references onto one base folio and corrupt its neighbours. Record
such ranges on a normally-empty list - before the range itself is
published - and fall back to 4K for any 2M window that overlaps one,
so the mapping order never exceeds the folio order; only tiny
RAM-edge tails lose 2M.
- GUP into a memmap-less range: GUP on a huge pfn PMD walks the struct
page (follow_huge_pmd -> pmd_page -> try_grab_folio) and would oops
for a range whose devm_memremap_pages() failed. Track such failed
ranges and fall back to 4K (pte_special) for any 2M window that
overlaps one, so GUP fails gracefully with -EFAULT while all normal
memory stays 2M.
Also make add_vtl0_mem() idempotent (an already-registered range returns
success, zapping any stale 4K PTEs so they refault as 2M) so
re-registration across servicing never reports a spurious failure or
leaves memory on 4K. Keep the failed-range bookkeeping consistent under
concurrent registration: re-check registration when recording a failure,
clear stale markers after the range is published so a transient -EBUSY
never strands valid memory on 4K, coalesce failed ranges so repeated
failures cannot grow the list without bound, and report success when a
concurrent add has already registered the range.
Fixes: 775741a ("Drivers: hv: mshv_vtl: use folio-aware inserters for huge VTL0 mappings")
Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
ec6650a to
9444981
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
drivers/hv/mshv_vtl_main.c:1349
- mshv_vtl_low_failed_clear() only removes failed-range entries that are fully covered by [start_pfn,last_pfn). If a failed entry was coalesced to cover a larger span, and a later successful devm_memremap_pages() registers only a subrange, the remaining failed marker will still intersect that subrange and mshv_vtl_low_span_failed() will keep forcing 4K fallback for memory that is now registered (and GUP-safe). Consider trimming/splitting partially-overlapping failed entries on success so only truly memmap-less PFNs remain marked failed.
spin_lock(&mshv_vtl_low_failed_lock);
list_for_each_entry_safe(r, tmp, &mshv_vtl_low_failed_ranges, list) {
if (r->start_pfn >= start_pfn && r->end_pfn <= last_pfn) {
list_del_rcu(&r->list);
kfree_rcu(r, rcu);
}
}
drivers/hv/mshv_vtl_main.c:1316
- mshv_vtl_low_failed_add() skips recording a failed marker only when the requested span is fully covered by a single registered range. If add_vtl0_mem() is called with a span that partially overlaps already-registered memory and devm_memremap_pages() fails, this code can still record the whole span as failed, causing mshv_vtl_low_span_failed() to force 4K fallback even for PFNs that do have a memmap. It may be worth either rejecting overlapping/partially-registered registration requests up front, or subtracting already-registered subranges before recording a failed marker.
This issue also appears on line 1343 of the same file.
spin_lock(&mshv_vtl_low_failed_lock);
/*
* Skip if a concurrent registration already succeeded: it publishes the
* range and then clears failed markers, so recording one now (after that
* clear) would strand valid memory on the 4K path. The success path
* clears under this same lock, so the check and the add are ordered.
*/
if (mshv_vtl_low_range_registered(range->start_pfn, range->end_pfn)) {
spin_unlock(&mshv_vtl_low_failed_lock);
return false;
}
78489eb
into
product/hcl-main/6.18
Under sustained VTL0 I/O the paravisor's /proc/meminfo PageTables grew from ~660 kB into the tens of MB, steadily eating into VTL2 memory. The low VTL0 mapping (/dev/mshv_vtl_low) was installing a 4K PTE for every page it touched instead of a 2M PMD, so each touched 2M region cost a full extra page table.
The regression came from gating the huge-fault path on a registered struct page. Underhill registers lower-VTL memory with the kernel lazily - only for ranges handed to a device for DMA (expose_va) - because a small VTL2 cannot afford struct pages for all of guest RAM. The CPU relay path, however, touches far more memory than is ever DMA'd, so most huge faults hit not-yet-registered pfns, failed the gate, and fell back to 4K.
Fix it by mapping huge VTL0 faults by raw pfn again, via vmf_insert_pfn_{pmd,pud}() with no registration gate (the pre-v6.15 behaviour). vmf_insert_pfn_pmd() dereferences no struct page, so a 2M map is valid even for an unregistered pfn and PageTables stays flat.
Going back to the raw-pfn path re-exposes three issues that the recent folio-based rework had addressed; handle each without giving up 2M:
rmap/RSS drift: vmf_insert_folio_{pmd,pud}() add a file rmap and RSS that zap_huge_{pmd,pud}() never reverse on this VM_MIXEDMAP, non-DAX VMA (vma_is_special_huge() true, vma_is_dax() false), leaking a folio reference and tripping a "Bad rss-counter state" BUG. The pfn inserters carry no such state, so this drift simply goes away.
GUP refcount race: a huge pfn PMD holds no folio reference, so a zap racing pin_user_pages() could drop the refcount to 0 and warn in try_grab_folio(). Rather than a per-mapping reference, take a permanent reference on each pgmap folio in add_vtl0_mem(); VTL0 memory lives for the partition's lifetime, so the count never reaches 0.
GUP into a memmap-less range: GUP on a huge pfn PMD walks the struct page (follow_huge_pmd -> pmd_page -> try_grab_folio) and would oops for a range whose devm_memremap_pages() failed. Track such failed ranges and map them 4K (pte_special) so GUP fails gracefully with -EFAULT while all normal memory stays 2M.
Also make add_vtl0_mem() idempotent (an already-registered range returns success) so re-registration across servicing no longer reports a spurious failure.
Fixes: 775741a ("Drivers: hv: mshv_vtl: use folio-aware inserters for huge VTL0 mappings")