From owner-svn-src-head@FreeBSD.ORG Fri Apr 11 02:07:51 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 4041A9D4; Fri, 11 Apr 2014 02:07:51 +0000 (UTC) Received: from exprod6og126.obsmtp.com (exprod6og126.obsmtp.com [64.18.1.77]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 34AF111C9; Fri, 11 Apr 2014 02:07:49 +0000 (UTC) Received: from peregrine.verisign.com ([216.168.239.74]) (using TLSv1) by exprod6ob126.postini.com ([64.18.5.12]) with SMTP ID DSNKU0dOb94idoz8irFJxq3PJknSWsHrtjNy@postini.com; Thu, 10 Apr 2014 19:07:50 PDT Received: from DUL1WNSMTP01.vcorp.ad.vrsn.com (dul1wnexcn03.vcorp.ad.vrsn.com [10.170.12.113]) by peregrine.verisign.com (8.13.6/8.13.4) with ESMTP id s3B27dOv024342; Thu, 10 Apr 2014 22:07:43 -0400 Received: from fri2spoulet-l1.vcorp.ad.vrsn.com ([10.100.64.12]) by DUL1WNSMTP01.vcorp.ad.vrsn.com with Microsoft SMTPSVC(7.5.7601.17514); Thu, 10 Apr 2014 22:07:38 -0400 Message-ID: <53474E52.6080809@verisign.com> Date: Fri, 11 Apr 2014 04:07:14 +0200 From: Julien Charbon User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 Newsgroups: gmane.os.freebsd.current.scm To: Gleb Smirnoff , John Baldwin Subject: Re: svn commit: r264321 - head/sys/netinet References: <201404101815.s3AIFZx3065541@svn.freebsd.org> <20140410192920.GT44326@FreeBSD.org> In-Reply-To: <20140410192920.GT44326@FreeBSD.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 11 Apr 2014 02:07:39.0518 (UTC) FILETIME=[D0AF71E0:01CF552A] 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 02:07:51 -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