MINOR: cfgparse-quic: activate pacing only via burst argument

Recently, pacing support was added for cubic congestion algorithm. This
was activated by using the new token "cubic-pacing" on quic-cc-algo.
Furthermore, it was possible to define a burst size with a new
parameters after congestion token between parenthesis.

This configuration is not oblivious to users. In particular, it can
cause to easily forgot to tweak burst size, which can dramatically
impact performance.

Simplify this by removing the extra "-pacing" suffix. Now, pacing will
be activated solely based on the burst parameter. If 0, burst is
considered as infinite and no pacing will be used. Pacing will be
activating for any positive burst. This better reflects the link between
pacing and burst and its importance.

Note that for the moment, if burst is specified, it will be ignored with
a warning for algorithm outside of cubic.

This is not a breaking change as pacing support was implemented in the
current dev version.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 5c32f907d01c93e1bda634fc7264d4cae67f6e13..b45fcf07316756f4aacb0ce825c44f60cb01d31b 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -17206,7 +17206,7 @@ proto <name>
   instance, it is possible to force the http/2 on clear TCP by specifying "proto
   h2" on the bind line.
 
-quic-cc-algo { cubic[-pacing] | newreno | bbr | nocc }[(<args,...>)]
+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.
@@ -17214,15 +17214,15 @@ quic-cc-algo { cubic[-pacing] | newreno | bbr | nocc }[(<args,...>)]
   Default value: cubic
 
   It is possible to active pacing if the algorithm is compatible. This is done
-  by using the suffix "-pacing" after the algorithm name. Pacing purpose is to
-  smooth emission of data without burst to reduce network loss. In some
-  scenario, it can significantly improve network throughput. However, it can
-  also increase CPU usage if haproxy is forced to wait too long between each
-  emission. Pacing support is still experimental, as such it requires
-  "expose-experimental-directives". BBR congestion control algorithm depends on
-  the pacing support which is in this case implicitely activated by "bbr".
-  Note that BBR haproxy implementation is still considered as experimental and
-  cannot be enabled without "expose-experimental-directives".
+  by specifying optional burst argument as described in the next paragraph.
+  Pacing purpose is to smooth emission of data without burst to reduce network
+  loss. In some scenario, it can significantly improve network throughput.
+  However, it can also increase CPU usage if haproxy is forced to wait too long
+  between each emission. Pacing support is still experimental, as such it
+  requires "expose-experimental-directives". BBR congestion control algorithm
+  depends on the pacing support which is in this case implicitely activated by
+  "bbr". Note that BBR haproxy implementation is still 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
@@ -17230,8 +17230,9 @@ quic-cc-algo { cubic[-pacing] | newreno | bbr | nocc }[(<args,...>)]
   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.
-  - count of datagrams to emit in a burst if pacing is activated. It must be
-    between the default value of 1 and 1024.
+  - burst size in bytes. By default, it is set to 0, which means unlimited. A
+    positive value up to 1024 can be specified to smooth emission with pacing.
+    See above paragraph for more explanation.
 
   Example:
       # newreno congestion control algorithm
@@ -17239,7 +17240,7 @@ quic-cc-algo { cubic[-pacing] | newreno | bbr | nocc }[(<args,...>)]
       # cubic congestion control algorithm with one megabytes as window
       quic-cc-algo cubic(1m)
       # cubic with pacing on top of it, with burst limited to 12 datagrams
-      quic-cc-algo cubic-pacing(,12)
+      quic-cc-algo cubic(,12)
 
   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/src/cfgparse-quic.c b/src/cfgparse-quic.c
index 3729bee7c9c25db74440fcf483a5330a3c216533..5d55eaf736380e2b89cbc09993b7b44f85d185b6 100644 (file)
--- a/src/cfgparse-quic.c
+++ b/src/cfgparse-quic.c
@@ -72,11 +72,11 @@ static unsigned long parse_window_size(const char *kw, char *value,
 
 /* Parse <value> as a number of datagrams allowed for burst.
  *
- * Returns the parsed value or 0 on error.
+ * Returns the parsed value or a negative error code.
  */
-static uint parse_burst(const char *kw, char *value, char **end_opt, char **err)
+static int parse_burst(const char *kw, char *value, char **end_opt, char **err)
 {
-       uint burst;
+       int burst;
 
        errno = 0;
        burst = strtoul(value, end_opt, 0);
@@ -85,15 +85,15 @@ static uint parse_burst(const char *kw, char *value, char **end_opt, char **err)
                goto fail;
        }
 
-       if (!burst || burst > 1024) {
-               memprintf(err, "'%s' : pacing burst value must be between 1 and 1024", kw);
+       if (burst < 0 || burst > 1024) {
+               memprintf(err, "'%s' : pacing burst value must be between 0 and 1024", kw);
                goto fail;
        }
 
        return burst;
 
  fail:
-       return 0;
+       return -1;
 }
 
 /* parse "quic-cc-algo" bind keyword */
@@ -101,7 +101,6 @@ static int bind_parse_quic_cc_algo(char **args, int cur_arg, struct proxy *px,
                                    struct bind_conf *conf, char **err)
 {
        struct quic_cc_algo *cc_algo = NULL;
-       const char *str_pacing = "-pacing";
        const char *algo = NULL;
        char *arg;
 
@@ -128,19 +127,6 @@ static int bind_parse_quic_cc_algo(char **args, int cur_arg, struct proxy *px,
                algo = QUIC_CC_CUBIC_STR;
                *cc_algo = quic_cc_algo_cubic;
                arg += strlen(QUIC_CC_CUBIC_STR);
-
-               if (strncmp(arg, str_pacing, strlen(str_pacing)) == 0) {
-                       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_rate = quic_cc_default_pacing_rate;
-                       cc_algo->pacing_burst = quic_cc_default_pacing_burst;
-                       conf->quic_pacing_burst = 1;
-                       arg += strlen(str_pacing);
-               }
        }
        else if (strncmp(arg, QUIC_CC_BBR_STR, strlen(QUIC_CC_BBR_STR)) == 0) {
                if (!experimental_directives_allowed) {
@@ -198,11 +184,25 @@ static int bind_parse_quic_cc_algo(char **args, int cur_arg, struct proxy *px,
                        goto out;
 
                if (*arg != ',') {
-                       uint burst = parse_burst(args[cur_arg], arg, &end_opt, err);
-                       if (!burst)
+                       int burst = parse_burst(args[cur_arg], arg, &end_opt, err);
+                       if (burst < 0)
                                goto fail;
 
-                       conf->quic_pacing_burst = burst;
+                       if (burst && cc_algo->type == QUIC_CC_ALGO_TP_CUBIC) {
+                               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;
+                               }
+
+                               conf->quic_pacing_burst = burst;
+                               cc_algo->pacing_rate = quic_cc_default_pacing_rate;
+                               cc_algo->pacing_burst = quic_cc_default_pacing_burst;
+                       }
+                       else {
+                               ha_warning("'%s' : burst parameter ignored for '%s' congestion algorithm\n",
+                                          args[cur_arg], algo);
+                       }
 
                        if (*end_opt == ')') {
                                goto out;