Re: [PATCH v3 net-next 16/16] tcp_bbr: add BBR congestion control

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 19 Sep 2016 14:10:39 -0700
Eric Dumazet <edumazet@google.com> wrote:

> On Mon, Sep 19, 2016 at 1:57 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> 
> > Looks good, but could I suggest a simple optimization.
> > All these parameters are immutable in the version of BBR you are submitting.
> > Why not make the values const? And eliminate the always true long-term bw estimate
> > variable?
> >  
> 
> We could do that.
> 
> We used to have variables (aka module params) while BBR was cooking in
> our kernels ;)
> 
> Are you sure generated code is indeed 'optimized' ?


It generates some slightly smaller code.
 	if (bbr->lt_rtt_cnt < bbr_lt_intvl_min_rtts)
- 3e7:	0f b6 c0             	movzbl %al,%eax
- 3ea:	83 f8 03             	cmp    $0x3,%eax
- 3ed:	0f 86 d4 00 00 00    	jbe    4c7 <bbr_lt_bw_sampling.isra.6+0x157>
+ 3e7:	3c 03                	cmp    $0x3,%al
+ 3e9:	0f 86 d1 00 00 00    	jbe    4c0 <bbr_lt_bw_sampling.isra.6+0x150>

And different code for abs
 		/* Is new bw close to the lt_bw from the previous interval? */
 		diff = abs(bw - bbr->lt_bw);
- 47a:	44 89 e2             	mov    %r12d,%edx
- 47d:	29 c2                	sub    %eax,%edx
- 47f:	89 d1                	mov    %edx,%ecx
- 481:	89 d3                	mov    %edx,%ebx
+ 475:	44 89 e3             	mov    %r12d,%ebx
+ 478:	29 c3                	sub    %eax,%ebx
+ 47a:	89 da                	mov    %ebx,%edx
+ 47c:	c1 fa 1f             	sar    $0x1f,%edx
+ 47f:	31 d3                	xor    %edx,%ebx
+ 481:	29 d3                	sub    %edx,%ebx

The biggest change is getting rid of the always true conditional.

-	u32 diff;
-
-	if (bbr->lt_bw &&  /* do we have bw from a previous interval? */
-	    bbr_lt_bw_estimator) {  /* using long-term bw estimator enabled? */
-		/* Is new bw close to the lt_bw from the previous interval? */
-		diff = abs(bw - bbr->lt_bw);
- 485:	c1 f9 1f             	sar    $0x1f,%ecx
-		if ((diff * BBR_UNIT <= bbr_lt_conv_thresh * bbr->lt_bw) ||
- 488:	c1 e2 05             	shl    $0x5,%edx
-	u32 diff;
-
-	if (bbr->lt_bw &&  /* do we have bw from a previous interval? */
-	    bbr_lt_bw_estimator) {  /* using long-term bw estimator enabled? */
-		/* Is new bw close to the lt_bw from the previous interval? */
-		diff = abs(bw - bbr->lt_bw);
- 48b:	31 cb                	xor    %ecx,%ebx
- 48d:	29 cb                	sub    %ecx,%ebx
-		if ((diff * BBR_UNIT <= bbr_lt_conv_thresh * bbr->lt_bw) ||
- 48f:	89 d9                	mov    %ebx,%ecx
- 491:	c1 e1 08             	shl    $0x8,%ecx
- 494:	39 d1                	cmp    %edx,%ecx
- 496:	0f 87 6e 01 00 00    	ja     60a <bbr_lt_bw_sampling.isra.6+0x29a>
+ 485:	89 d9                	mov    %ebx,%ecx
+ 487:	c1 e2 05             	shl    $0x5,%edx
+ 48a:	c1 e1 08             	shl    $0x8,%ecx
+ 48d:	39 d1                	cmp    %edx,%ecx
+ 48f:	0f 87 6e 01 00 00    	ja     603 <bbr_lt_bw_sampling.isra.6+0x293>


Overall, it really makes little difference. Actual file sizes come out the same.
The idea is more to document what is variable
and what is immutable in the algorithm.