inpcb: take a pcbinfo database out from single lock
Needs ReviewPublic
Actions

Authored by glebius on Thu, Jul 9, 8:20 PM.

Details

Reviewers

markj
pouria

Group Reviewers

network
transport

Summary

Initialize all three hashes (exact, wild, load balance group) with a per-
bucket lock. Nothing changes for the packet lookup KPI - it still uses
SMR section for thread safety. But connect(2) and bind(2) operations gain
parallelism now.

The main concept is that as we lookup inpcb database for editing, we are
accumulating bucket locks necessary to accomplish the operation. Once all
lookups are complete and we are good to go, the inpcb is inserted (or
moved) and accumulated lock context is released.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 74661
Build 71544: arc lint + arc unit

Event Timeline

glebius created this revision.Thu, Jul 9, 8:20 PM

Herald added a reviewer: transport. · View Herald TranscriptThu, Jul 9, 8:20 PM

Herald added subscribers: melifaro, ae, imp. · View Herald Transcript

glebius requested review of this revision.Thu, Jul 9, 8:20 PM

Harbormaster completed remote builds in B74661: Diff 181636.Thu, Jul 9, 8:21 PM

I got naive microbenchmark. It allocates ncpu threads, and each threads creates n UDP sockets that are connect(2)-ed to a random valid address:port. In a test virtual machine with 8 CPUs and 10000 sockets per thread I got 6x-7x speedup. If the test is adjusted to close(2) sockets when 50% allocation is reached, so that both insertions and removals are exercised at the same time, the speedup is roughly the same. I didn't yet do any real tests on hardware, but benefits are clear.

The patch doesn't yet address O(n) cycle when we are allocating ports to connect to a single remote IP address.

I got another naive benchmark. It exercises situation when we create as much as possible connections from us to a single peer:port eventually running out of ports. This exercises the worst case of the cycle in in_pcb_lport_dest(), which at the end of the test is executed for 10^4 times to find a port. While the cycle itself is not optimized at all, on a virtual machine with 8 CPUs the test passes 6-7 times faster than before the change.

The patch doesn't apply to main, is there some prerequisite patch?

sys/netinet/in_pcb.c
876	Please make a helper function which handles this copying.
sys/netinet/in_pcb_var.h
102	Why assert this?
sys/netinet6/in6_pcb.c
1210	Can we please have some helper function or macro which operates on the inpcbinfo_ctx and handles both the unlock and clearing of the pointer?

glebius added inline comments.Wed, Jul 15, 6:08 PM

sys/netinet/in_pcb.c
876	There is only one place like this. Are you sure there will be more value from such function than obfuscation?
sys/netinet/in_pcb_var.h
102	There is no strong requirement that it needs to be on stack. But if it is not, then the KPI is definitely used not the way it was designed. If you really insist, I can remove this assertion.
sys/netinet6/in6_pcb.c
1210	Yes, I can do that. Any opinion on the better name for `struct inpcbinfo_ctx` and its most common name `ipictx`? The helper macro you suggest definitely would have name derived from that.