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.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 0ebfd32919aac6d1a06bbd0b074007d68b80afe8..c61ba1bd640247497af3c191291d56e1e3ea475d 100644 (file)
--- a/doc/configuration.txt
+++ b/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

diff --git a/include/haproxy/global-t.h b/include/haproxy/global-t.h
index 6d4a3986c9ef43eb7882629e8642bd23f76554a4..be4fcc922036cd0fcd9f00508d8a1a4f1e097b02 100644 (file)
--- a/include/haproxy/global-t.h
+++ b/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) */

diff --git a/include/haproxy/quic_cc-t.h b/include/haproxy/quic_cc-t.h
index 59be6a3e731f05032e6fbf22f524cf0b9849f693..5f55eb2ed790af5fcfa4dc58f41036e5575f1c10 100644 (file)
--- a/include/haproxy/quic_cc-t.h
+++ b/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);

diff --git a/src/cfgparse-quic.c b/src/cfgparse-quic.c
index acb7d0d697dcc4015f2336fa4b98716308b60620..4d3a6f7ea70fe0a8a75b89c72dd4c875cd153951 100644 (file)
--- a/src/cfgparse-quic.c
+++ b/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 },

diff --git a/src/cfgparse.c b/src/cfgparse.c
index e57202a4f26707d746b55ffced1091a646d16911..b36be44eaf969937896841cea9f00aa5df5a23ec 100644 (file)
--- a/src/cfgparse.c
+++ b/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 @@ init_proxies_list_stage1:
                        } /* 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) {

diff --git a/src/haproxy.c b/src/haproxy.c
index 22a17adc7c4bed418c6f944f785b800ff1435dd4..561cab067de0aa3765f3f13bcc087f0d08e63f97 100644 (file)
--- a/src/haproxy.c
+++ b/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;
 

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 474c2983a4164e967705dbeec67f8664fd41a8ff..d8b43a913c848a712c5f97a0a602dcf75ba250cd 100644 (file)
--- a/src/mux_quic.c
+++ b/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)

diff --git a/src/quic_cc_cubic.c b/src/quic_cc_cubic.c
index 9783d8f857ba67b5110e3574565dd8dd7ebb8195..8d7b8543428460d0552e71c8c9562eeba30c679d 100644 (file)
--- a/src/quic_cc_cubic.c
+++ b/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,
 };

diff --git a/src/quic_cc_newreno.c b/src/quic_cc_newreno.c
index 989a7c1057e6574f77168fe1495bee643eab002c..400c5273f07beb3ffbd805db0a7f8dc2ca69d41a 100644 (file)
--- a/src/quic_cc_newreno.c
+++ b/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,
 };
 

diff --git a/src/quic_cc_nocc.c b/src/quic_cc_nocc.c
index b4088521b969c7ce4b1b3f6d30030ef6c9e5fb36..6f0e61c1d49588086e81070b8cd2ab458d9ec79a 100644 (file)
--- a/src/quic_cc_nocc.c
+++ b/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,
 };