vm_reserv: Fix list locking in vm_reserv_reclaim_contig()
ClosedPublic
Actions

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

Details

Reviewers

alc
kib

Commits

rG45910f663893: vm_reserv: Fix list locking in vm_reserv_reclaim_contig()
rGcec3990d3479: vm_reserv: Fix list locking in vm_reserv_reclaim_contig()
rG968079f253c1: vm_reserv: Fix list locking in vm_reserv_reclaim_contig()

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

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Mar 10 2021, 9:40 PM

Herald added a subscriber: imp. · View Herald TranscriptMar 10 2021, 9:40 PM

markj requested review of this revision.Mar 10 2021, 9:40 PM

Harbormaster completed remote builds in B37746: Diff 85516.Mar 10 2021, 9:40 PM

markj added reviewers: alc, kib.Mar 10 2021, 9:41 PM

markj added a subscriber: gallatin.

alc accepted this revision.Mar 11 2021, 12:15 AM

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

markj added inline comments.Mar 11 2021, 2:33 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?

alc added inline comments.Mar 11 2021, 4:02 AM

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.