Fix a race in vm_page_swapqueue().

[markj]

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.

[alc]

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]

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]

head/sys/vm/vm_page.c

3491

Sounds good.