Refactor PCB hash read lock (the epoch)
ClosedPublic
Actions

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

Details

Reviewers

None

Group Reviewers

network
transport

Commits

rS354477: Since r353292 on input path we are always in network epoch, when

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

glebius created this revision.Oct 30 2019, 5:54 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptOct 30 2019, 5:54 PM

Herald added a subscriber: ae. · View Herald Transcript

Harbormaster completed remote builds in B27282: Diff 63806.Oct 30 2019, 5:54 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.Nov 5 2019, 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.