Date: Wed, 5 Sep 2012 19:31:19 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: "Bjoern A. Zeeb" <bz@FreeBSD.org> Cc: Mikolaj Golub <trociny@FreeBSD.org>, 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 Message-ID: <alpine.BSF.2.00.1209051928460.5204@fledge.watson.org> In-Reply-To: <alpine.BSF.2.00.1209051559010.98832@ai.fobar.qr> References: <201209011033.q81AXsGb094283@svn.freebsd.org> <alpine.BSF.2.00.1209051559010.98832@ai.fobar.qr>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.1209051928460.5204>