From: Amaury Denoyelle Date: Thu, 21 Nov 2024 09:41:42 +0000 (+0100) Subject: MINOR: cfgparse-quic: activate pacing only via burst argument X-Git-Tag: v3.1-dev14~28 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=de86fd1e6cc30e0a2fae216e7f3d0f608bcbe379;p=thirdparty%2Fhaproxy.git 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 5c32f907d0..b45fcf0731 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -17206,7 +17206,7 @@ proto 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 }[()] +quic-cc-algo { cubic | newreno | bbr | nocc }[()] 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 }[()] 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 }[()] 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 }[()] # 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 3729bee7c9..5d55eaf736 100644 --- 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 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;