Load balance sockets with new SO_REUSEPORT_LB option

[johalun0_gmail.com]

Summary

This patch adds a new socket option, SO_REUSEPORT_LB, which allow multiple programs or threads to bind to the same port and incoming connections will be load balanced using a hash function.

Most of the code was copied from a similar patch for DragonflyBSD.

However, in DragonflyBSD, load balancing is a global on/off setting and can not be set per socket. This patch allows for simultaneous use of both the current SO_REUSEPORT and the new SO_REUSEPORT_LB options on the same system.

Required changes to structures
Globally change so_options from 16 to 32 bit value to allow for more options.
Add hashtable in pcbinfo to hold all SO_REUSEPORT_LB sockets.

Limitations
As DragonflyBSD, a load balance group is limited to 256 pcbs (256 programs or threads sharing the same socket).

[rwatson]

There is a surprising lack of locking assertions in the substantial new chunks of code proposed for in_pcb.c and in6_pcb.c. Could you add proper locking assertions so that we can better understand the locking protocol? Please also add a comment above new data-structure definitions to make clear what the locking requirements are. Please document the per-field locking requirements in in_pcb.h.

Could you explain why the lock order implied by soinherit() (between so and so_inh) deadlock free?

Could you upload a version of the patch with full context so that surrounding code can be inspected properly -- especially with respect to locking?

Could you make revisions to the patch to better implement style(9) -- e.g., return has parens, spaces between "if" and "(condition)", tabs + 4-space continuing-line indents? Also, declare all locals at the tops of functions. Remove unnecessary vertical whitespace. Line wrap at 77 or 78 characters.

Could you comment on the ABI/API/KBI/KPI implications of changing the type of so_options?

[rwatson]

In D11003#319673, @rwatson wrote:

There is a surprising lack of locking assertions in the substantial new chunks of code proposed for in_pcb.c and in6_pcb.c. Could you add proper locking assertions so that we can better understand the locking protocol? Please also add a comment above new data-structure definitions to make clear what the locking requirements are. Please document the per-field locking requirements in in_pcb.h.

This last sentence should have asked for per-field locking requirements in struct in_pcbinfo, both for consistency with existing fields, and also for the benefits of readers.

[johalun0_gmail.com]

Changes:

• Fix style
• Add lock assertions
• Acquire INP_HASH_RLOCK in_pcblookup_lbgroup_last()
• Acquire INP_WLOCK before calling soinherit()

[johalun0_gmail.com]

Thanks for the feedback!

In D11003#319673, @rwatson wrote:

There is a surprising lack of locking assertions in the substantial new chunks of code proposed for in_pcb.c and in6_pcb.c. Could you add proper locking assertions so that we can better understand the locking protocol? Please also add a comment above new data-structure definitions to make clear what the locking requirements are. Please document the per-field locking requirements in in_pcb.h.

Improved in the latest version.

Could you explain why the lock order implied by soinherit() (between so and so_inh) deadlock free?

Hmm.. No. Anyone else with confidence here?

Could you upload a version of the patch with full context so that surrounding code can be inspected properly -- especially with respect to locking?

https://github.com/johalun/freebsd/tree/d11003/ <- is this what you mean?

Could you make revisions to the patch to better implement style(9) -- e.g., return has parens, spaces between "if" and "(condition)", tabs + 4-space continuing-line indents? Also, declare all locals at the tops of functions. Remove unnecessary vertical whitespace. Line wrap at 77 or 78 characters.

Yes, there was a lot of misses here. Hopefully I got them all now.

Could you comment on the ABI/API/KBI/KPI implications of changing the type of so_options?

Should be minimal implications. Of course, userland needs to be updated to use the new option. setsockopt() already takes an int as argument so no problem there.

[sbruno]

Reopen at the request of the submitter.

This has been reverted from head at svn r332967.

[johalun0_gmail.com]

@gnn @rwatson Are we good to go?

[rwatson]

In D11003#319977, @johalun0_gmail.com wrote:

Could you explain why the lock order implied by soinherit() (between so and so_inh) deadlock free?

Hmm.. No. Anyone else with confidence here?

If no one can answer this question, then this patch cannot be committed. It is essential that we know that there is a locking protocol in place that prevents deadlock, as socket locking is a very subtle thing, and we've spent almost two decades getting the current model into a workable and well-documented shape. Part of the reason I'm pushing on this is that in your original patch, there's little or no use of the programmatic techniques we use to ensure correctness: proper lock documentation, assertions, etc. This gives me a low confidence that this work is correct. Better assertions and documentation will substantially increase that confidence -- in particular, in arguing about deadlock.

Could you upload a version of the patch with full context so that surrounding code can be inspected properly -- especially with respect to locking?

https://github.com/johalun/freebsd/tree/d11003/ <- is this what you mean?

I'd like you to upload a version of your patch to this review request that includes the full context so that I can click "show context" around your diff in phabricator and comment on it all together. I'm afraid I've forgotten the name of the command-line tool for interacting with phabricator, but my experience is that that is the best way to get this to happen. I assume it's also possible using git diff in some form, but I've never been lucky in getting it to work :-).

Could you make revisions to the patch to better implement style(9) -- e.g., return has parens, spaces between "if" and "(condition)", tabs + 4-space continuing-line indents? Also, declare all locals at the tops of functions. Remove unnecessary vertical whitespace. Line wrap at 77 or 78 characters.

Yes, there was a lot of misses here. Hopefully I got them all now.

Could you comment on the ABI/API/KBI/KPI implications of changing the type of so_options?

Should be minimal implications. Of course, userland needs to be updated to use the new option. setsockopt() already takes an int as argument so no problem there.

... plus presumably requiring netstat/sockstat/etc to be recompiled, and also changing the binary interface for kernel modules. Are there kernel modules in the base system that may make assumptions about the size of so_options? How about in the ports tree?

[johalun0_gmail.com]

Full context for all files.

[rwatson]

In D11003#320450, @johalun0_gmail.com wrote:

Full context for all files.

Thanks, will take a look at the updated version.

[johalun0_gmail.com]

Add entry in man page

[rwatson]

Mostly style and comment issues in this pass, although there are a few other queries -- e.g., relating to so and inh_so lock order.

I will likely have further comments on the contents, but wanted to get these out quickly since they were easy to report.

sys/kern/uipc_socket.c

1064

Comment should be a full sentence.

1086

This is a second socket-lock acquisition in a row. How can we convince ourselves that this is safe?

1136

Missing space after "if".

1140

Missing space after "if".

1141

Missing space after "if".

sys/netinet/in_pcb.c

229

Should there be a lock assertion here?

239

This vertical whitespace is probably not needed.

246

Should there be a lock assertion here?

257

Should there be a lock assertion here?

269

Can probably drop this blank line here.

271

Can probably drop this blank line here.

276

Add a "." at the end -- style(9) would like comments to be complete sentences.

279

In other in_..._inshash() functions, the pcbinfo argument is implied by the 'inp' argument. Do we need it to be explicit here?

299

Comment should be a full sentence.

310

Comment should be a full sentence.

322

This vertical whitespace is probably not needed.

333

Comment should be a full sentence.

341

This variable would ideally be declared at the top of the function, in style(9).

351

Comment should be a full sentence.

367

Here, as above, can pcbinfo be implied by inp?

390

Comment should be a full sentence.

393

Comment should be a full sentence.

400

Comment should be a full sentence.

408

This structure is visually confusing. Could we break some of this out into another function, or use another control-flow structure? Currently it's hard to see how the return fits.

784

Comment should be a full sentence.

1901

Should these assertions be before the if() clause above?

1925

This variable declaration would ideally be above.

2272

Comment should be a full sentence.

2524

Comment should be a full sentence.

2530

Use a C-style comment, not C++, for a block comment. Comment should be a full sentence.

2531

style(9) requires () around return arguments.

2662

Use a C-style comment.

sys/netinet/in_pcb.h

550

It might be useful for this comment to mention something about il_inp[] behaviour?

563

What are the locking properties of il_inpsiz, il_inpcnt, and il_inp?

677

I have no personal objection to this design choice, which is consistent with other hashes here, but someday we should think about something better for all of these hashes!

sys/netinet/tcp_subr.c

1960

I'm cautious about naming a variable "listen" -- historically, this would have shadowed a global symbol (listen()). I wonder if another name might be a better idea.

1972

The line wrapping here is funky. Make it consistently 78 characters wide.

2000

This vertical whitespace is not required.

sys/netinet6/in6_pcb.c

176 ↗

(On Diff #41882)

Use a C-style comment. Comment should be a full sentence.

904 ↗

(On Diff #41882)

style(9) prefers that these variables be declared above rather than in a block here.

1172 ↗

(On Diff #41882)

Comment should be a full sentence.

[johalun0_gmail.com]

sys/kern/uipc_socket.c

1086

By simply re-order the code a bit we can unlock the first one before we lock the other.

sys/netinet/in_pcb.c

229

in_pcblbgroup_{alloc,free,resize,reorder} are only called from in_pcb{ins,rem}lbgrouphash where locks are asserted. To assert locks again in these functions we'd need to pass the inpcb as argument. What do you recommend?

[johalun0_gmail.com]

Improving locking
Refactor
Fix style

[johalun0_gmail.com]

Added comment - unsure about locking in one place...

sys/netinet/tcp_subr.c

2003

How about this locking? Is it needed for access to the socket and if so, is it safe to lock another inpcb here?

[johalun0_gmail.com]

@rwatson If you have time, I would appreciate if you could take a look at the updates.

[jtl]

sys/netinet/tcp_subr.c

2003

The INP lock is needed to check inp_socket and ensure it doesn't change while you are messing with the socket. (Actually, it could probably be a read lock.)

Its safe to lock the inpcb first, and then lock that inpcb's socket while you hold the INP lock. If you want to lock two inpcbs, you will need to define a deadlock-free ordering. (See, for example, the way sched_ule.c does this in sched_switch_migrate().)

[johalun0_gmail.com]

• Omit the inheritance part for now so that we can get this committed. Need more time and eyes to figure out a safe way to solve the locking issues. (this is anyway not a critical function but more a nice-to-have thing)
• Add leading zeros to the rest of the SO_* options to indicate that they are now 32 bit.

[sbruno]

Is everyone on this review fine with me committing this? With the inheritance code removed, I believe there are no further objections or questions.

[jtl]

In D11003#327482, @sbruno wrote:

Is everyone on this review fine with me committing this? With the inheritance code removed, I believe there are no further objections or questions.

Have you addressed all of @rwatson's comments?

I've been meaning to review this, but have been delayed. I would appreciate a few extra days to look it over.

[johalun0_gmail.com]

Have you addressed all of @rwatson's comments?

Yes, it's all been addressed including a couple of extra things like adding to man pages.

[sbruno]

In D11003#327485, @jtl wrote:

In D11003#327482, @sbruno wrote:

Is everyone on this review fine with me committing this? With the inheritance code removed, I believe there are no further objections or questions.

Have you addressed all of @rwatson's comments?

I've been meaning to review this, but have been delayed. I would appreciate a few extra days to look it over.

@jtl I'd like to commit this at some point this week, but I also want your feedback on it. Should we hold off until next week to drop this into head?

[jtl]

In D11003#328091, @sbruno wrote:

In D11003#327485, @jtl wrote:

In D11003#327482, @sbruno wrote:

Is everyone on this review fine with me committing this? With the inheritance code removed, I believe there are no further objections or questions.

I've been meaning to review this, but have been delayed. I would appreciate a few extra days to look it over.

@jtl I'd like to commit this at some point this week, but I also want your feedback on it. Should we hold off until next week to drop this into head?

If I haven't reviewed it by this time Friday (i.e. in the next 48 hours), I think it is only fair that I not hold you up anymore.

I'm not sure if @rwatson is done reviewing it, or if others (@glebius, for example) want you to wait for them to review it.

[sbruno]

@johalun0_gmail.com Can this be regenerated against head when you get to it in the morning?

[johalun0_gmail.com]

Updated diff against today's kernel.

[pluknet]

sys/netinet/tcp_subr.c

1985

tcp_subr.c diff looks now unrelated, that is, only whitespace change here

[johalun0_gmail.com]

sys/netinet/tcp_subr.c

1985

Good catch! Will fix that.

[johalun0_gmail.com]

Remove whitespace change.

[sbruno]

Pending no further review comments, this will go in the tree on Monday, May 31. That way folks can yell at me when we all get together in Ottawa.