Page MenuHomeFreeBSD

(vm object 2) Don't release xbusy in vm_page_remove(), defer to vm_page_free_prep().
ClosedPublic

Authored by jeff on Sep 6 2019, 12:46 PM.

Details

Summary

This persists the busy state across compound operations like rename and replace. Eliminating spurious wakeups and visibility of incomplete state changes.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jeff retitled this revision from Don't release xbusy in vm_page_remove(), defer to vm_page_free_prep(). Some callers like vm_page_remove/replace want busy state to persist. to Don't release xbusy in vm_page_remove(), defer to vm_page_free_prep()..Sep 6 2019, 2:05 PM
jeff edited the summary of this revision. (Show Details)
jeff added reviewers: kib, markj, alc, dougm.
sys/vm/vm_page.c
3474 ↗(On Diff #61726)

Can't we move this under the m->object != NULL check?

sys/vm/vm_page.c
1213 ↗(On Diff #61726)

This is the strange action on the fake page structure that is going away.

Why do you need this chunk ?

sys/vm/vm_page.c
1213 ↗(On Diff #61726)

I could just set the busy state to zero. However, if you have some code which is waiting for an identity change to wake it up and it has sloppy atomicity it may never wakeup. I _think_ there would have to be another bug for that to happen but this seemed safer.

3474 ↗(On Diff #61726)

You mean to say that it couldn't possibly be busy with a NULL object? I believe that is true but I'm not sure how confident I am in that. Since this is tested as is I would prefer to do a follow up patch with extra asserts and re-arranging if you think it is valuable.

sys/vm/vm_page.c
1213 ↗(On Diff #61726)

I do not think it is needed. Note that the memory of vm_page is released a line below. Anybody sleeping on busy for the fake page when it goes to dismount is already broken.

sys/vm/vm_page.c
1213 ↗(On Diff #61726)

After the wakeup callers do not touch the page they were sleeping on. So it is valid to wait for the change in identity on something that is going away. I believe it is safer this way and carries forward existing behavior which would've issued the wakeup from remove().

This revision is now accepted and ready to land.Sep 10 2019, 7:38 PM
jeff retitled this revision from Don't release xbusy in vm_page_remove(), defer to vm_page_free_prep(). to (vm object 2) Don't release xbusy in vm_page_remove(), defer to vm_page_free_prep()..Sep 10 2019, 7:59 PM
markj added inline comments.
sys/vm/vm_page.c
3474 ↗(On Diff #61726)

In fact, it is not true anymore with this change. Fragments like this, in vm_object_collapse_scan() for example, would be wrong:

if (vm_page_remove(m)) {
    vm_page_free(m);
} else {
    vm_page_xunbusy(m);
}