From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=ognk0jiL; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 399DA5A0269 for ; Thu, 02 Jul 2026 04:55:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1782960928; bh=TXXoNKKRJ19G8Gd/wIxDVcE/s8D6IcF9xNZwNTfffeg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ognk0jiL2IeKK3hbkil68bWrON+nkAoEId+45+mCyZWc26MahGSuKeE7+cZk0pT1J dOXOhszhk2VEsQM+A+qDcSGOrLWgJNuEDB2PiuMf1oFkToJmsJLhvjgl0cfVDfKsIN c8mcGAFakcJZuMoF62I5UaMdsxlwhpMI4riibt8b3/gvDaaVTESKK0S2+/XNk6qM0C 0MCQ6Z/VSlkZZXF9oKIUDha4NkdGngINrNvsHGOC3XQsia6lviOMt4rwSYO8RLAG7L Weiar/RiyLpmmTmwXNCiXZ3dViM1UGxbDKNkLtfrp0ROwukp2IQ0lAth+keiKwAbdH wPzzF9K+NZpDw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4grM280sCMz58ln; Thu, 02 Jul 2026 12:55:28 +1000 (AEST) Date: Thu, 2 Jul 2026 12:55:14 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 6/8] tcp: Make TCP timer state per-caller and guard global tasks Message-ID: References: <20260616171052.3785909-1-lvivier@redhat.com> <20260616171052.3785909-7-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="bL66LoCXAuOKdyfj" Content-Disposition: inline In-Reply-To: <20260616171052.3785909-7-lvivier@redhat.com> Message-ID-Hash: RE5JIF3QCC2AMPUOOZD7LBHZ7QMJ6WZX X-Message-ID-Hash: RE5JIF3QCC2AMPUOOZD7LBHZ7QMJ6WZX 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: --bL66LoCXAuOKdyfj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jun 16, 2026 at 07:10:50PM +0200, Laurent Vivier wrote: > tcp_defer_handler() uses c->tcp.timer_run, c->tcp.keepalive_run, and > c->tcp.inactivity_run as global timer gates shared across all callers. > In multiqueue mode, multiple qpair workers will call tcp_defer_handler() > concurrently, causing races on these fields. It also unconditionally > runs tcp_payload_flush(), tcp_sock_refill_init(), and tcp_splice_refill() > which operate on global state. >=20 > Add timer_run, keepalive_run, and inactivity_run as parameters so each > caller provides its own per-qpair timer state. Remove the now-unused > fields from struct tcp_ctx and drop timer_init() which only initialised > c->tcp.timer_run. >=20 > Guard tcp_payload_flush() and socket pool refills with qpair =3D=3D 0 sin= ce > they operate on global buffers shared across all queue pairs. >=20 > Signed-off-by: Laurent Vivier > --- > passt.c | 37 +++++++++++++++++-------------------- > tcp.c | 51 +++++++++++++++++++++++++++++++-------------------- > tcp.h | 9 ++------- > 3 files changed, 50 insertions(+), 47 deletions(-) >=20 > diff --git a/passt.c b/passt.c > index bebc2b99f523..ca5973e17317 100644 > --- a/passt.c > +++ b/passt.c > @@ -96,16 +96,23 @@ struct passt_stats { > =20 > /** > * post_handler() - Run periodic and deferred tasks for L4 protocol hand= lers > - * @c: Execution context > - * @now: Current timestamp > - * @timer_run: Last time the flow timers ran > - * @qpair: Queue pair to process > + * @c: Execution context > + * @now: Current timestamp > + * @timer_run: Last time the flow timers ran > + * @tcp_timer_run: Last time TCP timers ran > + * @keepalive_run: Last time keepalives ran > + * @inactivity_run: Last time inactivity scan ran > + * @qpair: Queue pair to process > */ > static void post_handler(struct ctx *c, const struct timespec *now, > - struct timespec *timer_run, unsigned int qpair) > + struct timespec *timer_run, > + struct timespec *tcp_timer_run, > + time_t *keepalive_run, > + time_t *inactivity_run, unsigned int qpair) This is pretty bulk, and also threads several TCP specific things through this not TCP specific code path. This seems like a place where a qpair_ctx which holds these timestamps would be useful. > { > if (!c->no_tcp) > - tcp_defer_handler(c, now, qpair); > + tcp_defer_handler(c, now, tcp_timer_run, keepalive_run, > + inactivity_run, qpair); > =20 > flow_defer_handler(c, now, timer_run, qpair); > fwd_scan_ports_timer(c, now); > @@ -130,16 +137,6 @@ static void random_init(struct ctx *c) > srandom(seed); > } > =20 > -/** > - * timer_init() - Set initial timestamp for timer runs to current time > - * @c: Execution context > - * @now: Current timestamp > - */ > -static void timer_init(struct ctx *c, const struct timespec *now) > -{ > - c->tcp.timer_run =3D *now; > -} > - > /** > * proto_update_l2_buf() - Update scatter-gather L2 buffers in protocol = handlers > * @eth_d: Ethernet destination address, NULL if unchanged > @@ -221,8 +218,9 @@ static void print_stats(const struct ctx *c, const st= ruct passt_stats *stats, > */ > static void passt_worker(void *opaque, int nfds, struct epoll_event *eve= nts) > { > + static time_t keepalive_run, inactivity_run; > static struct passt_stats stats =3D { 0 }; > - static struct timespec flow_timer_run; > + static struct timespec flow_timer_run, tcp_timer_run; > struct ctx *c =3D opaque; > struct timespec now; > int i; > @@ -306,7 +304,8 @@ static void passt_worker(void *opaque, int nfds, stru= ct epoll_event *events) > print_stats(c, &stats, &now); > } > =20 > - post_handler(c, &now, &flow_timer_run, QPAIR_DEFAULT); > + post_handler(c, &now, &flow_timer_run, &tcp_timer_run, > + &keepalive_run, &inactivity_run, QPAIR_DEFAULT); > =20 > migrate_handler(c); > } > @@ -433,8 +432,6 @@ int main(int argc, char **argv) > =20 > isolate_postfork(&c); > =20 > - timer_init(&c, &now); > - > loop: > /* NOLINTBEGIN(bugprone-branch-clone): intervals can be the same */ > /* cppcheck-suppress [duplicateValueTernary, unmatchedSuppression] */ > diff --git a/tcp.c b/tcp.c > index f4fe866ba7c3..955012355d69 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -2988,17 +2988,19 @@ int tcp_init(struct ctx *c) > /** > * tcp_keepalive() - Send keepalives for connections which need it > * @c: Execution context > + * @now: Current timestamp > + * @last_run: Last time keepalives ran, updated on run > * @qpair: Queue pair to process > */ > -static void tcp_keepalive(struct ctx *c, const struct timespec *now, > - unsigned int qpair) > +static void tcp_keepalive(const struct ctx *c, const struct timespec *no= w, > + time_t *last_run, unsigned int qpair) > { > union flow *flow; > =20 > - if (now->tv_sec - c->tcp.keepalive_run < KEEPALIVE_INTERVAL) > + if (now->tv_sec - *last_run < KEEPALIVE_INTERVAL) > return; > =20 > - c->tcp.keepalive_run =3D now->tv_sec; > + *last_run =3D now->tv_sec; > =20 > flow_foreach_of_type(flow, FLOW_TCP) { > struct tcp_tap_conn *conn =3D &flow->tcp; > @@ -3021,18 +3023,20 @@ static void tcp_keepalive(struct ctx *c, const st= ruct timespec *now, > /** > * tcp_inactivity() - Scan for and close long-inactive connections > * @c: Execution context > + * @now: Current timestamp > + * @last_run: Last time inactivity scan ran, updated on run > * @qpair: Queue pair to process > */ > -static void tcp_inactivity(struct ctx *c, const struct timespec *now, > - unsigned int qpair) > +static void tcp_inactivity(const struct ctx *c, const struct timespec *n= ow, > + time_t *last_run, unsigned int qpair) > { > union flow *flow; > =20 > - if (now->tv_sec - c->tcp.inactivity_run < INACTIVITY_INTERVAL) > + if (now->tv_sec - *last_run < INACTIVITY_INTERVAL) > return; > =20 > debug("TCP inactivity scan"); > - c->tcp.inactivity_run =3D now->tv_sec; > + *last_run =3D now->tv_sec; > =20 > flow_foreach_of_type(flow, FLOW_TCP) { > struct tcp_tap_conn *conn =3D &flow->tcp; > @@ -3054,27 +3058,34 @@ static void tcp_inactivity(struct ctx *c, const s= truct timespec *now, > =20 > /** > * tcp_defer_handler() - Handler for TCP deferred tasks > - * @c: Execution context > - * @now: Current timestamp > - * @qpair: Queue pair to process > + * @c: Execution context > + * @now: Current timestamp > + * @timer_run: Last time TCP timers ran > + * @keepalive_run: Last time keepalives ran > + * @inactivity_run: Last time inactivity scan ran > + * @qpair: Queue pair to process > */ > /* cppcheck-suppress [constParameterPointer, unmatchedSuppression] */ > void tcp_defer_handler(struct ctx *c, const struct timespec *now, > - unsigned int qpair) > + struct timespec *timer_run, time_t *keepalive_run, > + time_t *inactivity_run, unsigned int qpair) This one is TCP specific, but it's still pretty bulky. > { > - tcp_payload_flush(c); > + if (qpair =3D=3D 0) > + tcp_payload_flush(c); > =20 > - if (timespec_diff_ms(now, &c->tcp.timer_run) < TCP_TIMER_INTERVAL) > + if (timespec_diff_ms(now, timer_run) < TCP_TIMER_INTERVAL) > return; > =20 > - c->tcp.timer_run =3D *now; > + *timer_run =3D *now; > =20 > - tcp_sock_refill_init(c); > - if (c->mode =3D=3D MODE_PASTA) > - tcp_splice_refill(c); > + if (qpair =3D=3D 0) { > + tcp_sock_refill_init(c); > + if (c->mode =3D=3D MODE_PASTA) > + tcp_splice_refill(c); > + } > =20 > - tcp_keepalive(c, now, qpair); > - tcp_inactivity(c, now, qpair); > + tcp_keepalive(c, now, keepalive_run, qpair); > + tcp_inactivity(c, now, inactivity_run, qpair); > } > =20 > /** > diff --git a/tcp.h b/tcp.h > index 490f1b140e44..64c75ba481bd 100644 > --- a/tcp.h > +++ b/tcp.h > @@ -32,7 +32,8 @@ int tcp_listen(const struct ctx *c, uint8_t pif, unsign= ed rule, > const union inany_addr *addr, const char *ifname, in_port_t port= ); > int tcp_init(struct ctx *c); > void tcp_defer_handler(struct ctx *c, const struct timespec *now, > - unsigned int qpair); > + struct timespec *timer_run, time_t *keepalive_run, > + time_t *inactivity_run, unsigned int qpair); > =20 > void tcp_update_l2_buf(const unsigned char *eth_d); > =20 > @@ -42,24 +43,18 @@ extern bool peek_offset_cap; > * struct tcp_ctx - Execution context for TCP routines > * @scan_in: Port scanning state for inbound packets > * @scan_out: Port scanning state for outbound packets > - * @timer_run: Timestamp of most recent timer run > * @pipe_size: Size of pipes for spliced connections > * @rto_max: Maximum retry timeout (in s) > * @syn_retries: SYN retries using exponential backoff timeout > * @syn_linear_timeouts: SYN retries before using exponential backoff ti= meout > - * @keepalive_run: Time we last issued tap-side keepalives > - * @inactivity_run: Time we last scanned for inactive connections > */ > struct tcp_ctx { > struct fwd_scan scan_in; > struct fwd_scan scan_out; > - struct timespec timer_run; > size_t pipe_size; > int rto_max; > uint8_t syn_retries; > uint8_t syn_linear_timeouts; > - time_t keepalive_run; > - time_t inactivity_run; > }; > =20 > #endif /* TCP_H */ > --=20 > 2.54.0 >=20 --=20 David Gibson (he or they) | 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 --bL66LoCXAuOKdyfj Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpF0wsACgkQzQJF27ox 2Ge3Tw/+KYaGjT1Emg9E3stGRkfOVQOPoIhPn4Jy4ypnx6HC7AXi6EBsS7V5xUju 2mDnjFmYa1W5lfJerqguqmdZCcE0paMQ8gxjjj0Xb5UlZrkBVN/jo+Ly9lwkzpmb v7H6CssA8QkeE8UTS+kMlfXLOiLX4Z/Bdmfu+YBg7vJw8QaSYVGQRqShPR2mQHtQ 01g+M/dXdS8VkbB/0bW3ZxM9Ji6wedyJ2KjkSBYnyRdHOCzQlAdai0bJArZQrYtA EpCyDFtjykrQq25WAYngUOxAjNrOo40kD6Mt5quFZdzh3DbTt0WWxlUb6uTZew1R AvTsZoR/OUxBI1kV31jggb/f56EPO2+MQcM4/0zOiMtbGngK1KeQofGgN4S1Kq51 vY/Clo6QMdygCKt9jkUm+LAUnHRMhuq7uehlrfLgaMmfDb+ZF+iqEPUNKJ9gp7jh aUKPRofz+S8RXfJcuq4jA6SupxohWwoYVotDhVnzIxssY4EAs3n2IuOKpCwnZ4Y5 yoXCUWJdrovzToNv1KJ6ZPAGQkNdOL0oFL/1HeiXQT7qa1rIykSzaBxCtpxro1YT Fh8hUvdfCs/QIaAHenWNjUEHyRuPXDOuBImZnSpE0V27QtgqV9EmTk87me7+8q6W 6SPWduA3XD6McazXQKBo7E3OcNHKSuSfk6bV3xiL9+CE5hZZtDw= =vN82 -----END PGP SIGNATURE----- --bL66LoCXAuOKdyfj--