Page MenuHomeFreeBSD

bhyve: vtnet: simplify thread synchronization
ClosedPublic

Authored by vmaffione on Jun 6 2019, 9:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 12:33 PM
Unknown Object (File)
Oct 3 2024, 6:20 PM
Unknown Object (File)
Sep 23 2024, 5:18 PM
Unknown Object (File)
Sep 18 2024, 5:00 AM
Unknown Object (File)
Sep 15 2024, 7:21 PM
Unknown Object (File)
Aug 11 2024, 8:43 PM
Unknown Object (File)
Aug 11 2024, 4:13 AM
Unknown Object (File)
Aug 11 2024, 2:54 AM
Subscribers

Details

Summary

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).

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.

vmaffione edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jun 7 2019, 8:26 PM
This revision was automatically updated to reflect the committed changes.