Date: Thu, 6 Sep 2012 09:23:32 +0300 From: Mikolaj Golub <to.my.trociny@gmail.com> To: Robert Watson <rwatson@FreeBSD.org> Cc: svn-src-stable@freebsd.org, svn-src-all@freebsd.org, "Bjoern A. Zeeb" <bz@FreeBSD.org>, src-committers@freebsd.org, svn-src-stable-8@freebsd.org Subject: Re: svn commit: r239983 - stable/8/sys/netinet Message-ID: <20120906062330.GA94205@gmail.com> In-Reply-To: <alpine.BSF.2.00.1209051928460.5204@fledge.watson.org> References: <201209011033.q81AXsGb094283@svn.freebsd.org> <alpine.BSF.2.00.1209051559010.98832@ai.fobar.qr> <alpine.BSF.2.00.1209051928460.5204@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--h31gzZEtNLTqOjlF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Wed, Sep 05, 2012 at 07:31:19PM +0100, Robert Watson wrote:
> 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.
Thanks! I am going to direct commit this patch to fix the mis-merge.
Sorry, will be much more careful with such things next time.
--
Mikolaj Golub
--h31gzZEtNLTqOjlF
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: inline; filename="tcp_timer.c.stable8.diff"
Index: stable/8/sys/netinet/tcp_timer.c
===================================================================
--- stable/8/sys/netinet/tcp_timer.c (revision 240156)
+++ stable/8/sys/netinet/tcp_timer.c (working copy)
@@ -494,7 +494,7 @@
callout_deactivate(&tp->t_timers->tt_rexmt);
if ((inp->inp_flags & INP_DROPPED) != 0) {
INP_WUNLOCK(inp);
- INP_INFO_RUNLOCK(&V_tcbinfo);
+ INP_INFO_WUNLOCK(&V_tcbinfo);
CURVNET_RESTORE();
return;
}
--h31gzZEtNLTqOjlF--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120906062330.GA94205>