Changeset View
Standalone View
sys/vm/vm_fault.c
Show First 20 Lines • Show All 830 Lines • ▼ Show 20 Lines | if ((fault_type & (VM_PROT_COPY | VM_PROT_WRITE)) != 0) { | ||||
((fs.object->type == OBJT_DEFAULT) || | ((fs.object->type == OBJT_DEFAULT) || | ||||
(fs.object->type == OBJT_SWAP)) && | (fs.object->type == OBJT_SWAP)) && | ||||
(is_first_object_locked = VM_OBJECT_TRYWLOCK(fs.first_object)) && | (is_first_object_locked = VM_OBJECT_TRYWLOCK(fs.first_object)) && | ||||
/* | /* | ||||
* We don't chase down the shadow chain | * We don't chase down the shadow chain | ||||
*/ | */ | ||||
fs.object == fs.first_object->backing_object) { | fs.object == fs.first_object->backing_object) { | ||||
/* | /* | ||||
* get rid of the unnecessary page | * grab the page and put it into the | ||||
* process'es object. | |||||
kib: _G_rab the page ... | |||||
*/ | */ | ||||
Not Done Inline ActionsI think that the comment above the "if" expression already explains what we're doing here better than this comment. The only thing that doesn't get mentioned in that comment is the need to rebusy the page. alc: I think that the comment above the "if" expression already explains what we're doing here… | |||||
vm_page_lock(fs.first_m); | vm_page_lock(fs.m); | ||||
vm_page_remove(fs.first_m); | vm_page_remove(fs.m); | ||||
vm_page_unlock(fs.first_m); | vm_page_unlock(fs.m); | ||||
if (vm_page_replace(fs.m, fs.first_object, | |||||
fs.first_pindex) != fs.first_m) | |||||
panic("%s: invalid page replacement", | |||||
kibUnsubmitted Not Done Inline ActionsI would prefer to see this done with KASSERT. E.g. you could unconditionally store mold from vm_page_replace() into a local var, and operate on the local below where fs.first_m is used in the current code (to pacify compilers which warn about assigned but not used variables, in !INVARIANTS case). KASSERT would be mret == fs.first_m. kib: I would prefer to see this done with KASSERT. E.g. you could unconditionally store mold from… | |||||
rlibbyAuthorUnsubmitted Not Done Inline ActionsOkay, I can do something like that. Here's another possibility. Currently existing callers of vm_page_replace are doing the same thing, they all know the actual page they are trying to replace and assert (panic) on it post-hoc. Again for what it's worth, this includes Isilon's internal callers. Therefore it seems to me that we could also either change the interface or provide a wrapper that essentially does void vm_page_replace_XXX(vm_page_t mold, vm_page_t mnew) { vm_page_t mtmp; mtmp = vm_page_replace(mnew, mold->object, mold->pindex); /* XXX KASSERT/panic/whatever */ } Another possibility would be to have the interface take all four args (object, pindex, mold, mnew) so that it could provide the strongest assertions. Thoughts? rlibby: Okay, I can do something like that.
Here's another possibility. Currently existing callers of… | |||||
kibUnsubmitted Not Done Inline ActionsInline vm_page_replace_XXX seems to be best for me. It would allow compiler to place only a call to vm_page_replace() in case of !INVARIANTS, and simplifies expression at the callee side. kib: Inline vm_page_replace_XXX seems to be best for me. It would allow compiler to place only a… | |||||
rlibbyAuthorUnsubmitted Not Done Inline ActionsOkay. Personally I think I would most like just to change the interface outright, but I will defer to your and alc's judgement here. For the wrapper, how does this look (somewhere in vm_page.h): static inline void vm_page_replace_check(vm_page_t mnew, vm_object_t object, vm_pindex_t pindex, vm_page_t mold) { vm_page_t mret; mret = vm_page_replace(mnew, object, pindex); KASSERT(mret == mold, ("invalid page replacement, mold=%p, mret=%p", mold, mret)); /* Unused if !INVARIANTS. */ (void)mold; (void)mret; } I was thinking to prefer the four-argument version to the two-argument one because I think the assertion mostly protects against mold not having had the right identity, rather than the radix code not working. If the cast-to-void pattern is not acceptable then I'd suggest INVARIANTS-dependent alternate macro definitions instead. rlibby: Okay. Personally I think I would most like just to change the interface outright, but I will… | |||||
kibUnsubmitted Not Done Inline ActionsIf changing the vm_page_replace() signature, mold is unused for !INVARIANTS case, but it must be passed. What you proposed above looks fine to me, thanks. kib: If changing the vm_page_replace() signature, mold is unused for !INVARIANTS case, but it must… | |||||
__func__); | |||||
vm_page_dirty(fs.m); | |||||
/* | /* | ||||
* grab the page and put it into the | * get rid of the unnecessary page | ||||
Done Inline ActionsComment is absolutely trivial, if you want to left it in code, please use the proper sentence. kib: Comment is absolutely trivial, if you want to left it in code, please use the proper sentence. | |||||
Not Done Inline ActionsNope, I agree. rlibby: Nope, I agree. | |||||
* process'es object. The page is | |||||
* automatically made dirty. | |||||
*/ | */ | ||||
if (vm_page_rename(fs.m, fs.first_object, | |||||
fs.first_pindex)) { | |||||
VM_OBJECT_WUNLOCK(fs.first_object); | |||||
unlock_and_deallocate(&fs); | |||||
goto RetryFault; | |||||
} | |||||
vm_page_lock(fs.first_m); | vm_page_lock(fs.first_m); | ||||
vm_page_free(fs.first_m); | vm_page_free(fs.first_m); | ||||
vm_page_unlock(fs.first_m); | vm_page_unlock(fs.first_m); | ||||
#if VM_NRESERVLEVEL > 0 | #if VM_NRESERVLEVEL > 0 | ||||
/* | /* | ||||
* Rename the reservation. | * Rename the reservation. | ||||
*/ | */ | ||||
vm_reserv_rename(fs.m, fs.first_object, | vm_reserv_rename(fs.m, fs.first_object, | ||||
▲ Show 20 Lines • Show All 751 Lines • Show Last 20 Lines |
_G_rab the page ...