From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 212B95A005E for ; Thu, 1 Dec 2022 19:49:42 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669920581; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=hCxtj8SzmabH9dBU4AatLwp738pCTUbFuv1gRPVIujM=; b=it6IUSs9qsbcJcsjRh1ewVTvHOPyOdBvDtO+3Rb9fmLZTXPATLdaH/fZoQpRGpXps7kgqI 3nqQtbkbNh1huKidC0tS2wxWGAUG9P5oEpqSjU9GRK17IyzZZRsOu0IsU2H0AZo3rb5u0V zpP/ELpozvFblyIyOceOPf2g09ARQS8= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-510-Jq0caQaEPF2Uxj2DxWPTzg-1; Thu, 01 Dec 2022 13:49:37 -0500 X-MC-Unique: Jq0caQaEPF2Uxj2DxWPTzg-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 5F7AE894E8A; Thu, 1 Dec 2022 18:49:37 +0000 (UTC) Received: from maya.cloud.tilaa.com (ovpn-208-4.brq.redhat.com [10.40.208.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C87C5492B2A; Thu, 1 Dec 2022 18:49:36 +0000 (UTC) Date: Thu, 1 Dec 2022 19:49:18 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 04/16] udp: Don't connect "forward" sockets for spliced flows Message-ID: <20221201194918.35eb94df@elisabeth> In-Reply-To: References: <20221124011659.1024901-1-david@gibson.dropbear.id.au> <20221124011659.1024901-5-david@gibson.dropbear.id.au> <20221125024759.5405714d@elisabeth> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: FUXY66QQXLXTUQNAHU6X6P3J3YNIPNUG X-Message-ID-Hash: FUXY66QQXLXTUQNAHU6X6P3J3YNIPNUG X-MailFrom: sbrivio@redhat.com 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.3 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: On Fri, 25 Nov 2022 18:07:32 +1100 David Gibson wrote: > On Fri, Nov 25, 2022 at 02:47:59AM +0100, Stefano Brivio wrote: > > Nit: > > > > On Thu, 24 Nov 2022 12:16:47 +1100 > > David Gibson wrote: > > > > > Currently we connect() the socket we use to forward spliced UDP flows. > > > However, we now only ever use sendto() rather than send() on this socket > > > so there's not actually any need to connect it. Don't do so. > > > > > > Rename a number of things that referred to "connect" or "conn" since that > > > would now be misleading. > > > > > > Signed-off-by: David Gibson > > > --- > > > udp.c | 85 ++++++++++++++++++++++++----------------------------------- > > > 1 file changed, 35 insertions(+), 50 deletions(-) > > > > > > diff --git a/udp.c b/udp.c > > > index 1e78da5..ed095ec 100644 > > > --- a/udp.c > > > +++ b/udp.c > > > @@ -45,23 +45,20 @@ > > > * > > > * - from init to namespace: > > > * > > > - * - forward direction: 127.0.0.1:5000 -> 127.0.0.1:80 in init from bound > > > - * socket s, with epoll reference: index = 80, splice = UDP_TO_NS > > > + * - forward direction: 127.0.0.1:5000 -> 127.0.0.1:80 in init from socket s, > > > + * with epoll reference: index = 80, splice = UDP_TO_NS > > > * - if udp_splice_to_ns[V4][5000].target_sock: > > > * - send packet to udp_splice_to_ns[V4][5000].target_sock, with > > > * destination port 80 > > > * - otherwise: > > > * - create new socket udp_splice_to_ns[V4][5000].target_sock > > > * - bind in namespace to 127.0.0.1:5000 > > > - * - connect in namespace to 127.0.0.1:80 (note: this destination port > > > - * might be remapped to another port instead) > > > * - add to epoll with reference: index = 5000, splice: UDP_BACK_TO_INIT > > > * - set udp_splice_to_ns[V4][5000].orig_sock to s > > > * - update udp_splice_to_ns[V4][5000].ts with current time > > > * > > > - * - reverse direction: 127.0.0.1:80 -> 127.0.0.1:5000 in namespace from > > > - * connected socket s, having epoll reference: index = 5000, > > > - * splice = UDP_BACK_TO_INIT > > > + * - reverse direction: 127.0.0.1:80 -> 127.0.0.1:5000 in namespace socket s, > > > + * having epoll reference: index = 5000, splice = UDP_BACK_TO_INIT > > > * - if udp_splice_to_ns[V4][5000].orig_sock: > > > * - send to udp_splice_to_ns[V4][5000].orig_sock, with destination port > > > * 5000 > > > @@ -69,7 +66,7 @@ > > > * > > > * - from namespace to init: > > > * > > > - * - forward direction: 127.0.0.1:2000 -> 127.0.0.1:22 in namespace from bound > > > + * - forward direction: 127.0.0.1:2000 -> 127.0.0.1:22 in namespace from > > > * socket s, with epoll reference: index = 22, splice = UDP_TO_INIT > > > * - if udp4_splice_to_init[V4][2000].target_sock: > > > * - send packet to udp_splice_to_init[V4][2000].target_sock, with > > > @@ -77,14 +74,12 @@ > > > * - otherwise: > > > * - create new socket udp_splice_to_init[V4][2000].target_sock > > > * - bind in init to 127.0.0.1:2000 > > > - * - connect in init to 127.0.0.1:22 (note: this destination port > > > - * might be remapped to another port instead) > > > * - add to epoll with reference: index = 2000, splice = UDP_BACK_TO_NS > > > * - set udp_splice_to_init[V4][2000].orig_sock to s > > > * - update udp_splice_to_init[V4][2000].ts with current time > > > * > > > - * - reverse direction: 127.0.0.1:22 -> 127.0.0.1:2000 in init from connected > > > - * socket s, having epoll reference: index = 2000, splice = UDP_BACK_TO_NS > > > + * - reverse direction: 127.0.0.1:22 -> 127.0.0.1:2000 in init from socket s, > > > + * having epoll reference: index = 2000, splice = UDP_BACK_TO_NS > > > * - if udp_splice_to_init[V4][2000].orig_sock: > > > * - send to udp_splice_to_init[V4][2000].orig_sock, with destination port > > > * 2000 > > > @@ -144,8 +139,7 @@ struct udp_tap_port { > > > * @orig_sock: Originating socket, bound to dest port in source ns of > > > * originating datagram > > > * @target_sock: Target socket, bound to source port of originating > > > - * datagram in dest ns, connected to dest port of > > > - * originating datagram in dest ns > > > + * datagram in dest ns > > > * @ts: Activity timestamp > > > */ > > > struct udp_splice_flow { > > > @@ -163,8 +157,8 @@ static struct udp_splice_flow udp_splice_to_init[IP_VERSIONS][NUM_PORTS]; > > > > > > enum udp_act_type { > > > UDP_ACT_TAP, > > > - UDP_ACT_NS_CONN, > > > - UDP_ACT_INIT_CONN, > > > + UDP_ACT_SPLICE_NS, > > > + UDP_ACT_SPLICE_INIT, > > > UDP_ACT_TYPE_MAX, > > > }; > > > > > > @@ -398,20 +392,19 @@ static void udp_sock6_iov_init(void) > > > } > > > > > > /** > > > - * udp_splice_connect() - Create and connect socket for "spliced" binding > > > + * udp_splice_new() - Create and prepare socket for "spliced" binding > > > * @c: Execution context > > > - * @v6: Set for IPv6 connections > > > + * @v6: Set for IPv6 sockets > > > * @bound_sock: Originating bound socket > > > * @src: Source port of original connection, host order > > > - * @dst: Destination port of original connection, host order > > > * @splice: UDP_BACK_TO_INIT from init, UDP_BACK_TO_NS from namespace > > > * > > > - * Return: connected socket, negative error code on failure > > > + * Return: Prepared socket, negative error code on failure > > > > prepared > > Fixed. Is there a particular rationale for capitalizing on the > parameters, but not the return description? Sorry, I started reviewing your v4 and noticed I forgot to answer this. It's actually inconsistent with the Kernel-doc style which pretty much inspires this: https://docs.kernel.org/doc-guide/kernel-doc.html so I started like that because in kernel's net/ directory the non-capitalised version is more common: $ grep -rnI "Return: [A-Z][ a-z]" | wc -l 66 $ grep -rnI "Return: [a-z]" | wc -l 298 and I'm simply used to that -- but it's also true I added 19 of those. My very personal interpretation of the rationale is that in this case we form a sentence, and at the same time sentences certainly don't start with '@'. Actually, if we capitalise them, we could at least refer to those as coding style guidelines. :) I'd rather "fix" this all at once when we get to https://bugs.passt.top/show_bug.cgi?id=35. -- Stefano