Re: [PATCH net-next 2/2] tcp: add NV congestion control

On 6/6/16, 4:01 PM, "David Miller" [off-list ref] wrote:

From: Lawrence Brakmo <redacted>
Date: Fri, 3 Jun 2016 13:37:58 -0700

quoted

+module_param(nv_enable, int, 0644);
+MODULE_PARM_DESC(nv_enable, "enable NV (congestion avoidance)
behavior");
+module_param(nv_pad, int, 0644);
+MODULE_PARM_DESC(nv_pad, "extra packets above congestion level");
+module_param(nv_pad_buffer, int, 0644);
+MODULE_PARM_DESC(nv_pad_buffer, "no growth buffer zone");
+module_param(nv_reset_period, int, 0644);
+MODULE_PARM_DESC(nv_reset_period, "nv_min_rtt reset period (secs)");
+module_param(nv_min_cwnd, int, 0644);
+MODULE_PARM_DESC(nv_min_cwnd, "NV will not decrease cwnd below this
value"
+		 " without losses");
+module_param(nv_dec_eval_min_calls, int, 0644);
+MODULE_PARM_DESC(nv_dec_eval_min_calls, "Wait for this many data
points"
+		 " before declaring congestion");
+module_param(nv_inc_eval_min_calls, int, 0644);
+MODULE_PARM_DESC(nv_inc_eval_min_calls, "Wait for this many data
points"
+		 " before allowing cwnd growth");
+module_param(nv_stop_rtt_cnt, int, 0644);
+MODULE_PARM_DESC(nv_stop_rtt_cnt, "Wait for this many RTTs before
stopping"
+		 " cwnd growth");
+module_param(nv_ssthresh_eval_min_calls, int, 0644);
+MODULE_PARM_DESC(nv_ssthresh_eval_min_calls, "Wait for this many data
points"
+		 " before declaring congestion during initial slow-start");
+module_param(nv_rtt_min_cnt, int, 0644);
+MODULE_PARM_DESC(nv_rtt_min_cnt, "Wait for this many RTTs before
declaring"
+		 " congestion");
+module_param(nv_cong_dec_mult, int, 0644);
+MODULE_PARM_DESC(nv_cong_dec_mult, "Congestion decrease factor");
+module_param(nv_ssthresh_factor, int, 0644);
+MODULE_PARM_DESC(nv_ssthresh_factor, "ssthresh factor");
+module_param(nv_rtt_factor, int, 0644);
+MODULE_PARM_DESC(nv_rtt_factor, "rtt averaging factor");
+module_param(nv_rtt_cnt_dec_delta, int, 0644);
+MODULE_PARM_DESC(nv_rtt_cnt_dec_delta, "decrease cwnd for this many
RTTs"
+		 " every 100 RTTs");
+module_param(nv_dec_factor, int, 0644);
+MODULE_PARM_DESC(nv_dec_factor, "decrease cwnd every ~192 RTTS by
factor/8");
+module_param(nv_loss_dec_factor, int, 0644);
+MODULE_PARM_DESC(nv_loss_dec_factor, "on loss new cwnd = cwnd * this /
1024");
+module_param(nv_cwnd_growth_rate_neg, int, 0644);
+MODULE_PARM_DESC(nv_cwnd_growth_rate_neg, "Applies when current cwnd
growth"
+		 " rate < Reno");
+module_param(nv_cwnd_growth_rate_pos, int, 0644);
+MODULE_PARM_DESC(nv_cwnd_growth_rate_pos, "Applies when current cwnd
growth"
+		 " rate >= Reno");
+module_param(nv_min_min_rtt, int, 0644);
+MODULE_PARM_DESC(nv_min_min_rtt, "lower bound for ca->nv_min_rtt");
+module_param(nv_max_min_rtt, int, 0644);
+MODULE_PARM_DESC(nv_max_min_rtt, "upper bound for ca->nv_min_rtt");

That's a disturbingly huge number of module parameters.  Even the first
one "nv_enable" is superfluous, just hook up another congestion control
algorithm.

Please trim this down to something reasonable.  The more of these you
have,
the more of them people will start wanting to use and depend upon, and
then
you're stuck with them whether you like it or not.

I will trim them down to something much smaller. My original intent was to
support experimentation, but those interested can use this version of the
patch to play with a larger number of parameters/tunables.

In regards to the parameter ³nv_enable², its function is to quickly allow
existing NV flows to revert to Reno behavior (by ³echo 0 >
/sys/module/tcp_nv/parameters/nv_enable²). The reason why this may be
necessary, is that under some conditions NV flows competing with more
aggressive flows will perform poorly. People may be more willing to test
NV if there is a quick and simple way to revert if necessary (without
having to restart the flows).

As to why NV could perform poorly, it has to do with the inherent
unfairness between flows that only decrease their cwnd when there are
losses (congestion control) and flows that decrease their cwnd when they
detect queue buildup (congestion avoidance) unless there is network
support. This is specially true when the RTTs for both types are small
(traffic within a DC), but the unfairness decreases as the RTTs of the
congestion control flows increases.

So, is it okay if I leave the parameter ³nv_enable² or would you still
prefer that I remove it?