Differential D4478
vm_fault_hold: use vm_page_replace instead of vm_page_rename Authored by rlibby on Dec 9 2015, 8:38 PM. Tags None Referenced Files
Details
Using vm_page_replace instead of vm_page_rename leads to slightly Follow up from D4326 and D4471. May depend on fortification of vm_page_replace. kyua test -k /usr/tests/sys/Kyuafile
Diff Detail
Event Timeline
Comment Actions vm_fault_hold: kib feedback: use KASSERTing version of vm_page_replace Updated to use D4497.
Comment Actions If we count vm_fault as a caller, then all three callers to vm_page_replace{,_checked} acquire the page lock on "mold" shortly (or even immediately) after vm_page_replace{,_checked} returns. I think that we should extend the coverage of the page lock on "mold" to cover the call to vm_page_replace{,_checked}. So, this use case would look like: vm_page_lock(fs.first_m);
vm_page_replace_checked(fs.m, fs.first_object,
fs.first_pindex, fs.first_m);
vm_page_free(fs.first_m);
vm_page_unlock(fs.first_m);
vm_page_dirty(fs.m);At the same time, I would add the equivalent assertion to the beginning of vm_page_replace() as you find at the beginning of vm_page_remove() on "mold": if ((mold->oflags & VPO_UNMANAGED) == 0)
vm_page_assert_locked(mold);This would address the safety question about clearing "mold->object" in vm_page_replace(). A similar observation could be made about vm_page_free, i.e., that we always call it after replace, and so it could be the last step in vm_page_replace. However, based upon the use of vm_page_replace in D4444, I would argue against calling vm_page_free from within vm_page_replace. Personally, I would have eliminated the "object" and "pindex" parameters to vm_page_replace_checked. I generally look at such redundancy as just offering callers another way to screw up the call. However, I don't feel strongly enough to argue this point. Comment Actions I agree with your suggestion of having the same page locking semantics as vm_page_remove. I think that vm_page_replace shoul basically behave like an optimized remove and insert.
I agree, and for what it's worth Isilon's internal callers also do not want the old page freed.
Yeah, I like that better as an interface too but it doesn't provide as strong of a check about the identity of the old page. Maybe another option would be to have two functions #define vm_page_assert_name(m, object, pindex) \
KASSERT(m->object == object, m->pindex == pindex, \
("%s: page %p not in object %p at %ju",
__func__, m, object, (uintmax_t)pindex))
void vm_page_replace(vm_page_t mold, vm_page_t mnew);and then callers would go back to both calling replace and asserting. vm_page_assert_name(fs.first_m, fs.first_object, fs.first_pindex); vm_page_replace(fs.first_m, fs.m); Anyway that leaves two things:
I can write this up in code if you would like. In terms of your D4444, I think this would allow you not to xbusy the page before calling vm_page_replace (just as the corresponding else branch there does not xbusy the page for the call to vm_page_remove). Comment Actions For illustration (this is assuming previous patches which are reviewed but not committed): If you agree with the direction here I can submit it for review, maybe after breaking up into smaller patches. Comment Actions Can we postpone this until the already marked reviews are committed ? What is needed to commit them ? Comment Actions Yeah, apologies for the jumble of patches. If there's a better way to submit a patch series I would appreciate a pointer. D4577 and D4497 just need a committer. I will coordinate with @cem today and see if he can help. For this one (D4478), I am just a little nervous about possible regressions, my reservations being the differences between vm_page_replace and current behavior (locking of mold, PQ_NONE assert). If we are comfortable that those are more cleanliness issues than actual problems then this patch too can proceed as is. Comment Actions Revise the locking in this patch as I've described above and get Conrad to commit it. The PQ_NONE assertion is not a problem in this case, i.e., vm_fault. We can deal with the internals of vm_page_replace later. Comment Actions Revised locking, as advised, just for vm_fault. It wasn't clear to me if you also wanted to see the other callers altered in this patch so I left it out for now (i.e. status quo for them). One last thing to point out. Since we now get the (mold) page lock but do an unconditional xunbusy in vm_page_replace, the possibility now seems to exist that we will recurse on the page lock if the busy waiters bit is somehow set. Again I am not sure if this is actually possible from vm_fault, and I did not encounter it in testing. (Eventually I think we should deal with this as vm_page_remove does.) Comment Actions first_m should not be yet added to the object page queue, and this prevents other threads from finding the first_m at all. As consequence, the page should not have waiters. But the point is valid and I do not think that vm_page_xunbusy() was designed to be used for the locked page. You may consider adding vm_page_unbusy_locked(). Comment Actions Okay, good. You can see my intended solution under vm_page_remove_xunbusy at that github link above. Do you want to proceed with this patch as-is and come back to the vm_page_replace internals? Or if you want to address the internals first then I can submit that patch on github as a diff here (but drop the vm_fault.c bits). I'm okay with however you guys want to approach this. Comment Actions I do not like the propagation of the locked checks around. Unlocked page may be only passed to vm_page_remove() from vm_page_free_toq(), it seems. Could we e.g. ensure that the page is always locked in free_toq() ? If not, I still suggest to have a function like vm_page_xunbusy_locked(), which does the same action as xunbusy(), but assumes that the page is already locked. Comment Actions Unfortuantely I don't think so. It looks like vm_page_free_toq is called with unlocked pages all over the pmap code via vm_page_free{,_zero}. I wonder if it is possible just to assert in vm_page_free_toq that unmanaged pages are not in a vm object anyway, and then not call vm_page_remove for them. I don't know enough to say.
If we can't enforce vm_page_remove is called with locked pages, then I think this would become if page is locked:
vm_page_xunbusy_locked()
else:
vm_page_xunbusy()and I'm not sure I see this as better. It would still have the lock check and it would lose the fast path where xunbusy works regardless of page lock status (the bit before vm_page_xunbusy_hard). Comment Actions By 'ensuring', I mean to check the page lock in vm_page_free_toq(), and lock if not. This is a single place to add the check, and free_toq() is already slow and lock-full, so I do not think that the additional lock, otherwise taken in the callees, makes much difference WRT speed.
Unmanaged pages do exist on the object queues, e.g. for OBJT_MGTMDEVICE or OBJT_PHYS pages.
Comment Actions I'd like to commit this patch. Are there any further comments on it?
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||