Forgot to update the manual pages as well since the mib variables
are now at the CC level not the lower one. Also make sure the internet draft
version is sited in the man.

Update the diff to take into account Neal Cardwell's comments on tcpm.. i.e.
make the comparison for rounds SEQ_GT not SEQ_GEQ.

Looks like just what we agreed too ;)

Ahh I see if you get a "RTO event" or an "Application limited" event you
remove the flag. But what about NDUPACK.. (where a loss is taking you
out of SS).. and for that matter maybe we should also when we leave
CSS to CA remove this flag...

One thing that is interesting to me about cubic is that it has this flag
"in slow start" that is added to the flags field, but I never see that
flag be removed.. i.e. when we enter CA.

Fix Richards magic numbers.

With a bit of testing I figured out I had missed a few things. Now this version
works per the draft. Note that I may have found a problem with the draft though.
Am now waiting on an answer from Praveen.

Ok this straightens out the allowed flag to be up at the ccv->flags
level and gets rid of the need for a socket option for the specific
allowing of hystart++ on the CC module. Instead if the flag gets
set any CC module that supports hystart++ can then use it.

Only question I have Gleb, is does the removal of PCBGROUP have any performance impacts for a user
that was using it and now no longer can?

In D33022#745780, @glebius wrote:

In D33022#745736, @bz wrote:

Given we don't seem to have a `man smr` could you at least expand SMR once in your title/description so people can find what they are looking for as it's virtually invisible to the world still (unless one finds the header file)?

Yes, absence of the manual page bugs me. Not even mention in uma(9). I will reword description.

I am usually not a big fan of this type of actions. I like
structures to be in a header file instead of buried in the
.c file.. but if you really think its a good idea.. i guess its ok.. my
feelings are not that strong about it.

Note missing yet from this diff is the ctl_output() function so you can actually enable it.

Firstly I'll note that there are other ways to achieve the proposed outcome than doing an atomic increment (e.g. deferring the increment until next time the wlock is held and the next log event is generated) but I think the atomic increment is the most straight forward option and is why I proposed it for this case. That being said, I'm only really concerned about making sure the "contract" is agreed and maintained, less so about how it's done.

As to the technical concerns about doing an atomic increment in this case, could you please help me understand the problem you think it could cause? My understanding (and I'm certainly no expert here so keen to be educated if I'm missing something) is that:

• If we hold the inp rlock the tcpcb can't go away so tp->t_logsn will always be a valid memory address to increment
• If we hold the inp rlock and do the increment atomically we have no possibility of variable corruption if we're racing with other rlock holders who may also try to increment
• If we hold the inp rlock we've excluded all inp wlock code paths from doing a non-atomic increment of the serial number
• If a wlock code path does a non-atomic increment of the serial number and then unlocks, the barrier semantics of the unlock force consistency of the serial number such that a subsequent atomic increment in an rlock path would be ensured to operate on the correct wlock-incremented serial number

Am I missing a scenario/sequence of operations where there could be a problem with the inp-rlocked atomic increment approach?

Michael and I have chatted about this. The only way you
can get in this state where you have an INP_RLOCK and
you are responding, is when you are getting a stand-alone
SYN. In that case due to DOS worries Gleb changed the code so that
we would get an RLOCK. Now this is a rare case unless you are
under a DOS attack. And the connection needs to be in the closed
state or its to a connection from a different port number. There
are also a couple of other cases where the SYN-CACHE unpack
fails (but unlikely to have a t_logstate set of course).

The whole point of this was that there were absolutely no logs what so ever when
tcp_respond() was used. So prior to this set of commits you *never* saw anything
when we responded to a packet (and that happens in many places in all stacks).
Now I would have to study when we use tcp_respond() with a INP_RLOCK() this
is a very narrow case I would imagine, but if you are not willing to get a INP_WLOCK()
that implies to me you are not changing the TCB... but thats what logging does.

I had some stray stuff in the previous diff (from the cc changes).

Address Peter Lei's comments sent to me in email:

Update the man page for mod_cc to include a new kernel configuration section listing the new
options and such.

This update gets it so we blow up if we don't have a CC module in the kernel of one sort or another.

Also forgot to mention, per the call I also bumped the module rev number
and added the lock asserts we discussed.

Per the transport call discussion I have updated things to:

1. be able to add an option for every CC you want to build in the config
2. be able to add a CC_DEFAULT (its required actually or compile will fail) to specify the default cc
3. All generic's get CC_NEWRENO and CC_DEFAULT=\"newreno\"

Address the points Lawrence makes that I agree with.

We will talk on the telchat today at 10/1am your time.. a bit late.

Note that Gleb is working on some changes to hpts though I don't think your changes conflict.

Fix a LOR as well as finish making newreno non-special. Basically we
now don't prohibit it from unloading/being a module. And we harness
the first cc module loaded/in the kernel as the default.

With help from Michael, I now know what gbe's comments mean. Fix all
sentences to have a nl.

The base stack does not maintain a sendmap. So what happens is you have
1448 byte segments .. you now must make them fit.. 1148.. so it splits each
sendmap entry into 2 segments...

In D32671#737389, @tuexen wrote:

Are you saying that you only retransmit once if the peer goes away? Could you share the packetdrill script?

Opps there is another issue here, a double bump of rtt_shift that should not be when persists happen. Only
bump it in the timer code.

Now let's update the manual pages to reflect the current state of affairs. At this
point I think I am done, so reviewers please have a look and give feedback!

This takes some of Lawrences private comments into account. We allow a CC without a size function to
exist and be loaded if it does not have a cb_init function. If there is a cb_init then we fail the load and
print a warning to the console as to why. I will need to next update the man page to talk about various
issues when loading and unloading modules.

Remove the spacing/emacs gifts.

Another fun POLA violation fixed. When unloading a module make it so that:

Fix up to sync up with the way we protect for unload. We have to hold the
lock through the time. This means a fun double lookup and possibly
restart if the size changed.

Just figured out another bit of broken-ness in the existing code. When we switch
CC modules, and we are established we need to run (if we have one) the conn_init()
function otherwise things might not go so well for the CC :)

Fix some bugs I found in the CC switching code. Next I need to test the failure and fallback with and without invariant

Cleanup a few typos in the header comments as well as lets get all
modules putting the common functions in there structure not in
the cc_mod_init() by setting a variable now that its in cc.c

This method is fine, I will withdraw my fab in favor if this :)

While where at it, there is a duplicate srtt that should be just one var at the higher scope. Lets fix
that and make the needed casts when comparing to slot. That way we never can get a
confused compiler :)