Should we eventually use this model for other protocols? (Not saying we have to do that now, but if we think we should it might be worth documenting as a TODO item)

Old bug: UDP has protocol PCB's now as well, so this should possibly say "TCP and UDP" (though UDP doesn't have connections)

Old bug: the "when modified" shouldn't have commas around it. This might be more clearly worded as:

A few fields, such as the global connection lists, require both the inp_lock and global pcblist lock to be write-locked while modified.

Still a bit of a mouthful. :-/

Old bug: this is a long sentence. I would probably split it up by s/lock, which/lock. This/

So this last sentence is missing a word (should be "these fields _requires_ that write" or some such), but it also duplicates the "A few fields" sentence less awkwardly.

Hmm, I think I would reword much of this as so perhaps starting with the "A few fields" sentence:

A few fields are protected by multiple locks as indicated in the locking notes below.  For
these fields, all of the listed locks must be write-locked for any modifications.  However,
these fields can be safely read while any one of the listed locks are read-locked.  This
model can permit greater concurrency for read operations.  For example, connections can
be looked up while only holding a read lock on the global pcblist lock.  This is important
for performance when attempting to find the connection for a packet given its IP and port
tuple.

One noteworthy exception is that the global pcbinfo lock follows a different set of rules in
relation to the inp_list field.  Rather than being write-locked for modifications and
read-locked for list iterations, it must be read-locked during modifications and write-locked
during list iterations.  This ensures that the relatively rare global list iterations safely
walk a stable snapshot of connections while allowing more common list modifications
to safely grab the pcblist lock just while adding or removing a connection from the global
list.

I think this comment might be helpful in cxgb_listen.c and t4_listen.c as well.

Do you need both locks to read ipi_gecnt and ipi_count? Would just the pcblist lock suffice?

Same question here.

Great catch. To read ipi_gecnt and ipi_count you need only INP_LIST lock. I fixed the in_pcb.h documentation as well.

Fixed as part as above comment fix.

Perhaps s/If currently/Currently/ here.

Hmm, switching this to a read lock here (really in tcp_tw_start()) will not provide the deadlock protection between locking two inpcbs at once.

Perhaps this is ok as the first inp is always one going into time-wait and the second inp is one that is already in time-wait (so there is a defined order)? As long as this is the only place we acquire two inp locks at once and we never acquire a tw inpcb lock before a non-tw inpcb then we should be safe. However, if that is true then the comments both here and in tcp_twstart() need updating. I'm not sure if it is true though. Sorry I didn't catch this on the first look.

Right, fixed.

You are totally right. And there are actually two places were two inp locks are acquired at once:

#1 In tcp_input() - if syncache_expand() succeed - two inp locks are held: The inp of the listen socket + the inp of the newly created socket.
#2 In tcp_tw_2msl_scan() called from tcp_twstart() (here)

I checked that #1 and #2 cannot collide, thus it is safe to use only INP_INFO read lock here. However:

• A comment update is required (Thanks, good catch)
• In tcp_tw_2msl_scan() it is not a requirement to hold two inp locks at same time. I am going to check if we can simply use the below classical pattern to avoid that:

#1 in tcp_twstart() takes a reference on the inp lock transitioning to TIME_WAIT state
#2 Release its inp lock
#3 Call tcp_tw_2msl_stop()
#4 Take back the inp lock and check inp status with in_pcbrele_wlocked()

If the change is clear enough I will submit it in a separate review. The goal being to have only place were two inp locks are held at same time, it is cleaner).

I think this probably sounds fine though I'd want to see the patch I think.

I guess the race is tcp_input() handling another packet for this connection while the inp lock is dropped. If you are careful to avoid updating the state (right now the state is set to TIME_WAIT before tcp_tw_2msl_stop() is called, so you'd have to move that) then I think you can get
duplicate calls to twstart() if there are dup FINs, so you'd need to just bail if you get a tw for
reuse but the inp already has a tw.

The other option is to mark the inp as in timewait but then handle the race that a SYN might come in and call tcp_twcheck() (so twcheck would need to handle the case that it has TIMEWAIT set, but there is no actual tw yet, possibly arranging for the twstart() to immediately delete the inp when it relocks the inp?

Hi @jhb,

I did the change and if it works well the patch is quite complex missing the initial goal of being cleaner. I would propose to leave the current INP locks as is, and I am going to thoroughly explain why it is ok with the current implementation.

A (real) cleaner solution would be that in tcp_twstart():

tw = uma_zalloc(V_tcptw_zone, M_NOWAIT);

always return an allocated tcptw because we took care to release in advance the oldest tcptw structs as soon as we get close to the V_tcptw_zone limit. And this would indeed makes the code cleaner.