From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202412 header.b=Hqu0f3xk; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id DDD855A061B for ; Wed, 22 Jan 2025 02:09:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202412; t=1737508167; bh=F4jd+w9fAtqDSWQ9vCS4nOq8S0fkg2Ps01Vx92vAyK4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Hqu0f3xk0p5nmL49yIe8sEKLrnBvG9pyHz1P7hmnTVaGECSiG/BuiMpBBmIeuhslT ng+MzQKMD31bMxjbsDKIUaFrlDfCoqwTLnJYY30F3HaiRtMT1tJRVWrhy/J5wupZJT P8nl5N8o7FJFsLDeLAbCtm81hm84SJdSDf/LbELLCI6hL3wbRzS1Z54+Xo3S2kj9L5 3HG9LBcOkH/M5jmmYHrjKTEkl+B9AVMIPagnwT9TrRuJqEuKsuO8y1+UOZjg/kBFe6 rzqemME9kv1PMe+V4WlXRCYmmZBIH8LriLTRediKY+VD74yVtPJySng+7FrvAaJgtD tuevwAAKT7FrA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Yd5Zb1Pnjz4xCy; Wed, 22 Jan 2025 12:09:27 +1100 (AEDT) Date: Wed, 22 Jan 2025 11:36:05 +1030 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] tcp: Set PSH flag for last incoming packets in a batch Message-ID: References: <20250120234958.3247370-1-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="R/nUY6PjVVcXKnst" Content-Disposition: inline In-Reply-To: <20250120234958.3247370-1-sbrivio@redhat.com> Message-ID-Hash: W7SALIUPU3B4RNQB7Y5YR5NB42N3YXQL X-Message-ID-Hash: W7SALIUPU3B4RNQB7Y5YR5NB42N3YXQL X-MailFrom: dgibson@gandalf.ozlabs.org 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: 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: --R/nUY6PjVVcXKnst Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 21, 2025 at 12:49:58AM +0100, Stefano Brivio wrote: > So far we omitted setting PSH flags for inbound traffic altogether: as > we ignore the nature of the data we're sending, we can't conclude that > some data is more or less urgent. This works fine with Linux guests, > as the Linux kernel doesn't do much with it, on input: it will > generally deliver data to the application layer without delay. >=20 > However, with Windows, things change: if we don't set the PSH flag on > interactive inbound traffic, we can expect long delays before the data > is delivered to the application. >=20 > This is very visible with RDP, where packets we send on behalf of the > RDP client are delivered with delays exceeding one second: >=20 > $ tshark -r rdp.pcap -td -Y 'frame.number in { 33170 .. 33173 }' --disa= ble-protocol tls > 33170 0.030296 93.235.154.248 =E2=86=92 88.198.0.164 54 TCP 49012 =E2= =86=92 3389 [ACK] Seq=3D13820 Ack=3D285229 Win=3D387968 Len=3D0 > 33171 0.985412 88.198.0.164 =E2=86=92 93.235.154.248 105 TCP 3389 =E2= =86=92 49012 [PSH, ACK] Seq=3D285229 Ack=3D13820 Win=3D63198 Len=3D51 > 33172 0.030373 93.235.154.248 =E2=86=92 88.198.0.164 54 TCP 49012 =E2= =86=92 3389 [ACK] Seq=3D13820 Ack=3D285280 Win=3D387968 Len=3D0 > 33173 1.383776 88.198.0.164 =E2=86=92 93.235.154.248 424 TCP 3389 =E2= =86=92 49012 [PSH, ACK] Seq=3D285280 Ack=3D13820 Win=3D63198 Len=3D370 >=20 > in this example (packet capture taken by passt), frame #33172 is a > mouse event sent by the RDP client, and frame #33173 is the first > event (display reacting to click) sent back by the server. This > appears as a 1.4 s delay before we get frame #33173. >=20 > If we set PSH, instead: >=20 > $ tshark -r rdp_psh.pcap -td -Y 'frame.number in { 314 .. 317 }' --disa= ble-protocol tls > 314 0.002503 93.235.154.248 =E2=86=92 88.198.0.164 170 TCP 51066 =E2= =86=92 3389 [PSH, ACK] Seq=3D7779 Ack=3D74047 Win=3D31872 Len=3D116 > 315 0.000557 88.198.0.164 =E2=86=92 93.235.154.248 54 TCP 3389 =E2=86= =92 51066 [ACK] Seq=3D79162 Ack=3D7895 Win=3D62872 Len=3D0 > 316 0.012752 93.235.154.248 =E2=86=92 88.198.0.164 170 TCP 51066 =E2= =86=92 3389 [PSH, ACK] Seq=3D7895 Ack=3D79162 Win=3D31872 Len=3D116 > 317 0.011927 88.198.0.164 =E2=86=92 93.235.154.248 107 TCP 3389 =E2= =86=92 51066 [PSH, ACK] Seq=3D79162 Ack=3D8011 Win=3D62756 Len=3D53 >=20 > here, in frame #116, our mouse event is delivered without a delay and > receives a response in approximately 12 ms. >=20 > Set PSH on the last segment for any batch we dequeue from the socket, > that is, set it whenever we know that we might not be sending data to > the same port for a while. Huh, fascinating. > Reported-by: NN708 > Link: https://bugs.passt.top/show_bug.cgi?id=3D107 > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson > --- > tcp_buf.c | 11 ++++++++--- > tcp_vu.c | 7 +++++-- > 2 files changed, 13 insertions(+), 5 deletions(-) >=20 > diff --git a/tcp_buf.c b/tcp_buf.c > index cbefa42..72d99c5 100644 > --- a/tcp_buf.c > +++ b/tcp_buf.c > @@ -239,9 +239,10 @@ int tcp_buf_send_flag(const struct ctx *c, struct tc= p_tap_conn *conn, int flags) > * @dlen: TCP payload length > * @no_csum: Don't compute IPv4 checksum, use the one from previous buff= er > * @seq: Sequence number to be sent > + * @push: Set PSH flag, last segment in a batch > */ > static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *co= nn, > - ssize_t dlen, int no_csum, uint32_t seq) > + ssize_t dlen, int no_csum, uint32_t seq, bool push) > { > struct tcp_payload_t *payload; > const uint16_t *check =3D NULL; > @@ -268,6 +269,7 @@ static void tcp_data_to_tap(const struct ctx *c, stru= ct tcp_tap_conn *conn, > payload->th.th_x2 =3D 0; > payload->th.th_flags =3D 0; > payload->th.ack =3D 1; > + payload->th.psh =3D push; > iov[TCP_IOV_PAYLOAD].iov_len =3D dlen + sizeof(struct tcphdr); > tcp_l2_buf_fill_headers(conn, iov, check, seq, false); > if (++tcp_payload_used > TCP_FRAMES_MEM - 1) > @@ -402,11 +404,14 @@ int tcp_buf_data_from_sock(const struct ctx *c, str= uct tcp_tap_conn *conn) > seq =3D conn->seq_to_tap; > for (i =3D 0; i < send_bufs; i++) { > int no_csum =3D i && i !=3D send_bufs - 1 && tcp_payload_used; > + bool push =3D false; > =20 > - if (i =3D=3D send_bufs - 1) > + if (i =3D=3D send_bufs - 1) { > dlen =3D last_len; > + push =3D true; > + } > =20 > - tcp_data_to_tap(c, conn, dlen, no_csum, seq); > + tcp_data_to_tap(c, conn, dlen, no_csum, seq, push); > seq +=3D dlen; > } > =20 > diff --git a/tcp_vu.c b/tcp_vu.c > index a216bb1..fad7065 100644 > --- a/tcp_vu.c > +++ b/tcp_vu.c > @@ -289,10 +289,11 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, > * @iov_cnt: Number of entries in @iov > * @check: Checksum, if already known > * @no_tcp_csum: Do not set TCP checksum > + * @push: Set PSH flag, last segment in a batch > */ > static void tcp_vu_prepare(const struct ctx *c, struct tcp_tap_conn *con= n, > struct iovec *iov, size_t iov_cnt, > - const uint16_t **check, bool no_tcp_csum) > + const uint16_t **check, bool no_tcp_csum, bool push) > { > const struct flowside *toside =3D TAPFLOW(conn); > bool v6 =3D !(inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr)); > @@ -334,6 +335,7 @@ static void tcp_vu_prepare(const struct ctx *c, struc= t tcp_tap_conn *conn, > memset(th, 0, sizeof(*th)); > th->doff =3D sizeof(*th) / 4; > th->ack =3D 1; > + th->psh =3D push; > =20 > tcp_fill_headers(conn, NULL, ip4h, ip6h, th, &payload, > *check, conn->seq_to_tap, no_tcp_csum); > @@ -443,6 +445,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct= tcp_tap_conn *conn) > struct iovec *iov =3D &elem[head[i]].in_sg[0]; > int buf_cnt =3D head[i + 1] - head[i]; > ssize_t dlen =3D iov_size(iov, buf_cnt) - hdrlen; > + bool push =3D i =3D=3D head_cnt - 1; > =20 > vu_set_vnethdr(vdev, iov->iov_base, buf_cnt); > =20 > @@ -451,7 +454,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct= tcp_tap_conn *conn) > check =3D NULL; > previous_dlen =3D dlen; > =20 > - tcp_vu_prepare(c, conn, iov, buf_cnt, &check, !*c->pcap); > + tcp_vu_prepare(c, conn, iov, buf_cnt, &check, !*c->pcap, push); > =20 > if (*c->pcap) { > pcap_iov(iov, buf_cnt, --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --R/nUY6PjVVcXKnst Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmeQRG4ACgkQzQJF27ox 2GctDg/8CAaiAluLJ7dYgACQ2mmdxG9rsw3uzlHUc9u7iDZ4a1bvGhSPQ3kOHQ8+ WQ2GJtYlV/gW7svpvoJPYXZhOANoHGy7Ocom0ZnSdeklvirSUneUx7KiRO5GAsJE Pj+1KSXesvCutGwbxr9aUUMdDf2glvBqEQrAHY1dCJmFC0pEqoj8MTFqCwPDhwJZ XOodvxK3PpT+7A6sG/E2E44+fnMK3PpBDh7t7Qta4yEO58NsCzAZ6HlZG1wBqGZR teQ0FZk9l/SXkfqouuzzl9cFUffnYxhbPdNo6C9fEkVXhWragItGfwpk7tCtJzxM 7wRiYDhkVgdVm3Zt8d/Wtd7PcknPmaBH35yzGMkPD18L8l7MDttsH7gsyEg87HWl czK3p31fYYt6Z/Fwrwdp/cUsyheNlKn2sWmCpc8VqJ0ElXiGpncFd4SQOTmOR7OQ NUwKIPuz6iyPD98D5CnZsHxAiob+ff6EYyA+K1dfHeS4nC895/em68EDrHOlPZsU IqMKO8D+ZyWIc1Vo9hks9nV6uBts2LUhPBXWb+WZtl/WHo+R322oShizT9oiuWVJ TcMONn3Zngz9hM3SkpfFHfZ97s56ZfFcz2B14gw95YEbBYPxY44jDnaYTJStDiP5 hj9ViCodLXEc4hWfHqdRcbGbX8yeKYiUrALzfNmmrIv/VKslVGY= =8wJd -----END PGP SIGNATURE----- --R/nUY6PjVVcXKnst--