Page MenuHomeFreeBSD

TCP Timer cleanup
ClosedPublic

Authored by rrs on Jul 6 2016, 11:12 AM.
Tags
None
Referenced Files
F103117014: D7136.id.diff
Thu, Nov 21, 6:05 AM
F103066120: D7136.id18547.diff
Wed, Nov 20, 11:55 AM
Unknown Object (File)
Wed, Nov 20, 10:56 AM
Unknown Object (File)
Wed, Nov 20, 5:19 AM
Unknown Object (File)
Tue, Nov 19, 4:37 AM
Unknown Object (File)
Tue, Nov 19, 2:51 AM
Unknown Object (File)
Mon, Nov 11, 11:30 AM
Unknown Object (File)
Sun, Nov 10, 12:17 PM
Subscribers

Details

Summary

The TCP timer code as been fraught with churn in the effort to fix its
racy use of timers. We have moved to ASYNC drain which cleans up a lot
of this but still have some hanging flags and such around that should be cleaned
up. Also the use of two locks at once in the timer code is problematic since the
timer code only really assures that one lock can be dealt with properly in the
drain functions. Therefore we will move to a "switch" locks type method so that
a future patch can get rid of the async drain all the way and move the lock
under the callout system (where it belongs).

Test Plan

Beat the heck out of it on a NF workload and hopefully get Versign to give it a go as well.

Note I do not intend for this to go into 11 but to wait until after head forks at some point unless
I hear a lot of pushback that we need it sooner

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rrs retitled this revision from to TCP Timer cleanup.
rrs updated this object.
rrs edited the test plan for this revision. (Show Details)
rrs added a reviewer: transport.
gallatin edited edge metadata.

In addition to helping the callout correctness, this seems to improve lock contention on tcbinfo, since this lock is now taken only when needed, not when entering every routine (thereby blocking writers, or blocking when a writer has the lock).

I've been running this over a day with ~100K connections at ~80Gb/s at Netflix (in a FreeBSD-11 context) with no issues. I've also tested it on a WITNESS + INVARIANTS kernel, and saw no LORs and no kassert failures (at 5Gb/s :)

This revision is now accepted and ready to land.Jul 13 2016, 8:41 PM
rrs edited edge metadata.

Fix it so we don't leak inp's.. the tcp_close() can return NULL in tp, this
would mean we would not do the reference count release. Instead use
the inp.

Also change the name to reflect what is actually being done (advice from Michael Tuexen :-D)

This revision now requires review to proceed.Jul 19 2016, 9:16 AM
rrs edited edge metadata.

Turns out when tp is returned NULL by the drop/close that means
it also nicely released the INP_WLOCK. We need to re-acqurire the lock
in that case so we can drop our inp reference.

I really need to learn how to type :-)

gallatin edited edge metadata.
This revision is now accepted and ready to land.Jul 26 2016, 3:32 PM
hiren added a reviewer: hiren.
hiren added a subscriber: hiren.

Been using a slight variation of this change without any problems.