Page MenuHomeFreeBSD

Refactor PCB hash read lock (the epoch)
ClosedPublic

Authored by glebius on Wed, Oct 30, 5:54 PM.

Details

Summary

Since r335356 INP_INFO_RLOCK() actually means entering network epoch.
Since r353292 we are in epoch whenever we process any incoming packet.
And we are moving towards entering epoch earlier for outgoing packets.

This branch reviews all places that do PCB hash locking. In some places
we just mechanically convert from INP_INFO_RLOCK() macro to NET_EPOCH_ENTER()
macro, to remove ambiguity. In the code that is supposed to already run
in the epoch, we remove the macro and we assert the epoch.

Since "read-locking" the PCB hash is no longer mutually exclusive
to write-locking it, some code can be greatly simplified. See
udp_output() and udp_output6(). These two functions now also
call into ip_output() and ip_output6() in epoch. For now that
creates recursion, but in the future all upper level protcols
will do that and IP level would assert epoch.

The change isn't intended to be committed at once. The split
into series of commits with more comments can be viewed here:

https://github.com/glebius/FreeBSD/commits/pcb_epoch

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

glebius created this revision.Wed, Oct 30, 5:54 PM
Herald added a subscriber: ae. · View Herald Transcript
bz added a subscriber: bz.Mon, Nov 4, 6:51 PM

In general I really like the changes. I've not looked into the large chunks of sysctls / UDP / TCP in too much detail. I'll try to do them based on their individual git changes the next days (at least sysctl and UDP). Hope someone else will do the TCP bits.

sys/netinet/in_pcb.c
2257 ↗(On Diff #63806)

I was contemplating if the assertion should (also) be in the callers of this private function (at least for documentation purposes) given their callers (at least in some individual changes in git) explicitly acquire the et around the calls?

sys/netinet/tcp_input.c
1424 ↗(On Diff #63806)

Unrelated whitespace change.

sys/netinet/toecore.c
394 ↗(On Diff #63806)

Why are we losing that assertion on what in git is a "mechanical replacement"? Is there a purpose for losing it here?

glebius marked an inline comment as done.Tue, Nov 5, 10:29 PM
glebius added inline comments.
sys/netinet/in_pcb.c
2257 ↗(On Diff #63806)

I also contemplated that. AFAIK, doesn't make sense, as there is no code flow that would pass the assertion. See my other reply on comment.

sys/netinet/toecore.c
394 ↗(On Diff #63806)

I usually removed the assertions in case when we:

a) asserted it somewhere earlier
b) would assert it in header of a function called on next line

We can spread assertions virtually everywhere, but at some point they become superfluous.

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Nov 7, 8:50 PM
This revision was automatically updated to reflect the committed changes.
glebius marked an inline comment as done.