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

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