Page MenuHomeFreeBSD
Differential D60188

vmm: don't hold the global PPT lock while resetting a device
Needs ReviewPublic
Actions

Authored by nick_spun.io on Wed, Sep 30, 11:47 PM.
Tags
None
Referenced Files
F174267214: D60188.id188285.diff
Thu, Oct 1, 9:08 PM
F174231167: D60188.id.diff
Thu, Oct 1, 1:47 PM
F174226184: D60188.id188280.diff
Thu, Oct 1, 12:41 PM
F174219162: D60188.id188281.diff
Thu, Oct 1, 11:11 AM
F174219095: D60188.id188278.diff
Thu, Oct 1, 11:10 AM
F174214259: D60188.id188279.diff
Thu, Oct 1, 10:19 AM
F174211859: D60188.id188280.diff
Thu, Oct 1, 9:53 AM
F174196104: D60188.id188285.diff
Thu, Oct 1, 7:20 AM
View All 15 Files
Subscribers
bcran
imp
rgrimes

Details

Reviewers
adrian
kbowling
markj
Group Reviewers
bhyve
Summary

ppt_assign_device() holds the global PPT lock across the save/reset/restore. pcie_flr() can wait up to the reset timeout, and every PPT operation on every VM stalls behind it.

Reserve the function with resetting and drop the lock for the reset, as ppt_reset_device() already does (D58864). Set ppt->vm first so a competing assign or ppt_detach() gets EBUSY at once instead of sleeping out the reset. Clear it if the assign fails.

Apply matching changes to ppt_unassign_device() which may actually take longer on properly-functioning hardware due to teardown duration.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 77587
Build 74470: arc lint + arc unit

Event Timeline

nick_spun.io created this revision.Wed, Sep 30, 11:47 PM
nick_spun.io held this revision as a draft.
Herald added a reviewer: bhyve. · View Herald TranscriptWed, Sep 30, 11:47 PM
Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript
Harbormaster completed remote builds in B77565: Diff 188278.Wed, Sep 30, 11:47 PM
nick_spun.io updated this revision to Diff 188279.Wed, Sep 30, 11:48 PM

set ppt->vm with the reservation

Harbormaster completed remote builds in B77566: Diff 188279.Wed, Sep 30, 11:48 PM
nick_spun.io updated this revision to Diff 188280.Wed, Sep 30, 11:48 PM

drop the redundant detach check

Harbormaster completed remote builds in B77567: Diff 188280.Wed, Sep 30, 11:48 PM
nick_spun.io updated this revision to Diff 188281.Wed, Sep 30, 11:48 PM

save/reset/restore unlocked

Harbormaster completed remote builds in B77568: Diff 188281.Wed, Sep 30, 11:48 PM
nick_spun.io added a comment.Thu, Oct 1, 12:03 AM

extends D58864 and D55065

nick_spun.io edited the summary of this revision. (Show Details)Thu, Oct 1, 12:07 AM
nick_spun.io added reviewers: adrian, kbowling, markj.
nick_spun.io published this revision for review.Thu, Oct 1, 1:42 AM
nick_spun.io retitled this revision from drop mtx during reset to vmm: don't hold the global PPT lock while resetting a device for assignment.Thu, Oct 1, 1:46 AM
nick_spun.io edited the summary of this revision. (Show Details)
nick_spun.io edited the summary of this revision. (Show Details)
nick_spun.io updated this revision to Diff 188285.Thu, Oct 1, 2:29 AM

rebased onto main

Harbormaster completed remote builds in B77569: Diff 188285.Thu, Oct 1, 2:29 AM
markj accepted this revision as: markj.Thu, Oct 1, 3:17 PM
markj added inline comments.
sys/amd64/vmm/io/ppt.c
448

I'd add a short comment here explaining why we unlock.

This revision is now accepted and ready to land.Thu, Oct 1, 3:17 PM
nick_spun.io updated this revision to Diff 188335.Thu, Oct 1, 6:17 PM

add a comment at the unlock

This revision now requires review to proceed.Thu, Oct 1, 6:17 PM
Harbormaster completed remote builds in B77584: Diff 188335.Thu, Oct 1, 6:17 PM
nick_spun.io marked an inline comment as done.Thu, Oct 1, 6:17 PM
adrian added inline comments.Thu, Oct 1, 6:53 PM
sys/amd64/vmm/io/ppt.c
453

since we've unlocked - what could happen in another thread before the lock is acquired? is there any other state that needs to be checked or re-checked?

491

Could the same thing happen here?

nick_spun.io marked an inline comment as done.Thu, Oct 1, 7:22 PM
nick_spun.io added inline comments.
sys/amd64/vmm/io/ppt.c
453
  • only the assigning thread changes resetting and ppt->vm during the window - this VM's ops go through ppt_find() which sleeps if resetting is set
  • other assigns/detach here will return EBUSY based on ppt->vm being set
  • everything else that walks the list has never taken ppt_mtx to begin with, and the other walkers skip this because ppt->vm isn't theirs - and our walkers can't run during an assign/bind
491

Yeah I guess we probably should drop the lock here too

nick_spun.io updated this revision to Diff 188344.Thu, Oct 1, 8:57 PM
nick_spun.io marked an inline comment as done.

drop the lock during unassign too

Harbormaster completed remote builds in B77587: Diff 188344.Thu, Oct 1, 8:57 PM
nick_spun.io edited the summary of this revision. (Show Details)Thu, Oct 1, 9:10 PM
nick_spun.io marked an inline comment as done.
nick_spun.io retitled this revision from vmm: don't hold the global PPT lock while resetting a device for assignment to vmm: don't hold the global PPT lock while resetting a device.Thu, Oct 1, 9:13 PM
adrian added a comment.Thu, Oct 1, 10:36 PM

ok, i'll wait for @markj to do a final review, and then i'll stamp it. Thanks!

Revision Contents
Changeset List

PathSize
sys/
amd64/
vmm/
io/
38 lines

Diff 188344

View Options

sys/amd64/vmm/io/ppt.c

Loading...