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