From nobody Thu Oct 24 17:14:39 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4XZCFq2VQTz5ZgBs; Thu, 24 Oct 2024 17:14:39 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4XZCFq1nwRz4cVY; Thu, 24 Oct 2024 17:14:39 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1729790079; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=8fTdezkn95zKkjXlkTYqMQt4tncAFLcQgMEXb0cYPN0=; b=amhk3R+1lMLhh6UZ7yafIlOm8d84TzuiOnbSJbLWByzKnsiNtpTzTPf4+GeKXsKLLXy8tF fQfrQL5nPci28t0EKvoVZb+xx9ROWXlylKsJXk37fsmBzGVD0huDku+GQHxqHP5SP0UT5L A1RXe4q2aeD70fLP3RjoNCiwlFRuwgZNOQeEmGqC9SoCeITimRtaSNeVbAXfEF8p77Oryx EbDy8jL97HRUj5H3xMi7jPA+HKKyDdyllI11GJ9rHTKaHETggre5ZfhRuefAWt3vgIYTUN KkMIJqheqzJr+Mlterfevwc3vInyI/Gdg/eLJJvvhK6rCkUEy5bXJFN4oCbgYg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1729790079; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=8fTdezkn95zKkjXlkTYqMQt4tncAFLcQgMEXb0cYPN0=; b=OT3xdH5CYc3r8rOEACtJONqEqUOxAWml51TxugnE5cyZOQMXv4DuqYMPE/kYbFIy+0Gbgk WKEuePhffqNJB6SxgQ9m3VrDZNfhzERVR0ceZ/scReg++y0e567pcMIJ1DHyPg/FV3zO6b nvCw/KpQhakbtbIUMyUtrcgLGQ7WtgNXWUBOmEFcscdp/x1CIvo33hWSo8BmR83fowl4IQ agNqEBK40SZfcHuH5ixNRMInMSX68eMbUUqv4zuIt7gBddOZWl5S6jRruuH56KL0mEEUBb 4O0x1w0R1UFttqgfmz/uqy75UEmNhAqeShDnHh9WjUQE4in0shPe3+95dwrCCQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1729790079; a=rsa-sha256; cv=none; b=nKWVrv1GfuuG3dfdbySBwW2wv40nwJViKkxR37ptut29Ut0fLLS3aS87HeSxaH2uJ4uesa qkV5FQJhL6r/OxHpnPOlx0RDj8DhLkGV+7z8lAmmIvTjNIr78gQEM7z4hKeXkFsQMRskrk UHlrUw9ehyGqe4jWuj9mg8z6/i4LfSrRtAGr5jv3JAmTttP4eZ9Av4L/ih/X4jZintKLtF n7B1chOj3/dV/GDgERJHcY88Cg+ErIk+6fsoVwKW/A41c2RnzRFyBj57oql4cWN8zf0i/z 0F0tqm517qOUhsvXN6P0bcGylgK4BE4j6a8W6qiZvMgoXL6XbTEUAxZwtAmKWg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4XZCFq19lczrJT; Thu, 24 Oct 2024 17:14:39 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 49OHEdlW000377; Thu, 24 Oct 2024 17:14:39 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 49OHEdL2000374; Thu, 24 Oct 2024 17:14:39 GMT (envelope-from git) Date: Thu, 24 Oct 2024 17:14:39 GMT Message-Id: <202410241714.49OHEdL2000374@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Gleb Smirnoff Subject: git: bffebc336f4e - main - tcp: use CALLOUT_TRYLOCK for the TCP callout List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: glebius X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: bffebc336f4ece4d18774c1ab8f555802cebf961 Auto-Submitted: auto-generated The branch main has been updated by glebius: URL: https://cgit.FreeBSD.org/src/commit/?id=bffebc336f4ece4d18774c1ab8f555802cebf961 commit bffebc336f4ece4d18774c1ab8f555802cebf961 Author: Gleb Smirnoff AuthorDate: 2024-10-24 16:58:37 +0000 Commit: Gleb Smirnoff CommitDate: 2024-10-24 17:14:03 +0000 tcp: use CALLOUT_TRYLOCK for the TCP callout This allows to remove the drop of the lock tcp_timer_enter(), which closes a sophisticated but possible race that involves three threads. In case we got a callout executing and two threads trying to close the connection, e.g. and interrupt and a syscall, then lock yielding in tcp_timer_enter() may transfer lock from one closing thread to the other closing thread, instead of the callout. Reviewed by: jtl Differential Revision: https://reviews.freebsd.org/D45747 --- sys/netinet/tcp_subr.c | 3 ++- sys/netinet/tcp_timer.c | 39 +++++++++++++++------------------------ 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c index 9b5f2651fb35..668d218b34a8 100644 --- a/sys/netinet/tcp_subr.c +++ b/sys/netinet/tcp_subr.c @@ -2309,7 +2309,8 @@ tcp_newtcpcb(struct inpcb *inp, struct tcpcb *listening_tcb) tp->t_hpts_cpu = HPTS_CPU_NONE; tp->t_lro_cpu = HPTS_CPU_NONE; - callout_init_rw(&tp->t_callout, &inp->inp_lock, CALLOUT_RETURNUNLOCKED); + callout_init_rw(&tp->t_callout, &inp->inp_lock, + CALLOUT_TRYLOCK | CALLOUT_RETURNUNLOCKED); for (int i = 0; i < TT_N; i++) tp->t_timers[i] = SBT_MAX; diff --git a/sys/netinet/tcp_timer.c b/sys/netinet/tcp_timer.c index ae6b97c09cdf..ae4753b2523f 100644 --- a/sys/netinet/tcp_timer.c +++ b/sys/netinet/tcp_timer.c @@ -949,35 +949,26 @@ tcp_timer_active(struct tcpcb *tp, tt_which which) /* * Stop all timers associated with tcpcb. - * * Called when tcpcb moves to TCPS_CLOSED. - * - * XXXGL: unfortunately our callout(9) is not able to fully stop a locked - * callout even when only two threads are involved: the callout itself and the - * thread that does callout_stop(). See where softclock_call_cc() swaps the - * callwheel lock to callout lock and then checks cc_exec_cancel(). This is - * the race window. If it happens, the tcp_timer_enter() won't be executed, - * however pcb lock will be locked and released, hence we can't free memory. - * Until callout(9) is improved, just keep retrying. In my profiling I've seen - * such event happening less than 1 time per hour with 20-30 Gbit/s of traffic. */ void tcp_timer_stop(struct tcpcb *tp) { - struct inpcb *inp = tptoinpcb(tp); - INP_WLOCK_ASSERT(inp); - - if (curthread->td_pflags & TDP_INTCPCALLOUT) { - int stopped __diagused; + INP_WLOCK_ASSERT(tptoinpcb(tp)); - stopped = callout_stop(&tp->t_callout); - MPASS(stopped == 0); - for (tt_which i = 0; i < TT_N; i++) - tp->t_timers[i] = SBT_MAX; - } else while(__predict_false(callout_stop(&tp->t_callout) == 0)) { - INP_WUNLOCK(inp); - kern_yield(PRI_UNCHANGED); - INP_WLOCK(inp); - } + /* + * We don't check return value from callout_stop(). There are two + * reasons why it can return 0. First, a legitimate one: we could have + * been called from the callout itself. Second, callout(9) has a bug. + * It can race internally in softclock_call_cc(), when callout has + * already completed, but cc_exec_curr still points at the callout. + */ + (void )callout_stop(&tp->t_callout); + /* + * In case of being called from callout itself, we must make sure that + * we don't reschedule. + */ + for (tt_which i = 0; i < TT_N; i++) + tp->t_timers[i] = SBT_MAX; }