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 CFD195A004F for ; Wed, 12 Jun 2024 08:33:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718173988; bh=r89fsCcXjiG17kSz2w7eSH/QNRHCOSDlq6bFZssbH0M=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FTX5GCwIhwi53/rOX8sxrU0IlGBeKB1tlGd69uPwkM/MFJZ24/yNIdNn+KSmSSyU9 2YIORwIcrC77qWPZ1PkNImr9ZHQdAGTAAkEF5VVr5taVqXS5kLaHCc+xqSK7YhkOSh wTOItCZU5ATZVO54+LbNZrSImBgr0quwvsXWvaFryRAK8pOH2I6Nu2UqO5aAhNE/J6 yhM+ZXLM41qHz/2l6eb4HXmOk4lP4Zky1hIIltrpuREVUvOyIaXuEoYp2iPx6RWPFS LBBipkMXJC8JNz/5erH+Me5tXEJBA/peasPGmmuAtK+k7a2RvGDLoBpFGLOBbMSmC7 GKt63ZnnIXFiA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VzbMS0kc9z4wb2; Wed, 12 Jun 2024 16:33:08 +1000 (AEST) Date: Wed, 12 Jun 2024 16:31:07 +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="P9HT3Xu1qL/mkFX8" Content-Disposition: inline In-Reply-To: Message-ID-Hash: S6ZNQ5SMTOJHZEDOL6R5ID2MJZN6ER3T X-Message-ID-Hash: S6ZNQ5SMTOJHZEDOL6R5ID2MJZN6ER3T 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: --P9HT3Xu1qL/mkFX8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jun 11, 2024 at 01:42:20PM +0200, Laurent Vivier wrote: > On 11/06/2024 07:31, David Gibson wrote: > > On Wed, Jun 05, 2024 at 05:21:22PM +0200, Laurent Vivier wrote: > > > This commit isolates the internal data structure management used for = storing > > > 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_head= er(). > > >=20 > > > tcp_fill_flag_header() is now a generic function that accepts paramet= ers such > > > as struct tcphdr and a data pointer. tcp_send_flag() utilizes this pa= rameter 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 develop= ments. > >=20 > > Thanks for the commit message, it makes this much clearer. > >=20 > > I have a number of comments below, but they're basically all cosmetic. > >=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 = struct ctx *c, > > > } > > > /** > > > - * tcp_send_flag() - Send segment with flags to tap (no payload) > > > + * tcp_fill_flag_header() - Prepare header for flags-only segment (n= o payload) > >=20 > > I don't love the name tcp_fill_flag_header(), although it's not > > terrible. Maybe tcp_prepare_flags() would be better? > >=20 > > > * @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 > > Worth noting this is an output parameter here... > >=20 > > > * > > > - * 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 > >=20 > > .. or, since optlen will always be positive on success cases, you > > could just return it. > >=20 >=20 > We cannot return optlen here as optlen can be 0 (it is not zero only with > SYN), and 0 means no flags to send. We can have flags to send with optlen > equal to 0. Oh, my mistake, sorry. We could change it to the l4len which would avoid that, but it looks like that would be more awkward in the caller. --=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 --P9HT3Xu1qL/mkFX8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZpQKoACgkQzQJF27ox 2GdknhAAkw33YArRI8BlZ0b1e9I4FZ28fUOmYfnK2GKOgTWBiIRxJI2cY9v/mcvZ vPhUPbHMdG7z5vWS20z32Psz88hGyqx55+PLQfuhsMQVhcPK7MhRSiQXDs6ewdy3 zBr+pVfsD/JYP9bs1dHwuqiNNEojRngSxJhZ1pUWWUnhh1MmOeLEwmz34fn9frZa 8ry5aX1ORZ+JU6b/HqBceTkPxm3IgtnilzX/1CYNJquwjjtcz13aoCh+e9K1uN2V s5NbFL40wuHrlt7ZGT2h2hZPNvOxEy4mgi9yFcgX+56I1+EodNtqF1ZINZvQ5mGb d9HC03QQXvb7q8/uYrApkrdt5ztQbs0rb7l2oClW2vqNYlTAt6SVcRmMBhNuDtt1 pqXDvnO1WV/xaFDPvq6hjFaQidQL5h/R43oNhQF+zMMIkw84KPSF3PZqN5rcHRzF qzbOuxvJ6VzvOMv0cHBy0edkrVV8AWX5k8zZ6U6fl+4QaoEu0VNuMxesoPMPfqXU CILx3Vi6dwYPjZgjTT9ndMzRd+ofvzBjChZHRZG3UKl9TV49IYqk/jjsezVtm3R+ FAEpPcLUrO+PI5j9VqeHSBZ9VlXQOH4omxKuxoHyP0kRXiGq8zlTxYWu27yTQeyt 7E6Nus0zVyt3H4U0yHI6zH3jGcuj5RME7dz0VGe3S+zBmvocRkQ= =a1tW -----END PGP SIGNATURE----- --P9HT3Xu1qL/mkFX8--