From owner-svn-src-head@FreeBSD.ORG Fri Apr 11 07:15:09 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E1B376A5 for ; Fri, 11 Apr 2014 07:15:08 +0000 (UTC) Received: from plane.gmane.org (plane.gmane.org [80.91.229.3]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 9A94F1F7F for ; Fri, 11 Apr 2014 07:15:08 +0000 (UTC) Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1WYVfz-0003vK-Dh for svn-src-head@freebsd.org; Fri, 11 Apr 2014 09:15:03 +0200 Received: from 217.30.88.7 ([217.30.88.7]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Fri, 11 Apr 2014 09:15:03 +0200 Received: from jcharbon by 217.30.88.7 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Fri, 11 Apr 2014 09:15:03 +0200 X-Injected-Via-Gmane: http://gmane.org/ To: svn-src-head@freebsd.org From: Julien Charbon Subject: Re: svn commit: r264321 - head/sys/netinet Date: Fri, 11 Apr 2014 04:07:14 +0200 Lines: 101 Message-ID: <53474E52.6080809@verisign.com> References: <201404101815.s3AIFZx3065541@svn.freebsd.org> <20140410192920.GT44326@FreeBSD.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: 217.30.88.7 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 In-Reply-To: <20140410192920.GT44326@FreeBSD.org> Cc: adrian@FreeBSD.org, src-committers@freebsd.org, svn-src-all@freebsd.org, rrs@FreeBSD.org, rwatson@FreeBSD.org, svn-src-head@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Apr 2014 07:15:09 -0000 Hi Gleb, On 10/04/14 21:29, Gleb Smirnoff wrote: > 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. That's indeed a good point. When I started prototyping this change having TW_WLOCK in tw_pcbrele() made sense but not anymore. Below a patch that removes TW_WLOCK from tw_pcbrele(), I will do another review pass with this change. Thanks. diff --git a/sys/netinet/tcp_timewait.c b/sys/netinet/tcp_timewait.c index c455be8..1aff6b4 100644 --- a/sys/netinet/tcp_timewait.c +++ b/sys/netinet/tcp_timewait.c @@ -131,22 +131,17 @@ tw_pcbref(struct tcptw *tw) } /* - * Drop a refcount on an tw elevated using tw_pcbref(). Return - * the tw lock released. + * Drop a refcount on an tw elevated using tw_pcbref(). */ static int tw_pcbrele(struct tcptw *tw) { - TW_WLOCK_ASSERT(V_tw_lock); KASSERT(tw->tw_refcount > 0, ("%s: refcount 0", __func__)); - if (!refcount_release(&tw->tw_refcount)) { - TW_WUNLOCK(V_tw_lock); + if (!refcount_release(&tw->tw_refcount)) return (0); - } uma_zfree(V_tcptw_zone, tw); - TW_WUNLOCK(V_tw_lock); return (1); } @@ -680,10 +675,8 @@ tcp_tw_2msl_stop(struct tcptw *tw, int reuse) crfree(tw->tw_cred); tw->tw_cred = NULL; - if (!reuse) { + if (!reuse) tw_pcbrele(tw); - return; - } TW_WUNLOCK(V_tw_lock); } @@ -728,7 +721,6 @@ tcp_tw_2msl_scan(void) /* Close timewait state */ if (INP_INFO_TRY_WLOCK(&V_tcbinfo)) { - TW_WLOCK(V_tw_lock); if (tw_pcbrele(tw)) continue; @@ -740,7 +732,6 @@ tcp_tw_2msl_scan(void) INP_INFO_WUNLOCK(&V_tcbinfo); } else { /* INP_INFO lock is busy, continue later */ - TW_WLOCK(V_tw_lock); tw_pcbrele(tw); break; } -- Julien