Page MenuHomeFreeBSD

vm_reserv: Fix list locking in vm_reserv_reclaim_contig()
ClosedPublic

Authored by markj on Mar 10 2021, 9:40 PM.

Details

Summary

The per-domain partpop queue is locked by the combination of the
per-domain lock and individual reservation mutexes.
vm_reserv_reclaim_contig() scans the queue looking for partially
populated reservations that can be reclaimed in order to satisfy the
caller's allocation.

During the scan, we drop the per-domain lock. At this point, the rvn
pointer may be invalidated. Take care to load rvn after re-acquiring
the per-domain lock.

Diff Detail

Repository
R10 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.Mar 10 2021, 9:40 PM
alc added inline comments.
sys/vm/vm_reserv.c
1347–1348

Umm, shouldn't this really be using TAILQ_PREV("marker") == rv? Unless TRASHIT() is guaranteed to be enabled, can't TAILQ_REMOVE("rv") leave rv's "next" field unchanged? See the old function vm_pageout_fallback_object_lock() in 11.x.

Further, I believe that fixing this makes testing !rv->inpartpopq redundant.

This revision is now accepted and ready to land.Mar 11 2021, 12:15 AM
sys/vm/vm_reserv.c
1347–1348

It's true that TAILQ_REMOVE() will leave the "next" field unchanged, but if the reservation is removed from the queue, !rv->inpartpopq will be true and we'll short-circuit. I think you're right that we could change this to if (TAILQ_PREV(marker, vm_reserv_queue, partpoq) != rv), but is the current code in fact incorrect?

sys/vm/vm_reserv.c
1347–1348

No, it's going to work. I shouldn't have implied otherwise. Let me instead say that using TAILQ_PREV("marker") is just going to be easier to reason about.

This revision now requires review to proceed.Mar 11 2021, 4:21 AM
This revision is now accepted and ready to land.Mar 11 2021, 4:28 AM