Page MenuHomeFreeBSD

vm_page_xbusy_claim(): Use atomics to update busy lock state.
ClosedPublic

Authored by markj on Tue, Jul 28, 3:08 PM.

Details

Summary

vm_page_xbusy_claim() could clobber the waiter bit. For its original
use, kernel memory pages, this was not a problem since nothing would
ever block on the busy lock for such pages. r363607 introduced a new
use where this could in principle be a problem.

Fix the problem by using atomic_cmpset. Since this macro is defined
only for INVARIANTS kernels, the extra overhead doesn't seem
prohibitive.

Reported by: vangyzen

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 requested review of this revision.Tue, Jul 28, 3:08 PM
markj created this revision.
cy added a subscriber: cy.Tue, Jul 28, 3:21 PM
kib accepted this revision.Tue, Jul 28, 3:43 PM
kib added inline comments.
sys/vm/vm_page.h
775 ↗(On Diff #75069)

I think you want to use _busy_lock as a name. If user code (in kernel) defines a busy_lock macro, we have conflict. The _[a-z0-9] namespace is reserved by std for file-local impl symbols.

This revision is now accepted and ready to land.Tue, Jul 28, 3:43 PM
emaste added a subscriber: emaste.Tue, Jul 28, 3:53 PM
markj updated this revision to Diff 75078.Tue, Jul 28, 4:08 PM
markj marked an inline comment as done.

Put the local var in the file-local namespace.

This revision now requires review to proceed.Tue, Jul 28, 4:08 PM
kib accepted this revision.Tue, Jul 28, 4:22 PM
This revision is now accepted and ready to land.Tue, Jul 28, 4:22 PM
vangyzen accepted this revision.Tue, Jul 28, 4:22 PM
vangyzen added inline comments.
sys/vm/vm_page.h
780 ↗(On Diff #75078)

I was expecting fcmpset, but this works, too.

markj added inline comments.Tue, Jul 28, 4:37 PM
sys/vm/vm_page.h
780 ↗(On Diff #75078)

I initially wrote it with fcmpset but I find this is a bit clearer. I don't see any real reason to prefer one over the other.

alc accepted this revision.Tue, Jul 28, 6:19 PM
This revision was automatically updated to reflect the committed changes.