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=202608 header.b=BgHB1L5x; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 4645C5A026E for ; Mon, 03 Aug 2026 08:41:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1785739263; bh=6CbPzfl5j8Bd/CnkLo+glWxiNAVvXUpizYd9WhcF4C0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BgHB1L5xAk6gqecueEi4/hTsh2/XvoAlFt8MyC3azUlgvQ9A3XPtX41c50P0apzCy /gpbCQTEuPPtLmj9xbyc6DZHKuLPGK/0BaCLVOgwU4J94Q2Jl7wcJpsmcOMTtWMnKc NGaLC37LBRg3gN8uLEGwK80PZrkxw50cjux9KB60K+OdwfrCMNj353HPSCO6jmySJP eoo/xh/vcj/5Rak51nF0jZIg8NbdnUEvrTLyWJzyK++HqKAW+CxWoIM7078s7TNmdw GJu1jfAAfLi8/lxg4R/qmbZN8FSJZPUnDQ0XpCAapGiceGuPJvyJC/10mARwGEnVPb v+1CsmDi44XiQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hD6Wg2C4fz4wCJ; Mon, 03 Aug 2026 16:41:03 +1000 (AEST) Date: Mon, 3 Aug 2026 16:40:52 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v2 06/10] tcp: Make TCP timer state per-caller and guard global tasks Message-ID: References: <20260731162329.3552800-1-lvivier@redhat.com> <20260731162329.3552800-7-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="KU0HRb5pMsx09XE9" Content-Disposition: inline In-Reply-To: <20260731162329.3552800-7-lvivier@redhat.com> Message-ID-Hash: HVOBCM5PRVTX7V72F3RKQUJ6O5RXWR4R X-Message-ID-Hash: HVOBCM5PRVTX7V72F3RKQUJ6O5RXWR4R 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: --KU0HRb5pMsx09XE9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 31, 2026 at 06:23:25PM +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 This patch has slightly different variants of the conerns I raised on the previous one. Unlike the flow_defer and flow_timer functions, the keepalive and inactivity handling absolutely *does* rely on not being called for the same flow more often than once every INTERVAL s. If it happens twice quickly due to a migration we could incorrectly time out a flow much earlier than it shuold be. That can be fixed pretty easily by setting conn->tap_inactive and conn->inactive to false on migration (I haven't looked ahead to see if you already do that). That would have the cost of maybe sometimes delaying the keepalive or inactivity handling longer than it should be. Again, probably harmless in practice (although as noted on the previous patch, in theory the delay could be indefinite, if the flow keeps migrating). > --- > 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 69fd7b12450a..25e1212b9452 100644 > --- a/passt.c > +++ b/passt.c > @@ -106,16 +106,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) > { > 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); > @@ -140,16 +147,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 > @@ -231,8 +228,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; > @@ -316,7 +314,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, &now); > } > @@ -453,8 +452,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 ee4ec599e055..ab7cbfa5de83 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -2989,17 +2989,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; > @@ -3022,18 +3024,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; > @@ -3055,27 +3059,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) > { > - tcp_payload_flush(c, now); > + if (qpair =3D=3D 0) > + tcp_payload_flush(c, now); > =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 eb4f8994f4b7..1500fae5162d 100644 > --- a/tcp.h > +++ b/tcp.h > @@ -31,7 +31,8 @@ int tcp_tap_handler(const struct ctx *c, unsigned int q= pair, uint8_t pif, > const struct timespec *now); > 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 > @@ -41,24 +42,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 --KU0HRb5pMsx09XE9 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpwN/AACgkQzQJF27ox 2GdB1A/9H3QS8g4jLW6w/CvY6QrBpbZ6HcOqPND6ViBycbu8z+MQuYDYoZhaNIVZ CcW9Qzj5skQpik7V1I3g9ypujYk+BM/+Xd43R8yfHbZzMVHyVsxMplqj60FbRhUa TGm/cDuHL+UmXJdkzmQbecUePL9zbqq1ejvZ9OB/3kU2bRVlcGF8NPvQTXwUnsfg T+q3JEj5hygC+LvUN68fXanAGhZ7PPYynRaAqwbVc8AoH/O6JhIcosDvJ/sApjIr j/H8JD69e/QRRxmBwZ2S62uBBubuIJgVBaGHqJvj0XUf0TLqXdp1Dsmpg9feKNBa frH6Fwebf1bZibeSqo8IadGPAHUNRdB4ExtxdOZnwgFdHj135U/wcspK++d4bVPC W2EDf9nhCtomp/d3i+eFOO35iXOo3CvQH5xd2nSQfzX/AsFStBtkIN9mR+SHNhlA DQi3ICgWLX+iHhJtG2NWxCX+DVpYh4+nWiSiPZYwmn9QZgGLxyNbk9vvRU2s2uA5 xrvVLmXV+FxpRf8jZmMDmMM2Y0RJOeN/BSv1Sde96zLH8LFUgIZAdeiBx92HeFmO fvq44O0FC8+4hGIP/rpZ4S4OBpOoYI02RhqiSWcJAriS4BQQKRmcn8rvTLaz4vo0 S63rVLAWaPUtOqeO8vIlomOjC2V310eNFnF7lHhozUftH4Qk3Ug= =iHmN -----END PGP SIGNATURE----- --KU0HRb5pMsx09XE9--