svn commit: r302099 - head/sys/netinet

[Marko Zec]

On Thu, 23 Jun 2016 00:34:03 +0000
"Bjoern A. Zeeb" <bz at freebsd.org> wrote:

> Author: bz
> Date: Thu Jun 23 00:34:03 2016
> New Revision: 302099
> URL: https://svnweb.freebsd.org/changeset/base/302099
> 
> Log:
>   Check the V_tcbinfo.ipi_count to hit 0 before doing the full TCP
> cleanup. That way timers can finish cleanly and we do not gamble with
> a DELAY(). 
>   Reviewed by:		gnn, jtl
>   Approved by:		re (gjb)
>   Obtained from:		projects/vnet
>   MFC after:		2 weeks
>   Sponsored by:		The FreeBSD Foundation
>   Differential Revision:	https://reviews.freebsd.org/D6923

As much as this change is welcome, it unnecesarily introduces a
mandatory 100 ms delay on each vnet teardown, which I already pointed
out in a comment to r301601 two weeks ago, which remained unanswered,
along with the question why a delay of 100 ms was introduced here, when
before r302099 the delay was only a single clock tick?  And furthermore
the delay computation expresion here is not style(9) compliant...

Hence, please rectify the above objections, perhaps by something like:

===================================================================
--- tcp_subr.c  (revision 302126)
+++ tcp_subr.c  (working copy)
@@ -739,10 +739,11 @@
         * Sleep to let all tcpcb timers really disappear and cleanup.
         */
        do {
-               pause("tcpdes", hz/10);
                INP_LIST_RLOCK(&V_tcbinfo);
                n = V_tcbinfo.ipi_count;
                INP_LIST_RUNLOCK(&V_tcbinfo);
+               if (n != 0)
+                       pause("tcpdes", hz / 100);
        } while (n != 0);
        tcp_hc_destroy();
        syncache_destroy();

Thanks,

Marko


> 
> Modified:
>   head/sys/netinet/tcp_subr.c
> 
> Modified: head/sys/netinet/tcp_subr.c
> ==============================================================================
> --- head/sys/netinet/tcp_subr.c	Thu Jun 23 00:32:58
> 2016	(r302098) +++ head/sys/netinet/tcp_subr.c	Thu Jun
> 23 00:34:03 2016	(r302099) @@ -731,18 +731,19 @@ tcp_init(void)
>  static void
>  tcp_destroy(void *unused __unused)
>  {
> -	int error;
> +	int error, n;
>  
>  	/*
>  	 * All our processes are gone, all our sockets should be
> cleaned
>  	 * up, which means, we should be past the tcp_discardcb()
> calls.
> -	 * Sleep to let all tcpcb timers really disappear and then
> cleanup.
> -	 * Timewait will cleanup its queue and will be ready to go.
> -	 * XXX-BZ In theory a few ticks should be good enough to
> make sure
> -	 * the timers are all really gone.  We should see if we
> could use a
> -	 * better metric here and, e.g., check a tcbcb count as an
> optimization?
> +	 * Sleep to let all tcpcb timers really disappear and
> cleanup. */
> -	DELAY(1000000 / hz);
> +	do {
> +		pause("tcpdes", hz/10);
> +		INP_LIST_RLOCK(&V_tcbinfo);
> +		n = V_tcbinfo.ipi_count;
> +		INP_LIST_RUNLOCK(&V_tcbinfo);
> +	} while (n != 0);
>  	tcp_hc_destroy();
>  	syncache_destroy();
>  	tcp_tw_destroy();
>