Re: git: b8d60729deef - main - tcp: Congestion control cleanup.

John:

This is fine to do, but I want to make sure everyone understands that
I was specifically asked to make compile fail on the transport call
if CC_XXXX or CC_DEFAULT was not defined. Its not how I had the code
originally but it was requested specifically.

I am fine with all the changes aka it showing up in DEFAULT that’s a
good solution. 

And I think Warner’s patch with an ifndef in cc.c works perfectly that
way if you are say netapp and don’t use newreno you can do a

nooptions CC_NEWRENO
options CC_CUBIC
options CC_DEFAULT=\”cubic\”

And it all just works for you ;)

R

> On Nov 12, 2021, at 12:01 PM, John Baldwin <jhb@freebsd.org> wrote:
> 
> On 11/12/21 8:19 AM, Jessica Clarke wrote:
>> On 11 Nov 2021, at 11:31, Randall Stewart <rrs@FreeBSD.org> wrote:
>>> 
>>> The branch main has been updated by rrs:
>>> 
>>> URL: https://www.google.com/url?q=https://cgit.FreeBSD.org/src/commit/?id%3Db8d60729deefa0bd13e6a395fcab4928e6e10445&source=gmail-imap&ust=1637341308000000&usg=AOvVaw0ox_w6BYoj1_WxabaN9-lG
>>> 
>>> commit b8d60729deefa0bd13e6a395fcab4928e6e10445
>>> Author:     Randall Stewart <rrs@FreeBSD.org>
>>> AuthorDate: 2021-11-11 11:28:18 +0000
>>> Commit:     Randall Stewart <rrs@FreeBSD.org>
>>> CommitDate: 2021-11-11 11:28:18 +0000
>>> 
>>>    tcp: Congestion control cleanup.
>>> 
>>>    NOTE: HEADS UP read the note below if your kernel config is not including GENERIC!!
>>> 
>>>    This patch does a bit of cleanup on TCP congestion control modules. There were some rather
>>>    interesting surprises that one could get i.e. where you use a socket option to change
>>>    from one CC (say cc_cubic) to another CC (say cc_vegas) and you could in theory get
>>>    a memory failure and end up on cc_newreno. This is not what one would expect. The
>>>    new code fixes this by requiring a cc_data_sz() function so we can malloc with M_WAITOK
>>>    and pass in to the init function preallocated memory. The CC init is expected in this
>>>    case *not* to fail but if it does and a module does break the
>>>    "no fail with memory given" contract we do fall back to the CC that was in place at the time.
>>> 
>>>    This also fixes up a set of common newreno utilities that can be shared amongst other
>>>    CC modules instead of the other CC modules reaching into newreno and executing
>>>    what they think is a "common and understood" function. Lets put these functions in
>>>    cc.c and that way we have a common place that is easily findable by future developers or
>>>    bug fixers. This also allows newreno to evolve and grow support for its features i.e. ABE
>>>    and HYSTART++ without having to dance through hoops for other CC modules, instead
>>>    both newreno and the other modules just call into the common functions if they desire
>>>    that behavior or roll there own if that makes more sense.
>>> 
>>>    Note: This commit changes the kernel configuration!! If you are not using GENERIC in
>>>    some form you must add a CC module option (one of CC_NEWRENO, CC_VEGAS, CC_CUBIC,
>>>    CC_CDG, CC_CHD, CC_DCTCP, CC_HTCP, CC_HD). You can have more than one defined
>>>    as well if you desire. Note that if you create a kernel configuration that does not
>>>    define a congestion control module and includes INET or INET6 the kernel compile will
>>>    break. Also you need to define a default, generic adds 'options CC_DEFAULT=\"newreno\"
>>>    but you can specify any string that represents the name of the CC module (same names
>>>    that show up in the CC module list under net.inet.tcp.cc). If you fail to add the
>>>    options CC_DEFAULT in your kernel configuration the kernel build will also break.
>> Not doing so breaks tinderbox, as well as configs not hooks up to
>> tinderbox. I don’t think this is acceptable.
> 
> We discussed this a bit on IRC, but I think in this case the default CC_*
> options belong in DEFAULTS like the default GEOM_PART_* options rather
> than in GENERIC.  (Though we mostly avoid changing DEFAULTS, this is one
> of the rare cases when I think it makes sense.)  Handling the default for
> CC_DEFAULT does not work in DEFAULTS since you can't later override it,
> but that could be handled by simply having the default for CC_DEFAULT live
> in the code itself under an #ifndef instead.
> 
> I think Warner is already testing a patchset to make this change.
> 
> -- 
> John Baldwin

------
Randall Stewart
rrs@netflix.com