Re: [RFC PATCH 04/11] Add a function to start/reduce a timer

[David Howells <dhowells@redhat.com>]

Thomas Gleixner <tglx@linutronix.de> 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