lkml.org 
[lkml]   [2017]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFC PATCH 04/11] Add a function to start/reduce a timer
Date
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

\
 
 \ /
  Last update: 2017-11-09 01:33    [W:0.094 / U:5.240 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site