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.129.124]) by passt.top (Postfix) with ESMTP id 16BC35A005E for ; Thu, 17 Nov 2022 08:30:37 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668670236; 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=kRQ4aS9C/i3LDQS9EGH5mn+TblWjvI+v6iwtaUGOchw=; b=SBh3s3ldr24rT4PX87EEKoeREz7SumnWhJ9rgpQvJ0WTJo4oU2aUAHKqc4LjFglvL0yxCB Kzh0aTor4wVTDcblCA7PyNolHGJxKMSufH+oBf8YHje7fyODBYZW46M7dPspTQJiiMTZqx 5/qN4BJbhCR7otKDlobjyABKjCOej1U= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-593-rr8PZJl1PJu6D1XBhiyj-g-1; Thu, 17 Nov 2022 02:30:33 -0500 X-MC-Unique: rr8PZJl1PJu6D1XBhiyj-g-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CF48D1C004F5; Thu, 17 Nov 2022 07:30:32 +0000 (UTC) Received: from maya.cloud.tilaa.com (ovpn-208-8.brq.redhat.com [10.40.208.8]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 73E572166B29; Thu, 17 Nov 2022 07:30:32 +0000 (UTC) Date: Thu, 17 Nov 2022 08:30:29 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 15/32] tcp: Unify part of spliced and non-spliced conn_from_sock path Message-ID: <20221117083029.3a062bea@elisabeth> In-Reply-To: References: <20221116044212.3876516-1-david@gibson.dropbear.id.au> <20221116044212.3876516-16-david@gibson.dropbear.id.au> <20221117005358.324ca3a0@elisabeth> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: KJZW4ZTQYWLIQBQUIFVVONEA3NBA7EUZ X-Message-ID-Hash: KJZW4ZTQYWLIQBQUIFVVONEA3NBA7EUZ 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 Thu, 17 Nov 2022 12:37:04 +1100 David Gibson wrote: > On Thu, Nov 17, 2022 at 12:53:58AM +0100, Stefano Brivio wrote: > > On Wed, 16 Nov 2022 15:41:55 +1100 > > David Gibson wrote: > > > > > In tcp_sock_handler() we split off to handle spliced sockets before > > > checking anything else. However the first steps of the "new connection" > > > path for each case are the same: allocate a connection entry and accept() > > > the connection. > > > > > > Remove this duplication by making tcp_conn_from_sock() handle both spliced > > > and non-spliced cases, with help from more specific tcp_tap_conn_from_sock > > > and tcp_splice_conn_from_sock functions for the later stages which differ. > > > > > > Signed-off-by: David Gibson > > > --- > > > tcp.c | 68 ++++++++++++++++++++++++++++++++++------------------ > > > tcp_splice.c | 58 +++++++++++++++++++++++--------------------- > > > tcp_splice.h | 4 ++++ > > > 3 files changed, 80 insertions(+), 50 deletions(-) > > > > > > diff --git a/tcp.c b/tcp.c > > > index 72d3b49..e66a82a 100644 > > > --- a/tcp.c > > > +++ b/tcp.c > > > @@ -2753,28 +2753,19 @@ static void tcp_connect_finish(struct ctx *c, struct tcp_tap_conn *conn) > > > } > > > > > > /** > > > - * tcp_conn_from_sock() - Handle new connection request from listening socket > > > + * tcp_tap_conn_from_sock() - Initialize state for non-spliced connection > > > * @c: Execution context > > > * @ref: epoll reference of listening socket > > > + * @conn: connection structure to initialize > > > + * @s: Accepted socket > > > + * @sa: Peer socket address (from accept()) > > > * @now: Current timestamp > > > */ > > > -static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > > > - const struct timespec *now) > > > +static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref, > > > + struct tcp_tap_conn *conn, int s, > > > + struct sockaddr *sa, > > > + const struct timespec *now) > > > { > > > - struct sockaddr_storage sa; > > > - struct tcp_tap_conn *conn; > > > - socklen_t sl; > > > - int s; > > > - > > > - if (c->tcp.conn_count >= TCP_MAX_CONNS) > > > - return; > > > - > > > - sl = sizeof(sa); > > > - s = accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); > > > - if (s < 0) > > > - return; > > > - > > > - conn = CONN(c->tcp.conn_count++); > > > conn->c.spliced = false; > > > conn->sock = s; > > > conn->timer = -1; > > > @@ -2784,7 +2775,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > > > if (ref.r.p.tcp.tcp.v6) { > > > struct sockaddr_in6 sa6; > > > > > > - memcpy(&sa6, &sa, sizeof(sa6)); > > > + memcpy(&sa6, sa, sizeof(sa6)); > > > > > > if (IN6_IS_ADDR_LOOPBACK(&sa6.sin6_addr) || > > > IN6_ARE_ADDR_EQUAL(&sa6.sin6_addr, &c->ip6.addr_seen) || > > > @@ -2813,7 +2804,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > > > } else { > > > struct sockaddr_in sa4; > > > > > > - memcpy(&sa4, &sa, sizeof(sa4)); > > > + memcpy(&sa4, sa, sizeof(sa4)); > > > > > > memset(&conn->a.a4.zero, 0, sizeof(conn->a.a4.zero)); > > > memset(&conn->a.a4.one, 0xff, sizeof(conn->a.a4.one)); > > > @@ -2846,6 +2837,37 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > > > tcp_get_sndbuf(conn); > > > } > > > > > > +/** > > > + * tcp_conn_from_sock() - Handle new connection request from listening socket > > > + * @c: Execution context > > > + * @ref: epoll reference of listening socket > > > + * @now: Current timestamp > > > + */ > > > +static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > > > + const struct timespec *now) > > > +{ > > > + struct sockaddr_storage sa; > > > + union tcp_conn *conn; > > > + socklen_t sl; > > > + int s; > > > + > > > + if (c->tcp.conn_count >= TCP_MAX_CONNS) > > > + return; > > > + > > > + sl = sizeof(sa); > > > + s = accept4(ref.r.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); > > > > Combined with 16/32 I'm not sure this is simplifying much -- it looks a > > bit unnatural there to get the peer address not "directly" from > > accept4(). On the other hand you drop a few lines -- I'm fine with > > it either way. > > Um.. I'm not really sure what you're getting at here. By "directly" I mean assigned by accept4() in the same function, instead of accept4() being done in the caller. That is, if I now look at tcp_tap_conn_from_sock() we have 'sa' there which comes as an argument, not directly a couple of lines above from accept4(), which would be quicker to review. On the other hand the function comment says "from accept()", so it's not much effort to figure that out either. -- Stefano