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

On Thu, Nov 11, 2021 at 11:31:17AM +0000, Randall Stewart 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.
>     
>     Reviewed by: Michael Tuexen
>     Sponsored by: Netflix Inc.
>     RELNOTES:YES
>     Differential Revision: https://reviews.freebsd.org/D32693

Hi Randall,

This change causes a panic when starting up a vnet jail.
V_default_cc_ptr is NULL in the new vnet, it doesn't automatically get
copied from the parent vnet.  I wrote the patch below and it allows my
system to boot, but it's just a hack and I'm not sure what the right
policy is.

diff --git a/sys/netinet/cc/cc.c b/sys/netinet/cc/cc.c
index 0a61aff37c96..f5ec10e487cf 100644
--- a/sys/netinet/cc/cc.c
+++ b/sys/netinet/cc/cc.c
@@ -309,6 +309,23 @@ cc_register_algo(struct cc_algo *add_cc)
 	return (err);
 }
 
+static void
+vnet_cc_sysinit(void *arg)
+{
+	struct cc_algo *cc;
+
+	if (IS_DEFAULT_VNET(curvnet))
+		return;
+
+	CURVNET_SET(vnet0);
+	cc = V_default_cc_ptr;
+	CURVNET_RESTORE();
+
+	V_default_cc_ptr = cc;
+}
+VNET_SYSINIT(vnet_cc_sysinit, SI_SUB_PROTO_IFATTACHDOMAIN, SI_ORDER_ANY,
+    vnet_cc_sysinit, NULL);
+
 /*
  * Perform any necessary tasks before we exit congestion recovery.
  */