nmdm/kern_conf: proper force-unload via destroy_dev_drain_pending()#2207
Open
MegaManSec wants to merge 5 commits into
Open
nmdm/kern_conf: proper force-unload via destroy_dev_drain_pending()#2207MegaManSec wants to merge 5 commits into
MegaManSec wants to merge 5 commits into
Conversation
Also fix the order (which is not important). Also fix a typo. Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
|
Thank you for taking the time to contribute to FreeBSD! All issues resolved. |
MegaManSec
force-pushed
the
nmdm-rebased
branch
4 times, most recently
from
e80ed05 to
532d7a3
Compare
If an nmdm device was created by accident such as by running `ls /dev/nmdmAA`, it would restrict the module from being unloaded, despite no data passing through it. With this patch, `kldunload nmdm` will still fail if any nmdm device still exists. `kldunload -f nmdm` will try to destroy all nmdm devices before unloading. If an nmdm modem is either opened in blocking mode or data is being passed through it, after five seconds of trying, it will not be destroyed and the unload will also fail. Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
On entry to nmdm_close(), the tty is already locked. nmdm_destroy() does not return before the tty is unlocked by tty_rel_gone(). This means that we need to lock the tty when the nmdm_close() returns. nmdm_destroy() assumes that the tty is already locked. Therefore, nmdm_unload() must lock the tty before calling nmdm_destroy(). Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
Expose a function that drains the two taskqueue_thread tasks used by destroy_dev_sched*() / tty_rel_gone() to perform deferred cdev teardown. Drivers that schedule asynchronous device destruction and then need to confirm all devices are gone before returning (e.g. from a module unload handler) can call destroy_dev_drain_pending() instead of busy-polling nmdm_count or sleeping with a fixed timeout. Reviewed by: kevans (concept) Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
The previous nmdm_unload() polled nmdm_count with a hard 5-second timeout after calling tty_rel_gone() on each pair. Replace that with a call to destroy_dev_drain_pending(), which flushes the taskqueue_thread tasks that carry tty_rel_gone() -> destroy_dev_sched_cb() -> tty_dealloc() -> nmdm_free() to completion synchronously. Additional changes to nmdm_destroy(): - Return EBUSY immediately (lock still held) if either side is open, so nmdm_unload() can fail fast without touching pairs that will not go away. - Guard tty_rel_gone() calls with !tty_gone() checks so the function is idempotent when called concurrently (e.g. close racing with unload). nmdm_free() now acquires nmdmsoftc_lock before ns_mtx (outer before inner) to match the ordering in nmdm_unload() and avoid a WITNESS lock-order violation. nmdm_close() no longer needs nmdmsoftc_lock at all; the tty lock alone is sufficient there. Reviewed by: kevans (concept) Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #1367, addressing the feedback from @kevans91 that was never completed.
The original PR fixed a bug where accidentally-created nmdm devices (e.g. via
ls /dev/nmdmAA) would prevent the module from unloading, and addedkldunload -fsupport. It was not merged because the unload path used a 5-second busy-wait instead of proper plumbing through the tty/cdev layers, and had a lock-ordering issue.This PR makes two additional changes on top of the original three commits:
kern_conf: add destroy_dev_drain_pending()Adds a new KPI that synchronously drains the
taskqueue_threadtasks responsible for deferreddestroy_dev_sched*()/tty_rel_gone()teardown. This is thetty_drain_freed → destroy_dev_drain_pending → taskqueue_drainplumbing @kevans91 described.nmdm: replace force-unload timeout with destroy_dev_drain_pending()destroy_dev_drain_pending()after scheduling destruction of all idle pairs.nmdm_destroy()now returnsEBUSYimmediately (lock still held) if either side is open, sonmdm_unload()fails fast for pairs that won't go away.nmdm_free()now acquiresnmdmsoftc_lockbeforens_mtx, matchingnmdm_unload()'s ordering and eliminating the WITNESS lock-order violation.nmdm_close()no longer needsnmdmsoftc_lockat all.