On vtnet device reset it is necessary to wait for threads to stop TX and RX processing.
However, the rx_in_progress variable (used for to wait for RX processing to stop) is actually
useless, and can be removed. Acquiring and releasing the RX lock is enough to synchronize
correctly. Moreover, it is possible to reset the device while holding both TX and RX locks,
so that the "resetting" variable becomes unnecessary for the RX thread, and can be protected
by the TX lock (instead of being volatile).
Details
- Reviewers
rgrimes markj jhb - Group Reviewers
bhyve - Commits
- rS349698: MFC r348834
rS349692: MFC r348834
rS348834: bhyve: vtnet: simplify thread synchronization
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
This looks ok to me. The comment above pci_vtnet_rxwait() should probably be updated to something like, "Make sure the rx thread sees that we're resetting."
(Full context diffs make it easier to review as then you can comment on other parts of the code.)
While this is ok, the empty locking block does make me think that there are probably some slightly hackish things elsewhere. Having resetting be volatile strikes me as one of those. You could instead hold both mutexes when modifying this variable and then it would need to be volatile for example. It would also avoid a future reader deciding they can just remove the "obvious nop" in rxwait. That is, you could have pci_vtnet_reset() look something like this:
pthread_mutex_lock(&sc->rx_mtx); pthread_mutex_lock(&sc->tx_mtx); sc->resetting = 1; /* Wait for transmit thread to drain. */ while (sc->tx_in_progress) { pthread_mutex_unlock(&sc->tx_mtx); usleep(10000); pthread_mutex_lock(&sc->tx_mtx); } sc->vsc_vx_ready = 0; sc->rx_merge = 1; sc->rx_vhdrlen = ...; vi_reset_dev(...); pthread_mutex_unlock(&sc->tx_mtx); pthread_mutex_unlock(&sc->rx_mtx);
You can then actually remove several of the 'resetting' checks (all of them in the RX path) as well as dropping the volatile. At that point, you could even push the rx_mtx down a bit in the reset path as you only need it before setting vsc_vx_ready to 0 until the end of the function.
Sorry for the diff context, I forgot about that.
Thanks for the suggestions, they look good to me, and I'll implement those in short.
Btw I would like to refactor/improve vtnet/tap/netmap support, so more patches will follow.