Page MenuHomeFreeBSD

exploit sparsity in swap_pager_copy
ClosedPublic

Authored by dougm on Fri, Nov 8, 7:42 AM.

Details

Summary

This is based on the observation by Yoshihiro Ota expressed in D22256, but exploits that observation differently.

Modify swp_pager_meta_free to create swp_pager_meta_copy_free that can implement swp_pager_meta_free and also do the "Transfer source to destination" work of swap_pager_copy. The benefit of this change is fewer pctrie accesses and more efficient freeing of trie elements in swap_pager_copy.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dougm created this revision.Fri, Nov 8, 7:42 AM

I don't think this code duplication in swap_pager_copy() is a good idea.
I updated D22256 to avoid the double lookup cost from the first revision.
I'm still testing the 2nd version though.

I've also bothered with the naming of swp_pager_meta_ctl(), this function indeed looks up and then optionally removal, too. On the other hand, I don't think reducing code reuse benefit duplicating code.

dougm updated this revision to Diff 64122.Sat, Nov 9, 7:52 PM

Define swp_pager_meta_copy_free to implement swp_pager_meta_free and part of swap_pager_copy, to avoid code duplication and to exploit some of the optimizations of the meta_free code when copying.

dougm updated this revision to Diff 64139.Sun, Nov 10, 6:53 AM
dougm edited the summary of this revision. (Show Details)

Discard the changes that eliminate swap_meta_ctl. Those changes distract from the primary purpose of this change.

pho added a comment.Sun, Nov 10, 9:30 AM

With Diff 64139 I got a "panic: vm_object_madvise: page 0xfffffe0000174cf8 is not managed", which to me seems unrelated.
https://people.freebsd.org/~pho/stress/log/dougm063.txt

dougm added a reviewer: kib.Sun, Nov 10, 9:23 PM
kib added inline comments.Sun, Nov 10, 9:57 PM
sys/vm/swap_pager.c
948 ↗(On Diff #64139)

How the 'represented by a resident page' part is checked ?

956 ↗(On Diff #64139)

I do not think this blank comment line is useful.

2012 ↗(On Diff #64139)

I think this part of the description is not longer valid.

2020 ↗(On Diff #64139)

Indent of the continuation line is wrong.

2046 ↗(On Diff #64139)

Again indent is wrong.

dougm updated this revision to Diff 64157.Sun, Nov 10, 11:17 PM
dougm marked 5 inline comments as done.

Fix indentation. Discard some comments.

sys/vm/swap_pager.c
2011 ↗(On Diff #64157)

This function now either transfer to another object or free.
The source doesn't keep at all.
I don't think "copy" is appropriate.

How about swp_pager_meta_xfer / swp_pager_meta_transfer with implication of transferring to NULL is a removal?

swp_pager_meta_free() is exclusive use for free.

dougm updated this revision to Diff 64163.Mon, Nov 11, 2:27 AM
dougm marked an inline comment as done.

Rename "copy" to "xfer" or "transfer" in several places.
Move a dstobject==NULL test from callee to caller.

alc added a comment.Mon, Nov 11, 9:29 AM

This looks like a reasonable change.

sys/vm/swap_pager.c
2006–2007 ↗(On Diff #64163)

I don't think that this sentence is true.

dougm updated this revision to Diff 64167.Mon, Nov 11, 9:43 AM
dougm marked an inline comment as done.

Remove an inaccurate comment.

kib accepted this revision.Mon, Nov 11, 10:04 AM
This revision is now accepted and ready to land.Mon, Nov 11, 10:04 AM
alc accepted this revision.Mon, Nov 11, 4:43 PM
This revision was automatically updated to reflect the committed changes.
alc added inline comments.Mon, Nov 11, 5:02 PM
sys/vm/swap_pager.c
950 ↗(On Diff #64167)

I don't think that the comment about the destination not being resident is guaranteed to be true.