On Mon, 19 Sep 2016 14:10:39 -0700
Eric Dumazet <eduma...@google.com> wrote:
> On Mon, Sep 19, 2016 at 1:57 PM, Stephen Hemminger
> <step...@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.
