Page MenuHomeFreeBSD

if: Wait for the network epoch after if_dead() on detach
Needs ReviewPublic

Authored by rcm on Sat, Oct 17, 1:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 18, 9:57 PM
Unknown Object (File)
Sun, Oct 18, 6:51 AM
Unknown Object (File)
Sun, Oct 18, 6:51 AM
Unknown Object (File)
Sun, Oct 18, 12:20 AM
Unknown Object (File)
Sun, Oct 18, 12:16 AM
Unknown Object (File)
Sat, Oct 17, 9:12 PM
Unknown Object (File)
Sat, Oct 17, 8:29 PM
Unknown Object (File)
Sat, Oct 17, 6:18 PM
Subscribers

Details

Reviewers
zlei
glebius
pouria
Group Reviewers
network
Summary

111c6b617b18 made if_detach() swap in if_dead's methods so that the
driver is not called once if_detach() has returned. With the network
epoch that no longer covers calls already in progress:
if_detach_internal() waits for the epoch before it flushes the
interface's routes and swaps in the dead methods, so a thread that
found the interface in between, such as a receive task on another
interface forwarding to it, can still be in the driver's transmit path
when if_detach() returns, and drivers free their transmit queues right
after ether_ifdetach().

Wait for the network epoch again after if_dead().

Sponsored by: Rubicon Communications, LLC ("Netgate")

Test Plan

On an 8-CPU virtual machine the added wait costs 2-8 us per interface
destroy, against the 19-22 ms a destroy already takes, nearly all of it
in NET_EPOCH_DRAIN_CALLBACKS().

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rcm requested review of this revision.Sat, Oct 17, 1:31 PM
rcm added a reviewer: network.
rcm added a reviewer: pouria.

Writing same comment as I already made in D60787. Ideally, we shouldn't need epoch_drain_callbacks() KPI nor even have it. If everything is coded correctly and all pieces of memory that are protected by epoch are freed with epoch deferral, we don't need it. See long discussion in D20109 that introduced it. The right fix is that drivers shall free if_softc and anything that hangs off it with deferred free. We don't even need if_dead if everything is done right.

I'd really suggest to work on making everything right instead of putting this plug back. If we fail to achieve this goal before stable/16, of course we should put it back.

P.S. We might still need this epoch_drain_callbacks() KPI to workaround module unload races. Something that you also are looking at. Since kldunload frees regions of code, not malloc(9)-ed memory. However, kernel linker expert may come with some better ideas.

This function is quite fragile and sensitive.
The more time we spend fixing issues, the more fragile it seems to become.

I'm afraid I'm not the right person to review this code, so I'll step down as a reviewer.