Changeset View
Standalone View
sys/vm/vm_fault.c
Show First 20 Lines • Show All 251 Lines • ▼ Show 20 Lines | |||||
static void | static void | ||||
vm_fault_fill_hold(vm_page_t *m_hold, vm_page_t m) | vm_fault_fill_hold(vm_page_t *m_hold, vm_page_t m) | ||||
{ | { | ||||
if (m_hold != NULL) { | if (m_hold != NULL) { | ||||
*m_hold = m; | *m_hold = m; | ||||
vm_page_lock(m); | vm_page_lock(m); | ||||
vm_page_hold(m); | vm_page_wire(m); | ||||
vm_page_unlock(m); | vm_page_unlock(m); | ||||
} | } | ||||
} | } | ||||
/* | /* | ||||
* Unlocks fs.first_object and fs.map on success. | * Unlocks fs.first_object and fs.map on success. | ||||
*/ | */ | ||||
static int | static int | ||||
▲ Show 20 Lines • Show All 231 Lines • ▼ Show 20 Lines | #endif | ||||
for (i = 0; i < npages; i++) { | for (i = 0; i < npages; i++) { | ||||
vm_page_change_lock(&m[i], &m_mtx); | vm_page_change_lock(&m[i], &m_mtx); | ||||
if ((fault_flags & VM_FAULT_WIRE) != 0) | if ((fault_flags & VM_FAULT_WIRE) != 0) | ||||
vm_page_wire(&m[i]); | vm_page_wire(&m[i]); | ||||
else | else | ||||
vm_page_activate(&m[i]); | vm_page_activate(&m[i]); | ||||
if (m_hold != NULL && m[i].pindex == fs->first_pindex) { | if (m_hold != NULL && m[i].pindex == fs->first_pindex) { | ||||
*m_hold = &m[i]; | *m_hold = &m[i]; | ||||
vm_page_hold(&m[i]); | vm_page_wire(&m[i]); | ||||
} | } | ||||
vm_page_xunbusy_maybelocked(&m[i]); | vm_page_xunbusy_maybelocked(&m[i]); | ||||
} | } | ||||
if (m_mtx != NULL) | if (m_mtx != NULL) | ||||
mtx_unlock(m_mtx); | mtx_unlock(m_mtx); | ||||
} | } | ||||
curthread->td_ru.ru_majflt++; | curthread->td_ru.ru_majflt++; | ||||
return (KERN_SUCCESS); | return (KERN_SUCCESS); | ||||
▲ Show 20 Lines • Show All 621 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) { | ||||
vm_page_lock(fs.m); | vm_page_lock(fs.m); | ||||
vm_page_dequeue(fs.m); | vm_page_dequeue(fs.m); | ||||
alc: We don't typically put a blank line before non-trivial comments that open a block. | |||||
vm_page_remove(fs.m); | vm_page_remove(fs.m); | ||||
vm_page_unlock(fs.m); | vm_page_unlock(fs.m); | ||||
vm_page_lock(fs.first_m); | vm_page_lock(fs.first_m); | ||||
vm_page_replace_checked(fs.m, fs.first_object, | vm_page_replace_checked(fs.m, fs.first_object, | ||||
fs.first_pindex, fs.first_m); | fs.first_pindex, 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); | ||||
vm_page_dirty(fs.m); | vm_page_dirty(fs.m); | ||||
▲ Show 20 Lines • Show All 167 Lines • ▼ Show 20 Lines | #endif | ||||
* can find it. | * can find it. | ||||
*/ | */ | ||||
if ((fault_flags & VM_FAULT_WIRE) != 0) | if ((fault_flags & VM_FAULT_WIRE) != 0) | ||||
vm_page_wire(fs.m); | vm_page_wire(fs.m); | ||||
else | else | ||||
vm_page_activate(fs.m); | vm_page_activate(fs.m); | ||||
if (m_hold != NULL) { | if (m_hold != NULL) { | ||||
*m_hold = fs.m; | *m_hold = fs.m; | ||||
vm_page_hold(fs.m); | vm_page_wire(fs.m); | ||||
} | } | ||||
vm_page_unlock(fs.m); | vm_page_unlock(fs.m); | ||||
vm_page_xunbusy(fs.m); | vm_page_xunbusy(fs.m); | ||||
/* | /* | ||||
* Unlock everything, and return | * Unlock everything, and return | ||||
*/ | */ | ||||
unlock_and_deallocate(&fs); | unlock_and_deallocate(&fs); | ||||
▲ Show 20 Lines • Show All 256 Lines • ▼ Show 20 Lines | for (mp = ma, va = addr; va < end; mp++, va += PAGE_SIZE) | ||||
VM_FAULT_NORMAL, mp) != KERN_SUCCESS) | VM_FAULT_NORMAL, mp) != KERN_SUCCESS) | ||||
goto error; | goto error; | ||||
} | } | ||||
return (count); | return (count); | ||||
error: | error: | ||||
for (mp = ma; mp < ma + count; mp++) | for (mp = ma; mp < ma + count; mp++) | ||||
if (*mp != NULL) { | if (*mp != NULL) { | ||||
vm_page_lock(*mp); | vm_page_lock(*mp); | ||||
vm_page_unhold(*mp); | if (vm_page_unwire(*mp, PQ_INACTIVE) && | ||||
(*mp)->object == NULL) | |||||
vm_page_free(*mp); | |||||
Not Done Inline ActionsIsn't it a good moment to introduce a VM KPI to wrap this op into something like vm_page_disown() ? (I do not insist on the name). Linuxkpi put_page() is yet another place. kib: Isn't it a good moment to introduce a VM KPI to wrap this op into something like vm_page_disown… | |||||
Done Inline ActionsIn the review description I suggested doing this in a future diff, but I can try it here. Why not handle it in vm_page_unwire_noq(), instead of introducing a new KPI? Note that vm_page_unwire() already checks whether m->object == NULL. markj: In the review description I suggested doing this in a future diff, but I can try it here. Why… | |||||
Not Done Inline ActionsBut how would you detect that it is fine to free the page ? Checking VPO_UNMANAGED == 0 and object == NULL ? I am not sure that this would not break any caller. kib: But how would you detect that it is fine to free the page ? Checking VPO_UNMANAGED == 0 and… | |||||
Done Inline ActionsYes, in that case I think it should be safe to free the page. Typically, upon the last unwire it is also the caller's responsibility to enqueue the unwired page. But this must not be done if m->object == NULL, as the page daemon does not handle it. Some auditing is required, but I think it is the right direction. I would like struct vm_page to have a general-purpose reference count with KPIs that implicitly handle freeing the page. Moreover, such a change would help avoid bugs like that fixed by r329216. markj: Yes, in that case I think it should be safe to free the page. Typically, upon the last unwire… | |||||
Done Inline ActionsI was being dismissive here, and the question requires more thought. I will explain why I chose to add the object == NULL checks. Suppose we unhold a page with hold_count == 1. If PG_UNHOLDFREE is set, we must have m->object == NULL. The inverse statement is almost always true as well. Looking through the vm_page_remove() callers:
Except for the CoW case mentioned above, I believe it is also true that if m->object == NULL in vm_page_unhold(), then PG_UNHOLDFREE must be set, so we will free the page. I will think about how to solve that one case. markj: I was being dismissive here, and the question requires more thought. I will explain why I chose… | |||||
Not Done Inline ActionsSo why not keep PG_UNHOLDFREE around, perhaps renamed ? This would fix the CoW case and removes very obscure detail of the wiring. kib: So why not keep PG_UNHOLDFREE around, perhaps renamed ? This would fix the CoW case and… | |||||
Done Inline ActionsCurrently vm_page_free_prep() panics if the page is wired. Are you suggesting we change that semantic? FWIW, I think PG_UNHOLDFREE is kind of hacky and complicates locking. Following this diff I wish to begin manipulating wire_count using atomics when the page is managed, as part of an effort to reduce the scope of the page lock. markj: Currently vm_page_free_prep() panics if the page is wired. Are you suggesting we change that… | |||||
Not Done Inline ActionsIf you merge hold and wire, then the semantic should be mostly a superposition of both. We allow freeing held pages, which means that it is reasonable to allow free wired pages after the merge. kib: If you merge hold and wire, then the semantic should be mostly a superposition of both. We… | |||||
Done Inline ActionsWell, my desired semantic is to permit vm_page_unwire() to free the page if there are no more references, similar to PG_UNHOLDFREE. However, my plan is to count the object reference in wire_count and have vm_page_unwire() free the page when the last reference is released, since this simplifies synchronization. I will post some more patches following this direction, to make discussion easier. markj: Well, my desired semantic is to permit vm_page_unwire() to free the page if there are no more… | |||||
Done Inline ActionsSee D20486. It is a large diff and still needs a bit of work, but it is stable for me. I will try to continue splitting off pieces into separate reviews; D20485 can be committed separately, for instance. Basically, with that change, vm_page_wire() will free the page upon the last reference. In particular, it has essentially the same semantics as PG_UNHOLDFREE. markj: See D20486. It is a large diff and still needs a bit of work, but it is stable for me. I will… | |||||
vm_page_unlock(*mp); | vm_page_unlock(*mp); | ||||
} | } | ||||
return (-1); | return (-1); | ||||
} | } | ||||
/* | /* | ||||
* Routine: | * Routine: | ||||
* vm_fault_copy_entry | * vm_fault_copy_entry | ||||
▲ Show 20 Lines • Show All 225 Lines • Show Last 20 Lines |
We don't typically put a blank line before non-trivial comments that open a block.