Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > +extern int reduce_timer(struct timer_list *timer, unsigned long expires);
>
> For new timer functions we really should use the timer_xxxx()
> convention. The historic naming convention is horrible.
>
> Aside of that timer_reduce() is kinda ugly but I failed to come up with
> something reasonable as well.
reduce_timer() sounds snappier, probably because the verb is first, but I can
make it timer_reduce() if you prefer. Or maybe timer_advance() - though
that's less clear as to the direction.
> > + if (options & MOD_TIMER_REDUCE) {
> > + if (time_before_eq(timer->expires, expires))
> > + return 1;
> > + } else {
> > + if (timer->expires == expires)
> > + return 1;
> > + }
>
> This hurts the common networking optimzation case. Please keep that check
> first:
The way the code stands, it doesn't make much difference because compiler
optimises away the MOD_TIMER_REDUCE case for __mod_timer() and optimises away
the other branch for reduce_timer().
> if (timer->expires == expires)
> return 1;
>
> if ((options & MOD_TIMER_REDUCE) &&
> time_before(timer->expires, expires))
> return 1;
>
> Also please check whether it's more efficient code wise to have that option
> thing or if an additional 'bool reduce' argument cerates better code.
It's a constant passed into an inline function - gcc-7's optimiser copes with
that for x86_64 at least. mod_timer() contains:
0xffffffff810bb7a0 <+31>: cmpq $0x0,0x8(%rdi)
0xffffffff810bb7a5 <+36>: mov %rsi,%r12
0xffffffff810bb7a8 <+39>: mov %rdi,%rbx
0xffffffff810bb7ab <+42>: je 0xffffffff810bb829 <mod_timer+168>
0xffffffff810bb7ad <+44>: cmp 0x10(%rdi),%rsi
0xffffffff810bb7b1 <+48>: movl $0x1,-0x38(%rbp)
0xffffffff810bb7b8 <+55>: je 0xffffffff810bba9f <mod_timer+798>
and reduce_timer() contains:
0xffffffff810bbaed <+31>: cmpq $0x0,0x8(%rdi)
0xffffffff810bbaf2 <+36>: mov %rsi,%r13
0xffffffff810bbaf5 <+39>: mov %rdi,%rbx
0xffffffff810bbaf8 <+42>: je 0xffffffff810bbb9d <reduce_timer+207>
0xffffffff810bbafe <+48>: cmp 0x10(%rdi),%rsi
0xffffffff810bbb02 <+52>: mov $0x1,%r14d
0xffffffff810bbb08 <+58>: jns 0xffffffff810bbe23 <reduce_timer+853>
As you can see, the relevant jump instruction is JE in one and JNS in the
other.
If I make the change you suggest with the equality check being unconditional,
mod_timer() is unchanged and reduce_timer() then contains:
0xffffffff810bbaed <+31>: cmpq $0x0,0x8(%rdi)
0xffffffff810bbaf2 <+36>: mov %rsi,%r13
0xffffffff810bbaf5 <+39>: mov %rdi,%rbx
0xffffffff810bbaf8 <+42>: je 0xffffffff810bbba9 <reduce_timer+219>
0xffffffff810bbafe <+48>: mov 0x10(%rdi),%rax
0xffffffff810bbb02 <+52>: mov $0x1,%r14d
0xffffffff810bbb08 <+58>: cmp %rax,%rsi
0xffffffff810bbb0b <+61>: je 0xffffffff810bbe2f <reduce_timer+865>
0xffffffff810bbb11 <+67>: cmp %rax,%rsi
0xffffffff810bbb14 <+70>: jns 0xffffffff810bbe2f <reduce_timer+865>
which smacks of a missed optimisation, since timer_before_eq() covers the ==
case. Doing:
long diff = timer->expires - expires;
if (diff == 0)
return 1;
if (options & MOD_TIMER_REDUCE &&
diff <= 0)
return 1;
gets me the same code in mod_timer() and the following in reduce_timer():
0xffffffff810bbaed <+31>: cmpq $0x0,0x8(%rdi)
0xffffffff810bbaf2 <+36>: mov %rsi,%r13
0xffffffff810bbaf5 <+39>: mov %rdi,%rbx
0xffffffff810bbaf8 <+42>: je 0xffffffff810bbba3 <reduce_timer+213>
0xffffffff810bbafe <+48>: mov 0x10(%rdi),%rax
0xffffffff810bbb02 <+52>: mov $0x1,%r14d
0xffffffff810bbb08 <+58>: sub %rsi,%rax
0xffffffff810bbb0b <+61>: test %rax,%rax
0xffffffff810bbb0e <+64>: jle 0xffffffff810bbe29 <reduce_timer+859>
which is marginally better - though I think it could still be optimised
better by the compiler.
Actually, something that might increase efficiency overall is to make
add_timer() an inline and forego the check - but that's a separate matter.
Thanks,
David
