Page MenuHomeFreeBSD

exploit sparsity in swap_pager_copy
ClosedPublic

Authored by dougm on Nov 8 2019, 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 - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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.

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 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.

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

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 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 marked an inline comment as done.

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

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 marked an inline comment as done.

Remove an inaccurate comment.

This revision is now accepted and ready to land.Nov 11 2019, 10:04 AM
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.