From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 305075A004E for ; Tue, 11 Jun 2024 07:33:31 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718084007; bh=emWSWOJvn73uVu9sbFAEpGi84DmWYvtmDB9RcDA6Oc0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hxATR+SIpLlS9vTcRVzt2sU18oK74i1J+2FLJnqZiiaIMRAzNtZE360RMRNwj7lGE O8aplGifsFk5DEftLVphbQhU6a3nKF4SjUHYLLJlzrIuOvzhudLB2lMpcHyZNsszTj W7gOe6V4qGe31JEvi3s+XkeMOaQYwmSEdfx30kHDOMS2Kb68sPmOoNjD1rwgEOYnDI Xk4Elxm9B835svdgcuBD0hrpq/WX4J8C4qBOYo/uK+UeXNSo90dLaksdk3B2eveXvQ bh80eYxMkmAkaGqfCuzeCFgcGxQ/84t+vVIOd8BBY4JsLGLSAxuArzBKcKFKcA3rzc NaEFynikMiPQA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Vyy532C3gz4wc5; Tue, 11 Jun 2024 15:33:27 +1000 (AEST) Date: Tue, 11 Jun 2024 15:31:48 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v5 1/8] tcp: extract buffer management from tcp_send_flag() Message-ID: References: <20240605152129.1641658-1-lvivier@redhat.com> <20240605152129.1641658-2-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JucZC3xTfW7YALO5" Content-Disposition: inline In-Reply-To: <20240605152129.1641658-2-lvivier@redhat.com> Message-ID-Hash: S3AZWIXZAUXIBDONBGJZSVLKIAJQLC4H X-Message-ID-Hash: S3AZWIXZAUXIBDONBGJZSVLKIAJQLC4H 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: --JucZC3xTfW7YALO5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 05, 2024 at 05:21:22PM +0200, Laurent Vivier wrote: > This commit isolates the internal data structure management used for stor= ing > data (e.g., tcp4_l2_flags_iov[], tcp6_l2_flags_iov[], tcp4_flags_ip[], > tcp4_flags[], ...) from the tcp_send_flag() function. The extracted > functionality is relocated to a new function named tcp_fill_flag_header(). >=20 > tcp_fill_flag_header() is now a generic function that accepts parameters = such > as struct tcphdr and a data pointer. tcp_send_flag() utilizes this parame= ter to > pass memory pointers from tcp4_l2_flags_iov[] and tcp6_l2_flags_iov[]. >=20 > This separation sets the stage for utilizing tcp_fill_flag_header() to > set the memory provided by the guest via vhost-user in future development= s. Thanks for the commit message, it makes this much clearer. I have a number of comments below, but they're basically all cosmetic. > Signed-off-by: Laurent Vivier > --- > tcp.c | 63 ++++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 39 insertions(+), 24 deletions(-) >=20 > diff --git a/tcp.c b/tcp.c > index 06acb41e4d90..68d4afa05a36 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -1549,24 +1549,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) I don't love the name tcp_fill_flag_header(), although it's not terrible. Maybe tcp_prepare_flags() would be better? > * @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 Worth noting this is an output parameter here... > * > - * 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 =2E. or, since optlen will always be positive on success cases, you could just return it. > */ > -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) > @@ -1588,20 +1589,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; > @@ -1635,17 +1627,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 > - l4len =3D tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > - 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); > @@ -1660,6 +1648,33 @@ 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; > +} > + Function comment, please. > +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++]; > + 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_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > + 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 --JucZC3xTfW7YALO5 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZn4SkACgkQzQJF27ox 2Gez2Q/+LVVnx8gwFpo8nIp9LHU5kmXGtzEX3OBeXnsedqfEU+W48+rgNEFw9Cn+ 63dFSGMit8McjUd9lW0ZNGITGO7rrf7ABo+szDdez02nYnHuNRtvNYjSR1qB1MGq YzlqRcaQMSjl1ZbBSOgd3r1KO9lPj/X7QfnxI1OG9mMKpP5hv8pJ5hxi6ZwevWAR TprjBWl7k7qk4bNNg/qiTiZNnrfTl+J4hJgy9aBfEZklaWSDEPszl9opjojoOEHi NJ9LY/8hZJuure230aQm7ZzCGSnc83n0JWiVF9hI2cL+wI23EuttfLPx4GvbLb2Z 3qZq/Yl/AaskBzquBIPPDTybKDbjY+2WPtwQihUvQiSZ/QMK+BOFcKJSPNnUIQrJ vAtrFlahz5OcX8hLPMXAP8BCMotfcD6lL/eDYg7ffLnekbuginfO+T201SPYcO3J adIRDALA5qhQexJ8u3JKBkLG+8DUsw8HCFRaua6MmXGcGuo1IRuOKKc41tLjn1lX 6tbITRiLI+v4pBtb/39T/ANSPodEVGVgb55wI2ugrQx0EsJ94a2Hj0Jy1h5CEw7h FbKaGX8S2qyBYD/d5pmIE7tJ5XIQuASU1MQsEiuXBtzZvdrRdzmOzTAgVVlysXHs iYC/rsEfvMsZpTKOFmhBmkhxmNW8+JAQ30wtDAr2TeYy82MkQSQ= =BGA1 -----END PGP SIGNATURE----- --JucZC3xTfW7YALO5--