MINOR: quic: transform pacing settings into a global option

Pacing support was previously activated on each bind line individually,
via an optional argument of quic-cc-algo keyword. Remove this optional
argument and introduce a global setting to enable/disable pacing. Pacing
activation is still flagged as experimental.

One important change is that previously BBR usage automatically
activated pacing support. This is not the case anymore, so users should
now always explicitely activate pacing if BBR is selected. A new warning
message will be displayed if this is not the case.

Another consequence of this change is that now pacing_inter callback is
always defined for every quic_cc_algo types. As such, QUIC MUX uses
global.tune.options to determine if pacing is required.

This should be backported up to 3.1, after a period of observation.

doc/configuration.txt

@@ -1694,6 +1694,7 @@ The following keywords are supported in the "global" section :
    - tune.quic.reorder-ratio
    - tune.quic.retry-threshold
    - tune.quic.socket-owner
+   - tune.quic.tx-pacing
    - tune.quic.zero-copy-fwd-send
    - tune.renice.runtime
    - tune.renice.startup
@@ -4287,6 +4288,19 @@ tune.quic.socket-owner { connection | listener }
   is used globally, it will be forced on every listener instance, regardless of
   their individual configuration.
 
+tune.quic.tx-pacing { on | off }
+  Enables ('on') or disables ('off') pacing support for QUIC emission. By
+  default it is disabled. The purpose of pacing is to smooth emission of data
+  to reduce network losses. In most scenario, it can significantly improve
+  network throughput by avoiding retransmissions. However, it can be useful to
+  deactivate it for networks with very high bandwidth/low latency
+  characteristics to prevent unwanted delay and reduce CPU consumption.
+
+  Pacing support is still experimental, as such it requires
+  "expose-experimental-directives".
+
+  See also the "quic-cc-algo" bind option.
+
 tune.quic.zero-copy-fwd-send { on | off }
   Enables ('on') of disabled ('off') the zero-copy sends of data for the QUIC
   multiplexer. It is enabled by default.
@@ -17287,40 +17301,31 @@ proto <name>
 
 quic-cc-algo { cubic | newreno | bbr | nocc }[(<args,...>)]
   This is a QUIC specific setting to select the congestion control algorithm
-  for any connection attempts to the configured QUIC listeners. They are similar
-  to those used by TCP.
+  for any connection attempts to the configured QUIC listeners. They are
+  similar to those used by TCP.
+
+  Pacing can be activated on top of the congestion algorithm to reduce loss and
+  improve throughput. This is performed via "tune.quic.tx-pacing" experimental
+  global keyword. Special care is required when using BBR as it relies on
+  pacing to work as expected. Using BBR without it may cause slowdowns or high
+  loss rates during transfers. Also note that haproxy's BBR implementation is
+  also considered as experimental and cannot be enabled without
+  "expose-experimental-directives".
 
   Default value: cubic
 
-  It is possible to enable pacing if the algorithm is compatible. This is done
-  by setting an optional integer argument as described in the next paragraph.
-  The purpose of pacing is to smooth emission of data to reduce network losses.
-  In most scenario, it can significantly improve network throughput by avoiding
-  retransmissions. Pacing support is still experimental, as such it requires
-  "expose-experimental-directives". Selecting BBR congestion algorithm will
-  implicitly activate pacing as it relies on it to work as expected. Explicitly
-  disabling pacing in conjunction with BBR may cause slowdowns or high loss
-  rates during transfers. Also note that haproxy's BBR implementation is also
-  considered as experimental and cannot be enabled without
-  "expose-experimental-directives".
-
   For further customization, a list of parameters can be specified after the
   algorithm token. It must be written between parenthesis, separated by a
   comma. Each argument is optional and can be empty if needed. Here is the
   mandatory order of each parameters :
   - maximum window size in bytes. It must be greater than 10k and smaller than
     4g. By default "tune.quic.frontend.default-max-window-size" value is used.
-  - pacing activation. By default, it is set to 0, which means pacing is not
-    used. To activate it, specify a positive value. Burst size will be
-    dynamically adjusted to adapt to the network conditions.
 
   Example:
       # newreno congestion control algorithm
       quic-cc-algo newreno
       # cubic congestion control algorithm with one megabytes as window
       quic-cc-algo cubic(1m)
-      # cubic with pacing activated on top of it
-      quic-cc-algo cubic(,1)
 
   A special value "nocc" may be used to force a fixed congestion window always
   set at the maximum size. It is reserved for debugging scenarios to remove any

include/haproxy/global-t.h

@@ -87,6 +87,7 @@
 #define GTUNE_LISTENER_MQ_ANY    (GTUNE_LISTENER_MQ_FAIR | GTUNE_LISTENER_MQ_OPT)
 #define GTUNE_QUIC_CC_HYSTART    (1<<29)
 #define GTUNE_QUIC_NO_UDP_GSO    (1<<30)
+#define GTUNE_QUIC_NO_PACING     (1<<31)
 
 #define NO_ZERO_COPY_FWD             0x0001 /* Globally disable zero-copy FF */
 #define NO_ZERO_COPY_FWD_PT          0x0002 /* disable zero-copy FF for PT (recv & send are disabled automatically) */

include/haproxy/quic_cc-t.h

@@ -137,9 +137,8 @@ struct quic_cc_algo {
 	void (*state_cli)(struct buffer *buf, const struct quic_cc_path *path);
 	void (*hystart_start_round)(struct quic_cc *cc, uint64_t pn);
 
-	/* Defined only if pacing is used. */
 	uint (*pacing_inter)(const struct quic_cc *cc);
-	uint (*pacing_burst)(const struct quic_cc *cc);
+	uint (*pacing_burst)(const struct quic_cc *cc); /* only set if pacing integrated with congestion algo */
 
 	struct quic_cc_drs *(*get_drs)(struct quic_cc *cc);
 	void (*on_transmit)(struct quic_cc *cc);

src/cfgparse-quic.c

@@ -70,32 +70,6 @@ static unsigned long parse_window_size(const char *kw, char *value,
 	return 0;
 }
 
-/* Parse <value> as pacing argument.
- *
- * Returns the parsed value or a negative error code.
- */
-static int parse_pacing(const char *kw, char *value, char **end_opt, char **err)
-{
-	int pacing;
-
-	errno = 0;
-	pacing = strtoul(value, end_opt, 0);
-	if (*end_opt == value || errno != 0) {
-		memprintf(err, "'%s' : could not parse pacing value", kw);
-		goto fail;
-	}
-
-	if (!pacing) {
-		memprintf(err, "'%s' : pacing value cannot be negative", kw);
-		goto fail;
-	}
-
-	return pacing;
-
- fail:
-	return -1;
-}
-
 /* parse "quic-cc-algo" bind keyword */
 static int bind_parse_quic_cc_algo(char **args, int cur_arg, struct proxy *px,
                                    struct bind_conf *conf, char **err)
@@ -183,39 +157,6 @@ static int bind_parse_quic_cc_algo(char **args, int cur_arg, struct proxy *px,
 			arg = end_opt;
 		}
 
-		if (*++arg == ')')
-			goto out;
-
-		if (*arg != ',') {
-			int pacing = parse_pacing(args[cur_arg], arg, &end_opt, err);
-			if (pacing < 0)
-				goto fail;
-
-			if (pacing) {
-				if (!experimental_directives_allowed) {
-					memprintf(err, "'%s' : support for pacing is experimental, must be allowed via a global "
-						  "'expose-experimental-directives'\n", args[cur_arg]);
-					goto fail;
-				}
-
-				cc_algo->pacing_inter = quic_cc_default_pacing_inter;
-			}
-			else if (!(cc_algo->flags & QUIC_CC_ALGO_FL_OPT_PACING)) {
-				ha_warning("'%s' : '%s' algorithm without pacing may cause slowdowns or high loss rates during transfers\n",
-				           args[cur_arg], algo);
-				cc_algo->pacing_inter = NULL;
-			}
-
-			if (*end_opt == ')') {
-				goto out;
-			}
-			else if (*end_opt != ',') {
-				memprintf(err, "'%s' : cannot parse pacing argument for '%s' algorithm", args[cur_arg], algo);
-				goto fail;
-			}
-			arg = end_opt;
-		}
-
 		if (*++arg != ')') {
 			memprintf(err, "'%s' : too many argument for '%s' algorithm", args[cur_arg], algo);
 			goto fail;
@@ -460,6 +401,18 @@ static int cfg_parse_quic_tune_on_off(char **args, int section_type, struct prox
 		else
 			global.tune.options &= ~GTUNE_QUIC_CC_HYSTART;
 	}
+	else if (strcmp(suffix, "tune.quic.tx-pacing") == 0) {
+		if (on) {
+			if (!experimental_directives_allowed) {
+				memprintf(err, "'%s' : support for pacing is experimental, must be allowed via a global "
+				          "'expose-experimental-directives'\n", args[0]);
+				return -1;
+			}
+			global.tune.options &= ~GTUNE_QUIC_NO_PACING;
+		}
+		else
+			global.tune.options |= GTUNE_QUIC_NO_PACING;
+	}
 
 	return 0;
 }
@@ -467,6 +420,7 @@ static int cfg_parse_quic_tune_on_off(char **args, int section_type, struct prox
 static struct cfg_kw_list cfg_kws = {ILH, {
 	{ CFG_GLOBAL, "tune.quic.socket-owner", cfg_parse_quic_tune_socket_owner },
 	{ CFG_GLOBAL, "tune.quic.cc-hystart", cfg_parse_quic_tune_on_off },
+	{ CFG_GLOBAL, "tune.quic.tx-pacing", cfg_parse_quic_tune_on_off },
 	{ CFG_GLOBAL, "tune.quic.cc.cubic.min-losses", cfg_parse_quic_tune_setting },
 	{ CFG_GLOBAL, "tune.quic.frontend.conn-tx-buffers.limit", cfg_parse_quic_tune_setting },
 	{ CFG_GLOBAL, "tune.quic.frontend.glitches-threshold", cfg_parse_quic_tune_setting },

src/cfgparse.c

@@ -70,6 +70,7 @@
 #include <haproxy/sink.h>
 #include <haproxy/mailers.h>
 #include <haproxy/namespace.h>
+#include <haproxy/quic_cc-t.h>
 #include <haproxy/quic_sock.h>
 #include <haproxy/obj_type-t.h>
 #include <haproxy/openssl-compat.h>
@@ -3052,6 +3053,21 @@ int check_config_validity()
 			} /* HTTP && bufsize < 16384 */
 #endif
 
+#ifdef USE_QUIC
+			if (bind_conf->xprt == xprt_get(XPRT_QUIC)) {
+				const struct quic_cc_algo *cc_algo = bind_conf->quic_cc_algo ?
+				  bind_conf->quic_cc_algo : default_quic_cc_algo;
+
+				if (!(cc_algo->flags & QUIC_CC_ALGO_FL_OPT_PACING) &&
+				    global.tune.options & GTUNE_QUIC_NO_PACING) {
+					ha_warning("Binding [%s:%d] for %s %s: using the selected congestion algorithm without pacing may cause slowdowns or high loss rates during transfers.\n",
+					           bind_conf->file, bind_conf->line,
+					           proxy_type_str(curproxy), curproxy->id);
+					err_code |= ERR_WARN;
+				}
+			}
+#endif /* USE_QUIC */
+
 			/* finish the bind setup */
 			ret = bind_complete_thread_setup(bind_conf, &err_code);
 			if (ret != 0) {

src/haproxy.c

@@ -1375,6 +1375,7 @@ static void init_args(int argc, char **argv)
 #endif
 #ifdef USE_QUIC
 	global.tune.options |= GTUNE_QUIC_SOCK_PER_CONN;
+	global.tune.options |= GTUNE_QUIC_NO_PACING;
 #endif
 	global.tune.options |= GTUNE_STRICT_LIMITS;
 

src/mux_quic.c

@@ -40,8 +40,7 @@ static void qmux_ctrl_room(struct qc_stream_desc *, uint64_t room);
 /* Returns true if pacing should be used for <conn> connection. */
 static int qcc_is_pacing_active(const struct connection *conn)
 {
-	const struct quic_conn *qc = conn->handle.qc;
-	return !!(qc->path->cc.algo->pacing_inter);
+	return !(global.tune.options & GTUNE_QUIC_NO_PACING);
 }
 
 static void qcs_free_ncbuf(struct qcs *qcs, struct ncbuf *ncbuf)

src/quic_cc_cubic.c

@@ -687,6 +687,8 @@ struct quic_cc_algo quic_cc_algo_cubic = {
 	.event       = quic_cc_cubic_event,
 	.slow_start  = quic_cc_cubic_slow_start,
 	.hystart_start_round = quic_cc_cubic_hystart_start_round,
+	.pacing_inter = quic_cc_default_pacing_inter,
+	.pacing_burst = NULL,
 	.state_trace = quic_cc_cubic_state_trace,
 	.state_cli   = quic_cc_cubic_state_cli,
 };

src/quic_cc_newreno.c

@@ -225,6 +225,8 @@ struct quic_cc_algo quic_cc_algo_nr = {
 	.event       = quic_cc_nr_event,
 	.slow_start  = quic_cc_nr_slow_start,
 	.hystart_start_round = quic_cc_nr_hystart_start_round,
+	.pacing_inter = quic_cc_default_pacing_inter,
+	.pacing_burst = NULL,
 	.state_trace = quic_cc_nr_state_trace,
 };
 

src/quic_cc_nocc.c

@@ -5,6 +5,7 @@
  */
 
 #include <haproxy/api-t.h>
+#include <haproxy/quic_cc.h>
 #include <haproxy/quic_conn-t.h>
 #include <haproxy/quic_trace.h>
 #include <haproxy/trace.h>
@@ -71,6 +72,8 @@ struct quic_cc_algo quic_cc_algo_nocc = {
 	.flags       = QUIC_CC_ALGO_FL_OPT_PACING,
 	.init        = quic_cc_nocc_init,
 	.event       = quic_cc_nocc_event,
+	.pacing_inter = quic_cc_default_pacing_inter,
+	.pacing_burst = NULL,
 	.slow_start  = quic_cc_nocc_slow_start,
 	.state_trace = quic_cc_nocc_state_trace,
 };