Date: Thu, 10 Apr 2014 16:04:00 -0400 From: John Baldwin <jhb@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: adrian@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, Julien Charbon <jcharbon@verisign.com>, rrs@freebsd.org, rwatson@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r264321 - head/sys/netinet Message-ID: <201404101604.01178.jhb@freebsd.org> 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
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201404101604.01178.jhb>