svn commit: r264321 - head/sys/netinet

[John Baldwin]

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