Opened 7 years ago

Last modified 6 years ago

#8466 assigned bug

VM caches aren't resized in some cases when cutting areas

Reported by: hamish Owned by: hamish
Priority: normal Milestone: R1
Component: System/Kernel Version: R1/Development
Keywords: kernel vm cache Cc:
Blocked By: Blocking: #9326
Has a Patch: yes Platform: All

Description

In some cases, if an area is partially mapped over (using mmap with MAP_FIXED, for example) the cache is not resized or split in two, resulting in memory being leaked for the overlapping region.

See the TODOs in cut_area(): http://cgit.haiku-os.org/haiku/tree/src/system/kernel/vm/vm.cpp#n609

Attachments (1)

0001-Resize-caches-in-all-cases-when-cutting-areas.patch (15.4 KB) - added by hamish 7 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 7 years ago by anevilyak

Owner: changed from axeld to bonefish
Status: newassigned

comment:2 Changed 7 years ago by hamish

Here's what I'm thinking of doing:

To handle the cut-start-of-area case, add a VMCache::Rebase() method as a counterpart to VMCache::Resize() which will move the virtual base of cache and free any pages no longer contained in the region.

To handle the cut-middle-of-area case, add a VMCache::MovePageRange(off_t offset, off_t size, VMCache* destination, off_t destinationOffset) method. Then create a new cache for the second area and move the pages there. I realise this is inconsistent with the other page moving methods because they are called on the destination cache, but it would be nice to iterate the page tree directly instead of calling LookupPage() repeatedly on the source cache.

Does this sound OK?

comment:3 in reply to:  2 ; Changed 7 years ago by bonefish

Replying to hamish:

Here's what I'm thinking of doing:

To handle the cut-start-of-area case, add a VMCache::Rebase() method as a counterpart to VMCache::Resize() which will move the virtual base of cache and free any pages no longer contained in the region.

Sounds good.

To handle the cut-middle-of-area case, add a VMCache::MovePageRange(off_t offset, off_t size, VMCache* destination, off_t destinationOffset) method. Then create a new cache for the second area and move the pages there. I realise this is inconsistent with the other page moving methods because they are called on the destination cache, but it would be nice to iterate the page tree directly instead of calling LookupPage() repeatedly on the source cache.

I don't see what would speak against iterating through the other cache's page tree.

As an alternative solution a status_t SplitCache(off_t offset, VMCache*& _newCache) could be introduced (or with a pre-allocated cache object, if that makes things easier to use). Would be less flexible, but ATM I can't think of a use case where additional flexibility was required.

comment:4 Changed 7 years ago by hamish

Has a Patch: set

comment:5 in reply to:  3 Changed 7 years ago by hamish

Replying to bonefish:

I don't see what would speak against iterating through the other cache's page tree.

Indeed, I didn't realise that the page tree was a public member.

This patch depends on my patch in #8233.

RE your comment there about guard sizes and cache resizing, I'm not sure quite what would be the expected behaviour in this case. Should the guard size be preserved after resizing, or should the guard be resized to prevent previously accessible pages becoming guarded? So I added a VMAnonymous(NoSwap)Cache::SetGuardSize() to let the caller decide what's best for them.

comment:6 Changed 7 years ago by hamish

Just realised this doesn't deal with swap properly in the middle-cut case.

comment:7 Changed 6 years ago by xyzzy

Blocking: 9326 added

comment:8 Changed 6 years ago by mmadia

Owner: changed from bonefish to hamish
Note: See TracTickets for help on using tickets.