Paths

Table of Contentst

Pull vm_object_scan_all_shadowed out of vm_object_backing_scan
ClosedPublic
Actions

Authored by cem on Dec 1 2015, 6:05 PM.

Details

Reviewers

kib
alc

Commits

rS291704: Pull vm_object_scan_all_shadowed out of vm_object_backing_scan

Summary

These two functions were largely unrelated, they just used the same same
loop logic to walk through a backing object's memq. Pull out the
all_shadowed test as its own function and eliminate
OBSC_TEST_ALL_SHADOWED. Rename vm_object_backing_scan to
vm_object_collapse_scan.

No functional change.

Sponsored by: EMC / Isilon Storage Division

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cem updated this revision to Diff 10644.Dec 1 2015, 6:05 PM

cem retitled this revision from to Pull vm_object_scan_all_shadowed out of vm_object_backing_scan.

cem updated this object.

cem edited the test plan for this revision. (Show Details)

cem added reviewers: kib, alc.

Herald added a subscriber: imp. · View Herald TranscriptDec 1 2015, 6:05 PM

Overall, I like the change.

sys/vm/vm_object.c
1475 ↗	(On Diff #10644)	I do not quite understand the reason for the second paragraph in the comment. I suspect it can be removed, we do not do collapse when backing object is not splittable.
1482 ↗	(On Diff #10644)	This comment is rather useless in the short function which consists only of the loop.
1483 ↗	(On Diff #10644)	for (p = TAILQ_FIRST(); p != NULL; p = TAILQ_NEXT(p, listq)) {
1485 ↗	(On Diff #10644)	Remove.
1492 ↗	(On Diff #10644)	I do not think that the 'busy' comment is relevant for the shadow scan. It has some significance for the collapse, where we unlock objects.
1496 ↗	(On Diff #10644)	Remove.
1513 ↗	(On Diff #10644)	Remove.

cem added inline comments.Dec 2 2015, 5:24 PM

sys/vm/vm_object.c
1475 ↗	(On Diff #10644)	Sure; I did not change the comments at all yet.
1482 ↗	(On Diff #10644)	It was useless before, too :). Again, I haven't (yet) changed the comments at all.
1483 ↗	(On Diff #10644)	Sure.
1492 ↗	(On Diff #10644)	It came from the `if (op & OBSC_TEST_ALL_SHADOWED) {` block, and I did not change the comments.

Make changes suggested by kib@.

cem marked 7 inline comments as done.Dec 2 2015, 6:19 PM

kib added inline comments.Dec 3 2015, 11:29 AM

sys/vm/vm_object.c
1464 ↗	(On Diff #10672)	The index calculation can be postponed after the check for the backing_object type, otherwise it is useless.
1471 ↗	(On Diff #10672)	This comment would describe the real bug, where we silently destroy the user data by ignoring allocated swap in the backing object. But it is not since besides the result of the function, we also test for the resident_page_count value in vm_object_collapse(). Might be, this comment should say something about the usage.
1476 ↗	(On Diff #10672)	I think we could use vm_page_find_least(backing_object, backing_offset_index) as the start page there and in the collapse() ? This could be left to the next change.
1526 ↗	(On Diff #10672)	Might be, simultaneously rewrite this loop in the for() form. I think it is more concise and easier to grasp from the short look.

Address 2nd round CR feedback.

sys/vm/vm_object.c
1471 ↗	(On Diff #10672)	I don't know this area well enough to rewriting comments. I'm happy with the small change to split the routine in two logical pieces.
1476 ↗	(On Diff #10672)	Next change.

kib accepted this revision.Dec 3 2015, 5:17 PM

kib edited edge metadata.

This revision is now accepted and ready to land.Dec 3 2015, 5:17 PM

Closed by commit rS291704: Pull vm_object_scan_all_shadowed out of vm_object_backing_scan (authored by cem). · Explain WhyDec 3 2015, 5:21 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

head/

sys/

vm/

vm_object.c

301 lines

Diff 10708

View Options

Pull vm_object_scan_all_shadowed out of vm_object_backing_scanClosedPublicActions