Re: git: b8d60729deef - main - tcp: Congestion control cleanup.
- Reply: Warner Losh : "Re: git: b8d60729deef - main - tcp: Congestion control cleanup."
- Reply: Randall Stewart via dev-commits-src-all : "Re: git: b8d60729deef - main - tcp: Congestion control cleanup."
- In reply to: Jessica Clarke : "Re: git: b8d60729deef - main - tcp: Congestion control cleanup."
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 12 Nov 2021 17:01:42 UTC
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://cgit.FreeBSD.org/src/commit/?id=b8d60729deefa0bd13e6a395fcab4928e6e10445 >> >> 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