From owner-svn-src-stable@FreeBSD.ORG Wed Sep 5 18:31:20 2012 Return-Path: Delivered-To: svn-src-stable@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 85D95106564A; Wed, 5 Sep 2012 18:31:20 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 0B1E98FC14; Wed, 5 Sep 2012 18:31:20 +0000 (UTC) Received: from fledge.watson.org (fledge.watson.org [65.122.17.41]) by cyrus.watson.org (Postfix) with ESMTPS id B4E2C46B09; Wed, 5 Sep 2012 14:31:19 -0400 (EDT) Date: Wed, 5 Sep 2012 19:31:19 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: "Bjoern A. Zeeb" In-Reply-To: Message-ID: References: <201209011033.q81AXsGb094283@svn.freebsd.org> User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Mikolaj Golub , svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, svn-src-stable-8@freebsd.org Subject: Re: svn commit: r239983 - stable/8/sys/netinet X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Sep 2012 18:31:20 -0000 On Wed, 5 Sep 2012, Bjoern A. Zeeb wrote: >> Log: >> MFC r239075: >> >> In tcp timers, check INP_DROPPED flag a little later, after >> callout_deactivate(), so if INP_DROPPED is set we return with the >> timer active flag cleared. >> >> For me this fixes negative keep timer values reported by `netstat -x' >> for connections in CLOSE state. > > panic: Lock tcp not read locked @ /w/src/sys/netinet/tcp_timer.c:497 > > reproducable on the cluster. Probably wrong all the way up to HEAD? This looks like a mis-merge -- in 8.x, read-locking of the inpcbinfo is not used in the TCP timer code, whereas in 9.x and later it is. There are important and subtle differences between inpcb/inpcb/inpcbhash locking across all supported major FreeBSD versions, as we have substantially refined our approach to improve performance and stability over the years. As a result, it is generally not safe to MFC TCP/UDP locking changes without extreme care. I would recommend seeking at least one, if not multiple, reviewers for changes and MFCs along these lines. Even with you and others doing line-by-line reviews of much of the original work, we ran into quite a few bugs due to the complexity and difficulty in reviewing the code. Robert > > >> Modified: >> stable/8/sys/netinet/tcp_timer.c >> Directory Properties: >> stable/8/sys/ (props changed) >> >> Modified: stable/8/sys/netinet/tcp_timer.c >> ============================================================================== >> --- stable/8/sys/netinet/tcp_timer.c Sat Sep 1 10:32:40 2012 >> (r239982) >> +++ stable/8/sys/netinet/tcp_timer.c Sat Sep 1 10:33:53 2012 >> (r239983) >> @@ -176,13 +176,18 @@ tcp_timer_delack(void *xtp) >> return; >> } >> INP_WLOCK(inp); >> - if ((inp->inp_flags & INP_DROPPED) || >> callout_pending(&tp->t_timers->tt_delack) >> - || !callout_active(&tp->t_timers->tt_delack)) { >> + if (callout_pending(&tp->t_timers->tt_delack) || >> + !callout_active(&tp->t_timers->tt_delack)) { >> INP_WUNLOCK(inp); >> CURVNET_RESTORE(); >> return; >> } >> callout_deactivate(&tp->t_timers->tt_delack); >> + if ((inp->inp_flags & INP_DROPPED) != 0) { >> + INP_WUNLOCK(inp); >> + CURVNET_RESTORE(); >> + return; >> + } >> >> tp->t_flags |= TF_ACKNOW; >> TCPSTAT_INC(tcps_delack); >> @@ -222,7 +227,7 @@ tcp_timer_2msl(void *xtp) >> } >> INP_WLOCK(inp); >> tcp_free_sackholes(tp); >> - if ((inp->inp_flags & INP_DROPPED) || >> callout_pending(&tp->t_timers->tt_2msl) || >> + if (callout_pending(&tp->t_timers->tt_2msl) || >> !callout_active(&tp->t_timers->tt_2msl)) { >> INP_WUNLOCK(tp->t_inpcb); >> INP_INFO_WUNLOCK(&V_tcbinfo); >> @@ -230,6 +235,12 @@ tcp_timer_2msl(void *xtp) >> return; >> } >> callout_deactivate(&tp->t_timers->tt_2msl); >> + if ((inp->inp_flags & INP_DROPPED) != 0) { >> + INP_WUNLOCK(inp); >> + INP_INFO_WUNLOCK(&V_tcbinfo); >> + CURVNET_RESTORE(); >> + return; >> + } >> /* >> * 2 MSL timeout in shutdown went off. If we're closed but >> * still waiting for peer to close and connection has been idle >> @@ -293,14 +304,20 @@ tcp_timer_keep(void *xtp) >> return; >> } >> INP_WLOCK(inp); >> - if ((inp->inp_flags & INP_DROPPED) || >> callout_pending(&tp->t_timers->tt_keep) >> - || !callout_active(&tp->t_timers->tt_keep)) { >> + if (callout_pending(&tp->t_timers->tt_keep) || >> + !callout_active(&tp->t_timers->tt_keep)) { >> INP_WUNLOCK(inp); >> INP_INFO_WUNLOCK(&V_tcbinfo); >> CURVNET_RESTORE(); >> return; >> } >> callout_deactivate(&tp->t_timers->tt_keep); >> + if ((inp->inp_flags & INP_DROPPED) != 0) { >> + INP_WUNLOCK(inp); >> + INP_INFO_WUNLOCK(&V_tcbinfo); >> + CURVNET_RESTORE(); >> + return; >> + } >> /* >> * Keep-alive timer went off; send something >> * or drop connection if idle for too long. >> @@ -388,14 +405,20 @@ tcp_timer_persist(void *xtp) >> return; >> } >> INP_WLOCK(inp); >> - if ((inp->inp_flags & INP_DROPPED) || >> callout_pending(&tp->t_timers->tt_persist) >> - || !callout_active(&tp->t_timers->tt_persist)) { >> + if (callout_pending(&tp->t_timers->tt_persist) || >> + !callout_active(&tp->t_timers->tt_persist)) { >> INP_WUNLOCK(inp); >> INP_INFO_WUNLOCK(&V_tcbinfo); >> CURVNET_RESTORE(); >> return; >> } >> callout_deactivate(&tp->t_timers->tt_persist); >> + if ((inp->inp_flags & INP_DROPPED) != 0) { >> + INP_WUNLOCK(inp); >> + INP_INFO_WUNLOCK(&V_tcbinfo); >> + CURVNET_RESTORE(); >> + return; >> + } >> /* >> * Persistance timer into zero window. >> * Force a byte to be output, if possible. >> @@ -461,14 +484,20 @@ tcp_timer_rexmt(void * xtp) >> return; >> } >> INP_WLOCK(inp); >> - if ((inp->inp_flags & INP_DROPPED) || >> callout_pending(&tp->t_timers->tt_rexmt) >> - || !callout_active(&tp->t_timers->tt_rexmt)) { >> + if (callout_pending(&tp->t_timers->tt_rexmt) || >> + !callout_active(&tp->t_timers->tt_rexmt)) { >> INP_WUNLOCK(inp); >> INP_INFO_WUNLOCK(&V_tcbinfo); >> CURVNET_RESTORE(); >> return; >> } >> callout_deactivate(&tp->t_timers->tt_rexmt); >> + if ((inp->inp_flags & INP_DROPPED) != 0) { >> + INP_WUNLOCK(inp); >> + INP_INFO_RUNLOCK(&V_tcbinfo); >> + CURVNET_RESTORE(); >> + return; >> + } >> tcp_free_sackholes(tp); >> /* >> * Retransmission timer went off. Message has not >> > > -- > Bjoern A. Zeeb You have to have visions! > Stop bit received. Insert coin for new address family. >