From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 015A35A0304 for ; Sat, 01 Jun 2024 08:38:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1717223904; bh=Ily/ZE3FW3spR3OzaHqH9lWWVOKmZNZvYp/1DMval8I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Rd4NTHKRrbAUTpxiRMol/+v6WVi+6I66trTVIpdXpc3dAi7f3Z/60BF05rdatG8Tt BYvbtBIw8z5eiYP6pcJmPmrZDpZDsPbEvs3hU26wbk4T/GMcs8hLh92RBVuzsrDLol oazRjLwXQKbt2BR4c7r+nMmViPqa7QtISG3bn/eT/ih4c6QyJaFzpErUSaBosjarza viS9nzywC9zKcxaXJ2qTe+hwqS0vuRwapO8GhgNf9toh5zdgxqzP33bmam1UbVRXw4 bQjtrq/rLmf2vooFO6JpGFmM/5O3hfGBD6w+aawr6tH+S+6FeboDAi0KCur4pxa1vn PUGxZJa5oY5iQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Vrr0c6XTvz4xFK; Sat, 1 Jun 2024 16:38:24 +1000 (AEST) Date: Sat, 1 Jun 2024 15:43:08 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v4 02/10] tcp: extract buffer management from tcp_send_flag() Message-ID: References: <20240531142344.1420034-1-lvivier@redhat.com> <20240531142344.1420034-3-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QYc7HdbeqIDJc6lB" Content-Disposition: inline In-Reply-To: <20240531142344.1420034-3-lvivier@redhat.com> Message-ID-Hash: EXVDEJPYQQBPHHB6C57Q52FNFRJZLUUK X-Message-ID-Hash: EXVDEJPYQQBPHHB6C57Q52FNFRJZLUUK 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: --QYc7HdbeqIDJc6lB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, May 31, 2024 at 04:23:36PM +0200, Laurent Vivier wrote: I think this needs a commit message. In particular expanding on what you mean by "buffer management" would be helpful. > Signed-off-by: Laurent Vivier > --- > tcp.c | 87 ++++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 54 insertions(+), 33 deletions(-) >=20 > diff --git a/tcp.c b/tcp.c > index 6f221995f3bc..a6f43010f58f 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -1518,24 +1518,25 @@ static void tcp_update_seqack_from_tap(const stru= ct ctx *c, > } > =20 > /** > - * tcp_send_flag() - Send segment with flags to tap (no payload) > + * tcp_fill_flag_header() - Prepare header for flags-only segment (no pa= yload) > * @c: Execution context > * @conn: Connection pointer > * @flags: TCP flags: if not set, send segment only if ACK is due > + * @th: TCP header to update > + * @data: buffer to store TCP option > + * @optlen: size of the TCP option buffer > * > - * Return: negative error code on connection reset, 0 otherwise > + * Return: < 0 error code on connection reset, > + * 0 if there is no flag to send > + * 1 otherwise > */ > -static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int f= lags) > +static int tcp_fill_flag_header(struct ctx *c, struct tcp_tap_conn *conn, > + int flags, struct tcphdr *th, char *data, > + size_t *optlen) > { > - struct tcp_flags_t *payload; > struct tcp_info tinfo =3D { 0 }; > socklen_t sl =3D sizeof(tinfo); > int s =3D conn->sock; > - size_t optlen =3D 0; > - struct tcphdr *th; > - struct iovec *iov; > - size_t l4len; > - char *data; > =20 > if (SEQ_GE(conn->seq_ack_to_tap, conn->seq_from_tap) && > !flags && conn->wnd_to_tap) > @@ -1557,20 +1558,11 @@ static int tcp_send_flag(struct ctx *c, struct tc= p_tap_conn *conn, int flags) > if (!tcp_update_seqack_wnd(c, conn, flags, &tinfo) && !flags) > return 0; > =20 > - if (CONN_V4(conn)) > - iov =3D tcp4_l2_flags_iov[tcp4_flags_used++]; > - else > - iov =3D tcp6_l2_flags_iov[tcp6_flags_used++]; > - > - payload =3D iov[TCP_IOV_PAYLOAD].iov_base; > - th =3D &payload->th; > - data =3D payload->opts; > - > if (flags & SYN) { > int mss; > =20 > /* Options: MSS, NOP and window scale (8 bytes) */ > - optlen =3D OPT_MSS_LEN + 1 + OPT_WS_LEN; > + *optlen =3D OPT_MSS_LEN + 1 + OPT_WS_LEN; > =20 > *data++ =3D OPT_MSS; > *data++ =3D OPT_MSS_LEN; > @@ -1604,26 +1596,13 @@ static int tcp_send_flag(struct ctx *c, struct tc= p_tap_conn *conn, int flags) > flags |=3D ACK; > } > =20 > - th->doff =3D (sizeof(*th) + optlen) / 4; > + th->doff =3D (sizeof(*th) + *optlen) / 4; > =20 > th->ack =3D !!(flags & ACK); > th->rst =3D !!(flags & RST); > th->syn =3D !!(flags & SYN); > th->fin =3D !!(flags & FIN); > =20 > - if (CONN_V4(conn)) { > - l4len =3D tcp_fill_headers4(c, conn, iov[TCP_IOV_TAP].iov_base, > - iov[TCP_IOV_IP].iov_base, > - iov[TCP_IOV_PAYLOAD].iov_base, optlen, > - NULL, conn->seq_to_tap); > - } else { > - l4len =3D tcp_fill_headers6(c, conn, iov[TCP_IOV_TAP].iov_base, > - iov[TCP_IOV_IP].iov_base, > - iov[TCP_IOV_PAYLOAD].iov_base, optlen, > - conn->seq_to_tap); > - } > - iov[TCP_IOV_PAYLOAD].iov_len =3D l4len; > - > if (th->ack) { > if (SEQ_GE(conn->seq_ack_to_tap, conn->seq_from_tap)) > conn_flag(c, conn, ~ACK_TO_TAP_DUE); > @@ -1638,6 +1617,48 @@ static int tcp_send_flag(struct ctx *c, struct tcp= _tap_conn *conn, int flags) > if (th->fin || th->syn) > conn->seq_to_tap++; > =20 > + return 1; > +} > + > +static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int f= lags) > +{ > + struct tcp_flags_t *payload; > + size_t optlen =3D 0; > + struct iovec *iov; > + size_t l4len; > + int ret; > + > + if (CONN_V4(conn)) { > + iov =3D tcp4_l2_flags_iov[tcp4_flags_used++]; Hrm.. it does occur to me that if you avoided the previous patch, keeping a single dispatcher for filling in the IP headers, then setting the iov pointer would become the only v4/v6 dependent part of this function, helping us down the road of unifying that more. Obviously there would be a separate CONN_v4() check hidden inside fill_headers(), but as I've said I'm already aiming to replace those with more specific tests. > + payload =3D iov[TCP_IOV_PAYLOAD].iov_base; > + > + ret =3D tcp_fill_flag_header(c, conn, flags, &payload->th, > + payload->opts, &optlen); > + if (ret <=3D 0) > + return ret; > + > + l4len =3D tcp_fill_headers4(c, conn, iov[TCP_IOV_TAP].iov_base, > + iov[TCP_IOV_IP].iov_base, > + iov[TCP_IOV_PAYLOAD].iov_base, optlen, > + NULL, conn->seq_to_tap); > + } else { > + iov =3D tcp6_l2_flags_iov[tcp6_flags_used++]; > + > + payload =3D iov[TCP_IOV_PAYLOAD].iov_base; > + > + ret =3D tcp_fill_flag_header(c, conn, flags, &payload->th, > + payload->opts, &optlen); > + if (ret <=3D 0) > + return ret; > + > + l4len =3D tcp_fill_headers6(c, conn, iov[TCP_IOV_TAP].iov_base, > + iov[TCP_IOV_IP].iov_base, > + iov[TCP_IOV_PAYLOAD].iov_base, optlen, > + conn->seq_to_tap); > + } > + iov[TCP_IOV_PAYLOAD].iov_len =3D l4len; > + > if (flags & DUP_ACK) { > struct iovec *dup_iov; > int i; --=20 David Gibson | 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 --QYc7HdbeqIDJc6lB Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZatOcACgkQzQJF27ox 2Ge1VQ//VKnP4DpNTlqAVR1oipHzsidRWNvbCmn+hLh7q9j/eDOzcmWmyOLtgm1B S5aUE6S9yT3xyYZ3608X+MYd4D5qxi3xG6y30sTVKeEvKOP+nwwQvjaqq/lqZdpP Js+o75cv6eE8uOvGBNHYRLmd7El4S0LPvWBl6gmMMFsDP2aOuO855XGgrZ9h7wA9 WwvTiZfM0sPFTlxGgS7G8sklHrwBDkfzaePAg76/PqiWE/es/n6eRFornfs3pfpJ bzOkRiMjhzydtzimF8xkmyificmXrA8gTpU3gDzJloNUTc0o4NOwFrWaE4ip5h5n VXD3WK0tHuq5oj8RvmFDrCFkqi0gfFaOBHeWBOIDBpz9mxltQflmI5oSfjsoyeTQ K6BZVtgnOTjw/iKsubhpZaaa/6sClD1Yo3Pzvirn+0B02xWZv0wxF7/CZ4ZtNfWZ 0y3sNKqdQlhannHr5TzqwuHf99OTR9n47SbVbbl1cniDduAdcPHgwLjd3QGNToOe doo6I6Fd605YyKk3F/eU6ttgUZJCniyjcsCl3+wE5vX9Sz/crQ03El0OQl4eQRcq gFxrNKbmeRA/EV3V9NRib2OCTyBogqm73dbzmWviaIV/AzJjNrOmEDjzoswW6xNa 4ucvXtgUXyBWnM1ILoOhlnD9+EDrCXEwxxVXcgGX98RrscWQnAM= =ErrV -----END PGP SIGNATURE----- --QYc7HdbeqIDJc6lB--