From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 640DE5A005E for ; Sun, 15 Jan 2023 01:31:22 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4NvbgB6tYMz4xN6; Sun, 15 Jan 2023 11:31:18 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1673742678; bh=MLGqOSovkMX66qQyqXg+isaM/p41NVzMOaV2uoM/oS8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PK9YpDroRdsjcaXJPXJFXEHCol5021YvwSyOAEpExPhM7Vx5BjohJrmkutlNcQKPy 2KR+zG+Jei+mdrPWYgtxwkfgWpdhBuq8O/odPbpMqCmJYNLvQIVlKzso3GKuNDszOY vjUG4T4OafxdYwsjtkbBNxb5SojUBlZzitBpgse0= Date: Sun, 15 Jan 2023 10:26:31 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/5] tcp: Move socket pool declarations around Message-ID: References: <20230109005026.24512-1-david@gibson.dropbear.id.au> <20230109005026.24512-4-david@gibson.dropbear.id.au> <20230112190925.73f2c128@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dC0827487kDwpCPC" Content-Disposition: inline In-Reply-To: <20230112190925.73f2c128@elisabeth> Message-ID-Hash: LG5YAR5BDDGGEX34NLCW2RKBRHF3QQJ2 X-Message-ID-Hash: LG5YAR5BDDGGEX34NLCW2RKBRHF3QQJ2 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.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: --dC0827487kDwpCPC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 12, 2023 at 07:09:25PM +0100, Stefano Brivio wrote: > On Mon, 9 Jan 2023 11:50:24 +1100 > David Gibson wrote: >=20 > > tcp_splice.c has some explicit extern declarations to access the socket > > pools. This is pretty dangerous - if we changed the type of these > > variables in tcp.c, we'd have tcp.c and tcp_splice.c using the memory > > differently with no compiler error. So, move the extern declarations to > > tcp_conn.h so they're visible to both tcp.c and tcp_splice.c, but not t= he > > rest of pasta. > >=20 > > In fact the pools for the guest namespace are necessarily only used by > > tcp_splice.c - we have no sockets on the guest side if we're not splici= ng. > > So move those declarations - and the functions that deal exclusively wi= th > > them - to tcp_splice.c > >=20 > > Signed-off-by: David Gibson > > --- > > tcp.c | 41 ++++------------------------------------- > > tcp.h | 2 -- > > tcp_conn.h | 10 ++++++++-- > > tcp_splice.c | 50 +++++++++++++++++++++++++++++++++++++++++++------- > > 4 files changed, 55 insertions(+), 48 deletions(-) > >=20 > > diff --git a/tcp.c b/tcp.c > > index 4da823c..19fd17b 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -370,8 +370,6 @@ struct tcp6_l2_head { /* For MSS6 macro: keep in sy= nc with tcp6_l2_buf_t */ > > #define FIN_TIMEOUT 60 > > #define ACT_TIMEOUT 7200 > > =20 > > -#define TCP_SOCK_POOL_TSH 16 /* Refill in ns if > x used */ > > - > > #define LOW_RTT_TABLE_SIZE 8 > > #define LOW_RTT_THRESHOLD 10 /* us */ > > =20 > > @@ -595,11 +593,9 @@ static inline struct tcp_tap_conn *conn_at_idx(int= index) > > /* Table for lookup from remote address, local port, remote port */ > > static struct tcp_tap_conn *tc_hash[TCP_HASH_TABLE_SIZE]; > > =20 > > -/* Pools for pre-opened sockets */ > > +/* Pools for pre-opened sockets (init namespace) */ > > int init_sock_pool4 [TCP_SOCK_POOL_SIZE]; > > int init_sock_pool6 [TCP_SOCK_POOL_SIZE]; > > -int ns_sock_pool4 [TCP_SOCK_POOL_SIZE]; > > -int ns_sock_pool6 [TCP_SOCK_POOL_SIZE]; > > =20 > > /** > > * tcp_conn_epoll_events() - epoll events mask for given connection st= ate > > @@ -2988,7 +2984,7 @@ static int tcp_ns_socks_init(void *arg) > > * @pool: Pool of sockets to refill > > * @af: Address family to use > > */ > > -static void tcp_sock_refill_pool(const struct ctx *c, int pool[], int = af) > > +void tcp_sock_refill_pool(const struct ctx *c, int pool[], int af) > > { > > int i; > > =20 > > @@ -3022,26 +3018,6 @@ static void tcp_sock_refill_init(const struct ct= x *c) > > tcp_sock_refill_pool(c, init_sock_pool6, AF_INET6); > > } > > =20 > > -/** > > - * tcp_sock_refill_ns() - Refill pools of pre-opened sockets in namesp= ace > > - * @arg: Execution context cast to void * > > - * > > - * Return: 0 > > - */ > > -static int tcp_sock_refill_ns(void *arg) > > -{ > > - const struct ctx *c =3D (const struct ctx *)arg; > > - > > - ns_enter(c); > > - > > - if (c->ifi4) > > - tcp_sock_refill_pool(c, ns_sock_pool4, AF_INET); > > - if (c->ifi6) > > - tcp_sock_refill_pool(c, ns_sock_pool6, AF_INET6); > > - > > - return 0; > > -} > > - > > /** > > * tcp_init() - Get initial sequence, hash secret, initialise per-sock= et data > > * @c: Execution context > > @@ -3090,8 +3066,6 @@ int tcp_init(struct ctx *c) > > =20 > > memset(init_sock_pool4, 0xff, sizeof(init_sock_pool4)); > > memset(init_sock_pool6, 0xff, sizeof(init_sock_pool6)); > > - memset(ns_sock_pool4, 0xff, sizeof(ns_sock_pool4)); > > - memset(ns_sock_pool6, 0xff, sizeof(ns_sock_pool6)); > > memset(tcp_sock_init_ext, 0xff, sizeof(tcp_sock_init_ext)); > > memset(tcp_sock_ns, 0xff, sizeof(tcp_sock_ns)); > > =20 > > @@ -3101,8 +3075,6 @@ int tcp_init(struct ctx *c) > > tcp_splice_init(c); > > =20 > > NS_CALL(tcp_ns_socks_init, c); > > - > > - NS_CALL(tcp_sock_refill_ns, c); > > } > > =20 > > return 0; > > @@ -3255,11 +3227,6 @@ void tcp_timer(struct ctx *c, const struct times= pec *ts) > > } > > =20 > > tcp_sock_refill_init(c); > > - if (c->mode =3D=3D MODE_PASTA) { > > - if ((c->ifi4 && ns_sock_pool4[TCP_SOCK_POOL_TSH] < 0) || > > - (c->ifi6 && ns_sock_pool6[TCP_SOCK_POOL_TSH] < 0)) > > - NS_CALL(tcp_sock_refill_ns, c); > > - > > - tcp_splice_pipe_refill(c); > > - } > > + if (c->mode =3D=3D MODE_PASTA) > > + tcp_splice_refill(c); > > } > > diff --git a/tcp.h b/tcp.h > > index 739b451..236038f 100644 > > --- a/tcp.h > > +++ b/tcp.h > > @@ -11,8 +11,6 @@ > > #define TCP_CONN_INDEX_BITS 17 /* 128k */ > > #define TCP_MAX_CONNS (1 << TCP_CONN_INDEX_BITS) > > =20 > > -#define TCP_SOCK_POOL_SIZE 32 > > - > > struct ctx; > > =20 > > void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t eve= nts, > > diff --git a/tcp_conn.h b/tcp_conn.h > > index 70f4a7c..9951b0a 100644 > > --- a/tcp_conn.h > > +++ b/tcp_conn.h > > @@ -182,11 +182,17 @@ union tcp_conn { > > /* TCP connections */ > > extern union tcp_conn tc[]; > > =20 > > +/* Socket pools */ > > +#define TCP_SOCK_POOL_SIZE 32 > > + > > +extern int init_sock_pool4 [TCP_SOCK_POOL_SIZE]; > > +extern int init_sock_pool6 [TCP_SOCK_POOL_SIZE]; > > + > > void tcp_splice_conn_update(struct ctx *c, struct tcp_splice_conn *new= ); > > void tcp_table_compact(struct ctx *c, union tcp_conn *hole); > > void tcp_splice_destroy(struct ctx *c, union tcp_conn *conn_union); > > void tcp_splice_timer(struct ctx *c, union tcp_conn *conn_union); > > -void tcp_splice_pipe_refill(const struct ctx *c); > > - > > +void tcp_sock_refill_pool(const struct ctx *c, int pool[], int af); > > +void tcp_splice_refill(const struct ctx *c); > > =20 > > #endif /* TCP_CONN_H */ > > diff --git a/tcp_splice.c b/tcp_splice.c > > index 72b1672..688d776 100644 > > --- a/tcp_splice.c > > +++ b/tcp_splice.c > > @@ -61,11 +61,11 @@ > > #define TCP_SPLICE_CONN_PRESSURE 30 /* % of conn_count */ > > #define TCP_SPLICE_FILE_PRESSURE 30 /* % of c->nofile */ > > =20 > > -/* From tcp.c */ > > -extern int init_sock_pool4 [TCP_SOCK_POOL_SIZE]; > > -extern int init_sock_pool6 [TCP_SOCK_POOL_SIZE]; > > -extern int ns_sock_pool4 [TCP_SOCK_POOL_SIZE]; > > -extern int ns_sock_pool6 [TCP_SOCK_POOL_SIZE]; > > +/* Pools for pre-opened sockets (guest namespace) */ >=20 > What you mean by "guest namespace" is perfectly clear, but strictly > speaking, that's not a thing, and I'm a bit concerned that it might > cause confusion (especially with passt). Hm, right. > What about using "init" (instead of "init namespace") above, and > "namespace" here? Yeah, alright. I don't love this either, since just "namespace" and "init" are also kind of ambiguous without context. But, it's more consistent with the terminology in other places, so it will do for now. --=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 --dC0827487kDwpCPC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmPDSDAACgkQgypY4gEw YSL88BAAqEzFJ6R91cuUYtC0j3KIJjZ1MO/ut+S2/yCUBKhVDRmGeuncWkt1JyzW C7RT5Sd8V+FcRS5K9BlJtiFhj6cMFaD9LluA//uDPFLN0+cHw+vsy1XOYnEa7zuL vB+RdEuLJHDjFj0PUOxEO0WoOb35+UKBcxCd2RY7Nh5PoPexQOPKBGDdyvMt9a9Z girwgWEFYizCn9vLAGSs1X30WGYL4UZ5z71vPUblRaq01M/YNibTz02At7ElnTyr 0glQrt7bXsZ7KRM8oVpVeZCAn1oUXQxcLsR9Ose9nhGWswbO8aHV6xB5PMxGK7VU Bpt8bObQkqzQIiI5ZKM8pV5k8yNNY+rTsOxQNd/luBF/pG1tf57l1wxdXiUiM1QL OLxsj5G/QJ20yYPjnJ11NO8p2cL4IFOh2JMCN5enPrJ6SmVeHkhAEsLd2nMqC7MO O2EVTpIwCntNEzX7dpmXCfzwK1oTyNdAKEME8e5FvAnBcGg8/QYUeD6s0UgtbHyq r8/eBBIHZYk8m0J4MNw3xJkWnrI8XwclMTlOy1z6hMCDbBzG+X15AUVhjjlSY7TG gdPEkTmZsnea1QLTT7pfokRJg39AP0SyB4X4MyMRUl23FH0SMZnHvhrAIiVcERnR 32dbCfMeeoZZqimouUGKoljTDOPV+pahgTuvlA1isYpGjqpQN5M= =DtFj -----END PGP SIGNATURE----- --dC0827487kDwpCPC--