Remove redundant lookup of the old page from vm_page_replace.
Verification that the old page exists is already done by
vm_radix_replace.
Follow up from D4326.
Details
Details
- Reviewers
kib alc - Group Reviewers
Contributor Reviewers (ports) - Commits
- rS292067: vm_page_replace: remove redundant radix lookup
kyua test -k /usr/tests/sys/Kyuafile
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/vm/vm_page.c | ||
|---|---|---|
| 1346 ↗ | (On Diff #10981) | This shouldn't matter, since the object is locked, but still I would prefer to have unbusy happens after the old page is removed from the object queue. |
Comment Actions
vm_page_replace: remove from tailq before null'ing object
Restore the original sequence of tailq removal and object NULLing. It
makes sense to operate on the listq while object is not NULL since
listq is supposed to be protected by the object lock of the page.
We are of course still operating without page locks.
| sys/vm/vm_page.c | ||
|---|---|---|
| 1350 ↗ | (On Diff #10985) | Yeah, realized the same and made the change. |
| sys/vm/vm_page.c | ||
|---|---|---|
| 1345 ↗ | (On Diff #10985) | How about: "Keep the resident page list in sorted order." That would better explain why we do the replacement this way. |
| sys/vm/vm_page.c | ||
|---|---|---|
| 1345–1346 ↗ | (On Diff #10989) | Fixed up comment as suggested. |
Comment Actions
For the current set of callers, the page lock doesn't matter. I'd propose that you develop the patch for vm_fault() next, and I'll think about the page locking in the meantime.
Comment Actions
Okay, will do. The patch to vm_fault_hold itself should be pretty straightforward, but it should be gated by our confidence in the correctness of vm_page_replace.
With respect to the page locking, I think we would at least not be introducing any regressions if we make vm_page_replace do what vm_page_insert and vm_page_remove do, which is unlocked for insert, and locked for remove of a managed page.
Incidentally, in vm_page_remove, the xbusy state of the page is handled but not required. And, for what it's worth, internally Isilon has a couple callers of vm_page_replace that are xbusy'ing the page solely to satisfy the interface (related to zero-copy and copy-on-write implementations).