Page MenuHomeFreeBSD

Fix a race in vm_page_swapqueue().
ClosedPublic

Authored by markj on Sep 25 2019, 6:00 PM.

Details

Summary

This function moves a page from one queue to another. To change the
"queue" field from one index to another, the page queue lock for the old
queue must be held. When the cmpset succeeds, this lock no longer
protects the page queue state, so it is incorrect to call
vm_pagequeue_remove() after that point. Thus, we must preemptively
remove the page from its queue, and re-insert it if the state commit
fails.

Also remove the assertion about VPO_UNMANAGED. There is no guarantee
that the page has not been reallocated as an unmanaged page. In this
case, though, the cmpset must fail since the "queue" field for an
unmanaged page will be PQ_NONE, and we assert oldq != PQ_NONE.

BTW, my aim is to convert all queue state updates (i.e. modifications to
"queue", "aflags" and "act_count" fields) into transactional blocks,
using a 32-bit atomic cmpset on the vm_page structure. In my tree I
added a counter for failed cmpsets, and found that they are very rare in
practice.

Diff Detail

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

Event Timeline

markj created this revision.Sep 25 2019, 6:00 PM
kib accepted this revision.Sep 26 2019, 8:46 PM
This revision is now accepted and ready to land.Sep 26 2019, 8:46 PM
This revision was automatically updated to reflect the committed changes.
alc added inline comments.Sep 29 2019, 6:41 PM
head/sys/vm/vm_page.c
3491

You could shorten/simplify this by writing:

queued = (m->aflags & PGA_ENQUEUED) != 0;
if (__predict_true(queued))
markj added inline comments.Sep 29 2019, 6:50 PM
head/sys/vm/vm_page.c
3491

I have a larger patch which generalizes the logic in this function. Basically, all queue state updates will use the new vm_page_pqstate_commit(), which wraps the cmpset and handles removal of the page from its queue. It also eliminates the extra atomic operations we currently have here: right now we are clearing PGA_ENQUEUED and setting m->queue in separate atomic operations.

I implemented your suggestion in my patch. If it's ok with you, I will simply leave this function as-is for now to avoid an extra commit.

alc added inline comments.Sep 29 2019, 7:24 PM
head/sys/vm/vm_page.c
3491

Sounds good.