Bug 238478
| Summary: | TCP Cubic code bug in cubic_ack_received | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Vidhi Goel <Goel.vidhi07> |
| Component: | kern | Assignee: | Richard Scheffenegger <rscheff> |
| Status: | Closed FIXED | ||
| Severity: | Affects Many People | CC: | net, rgrimes, rscheff |
| Priority: | --- | Flags: | koobs:
mfc-stable12+
|
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
Yes, this is the correct way to file a code audit issue. Thanks. Since having fixed numerous cubic related bugs, looking further into this. Currently working to validate the fix in D24657 Actually, concave and convex regions are defined with respect to the previous cwnd (max_cwnd in the code), not the currently used cwnd. The conditional in that space is there to prevent cwnd from shrinking, should it have outgrown the currently calculated w_cubic_next value (e.g. during slow-start). So, after a close inspection, showing the cwnd to follow both convex and concave trajectories properly, I believe the current code is correct. As there are some style issues, and the comment could be expanded around this, will commit a patch against this PR nevertheless. A commit references this bug: Author: rscheff Date: Tue Jul 21 16:21:53 UTC 2020 New revision: 363397 URL: https://svnweb.freebsd.org/changeset/base/363397 Log: Fix style and comment around concave/convex regions in TCP cubic. In cubic, the concave region is when snd_cwnd starts growing slower towards max_cwnd (cwnd at the time of the congestion event), and the convex region is when snd_cwnd starts to grow faster and eventually appearing like slow-start like growth. PR: 238478 Reviewed by: tuexen (mentor), rgrimes (mentor) Approved by: tuexen (mentor), rgrimes (mentor) MFC after: 2 weeks Sponsored by: NetApp, Inc. Differential Revision: https://reviews.freebsd.org/D24657 Changes: head/sys/netinet/cc/cc_cubic.c A commit references this bug: Author: rscheff Date: Wed Aug 19 10:40:03 UTC 2020 New revision: 364378 URL: https://svnweb.freebsd.org/changeset/base/364378 Log: MFC r363397: Fix style and comment around concave/convex regions in TCP cubic. In cubic, the concave region is when snd_cwnd starts growing slower towards max_cwnd (cwnd at the time of the congestion event), and the convex region is when snd_cwnd starts to grow faster and eventually appearing like slow-start like growth. PR: 238478 Reviewed by: tuexen (mentor), rgrimes (mentor) Sponsored by: NetApp, Inc. Differential Revision: https://reviews.freebsd.org/D24657 Changes: _U stable/12/ stable/12/sys/netinet/cc/cc_cubic.c |
I am not sure if this is the right way to file a code audit issue. Recently I was referring to TCP Cubic source code and found one problem in function cubic_ack_received. 1. The first condition checks whether the computed cubic window is less than TCP friendly window and sets accordingly 2. Else, it checks whether the current cwnd is less than the computed cubic window. This is the concave region but the freebsd code considers this as concave or convex. This is incorrect as it would fail to set cwnd when (cwnd > w_cubic_next) According to the draft, cwnd should be set to (W_cubic_next - cwnd)/cwnd in both concave and convex region. So the code should look like this: if (w_cubic_next < w_tf) { set cwnd <- w_tf } else if (cwnd < w_cubic_next) { // This is concave region set cwnd <- (W_cubic_next - cwnd)/cwnd } else { // This is convex region set cwnd <- (W_cubic_next - cwnd)/cwnd } Or if (w_cubic_next < w_tf) { set cwnd <- w_tf } else { set cwnd <- (W_cubic_next - cwnd)/cwnd }