kernel/vm: Fix problems in merging caches of differing sizes.

Previously, we'd wind up adding pages from the source to the consumer
that were potentially or actually outside the consumer's bounds.
Now we check the consumer's size and ignore any pages that we don't
want or need; they'll just be freed along with the source cache.

While at it, drop VMAnonymousCache::_MergePagesSmallerSource; it
was the same as the base class's implementation of Merge preceding
this commit; and add a comment to _MergePagesSmallerConsumer noting
that some of the pages may be busy (indeed, I manage to trigger an
assert related to copy-on-write in here at least once.)

I discovered this problem because the page commitment size ASSERT()s
triggered inside Resize() and Rebase(); but the out-of-range pages
already existed in the cache before those functions were called. So,
I've also added an ASSERT to MovePage() that would have caught this
problem more directly.
This commit is contained in:
Augustin Cavalier
2024-12-16 22:52:59 -05:00
parent d4d55c1fbb
commit 74c037e938
3 changed files with 21 additions and 31 deletions
+12 -27
View File
@@ -1045,10 +1045,13 @@ VMAnonymousCache::Merge(VMCache* _source)
_MergeSwapPages(source);
// Move all not shadowed pages from the source to the consumer cache.
if (source->page_count < page_count)
_MergePagesSmallerSource(source);
else
if (source->page_count > page_count
&& source->virtual_base == virtual_base
&& source->virtual_end == virtual_end) {
_MergePagesSmallerConsumer(source);
} else {
VMCache::Merge(source);
}
}
@@ -1215,28 +1218,6 @@ VMAnonymousCache::_Commit(off_t size, int priority)
}
void
VMAnonymousCache::_MergePagesSmallerSource(VMAnonymousCache* source)
{
// The source cache has less pages than the consumer (this cache), so we
// iterate through the source's pages and move the ones that are not
// shadowed up to the consumer.
for (VMCachePagesTree::Iterator it = source->pages.GetIterator();
vm_page* page = it.Next();) {
// Note: Removing the current node while iterating through a
// IteratableSplayTree is safe.
vm_page* consumerPage = LookupPage(
(off_t)page->cache_offset << PAGE_SHIFT);
if (consumerPage == NULL) {
// the page is not yet in the consumer cache - move it upwards
ASSERT_PRINT(!page->busy, "page: %p", page);
MovePage(page);
}
}
}
void
VMAnonymousCache::_MergePagesSmallerConsumer(VMAnonymousCache* source)
{
@@ -1244,8 +1225,12 @@ VMAnonymousCache::_MergePagesSmallerConsumer(VMAnonymousCache* source)
// consumer's pages to the source (freeing shadowed ones) and finally just
// all pages of the source back to the consumer.
for (VMCachePagesTree::Iterator it = pages.GetIterator();
vm_page* page = it.Next();) {
// It is possible that some of the pages we are moving here are actually "busy".
// Since all the pages that belong to this cache will belong to it again by
// the time we unlock, that should be fine.
VMCachePagesTree::Iterator it = pages.GetIterator();
while (vm_page* page = it.Next()) {
// If a source page is in the way, remove and free it.
vm_page* sourcePage = source->LookupPage(
(off_t)page->cache_offset << PAGE_SHIFT);
-2
View File
@@ -90,8 +90,6 @@ private:
swap_addr_t _SwapBlockGetAddress(off_t pageIndex);
status_t _Commit(off_t size, int priority);
void _MergePagesSmallerSource(
VMAnonymousCache* source);
void _MergePagesSmallerConsumer(
VMAnonymousCache* source);
void _MergeSwapPages(VMAnonymousCache* source);
+9 -2
View File
@@ -852,6 +852,7 @@ VMCache::MovePage(vm_page* page, off_t offset)
AssertLocked();
oldCache->AssertLocked();
ASSERT(offset >= virtual_base && (offset + B_PAGE_SIZE) <= virtual_end);
// remove from old cache
oldCache->pages.Remove(page);
@@ -1391,8 +1392,14 @@ VMCache::Fault(struct VMAddressSpace *aspace, off_t offset)
void
VMCache::Merge(VMCache* source)
{
for (VMCachePagesTree::Iterator it = source->pages.GetIterator();
vm_page* page = it.Next();) {
const page_num_t firstOffset = ROUNDDOWN(virtual_base, B_PAGE_SIZE) >> PAGE_SHIFT,
endOffset = (page_num_t)((virtual_end + B_PAGE_SIZE - 1) >> PAGE_SHIFT);
VMCachePagesTree::Iterator it = source->pages.GetIterator();
while (vm_page* page = it.Next()) {
if (page->cache_offset < firstOffset || page->cache_offset >= endOffset)
continue;
// Note: Removing the current node while iterating through a
// IteratableSplayTree is safe.
vm_page* consumerPage = LookupPage(