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 DD8A05A004F for ; Wed, 12 Jun 2024 08:33:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718173988; bh=jRjtrC6bNYbVznefDca1sED8NUcPkuVihZWZKkGKgp0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fWNUNC//xSVg9lslvhtF6kvDgwDBcRNNG5pQxn2lkdnUg1Ut3V9gFA+EhEri+rGFl LUIBwm2bq1OSdpREA/T7rBtLJGDiR2AVfIeVZC9FVVHqzNQ7ybekJoTR2UbKoZAbbM lPhcoiid8HD4ZnodvOSjZI5t7K/3tfXbK4My/ZQVjo8ClhTID99qrjBSTil9bQmYvm RsWMXVlP+p+AYRsO2JYsdY9PxpI5xe5kEM/xzl2xnOZwAXXd4wXNgZszfsHXmpsO2d vYk1doWF2rHKMti8SEQQOQNX/8yGL5hliz2hOV1gN9ctob0J2DH20GMCriRjfRGU+S vcquKCiUXyLlQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VzbMS0ssbz4wcp; Wed, 12 Jun 2024 16:33:08 +1000 (AEST) Date: Wed, 12 Jun 2024 16:32:57 +1000 From: David Gibson To: Stefano Brivio 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> <20240612000904.59b438c5@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ifhNbn/ZbxhJPB3e" Content-Disposition: inline In-Reply-To: <20240612000904.59b438c5@elisabeth> Message-ID-Hash: CHDVMWBR5MJ2NELHVL5VIIXGQMATYRAV X-Message-ID-Hash: CHDVMWBR5MJ2NELHVL5VIIXGQMATYRAV 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: Laurent Vivier , 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: --ifhNbn/ZbxhJPB3e Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 12, 2024 at 12:09:04AM +0200, Stefano Brivio wrote: > On Wed, 5 Jun 2024 17:21:22 +0200 > Laurent Vivier wrote: >=20 > > This commit isolates the internal data structure management used for st= oring > > 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 parameter= s such > > as struct tcphdr and a data pointer. tcp_send_flag() utilizes this para= meter 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 developme= nts. > >=20 > > 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 st= ruct 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 = payload) > > * @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 >=20 > Now, this becomes an output parameter if SYN is set in flags, but it's > otherwise an input parameter (and it must be zero, otherwise the data > offset field we send will be wrong). >=20 > I think having it always as output parameter (that is, setting it to > zero on non-SYN in this function, not in the caller) would be less > fragile and easier to describe in the comment, too. I agree. > Or, even simpler, pass it as input parameter, and calculate it in the > caller. The caller sets 'flags' anyway. Eh... it seems to me that the knowledge of how to translate the flags bits to a specific length better belongs here, so I don't know that's a great idea. > > * > > - * Return: negative error code on connection reset, 0 otherwise > > + * Return: < 0 error code on connection reset, > > + * 0 if there is no flag to send >=20 > As you use one tab to indent "1 otherwise" below, you could use one > here as well. >=20 > > + * 1 otherwise > > */ > > -static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int= flags) > > +static int tcp_fill_flag_header(struct ctx *c, struct tcp_tap_conn *co= nn, > > + 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 = tcp_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 = tcp_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 t= cp_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= flags) > > +{ > > + 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 --=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 --ifhNbn/ZbxhJPB3e Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZpQRgACgkQzQJF27ox 2GeyAw/8CRDQrzD9wFF562FowVkIYAsS6b2Yzu4pkysIvKKCV4/+hZXsA7eZChLW xeViwCjfghpxMpWhKpLPA1ESP9oxebWVUDpZWrhnHn8cYaduP4AqqL6X5UsF+0FT 4vMtHnOxZcf2xGZ8ATA8RVYCdcCIn7WoWN6Ahd/1n/kndSRUG0cyFpkXcRAmUmah +fB6ka/CNzaQ29M8UcJIIu7HEpoMz/2s+/hEaoyuqpUCIID7CJFunJ0HmgvjxNhd 9d+a+deZTdsQqnFaYcw+pukbvI95Jwp0iRTTyClumhwNrj3S3pKhx05LSmuBLUuq 9mrVoH1yWVhm7+GIfwfoOlDBvDxi0SoCVxYUGv5qU1+p1+y3tteeda51WCtuxxyE x4OyUHa5P4CkALZ/B7wjDfMwGeLdgzDocRBWyfNBqTeFOhs08fpdm00k+KPW7M0Z SN+Ow+sJU/QEUzau3ELmEycTdO81oLYuPYJN7D9HyVmTQ1Tm4q6jc7DlYETsa1dH WRwJww5mYfCiFg9RiTQYky145uT22tDw3+4nxK04y/nLkRkcDC5IEwS1jKcznT9J l46nvXCwiO7KairVTYpq29pbyHy8bD/RxJKF5Vp9Ddt4SLv+FuLbWQ7/vMJKZCsm 9MiDis3trYX2vs4xuH8/ApJw2LDCxwlkdFQH+DqYrLa2HB0tPzk= =0y8X -----END PGP SIGNATURE----- --ifhNbn/ZbxhJPB3e--