From owner-svn-src-all@FreeBSD.ORG Thu Apr 10 20:06:12 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E56EAF2D; Thu, 10 Apr 2014 20:06:12 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id BA4341D92; Thu, 10 Apr 2014 20:06:12 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 72E0DB96E; Thu, 10 Apr 2014 16:06:11 -0400 (EDT) From: John Baldwin To: Gleb Smirnoff Subject: Re: svn commit: r264321 - head/sys/netinet Date: Thu, 10 Apr 2014 16:04:00 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20130906; KDE/4.5.5; amd64; ; ) References: <201404101815.s3AIFZx3065541@svn.freebsd.org> <20140410192920.GT44326@FreeBSD.org> In-Reply-To: <20140410192920.GT44326@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201404101604.01178.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 10 Apr 2014 16:06:11 -0400 (EDT) Cc: adrian@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, Julien Charbon , rrs@freebsd.org, rwatson@freebsd.org, svn-src-head@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Apr 2014 20:06:13 -0000 On Thursday, April 10, 2014 3:29:20 pm Gleb Smirnoff wrote: > Hi, > > one comment: > > On Thu, Apr 10, 2014 at 06:15:35PM +0000, John Baldwin wrote: > J> +/* > J> + * Drop a refcount on an tw elevated using tw_pcbref(). Return > J> + * the tw lock released. > J> + */ > J> +static int > J> +tw_pcbrele(struct tcptw *tw) > J> +{ > J> + > J> + TW_WLOCK_ASSERT(V_tw_lock); > J> + KASSERT(tw->tw_refcount > 0, ("%s: refcount 0", __func__)); > J> + > J> + if (!refcount_release(&tw->tw_refcount)) { > J> + TW_WUNLOCK(V_tw_lock); > J> + return (0); > J> + } > J> + > J> + uma_zfree(V_tcptw_zone, tw); > J> + TW_WUNLOCK(V_tw_lock); > J> + return (1); > J> +} > > We can unlock before uma_zfree(), that would reduce contention. > > Why do we need the lock in this function at all? We don't manipulate > the TAILQ. I believe you are correct, and it would fix one of the locking assymetries I had noted in my review. Humm, and now that I look again, I think I see a lock leak in scan() if you lose the race on freeing the timewait structure: Index: tcp_timewait.c =================================================================== --- tcp_timewait.c (revision 264321) +++ tcp_timewait.c (working copy) @@ -731,8 +731,10 @@ tcp_tw_2msl_scan(void) /* Close timewait state */ if (INP_INFO_TRY_WLOCK(&V_tcbinfo)) { TW_WLOCK(V_tw_lock); - if (tw_pcbrele(tw)) + if (tw_pcbrele(tw)) { + INP_INFO_WUNLOCK(&V_tcbinfo); continue; + } KASSERT(tw->tw_inpcb != NULL, ("%s: tw->tw_inpcb == NULL", __func__)); -- John Baldwin