inpcb: take a pcbinfo database out from single lock

[glebius]

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.

[glebius]

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.

[glebius]

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.

[markj]

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

sys/netinet/in_pcb.c

862

Please make a helper function which handles this copying.

sys/netinet/in_pcb_var.h

102

Why assert this?

sys/netinet6/in6_pcb.c

1208

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]

sys/netinet/in_pcb.c

862

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

1208

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.

[glebius]

• Rebase. Should now apply to main.

[pouria]

sys/netinet/in_pcb.c

555

Can we use MTX_NEW here?

sys/netinet6/in6_pcb.c

782

Is this comment still valid? (there is also another one below)

[glebius]

sys/netinet/in_pcb.c

555

Why? This flag is needed to hint the mutex(9) that it should not panic if mtx_init() sees something instead of zeroed memory. This part of inpcb is always zeroed.

sys/netinet6/in6_pcb.c

782

Yep, this comment is traveling through the code for years. Originates from Robert Watson and very early days of the network stack getting rid of Giant. As I understand it the concern was that we are checking inp_vflag locklessly. AFAIK, the inpcb can't mutate from IPv6 one to not IPv6 one. I agree all these comments need to be cleansed up with proper commit message. They are also in in6_pcb.c and in raw_ip.c.

[pouria]

sys/netinet/in_pcb.c

319

I know we already have to many assert/mpass here.
But why not using INPBUCKET_LOCK_ASSERT here?
As far as I learned from our style of coding. We use MPASS and lock assert mostly for documentation since it's inside INVARIANT.

555

Why? This flag is needed to hint the mutex(9) that it should not panic if mtx_init() sees something instead of zeroed memory.

Oh, manual says:

MTX_NEW Do not check for double-init

I thought it would prevent redundant check.

This part of inpcb is always zeroed.

Exactly.

733

Not important at all, but to me as a non-native speaker, the comma placement in 'returns with locked context, fills lportp' made me initially read it as filling lportp being conditional on the lock.

Same comment is on other functions below too.

sys/netinet/in_pcb.h

416–421

Why u_short to u_int? why not uint16_t?
It should not be possible to overflow that in context of ports.

[markj]

sys/netinet/in_pcb.c

560

562

2314

sys/netinet/in_pcb_var.h

49

Maybe in_pcblookup_ctx? Or even just struct in_pcblookup.

102

I'd prefer to either omit this assertion, or add a comment explaining why the correctness of this code depends on the ctx being stack-allocated. (I don't see why.)

[glebius]

sys/netinet/in_pcb.c

319

This is how it is going to look like :)

INPBUCKET_LOCK_ASSERT(&inp->inp_pcbinfo->ipi_lbgrouphashbase[INP_PCBPORTHASH(inp->inp_lport, inp->inp_pcbinfo->ipi_lbgrouphashmask)]);

Or introduce couple new variables under #ifdef INVARIANTS for the check.

In this particular case I decided it is easier to leave comment and check all callers. I'm not strong on that. If you and Mark want - I can add this bulky assertion.

sys/netinet/in_pcb.h

416–421

Cause new code uses atomic(9) on them.

[glebius]

• Address feedback.

[glebius]

sys/netinet/in_pcb_var.h

49

The term lookup seems to be occupied already by the fast path - we lookup an inpcb in ip_input() for a packet arrived. Here we have context of an operation that edits: adds, moves or removes an inpcb. It also is associated with the pcbinfo database, not with a single inpcb.

Note that structure name hints for a variable name and prefixes for the inline functions defined below.

[pouria]

sys/netinet/in_pcb.c

319

Wow, no thanks! :)

sys/netinet/in_pcb.h

416–421

Make sense, so why not uint32_t? :D
atomic should be fine (as defined in its manual) with uint32_t AFAICU.
I just want to learn honestly!

[glebius]

sys/netinet/in_pcb.h

416–421

I'm also not expert around atomics, learning too. My rationale was that atomic_int is guaranteed to be present on any arch, while fixed-length atomics may be not. At the same time there is implicit knowledge that int is at least 32-bit long, a de facto standard. Thus int is going to be enough and is known to compile everywhere.

[markj]

sys/netinet/in_pcb.c

319

... or introduce a macro for this, since it could be used elsewhere too.

sys/netinet/in_pcb.h

416–421

We have atomic_subword.h to provide emulation for platforms that do not provide appropriate instructions. I think you would have to extend it to support your use of atomic_fetchadd.

sys/netinet/in_pcb_var.h

49

Then let's keep the current name and remove the XXX REVIEWERS comment?

[glebius]

sys/netinet/in_pcb.h

416–421

Do we really need to? IMHO, just using int is fine.

sys/netinet/in_pcb_var.h

49

Sure, the XXX REVIEWERS not going to be pushed. I was afraid that current name is not nice and was hoping a better name could be found, but looks like the current name has no objections, so will leave it as is.

[markj]

sys/netinet/in_pcb.h

416–421

No, I just mean that if you want to keep using 16-bit fields, there is a path forward. I think int is okay.