From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=AISLkb7f; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 6C8835A061B for ; Tue, 04 Nov 2025 05:42:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1762231351; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YM6PsfR7N2teXgyJAGZKuSDYRTIpv52A0u2OOBJK08Q=; b=AISLkb7fmlkNxzDGfK4s6hy4Mq3JJZ70zgxlG5J6A+e8RfgR0gyFeyMbuiT94S5Cz/ocW7 USHDRUhmOxU8GNTuDKZXXI67rPVITZZxUGjYo3PMjQGFODAqqfKxGbhlL4N6wB30Eqla+C RFCCQHSWmIMqn4zUApFchUmdG0Nkhvs= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-318-EtjEZhlbMtuh4LQN0bTsnA-1; Mon, 03 Nov 2025 23:42:29 -0500 X-MC-Unique: EtjEZhlbMtuh4LQN0bTsnA-1 X-Mimecast-MFC-AGG-ID: EtjEZhlbMtuh4LQN0bTsnA_1762231349 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-47113538d8cso27615205e9.1 for ; Mon, 03 Nov 2025 20:42:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1762231348; x=1762836148; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=vVfggCvQ5E815zzn+LC5lz6eLc9I0os5rkdPj4Ew76Y=; b=XK+csGBySwdLkDHTGc7phSrOFeKah4Xw1DjlpYY5ofxeicnuezEpmJFcuvFYlyFQks ZYtH424+xc75X9BrY/u2+um4T6o92AQ+9dKQ48lFf3fObHCwdg3n/S2Tw3CFMXKs7S9z DMHY8U7V6RF6SHdkRnC8Nb/1GLvOCvfP6UoDbc5I3iyMVKngBvAUeiK86azD7YIvpcwd na5IDSFvmtPvK0prF4JzdOY5Zip6h4RgtkmY3EFZyVUZOa1f+EvYhtghPcbT7ntt0R1X xFBRdBJf4CY3QCjT5YeD3yHTfnw9MHImAmXEtoQ3jJ8s89VbQazWmrMX+YG+DMBoJ2B8 W3Og== X-Forwarded-Encrypted: i=1; AJvYcCUI2ETYO1mppjPFvG9vQQGS5UQQ5CIf3k98QBErWiyFyVd2vj6MTOj8m4msDv/4g6Kj56UEcHKLkX8=@passt.top X-Gm-Message-State: AOJu0Yy3hmTC6sEpdqcdpFAsI3GII9+/vCLAGBg+QeQmDuATsBJilXux yny33pQ1ssXZT4wgDhyo/vNXX5sX5ljXcQtL2cZS52oHp7IrjlsnR4nJXqPEqAwIiZ0D8AEk2kG +AlA4Pj5PhIX0hzj/l9AkDUsPp4FeO4/dNSuVpHLsCLLqbA/U8Eoybw== X-Gm-Gg: ASbGncv448N/WFk7MjBP2/T0W92EBoDwySTScW5l6OBFBQX00hdiXAGEkBkEY6TVnzn ne/CKB+KIn2mj05E0jW/x89BMFql6QqBQHU8D4FRVSgXDUr9agSyOKQ03r4vue6L0kueB3Obcg4 Ylf0K+o+dLx14pZ0jG5wKELnSdGtpfix7JrumtgpsvMQL4ZgT6aAyWCaXUv5zin1QrP3LQONv6s X+Psg+jmylLW2SWj3UOr6mUPKQyyeVOwHEsm8SlZbEXbdl13bcughqIhC/BZZIDOQjHrqhQD4Cp CnmPa/huNe195hGXax2DMicj7Fpqqmpn7XhxNKPjBJP8t63FRrfawykH9WE4apkwmxfBS+gFNPA 4XLOx3Ncoiw== X-Received: by 2002:a05:600c:8b8c:b0:471:b5d:2db9 with SMTP id 5b1f17b1804b1-477308ab095mr135256455e9.21.1762231348370; Mon, 03 Nov 2025 20:42:28 -0800 (PST) X-Google-Smtp-Source: AGHT+IGOAYCMay1cgIOafMW1e7MNiQ1Ud3/k4N/FhJjoqexu4QICCecy69wbQ/H50GF/oBXeAIGzhA== X-Received: by 2002:a05:600c:8b8c:b0:471:b5d:2db9 with SMTP id 5b1f17b1804b1-477308ab095mr135256265e9.21.1762231347862; Mon, 03 Nov 2025 20:42:27 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4773c3a77b1sm191530145e9.17.2025.11.03.20.42.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Nov 2025 20:42:26 -0800 (PST) Date: Tue, 4 Nov 2025 05:42:25 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v7 4/5] tcp: Update data retransmission timeout Message-ID: <20251104054225.2b2a5a8c@elisabeth> In-Reply-To: References: <20251031054242.7334-1-yuhuang@redhat.com> <20251031054242.7334-5-yuhuang@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: P2bJi7JTxduwhkfbdDkdSp7Y_f29Y3ksqddOXf5KdTM_1762231349 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: RY63TGERQGLI6QBXFBH74LAWBOPURHTT X-Message-ID-Hash: RY63TGERQGLI6QBXFBH74LAWBOPURHTT X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Yumei Huang , passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon, 3 Nov 2025 20:32:14 +1100 David Gibson wrote: > On Mon, Nov 03, 2025 at 10:57:27AM +0800, Yumei Huang wrote: > > On Mon, Nov 3, 2025 at 9:38=E2=80=AFAM David Gibson wrote: =20 > > > > > > On Fri, Oct 31, 2025 at 01:42:41PM +0800, Yumei Huang wrote: =20 > > > > Use an exponential backoff timeout for data retransmission accordin= g > > > > to RFC 2988 and RFC 6298. Set the initial RTO to one second as disc= ussed > > > > in Appendix A of RFC 6298. > > > > > > > > Also combine the macros defining the initial RTO for both SYN and A= CK. > > > > > > > > Signed-off-by: Yumei Huang > > > > Reviewed-by: David Gibson =20 > > > > > > As reported, the carried over R-b was a minor mistake, since the code > > > has changed, but here's a new one: > > > > > > Reviewed-by: David Gibson > > > > > > Small comment below, though. > > > =20 > > > > --- > > > > tcp.c | 30 ++++++++++++------------------ > > > > 1 file changed, 12 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/tcp.c b/tcp.c > > > > index bada88a..96ee56a 100644 > > > > --- a/tcp.c > > > > +++ b/tcp.c > > > > @@ -179,16 +179,13 @@ > > > > * > > > > * Timeouts are implemented by means of timerfd timers, set based = on flags: > > > > * > > > > - * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest during= handshake > > > > - * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this= time, resend > > > > - * SYN. It's the starting timeout for the first SYN retry. Retry= for > > > > - * TCP_MAX_RETRIES or (tcp_syn_retries + tcp_syn_linear_timeouts= ) times, > > > > - * reset the connection > > > > - * > > > > - * - ACK_TIMEOUT: if no ACK segment was received from tap/guest, a= fter sending > > > > - * data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re-send = data from the > > > > - * socket and reset sequence to what was acknowledged. If this p= ersists for > > > > - * more than TCP_MAX_RETRIES times in a row, reset the connectio= n > > > > + * - RTO_INIT: if no ACK segment was received from tap/guest, eith= er during > > > > + * handshake (flag ACK_FROM_TAP_DUE without ESTABLISHED event) o= r after > > > > + * sending data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), = re-send data > > > > + * from the socket and reset sequence to what was acknowledged. = This is the > > > > + * timeout for the first retry, in seconds. Retry for TCP_MAX_RE= TRIES times > > > > + * for established connections, or (tcp_syn_retries + > > > > + * tcp_syn_linear_timeouts) times during the handshake, reset th= e connection > > > > * > > > > * - FIN_TIMEOUT: if a FIN segment was sent to tap/guest (flag ACK= _FROM_TAP_DUE > > > > * with TAP_FIN_SENT event), and no ACK is received within this = time, reset > > > > @@ -342,8 +339,7 @@ enum { > > > > #define WINDOW_DEFAULT 14600 /* RF= C 6928 */ > > > > > > > > #define ACK_INTERVAL 10 /* ms */ > > > > -#define SYN_TIMEOUT_INIT 1 /* s, RFC 692= 8 */ > > > > -#define ACK_TIMEOUT 2 > > > > +#define RTO_INIT 1 /* s, RFC 629= 8 */ > > > > #define FIN_TIMEOUT 60 > > > > #define ACT_TIMEOUT 7200 > > > > > > > > @@ -589,12 +585,10 @@ static void tcp_timer_ctl(const struct ctx *c= , struct tcp_tap_conn *conn) > > > > if (conn->flags & ACK_TO_TAP_DUE) { > > > > it.it_value.tv_nsec =3D (long)ACK_INTERVAL * 1000 * 1= 000; > > > > } else if (conn->flags & ACK_FROM_TAP_DUE) { > > > > - if (!(conn->events & ESTABLISHED)) { > > > > - int exp =3D conn->retries - c->tcp.syn_linear= _timeouts; =20 > > > > > > I didn't spot it in the previous patch, but this is (theoretically) > > > buggy. conn->retries is unsigned, so the subtraction will be > > > performed unsigned and only then cast to signed. I think that will > > > probably do the right thing in practice, but I don't think that's > > > guaranteed by the C standard (and might even be UB). =20 > >=20 > > I'm not sure, but I just googled it. IIUC, the uint8_t (conn->retries > > and c->tcp.syn_linear_timeouts) will go through integer promotion > > before subtraction. So the line is like: > >=20 > > int exp =3D (int) conn->retries - (int) c->tcp.syn_linear_timeouts; > >=20 > > Please correct me if I'm wrong. =20 >=20 > Huh, I thought it would only be promoted if one of the operands was an > int. I thought and I think so too, yes. > But C promotion rules are really confusing, so I could well be wrong. Some references linked at: https://archives.passt.top/passt-dev/20240103080834.24fa0a7a@elisabeth/ and here, it's about paragraph 6.3.1.8, "Usual arithmetic conversions" of C11 (that's what passt uses right now). No double or float here, so this is the relevant text: Otherwise, the integer promotions are performed on both operands. Then the following rules are applied to the promoted operands: If both operands have the same type, then no further conversion is needed now, are they really the same type? The standard doesn't define uint8_t. If we see it as an unsigned int, with storage limited to 8 bits, then I would call them the same type. If we see c->tcp.syn_linear_timeouts as a character type instead (6.3.1.1, "Boolean, characters, and integers"), then the integer conversion rank of unsigned int (conn->retries) is greater than the rank of char, and the same text applies (promotion of char to unsigned int). But I'm fairly sure that's not the case. We used uint8_t, not short, and not char. All in all I don't think there's any possibility of promotion to a signed int: for that, you would need one of the operands to be signed. Or two unsigned chars/shorts (see examples below). If the operation is performed unsigned, and that's what gcc appears to do here, promoting both operands to unsigned int (32-bit): --- $ cat pro_uh_oh_motion.c #include #include int main() { =09uint8_t a =3D 0; =09struct { unsigned b:3; } x =3D { 7 }; =09printf("%u %i\n", a - x.b, a - x.b); } $ gcc -o pro_uh_oh_motion{,.c} $ ./pro_uh_oh_motion 4294967289 -7 --- ...do we really have a problem with that? To me it looks like the behaviour is defined, and it's what we want. These pages: https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer= +conversion+rules https://wiki.sei.cmu.edu/confluence/display/c/EXP14-C.+Beware+of+integer+= promotion+when+performing+bitwise+operations+on+integer+types+smaller+than+= int show some problematic examples. This one is close to our case: unsigned short x =3D 45000, y =3D 50000; unsigned int z =3D x * y; but there we have implicit promotion of the unsigned shorts to int, which doesn't happen in our case, because we already have an unsigned int. See also the examples with two unsigned shorts from: https://cryptoservices.github.io/fde/2018/11/30/undefined-behavior.html and there the operation would be performed as signed int. On top of that, you have the fact that the original types can't store the result of the operation. Here it's not the case. --=20 Stefano