Message ID | 1584524612-24470-25-git-send-email-ilpo.jarvinen@helsinki.fi |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | : Accurate ECN for TCP | expand |
On 3/18/20 2:43 AM, Ilpo Järvinen wrote: > From: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > > As SACK blocks tend to eat all option space when there are > many holes, it is useful to compromise on sending many SACK > blocks in every ACK and try to fit AccECN option there > by reduction the number of SACK blocks. But never go below > two SACK blocks because of AccECN option. > > As AccECN option is often not put to every ACK, the space > hijack is usually only temporary. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > --- > net/ipv4/tcp_output.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 4cc590a47f43..0aec2c57a9cc 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -756,6 +756,21 @@ static int tcp_options_fit_accecn(struct tcp_out_options *opts, int required, > if (opts->num_ecn_bytes < required) > return 0; Have you tested this patch ? (You forgot to remove the prior 2 lines) > > + if (opts->num_ecn_bytes < required) { > + if (opts->num_sack_blocks > 2) { > + /* Try to fit the option by removing one SACK block */ > + opts->num_sack_blocks--; > + size = tcp_options_fit_accecn(opts, required, > + remaining + TCPOLEN_SACK_PERBLOCK, > + max_combine_saving); > + if (opts->options & OPTION_ACCECN) > + return size - TCPOLEN_SACK_PERBLOCK; > + > + opts->num_sack_blocks++; > + } > + return 0; > + } > + > opts->options |= OPTION_ACCECN; > return size; > } >
On Wed, 18 Mar 2020, Eric Dumazet wrote: > > > On 3/18/20 2:43 AM, Ilpo Järvinen wrote: > > From: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > > > > As SACK blocks tend to eat all option space when there are > > many holes, it is useful to compromise on sending many SACK > > blocks in every ACK and try to fit AccECN option there > > by reduction the number of SACK blocks. But never go below > > two SACK blocks because of AccECN option. > > > > As AccECN option is often not put to every ACK, the space > > hijack is usually only temporary. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi> > > --- > > net/ipv4/tcp_output.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > > index 4cc590a47f43..0aec2c57a9cc 100644 > > --- a/net/ipv4/tcp_output.c > > +++ b/net/ipv4/tcp_output.c > > @@ -756,6 +756,21 @@ static int tcp_options_fit_accecn(struct tcp_out_options *opts, int required, > > if (opts->num_ecn_bytes < required) > > return 0; > > Have you tested this patch ? > > (You forgot to remove the prior 2 lines) > > > > > + if (opts->num_ecn_bytes < required) { Yes and no. There was no unit test for this particular condition but I added a few now (with and w/o timestamps). I also managed to find and fix a byte-order related bug related to non-fullsized option while making those tests. (I didn't actually forget to remove it. I managed to add the problem during a botched conflict merge when I reorganized some of the code.) Thanks for taking a look.
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 4cc590a47f43..0aec2c57a9cc 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -756,6 +756,21 @@ static int tcp_options_fit_accecn(struct tcp_out_options *opts, int required, if (opts->num_ecn_bytes < required) return 0; + if (opts->num_ecn_bytes < required) { + if (opts->num_sack_blocks > 2) { + /* Try to fit the option by removing one SACK block */ + opts->num_sack_blocks--; + size = tcp_options_fit_accecn(opts, required, + remaining + TCPOLEN_SACK_PERBLOCK, + max_combine_saving); + if (opts->options & OPTION_ACCECN) + return size - TCPOLEN_SACK_PERBLOCK; + + opts->num_sack_blocks++; + } + return 0; + } + opts->options |= OPTION_ACCECN; return size; }