Date: Fri, 12 Nov 2021 10:09:07 -0700 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: Jessica Clarke <jrtc27@freebsd.org>, Randall 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: <CANCZdforHrLw4JqQhA1gtkJqmiCqCUUbJDWGj3kJLCbjq1GHng@mail.gmail.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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Nov 12, 2021 at 10:01 AM 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://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. > Yes. my universe is running now. https://reviews.freebsd.org/D32964 is what I'm testing, if people want to follow along at home or help refine it while the first universe is run... Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdforHrLw4JqQhA1gtkJqmiCqCUUbJDWGj3kJLCbjq1GHng>