| Message ID | 20240815214035.1145228-4-mrzhang97@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp_cubic: fix to achieve at least the same throughput as Reno | expand |
On Thu, Aug 15, 2024 at 5:41 PM Mingrui Zhang <mrzhang97@gmail.com> wrote: > > The original code estimates RENO snd_cwnd using the estimated > RENO snd_cwnd at the current time (i.e., tcp_cwnd). > > The patched code estimates RENO snd_cwnd using the estimated > RENO snd_cwnd after one RTT (i.e., tcp_cwnd_next_rtt), > because ca->cnt is used to increase snd_cwnd for the next RTT. > > Fixes: 89b3d9aaf467 ("[TCP] cubic: precompute constants") > Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com> > Signed-off-by: Lisong Xu <xu@unl.edu> > --- > v2->v3: Corrent the "Fixes:" footer content > v1->v2: Separate patches > > net/ipv4/tcp_cubic.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c > index 7bc6db82de66..a1467f99a233 100644 > --- a/net/ipv4/tcp_cubic.c > +++ b/net/ipv4/tcp_cubic.c > @@ -315,8 +315,11 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked) > ca->tcp_cwnd++; > } > > - if (ca->tcp_cwnd > cwnd) { /* if bic is slower than tcp */ > - delta = ca->tcp_cwnd - cwnd; > + /* Reno cwnd one RTT in the future */ > + u32 tcp_cwnd_next_rtt = ca->tcp_cwnd + (ca->ack_cnt + cwnd) / delta; > + > + if (tcp_cwnd_next_rtt > cwnd) { /* if bic is slower than Reno */ > + delta = tcp_cwnd_next_rtt - cwnd; > max_cnt = cwnd / delta; > if (ca->cnt > max_cnt) > ca->cnt = max_cnt; > -- I'm getting a compilation error with clang: net/ipv4/tcp_cubic.c:322:7: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement] u32 tcp_cwnd_next_rtt = ca->tcp_cwnd + (ca->ack_cnt + cwnd) / delta; Can you please try something like the following: - u32 scale = beta_scale; + u32 scale = beta_scale, tcp_cwnd_next_rtt; ... + tcp_cwnd_next_rtt = ca->tcp_cwnd + (ca->ack_cnt + cwnd) / delta; Thanks! neal
On 8/16/24 13:32, Neal Cardwell wrote: > On Thu, Aug 15, 2024 at 5:41 PM Mingrui Zhang <mrzhang97@gmail.com> wrote: >> The original code estimates RENO snd_cwnd using the estimated >> RENO snd_cwnd at the current time (i.e., tcp_cwnd). >> >> The patched code estimates RENO snd_cwnd using the estimated >> RENO snd_cwnd after one RTT (i.e., tcp_cwnd_next_rtt), >> because ca->cnt is used to increase snd_cwnd for the next RTT. >> >> Fixes: 89b3d9aaf467 ("[TCP] cubic: precompute constants") >> Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com> >> Signed-off-by: Lisong Xu <xu@unl.edu> >> --- >> v2->v3: Corrent the "Fixes:" footer content >> v1->v2: Separate patches >> >> net/ipv4/tcp_cubic.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c >> index 7bc6db82de66..a1467f99a233 100644 >> --- a/net/ipv4/tcp_cubic.c >> +++ b/net/ipv4/tcp_cubic.c >> @@ -315,8 +315,11 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked) >> ca->tcp_cwnd++; >> } >> >> - if (ca->tcp_cwnd > cwnd) { /* if bic is slower than tcp */ >> - delta = ca->tcp_cwnd - cwnd; >> + /* Reno cwnd one RTT in the future */ >> + u32 tcp_cwnd_next_rtt = ca->tcp_cwnd + (ca->ack_cnt + cwnd) / delta; >> + >> + if (tcp_cwnd_next_rtt > cwnd) { /* if bic is slower than Reno */ >> + delta = tcp_cwnd_next_rtt - cwnd; >> max_cnt = cwnd / delta; >> if (ca->cnt > max_cnt) >> ca->cnt = max_cnt; >> -- > I'm getting a compilation error with clang: > > net/ipv4/tcp_cubic.c:322:7: error: mixing declarations and code > is incompatible with standards before C99 > [-Werror,-Wdeclaration-after-statement] > u32 tcp_cwnd_next_rtt = ca->tcp_cwnd + (ca->ack_cnt + cwnd) / delta; > > Can you please try something like the following: > > - u32 scale = beta_scale; > + u32 scale = beta_scale, tcp_cwnd_next_rtt; > ... > + tcp_cwnd_next_rtt = ca->tcp_cwnd + (ca->ack_cnt + cwnd) / delta; > > Thanks! > neal Thank you, Neal, We have tried your suggested changes, and they also work for our compile and experiment tests. We did not find this issue because we have only tried to compile with the default Makefile with GCC. I agree with your changes, it is conform to the existing codes and compatible with that standards. Thanks, Mingrui
diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c index 7bc6db82de66..a1467f99a233 100644 --- a/net/ipv4/tcp_cubic.c +++ b/net/ipv4/tcp_cubic.c @@ -315,8 +315,11 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked) ca->tcp_cwnd++; } - if (ca->tcp_cwnd > cwnd) { /* if bic is slower than tcp */ - delta = ca->tcp_cwnd - cwnd; + /* Reno cwnd one RTT in the future */ + u32 tcp_cwnd_next_rtt = ca->tcp_cwnd + (ca->ack_cnt + cwnd) / delta; + + if (tcp_cwnd_next_rtt > cwnd) { /* if bic is slower than Reno */ + delta = tcp_cwnd_next_rtt - cwnd; max_cnt = cwnd / delta; if (ca->cnt > max_cnt) ca->cnt = max_cnt;
The original code estimates RENO snd_cwnd using the estimated RENO snd_cwnd at the current time (i.e., tcp_cwnd). The patched code estimates RENO snd_cwnd using the estimated RENO snd_cwnd after one RTT (i.e., tcp_cwnd_next_rtt), because ca->cnt is used to increase snd_cwnd for the next RTT. Fixes: 89b3d9aaf467 ("[TCP] cubic: precompute constants") Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com> Signed-off-by: Lisong Xu <xu@unl.edu> --- v2->v3: Corrent the "Fixes:" footer content v1->v2: Separate patches net/ipv4/tcp_cubic.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)