Date: Sun, 14 Nov 2021 10:03:06 -0500 From: Randall Stewart via dev-commits-src-all <dev-commits-src-all@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Jessica Clarke <jrtc27@freebsd.org>, Randall Ray Stewart <rrs@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: b8d60729deef - main - tcp: Congestion control cleanup. Message-ID: <5A60AA15-F560-44D9-89A0-BD0A197E5E58@netflix.com> In-Reply-To: <2a564b11-b1f4-a4fe-745b-27f45fb134eb@FreeBSD.org> References: <202111111131.1ABBVH6s017371@gitrepo.freebsd.org> <DDB6E732-7054-4C57-ADFD-4534CBAD5109@freebsd.org> <2a564b11-b1f4-a4fe-745b-27f45fb134eb@FreeBSD.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] 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 [-- Attachment #2 --] 0 *H 010 `He 0 *H 6020 s&0 *H 0G10URandall Stewart1 0 *H rrs@netflix.com1 0 UUS0 210124122014Z 260124122014Z0G10URandall Stewart1 0 *H rrs@netflix.com1 0 UUS0"0 *H 0 ݧEc}=ӷ y}|V(Qb5r/5Zľ4(N淪@W;{U;b2A`b6x:A H ,%vg Ag6rH!\>Dp V@{) &r)>#bmK:qkӷަf %ÐN-n S>;T_a&t~ f<sd?pɋPT63LEC 0 0 *H/ 0 U 0 *H r_mcb3Oj0{V[%4S-? g_M8=ڸ@ ұLS@ I9o¦Ep/ht Һ,ng*1wH}`QC,y3Cf! dh} 9<c8;8˰3d V 3bAV hҙvcɻtQoYܗ. { " o* jjH100U0G10URandall Stewart1 0 *H rrs@netflix.com1 0 UUS s&0 `He i0 *H 1 *H 0 *H 1 211114150306Z0/ *H 1" QNw<=Skuw?ʈ "0 *H QL ðj3ƊTc351'ќXRi5\68V=@ S( *}iBd8WWEZF"pkv`^ТC_nY5Iq'2ح]wL7vpy{`[lE1.3q OSQ؎::i ,QR^rc`3ˏWA3kǧ (fJpQ~&^Ahelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5A60AA15-F560-44D9-89A0-BD0A197E5E58>