Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 11 Apr 2014 04:07:14 +0200
From:      Julien Charbon <jcharbon@verisign.com>
To:        Gleb Smirnoff <glebius@FreeBSD.org>, John Baldwin <jhb@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
Subject:   Re: svn commit: r264321 - head/sys/netinet
Message-ID:  <53474E52.6080809@verisign.com>
In-Reply-To: <20140410192920.GT44326@FreeBSD.org>
References:  <201404101815.s3AIFZx3065541@svn.freebsd.org> <20140410192920.GT44326@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help 

  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

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53474E52.6080809>