| Message ID | 20240815001718.2845791-2-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 Wed, Aug 14, 2024 at 8:19 PM Mingrui Zhang <mrzhang97@gmail.com> wrote: > > The original code bypasses bictcp_update() under certain conditions > to reduce the CPU overhead. Intuitively, when last_cwnd==cwnd, > bictcp_update() is executed 32 times per second. As a result, > it is possible that bictcp_update() is not executed for several > RTTs when RTT is short (specifically < 1/32 second = 31 ms and > last_cwnd==cwnd which may happen in small-BDP networks), > thus leading to low throughput in these RTTs. > > The patched code executes bictcp_update() 32 times per second > if RTT > 31 ms or every RTT if RTT < 31 ms, when last_cwnd==cwnd. > > Thanks > Mingrui, and Lisong > > Fixes: 91a4599c2ad8 ("tcp_cubic: fix to run bictcp_update() at least once per RTT") The Fixes: footer is not for restating the commit title in your new patch. Instead, it should list the SHA1 and first commit message line from the old commit (potentially years ago) that has buggy behavior that you are fixing. That way the Linux maintainers know which Linux releases have the bug, and they know how far back in release history the fix should be applied, when backporting fixes to stable releases. More information: https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes Please update the Fixes footers in the three commits and repost. :-) Thanks! neal > Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com> > Signed-off-by: Lisong Xu <xu@unl.edu> > > --- > net/ipv4/tcp_cubic.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c > index 5dbed91c6178..11bad5317a8f 100644 > --- a/net/ipv4/tcp_cubic.c > +++ b/net/ipv4/tcp_cubic.c > @@ -218,8 +218,12 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked) > > ca->ack_cnt += acked; /* count the number of ACKed packets */ > > + /* Update 32 times per second if RTT > 1/32 second, > + * every RTT if RTT < 1/32 second > + * even when last_cwnd == cwnd > + */ > if (ca->last_cwnd == cwnd && > - (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32) > + (s32)(tcp_jiffies32 - ca->last_time) <= min(HZ / 32, usecs_to_jiffies(ca->delay_min))) > return; > > /* The CUBIC function can update ca->cnt at most once per jiffy. > -- > 2.34.1 >
On Wed, Aug 14, 2024 at 8:19 PM Mingrui Zhang <mrzhang97@gmail.com> wrote: > > The original code bypasses bictcp_update() under certain conditions > to reduce the CPU overhead. Intuitively, when last_cwnd==cwnd, > bictcp_update() is executed 32 times per second. As a result, > it is possible that bictcp_update() is not executed for several > RTTs when RTT is short (specifically < 1/32 second = 31 ms and > last_cwnd==cwnd which may happen in small-BDP networks), > thus leading to low throughput in these RTTs. > > The patched code executes bictcp_update() 32 times per second > if RTT > 31 ms or every RTT if RTT < 31 ms, when last_cwnd==cwnd. > > Thanks > Mingrui, and Lisong Another note: please remove these commit message lines like this with "Thanks" and your names, from each of the 3 commits. Please run "git log" in your git Linux working directory to get a sense of the conventions for Linux commit messages. :-) Thanks! neal > Fixes: 91a4599c2ad8 ("tcp_cubic: fix to run bictcp_update() at least once per RTT") > Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com> > Signed-off-by: Lisong Xu <xu@unl.edu> > > --- > net/ipv4/tcp_cubic.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c > index 5dbed91c6178..11bad5317a8f 100644 > --- a/net/ipv4/tcp_cubic.c > +++ b/net/ipv4/tcp_cubic.c > @@ -218,8 +218,12 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked) > > ca->ack_cnt += acked; /* count the number of ACKed packets */ > > + /* Update 32 times per second if RTT > 1/32 second, > + * every RTT if RTT < 1/32 second > + * even when last_cwnd == cwnd > + */ > if (ca->last_cwnd == cwnd && > - (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32) > + (s32)(tcp_jiffies32 - ca->last_time) <= min(HZ / 32, usecs_to_jiffies(ca->delay_min))) > return; > > /* The CUBIC function can update ca->cnt at most once per jiffy. > -- > 2.34.1 >
On 8/15/24 09:43, Neal Cardwell wrote: > On Wed, Aug 14, 2024 at 8:19 PM Mingrui Zhang <mrzhang97@gmail.com> wrote: >> The original code bypasses bictcp_update() under certain conditions >> to reduce the CPU overhead. Intuitively, when last_cwnd==cwnd, >> bictcp_update() is executed 32 times per second. As a result, >> it is possible that bictcp_update() is not executed for several >> RTTs when RTT is short (specifically < 1/32 second = 31 ms and >> last_cwnd==cwnd which may happen in small-BDP networks), >> thus leading to low throughput in these RTTs. >> >> The patched code executes bictcp_update() 32 times per second >> if RTT > 31 ms or every RTT if RTT < 31 ms, when last_cwnd==cwnd. >> >> Thanks >> Mingrui, and Lisong >> >> Fixes: 91a4599c2ad8 ("tcp_cubic: fix to run bictcp_update() at least once per RTT") > The Fixes: footer is not for restating the commit title in your new > patch. Instead, it should list the SHA1 and first commit message line > from the old commit (potentially years ago) that has buggy behavior > that you are fixing. That way the Linux maintainers know which Linux > releases have the bug, and they know how far back in release history > the fix should be applied, when backporting fixes to stable releases. > > More information: > https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > Please update the Fixes footers in the three commits and repost. :-) > > Thanks! > neal > Thank you again, Neal. I appreciate your detailed instructions on Fixes footer. I misunderstood its meaning. Thanks Mingrui >> Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com> >> Signed-off-by: Lisong Xu <xu@unl.edu> >> >> --- >> net/ipv4/tcp_cubic.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c >> index 5dbed91c6178..11bad5317a8f 100644 >> --- a/net/ipv4/tcp_cubic.c >> +++ b/net/ipv4/tcp_cubic.c >> @@ -218,8 +218,12 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked) >> >> ca->ack_cnt += acked; /* count the number of ACKed packets */ >> >> + /* Update 32 times per second if RTT > 1/32 second, >> + * every RTT if RTT < 1/32 second >> + * even when last_cwnd == cwnd >> + */ >> if (ca->last_cwnd == cwnd && >> - (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32) >> + (s32)(tcp_jiffies32 - ca->last_time) <= min(HZ / 32, usecs_to_jiffies(ca->delay_min))) >> return; >> >> /* The CUBIC function can update ca->cnt at most once per jiffy. >> -- >> 2.34.1 >>
diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c index 5dbed91c6178..11bad5317a8f 100644 --- a/net/ipv4/tcp_cubic.c +++ b/net/ipv4/tcp_cubic.c @@ -218,8 +218,12 @@ static inline void bictcp_update(struct bictcp *ca, u32 cwnd, u32 acked) ca->ack_cnt += acked; /* count the number of ACKed packets */ + /* Update 32 times per second if RTT > 1/32 second, + * every RTT if RTT < 1/32 second + * even when last_cwnd == cwnd + */ if (ca->last_cwnd == cwnd && - (s32)(tcp_jiffies32 - ca->last_time) <= HZ / 32) + (s32)(tcp_jiffies32 - ca->last_time) <= min(HZ / 32, usecs_to_jiffies(ca->delay_min))) return; /* The CUBIC function can update ca->cnt at most once per jiffy.
The original code bypasses bictcp_update() under certain conditions to reduce the CPU overhead. Intuitively, when last_cwnd==cwnd, bictcp_update() is executed 32 times per second. As a result, it is possible that bictcp_update() is not executed for several RTTs when RTT is short (specifically < 1/32 second = 31 ms and last_cwnd==cwnd which may happen in small-BDP networks), thus leading to low throughput in these RTTs. The patched code executes bictcp_update() 32 times per second if RTT > 31 ms or every RTT if RTT < 31 ms, when last_cwnd==cwnd. Thanks Mingrui, and Lisong Fixes: 91a4599c2ad8 ("tcp_cubic: fix to run bictcp_update() at least once per RTT") Signed-off-by: Mingrui Zhang <mrzhang97@gmail.com> Signed-off-by: Lisong Xu <xu@unl.edu> --- net/ipv4/tcp_cubic.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)