Page MenuHomeFreeBSD

Refactor PCB hash read lock (the epoch)
ClosedPublic

Authored by glebius on Oct 30 2019, 5:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 17, 2:01 PM
Unknown Object (File)
Dec 20 2023, 2:42 AM
Unknown Object (File)
Sep 21 2023, 5:41 PM
Unknown Object (File)
Sep 11 2023, 4:58 AM
Unknown Object (File)
Jul 7 2023, 11:27 PM
Unknown Object (File)
Jun 25 2023, 10:14 AM
Unknown Object (File)
Jun 25 2023, 10:12 AM
Unknown Object (File)
Jun 25 2023, 10:11 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 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.Nov 7 2019, 8:50 PM
This revision was automatically updated to reflect the committed changes.
glebius marked an inline comment as done.