| Message ID | 150428047736.25051.11186891058355974569.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 1 Sep 2017, David Howells wrote: > Add a function, similar to mod_timer(), that will start a timer it isn't s/it /if it / > running and will modify it if it is running and has an expiry time longer > than the new time. If the timer is running with an expiry time that's the > same or sooner, no change is made. > > The function looks like: > > int reduce_timer(struct timer_list *timer, unsigned long expires); Well, yes. But what's the purpose of this function? You explain the what, but not the why. > +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. > static inline int > -__mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) > +__mod_timer(struct timer_list *timer, unsigned long expires, unsigned int options) > { > struct timer_base *base, *new_base; > unsigned int idx = UINT_MAX; > @@ -938,8 +941,13 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) > * same array bucket then just return: > */ > if (timer_pending(timer)) { > - if (timer->expires == expires) > - return 1; > + 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: 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. Thanks, tglx
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
diff --git a/include/linux/timer.h b/include/linux/timer.h index e6789b8757d5..6ec5d897606d 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -187,6 +187,7 @@ extern void add_timer_on(struct timer_list *timer, int cpu); extern int del_timer(struct timer_list * timer); extern int mod_timer(struct timer_list *timer, unsigned long expires); extern int mod_timer_pending(struct timer_list *timer, unsigned long expires); +extern int reduce_timer(struct timer_list *timer, unsigned long expires); /* * The jiffies value which is added to now, when there is no timer diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 8f5d1bf18854..6fcbad70a924 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -922,8 +922,11 @@ static struct timer_base *lock_timer_base(struct timer_list *timer, } } +#define MOD_TIMER_PENDING_ONLY 0x01 +#define MOD_TIMER_REDUCE 0x02 + static inline int -__mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) +__mod_timer(struct timer_list *timer, unsigned long expires, unsigned int options) { struct timer_base *base, *new_base; unsigned int idx = UINT_MAX; @@ -938,8 +941,13 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) * same array bucket then just return: */ if (timer_pending(timer)) { - if (timer->expires == expires) - return 1; + if (options & MOD_TIMER_REDUCE) { + if (time_before_eq(timer->expires, expires)) + return 1; + } else { + if (timer->expires == expires) + return 1; + } /* * We lock timer base and calculate the bucket index right @@ -949,6 +957,13 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) */ base = lock_timer_base(timer, &flags); + if (timer_pending(timer) && + options & MOD_TIMER_REDUCE && + time_before_eq(timer->expires, expires)) { + ret = 1; + goto out_unlock; + } + clk = base->clk; idx = calc_wheel_index(expires, clk); @@ -958,7 +973,10 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) * subsequent call will exit in the expires check above. */ if (idx == timer_get_idx(timer)) { - timer->expires = expires; + if (!(options & MOD_TIMER_REDUCE)) + timer->expires = expires; + else if (time_after(timer->expires, expires)) + timer->expires = expires; ret = 1; goto out_unlock; } @@ -967,7 +985,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) } ret = detach_if_pending(timer, base, false); - if (!ret && pending_only) + if (!ret && options & MOD_TIMER_PENDING_ONLY) goto out_unlock; debug_activate(timer, expires); @@ -1030,7 +1048,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) */ int mod_timer_pending(struct timer_list *timer, unsigned long expires) { - return __mod_timer(timer, expires, true); + return __mod_timer(timer, expires, MOD_TIMER_PENDING_ONLY); } EXPORT_SYMBOL(mod_timer_pending); @@ -1056,11 +1074,26 @@ EXPORT_SYMBOL(mod_timer_pending); */ int mod_timer(struct timer_list *timer, unsigned long expires) { - return __mod_timer(timer, expires, false); + return __mod_timer(timer, expires, 0); } EXPORT_SYMBOL(mod_timer); /** + * reduce_timer - modify a timer's timeout if it would reduce the timeout + * @timer: the timer to be modified + * @expires: new timeout in jiffies + * + * reduce_timer() is very similar to mod_timer(), except that it will only + * modify a running timer if that would reduce the expiration time (it will + * start a timer that isn't running). + */ +int reduce_timer(struct timer_list *timer, unsigned long expires) +{ + return __mod_timer(timer, expires, MOD_TIMER_REDUCE); +} +EXPORT_SYMBOL(reduce_timer); + +/** * add_timer - start a timer * @timer: the timer to be added * @@ -1707,7 +1740,7 @@ signed long __sched schedule_timeout(signed long timeout) expire = timeout + jiffies; setup_timer_on_stack(&timer, process_timeout, (unsigned long)current); - __mod_timer(&timer, expire, false); + __mod_timer(&timer, expire, 0); schedule(); del_singleshot_timer_sync(&timer);
Add a function, similar to mod_timer(), that will start a timer it isn't running and will modify it if it is running and has an expiry time longer than the new time. If the timer is running with an expiry time that's the same or sooner, no change is made. The function looks like: int reduce_timer(struct timer_list *timer, unsigned long expires); Signed-off-by: David Howells <dhowells@redhat.com> cc: Thomas Gleixner <tglx@linutronix.de> --- include/linux/timer.h | 1 + kernel/time/timer.c | 49 +++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 8 deletions(-)