Details

Reviewers

alc
kib
markj

Commits

rS327410: Previously, swap_pager_copy() freed swap blocks one at at time, via

Summary

swap_pager_copy frees blocks one at at time, via swp_pager_meta_ctl, with no opportunity to recognize freeing of consecutive blocks and free fewer block ranges. To open that opportunity, this change removes the SWM_FREE option from swp_pager_meta_ctl, and compels the caller to do the freeing when a valid block address is returned. In swap_pager_copy, these frees are aggregated, so that a sequence of them can be done at one time.

The only other caller to swp_pager_meta_ctl that passes SWM_FREE, swp_pager_unswapped, is also modified to handle its single free explicitly.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dougm created this revision.Nov 29 2017, 8:40 AM

dougm added a reviewer: alc.

dougm added reviewers: kib, markj.Dec 30 2017, 6:52 AM

kib accepted this revision.Dec 30 2017, 2:38 PM

kib added inline comments.

sys/vm/swap_pager.c
314	Perhaps change the value to 0x1 ?
943	Move the variables to the function' locals block, per style.

This revision is now accepted and ready to land.Dec 30 2017, 2:38 PM

Adopt reviewer suggestions.

This revision now requires review to proceed.Dec 30 2017, 4:45 PM

dougm marked 2 inline comments as done.Dec 30 2017, 4:51 PM

alc added inline comments.Dec 30 2017, 7:30 PM

sys/vm/swap_pager.c
2004	Please remove the "and vm_page_t". It was a stale reference even before your proposed change.
2006	"... of either lookup up or removing swapblk ..."
2007	"... meta data. It returns ..."
2008	"... looked up, popped, or ..."
2010	This last sentence makes no sense to me. Just delete it.
2016	I don't see the point of the ".. pop it out". However, I do see the point of saying that this option does not free the swap block.

Adopt reviewer comments about comments.

dougm marked 6 inline comments as done.Dec 30 2017, 8:23 PM

alc added inline comments.Dec 31 2017, 12:25 AM

sys/vm/swap_pager.c
968–969	The "else" should be on the same line as the "}".

Fix 'else' spacing.

dougm marked an inline comment as done.Dec 31 2017, 12:31 AM

Is there some reason why we shouldn't do

srcaddr = swp_pager_meta_ctl(srcobject, i + offset, SWM_POP);
if (srcaddr == SWAPBLK_NONE)
        continue;

before the

dstaddr = swp_pager_meta_ctl(dstobject, i, 0);

That would seem to eliminate unnecessary lookups on the dstobject, which would be another improvement over the existing code.

alc added inline comments.Dec 31 2017, 12:52 AM

sys/vm/swap_pager.c
938	Let's capitalize the "t" in "transfer" while we're in the neighborhood.

Capitalize in a comment.

As swap_pager_meta_ctl(,,0) looks side-effect free, I agree that it can be called to compute srcaddr before deciding whether to compute dstaddr (with side effects). The comment that precedes all this seems like gibberish, and is mostly deleted.

dougm marked an inline comment as done.Dec 31 2017, 1:03 AM

alc added inline comments.Dec 31 2017, 3:31 AM

sys/vm/swap_pager.c
947	I don't think that this comment adds anything useful. Just delete it.

Drop unwanted comment.

dougm marked an inline comment as done.Dec 31 2017, 3:36 AM

alc accepted this revision.Dec 31 2017, 3:45 AM

This revision is now accepted and ready to land.Dec 31 2017, 3:45 AM

Closed by commit rS327410: Previously, swap_pager_copy() freed swap blocks one at at time, via (authored by alc). · Explain WhyDec 31 2017, 4:01 AM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptDec 31 2017, 4:01 AM

allow frees to aggregate in swap_pager_copy
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37249

sys/vm/swap_pager.c

allow frees to aggregate in swap_pager_copyClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37249

sys/vm/swap_pager.c

allow frees to aggregate in swap_pager_copy
ClosedPublic
Actions

Revision Contents
Changeset List