Page MenuHomeFreeBSD
Differential D13290

allow frees to aggregate in swap_pager_copy
ClosedPublic
Actions

Authored by dougm on Nov 29 2017, 8:40 AM.
Tags
None
Referenced Files
F101900453: D13290.id37268.diff
Tue, Nov 5, 9:38 AM
F101900181: D13290.id37285.diff
Tue, Nov 5, 9:34 AM
Unknown Object (File)
Sun, Nov 3, 2:54 PM
Unknown Object (File)
Sun, Oct 27, 11:24 PM
Unknown Object (File)
Sep 30 2024, 3:38 AM
Unknown Object (File)
Sep 21 2024, 9:03 AM
Unknown Object (File)
Sep 18 2024, 7:16 PM
Unknown Object (File)
Sep 17 2024, 8:22 PM
View All 80 Files
Subscribers
imp

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

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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 ↗(On Diff #35944)

Perhaps change the value to 0x1 ?

943 ↗(On Diff #35944)

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
dougm updated this revision to Diff 37236.Dec 30 2017, 4:45 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 ↗(On Diff #37236)

Please remove the "and vm_page_t". It was a stale reference even before your proposed change.

2006 ↗(On Diff #37236)

"... of either lookup up or removing swapblk ..."

2007 ↗(On Diff #37236)

"... meta data. It returns ..."

2008 ↗(On Diff #37236)

"... looked up, popped, or ..."

2010 ↗(On Diff #37236)

This last sentence makes no sense to me. Just delete it.

2016 ↗(On Diff #37236)

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.

dougm updated this revision to Diff 37249.Dec 30 2017, 8:22 PM

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 ↗(On Diff #37236)

The "else" should be on the same line as the "}".

dougm updated this revision to Diff 37268.Dec 31 2017, 12:31 AM

Fix 'else' spacing.

dougm marked an inline comment as done.Dec 31 2017, 12:31 AM
alc added a comment.Dec 31 2017, 12:51 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 ↗(On Diff #37268)

Let's capitalize the "t" in "transfer" while we're in the neighborhood.

dougm updated this revision to Diff 37273.Dec 31 2017, 1:02 AM

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 ↗(On Diff #37273)

I don't think that this comment adds anything useful. Just delete it.

dougm updated this revision to Diff 37285.Dec 31 2017, 3:36 AM

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

Revision Contents
Changeset List

PathSize
head/
sys/
vm/
86 lines

Diff 37286

View Options

head/sys/vm/swap_pager.c

Loading...