| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 10 Dec 2022 21:35:03 +0800
From: Weiping Zhang <zwp10758@...il.com>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: Neal Cardwell <ncardwell@...gle.com>,
Weiping Zhang <zhangweiping@...iglobal.com>,
edumazet@...gle.com, davem@...emloft.net, yoshfuji@...ux-ipv6.org,
dsahern@...nel.org, kuba@...nel.org, pabeni@...hat.com,
netdev@...r.kernel.org
Subject: Re: [RFC PATCH] tcp: correct srtt and mdev_us calculation
Sorry to reply to late caused by bad cold,
On Tue, Dec 6, 2022 at 5:29 PM Jakub Sitnicki <jakub@...udflare.com> wrote:
>
> On Mon, Dec 05, 2022 at 02:15 PM -05, Neal Cardwell wrote:
> > On Mon, Dec 5, 2022 at 1:02 PM Weiping Zhang
> > <zhangweiping@...iglobal.com> wrote:
> >>
> >> From the comments we can see that, rtt = 7/8 rtt + 1/8 new,
> >> but there is an mistake,
> >>
> >> m -= (srtt >> 3);
> >> srtt += m;
> >>
> >> explain:
> >> m -= (srtt >> 3); //use t stands for new m
> >> t = m - srtt/8;
> >>
> >> srtt = srtt + t
> >> = srtt + m - srtt/8
> >> = srtt 7/8 + m
> >>
> >> Test code:
> >>
> >> #include<stdio.h>
> >>
> >> #define u32 unsigned int
> >>
> >> static void test_old(u32 srtt, long mrtt_us)
> >> {
> >> long m = mrtt_us;
> >> u32 old = srtt;
> >>
> >> m -= (srtt >> 3);
> >> srtt += m;
> >>
> >> printf("%s old_srtt: %u mrtt_us: %ld new_srtt: %u\n", __func__, old, mrtt_us, srtt);
> >> }
> >>
> >> static void test_new(u32 srtt, long mrtt_us)
> >> {
> >> long m = mrtt_us;
> >> u32 old = srtt;
> >>
> >> m = ((m - srtt) >> 3);
> >> srtt += m;
> >>
> >> printf("%s old_srtt: %u mrtt_us: %ld new_srtt: %u\n", __func__, old, mrtt_us, srtt);
> >> }
> >>
> >> int main(int argc, char **argv)
> >> {
> >> u32 srtt = 100;
> >> long mrtt_us = 90;
> >>
> >> test_old(srtt, mrtt_us);
> >> test_new(srtt, mrtt_us);
> >>
> >> return 0;
> >> }
> >>
> >> ./a.out
> >> test_old old_srtt: 100 mrtt_us: 90 new_srtt: 178
> >> test_new old_srtt: 100 mrtt_us: 90 new_srtt: 98
> >>
> >> Signed-off-by: Weiping Zhang <zhangweiping@...iglobal.com>
> >
> > Please note that this analysis and this test program do not take
> > account of the fact that srtt in the Linux kernel is maintained in a
> > form where it is shifted left by 3 bits, to maintain a 3-bit
> > fractional part. That is why at first glance it would seem there is a
> > missing multiplication of the new sample by 1/8. By not shifting the
> > new sample when it is added to srtt, the new sample is *implicitly*
> > multiplied by 1/8.
>
> Nifty. And it's documented.
>
> struct tcp_sock {
> …
> u32 srtt_us; /* smoothed round trip time << 3 in usecs */
>
> Thanks for the hint.
>
> >> net/ipv4/tcp_input.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> >> index 0640453fce54..0242bb31e1ce 100644
> >> --- a/net/ipv4/tcp_input.c
> >> +++ b/net/ipv4/tcp_input.c
> >> @@ -848,7 +848,7 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
As comments by Neal, srtt use lowest 3bits for fraction part, the
@mrtt_us does not include fraction part,
so m - srtt is not suitable, since m -= (srtt >> 3) will get the delta
which exclude fraction part, and m will also be
used to calculate mdev_us.
if we calculate srtt by: srtt = srtt *7/8 + (mrtt_us << 3)* 1 / 8,
it's more readable, append 3bits 0 for fraction part.
srtt = srtt *7/8 + (mrtt_us << 3)* 1 / 8
srtt *7/8 => srtt - srtt >> 3
(mrtt_us << 3)* 1 / 8 => mrtt_us
then
srtt = srtt - srtt >> 3 + mrtt_us;
srtt = srtt + mrtt_us - srtt >> 3
it's same as current code^_^
my previous patch does not consider the fraction part, it generates a
wrong result.
I write a new test code to decode the fraction part:
test_old old_srtt: 100 => 12.4 mrtt_us: 90
new_srtt: 178 => 22.2
test_new old_srtt: 100 => 12.4 mrtt_us: 90
new_srtt: 98 => 12.2
#include<stdio.h>
#include<stdlib.h>
#define u32 unsigned int
static void test_old(u32 srtt, long mrtt_us)
{
long m = mrtt_us;
u32 old = srtt;
m -= (srtt >> 3);
srtt += m;
printf("%s old_srtt: %8u => %8u.%u mrtt_us: %8ld new_srtt: %8u
=> %8u.%u\n", __func__, old, old >> 3, old & 7, mrtt_us, srtt,
srtt>>3, srtt & 7);
//printf("%s old_srtt>>3: %8u mrtt_us: %8ld new_srtt>>3: %u\n",
__func__, old>>3, mrtt_us, srtt>>3);
}
static void test_new(u32 srtt, long mrtt_us)
{
long m = mrtt_us;
u32 old = srtt;
m = ((m - srtt) >> 3);
srtt += m;
printf("%s old_srtt: %8u => %8u.%u mrtt_us: %8ld new_srtt: %8u
=> %8u.%u\n", __func__, old, old >> 3, old & 7, mrtt_us, srtt,
srtt>>3, srtt & 7);
//printf("%s old_srtt>>3: %8u mrtt_us: %8ld new_srtt>>3: %u\n",
__func__, old>>3, mrtt_us, srtt>>3);
}
int main(int argc, char **argv)
{
u32 srtt = atoi(argv[1]);
long mrtt_us = atoi(argv[2]);
test_old(srtt, mrtt_us);
test_new(srtt, mrtt_us);
return 0;
}
> >> * that VJ failed to avoid. 8)
> >> */
> >> if (srtt != 0) {
> >> - m -= (srtt >> 3); /* m is now error in rtt est */
> >> + m = (m - srtt >> 3); /* m is now error in rtt est */
> >> srtt += m; /* rtt = 7/8 rtt + 1/8 new */
> >> if (m < 0) {
> >> m = -m; /* m is now abs(error) */
> >> @@ -864,7 +864,7 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
> >> if (m > 0)
> >> m >>= 3;
> >> } else {
> >> - m -= (tp->mdev_us >> 2); /* similar update on mdev */
> >> + m = (m - tp->mdev_us >> 2); /* similar update on mdev */
> >> }
> >> tp->mdev_us += m; /* mdev = 3/4 mdev + 1/4 new */
> >> if (tp->mdev_us > tp->mdev_max_us) {
> >> --
> >> 2.34.1
> >
> > AFAICT this proposed patch does not change the behavior of the code
> > but merely expresses the same operations with slightly different
> > syntax. Am I missing something? :-)
>
> I've been wondering about that too. There's a change hiding behind
> operator precedence. Would be better expressed with explicitly placed
> parenthesis:
>
> m = (m - srtt) >> 3; /* m is now error in rtt est */
Powered by blists - more mailing lists