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
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "[email protected]"
