From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753432AbdKIAd0 (ORCPT ); Wed, 8 Nov 2017 19:33:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46864 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752622AbdKIAdX (ORCPT ); Wed, 8 Nov 2017 19:33:23 -0500 Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 From: David Howells In-Reply-To: References: <150428045304.25051.1778333106306853298.stgit@warthog.procyon.org.uk> <150428047736.25051.11186891058355974569.stgit@warthog.procyon.org.uk> To: Thomas Gleixner Cc: dhowells@redhat.com, linux-fsdevel@vger.kernel.org, linux-afs@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 04/11] Add a function to start/reduce a timer MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <8076.1510187601.1@warthog.procyon.org.uk> Date: Thu, 09 Nov 2017 00:33:21 +0000 Message-ID: <8077.1510187601@warthog.procyon.org.uk> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 09 Nov 2017 00:33:23 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thomas Gleixner 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 0xffffffff810bb7ad <+44>: cmp 0x10(%rdi),%rsi 0xffffffff810bb7b1 <+48>: movl $0x1,-0x38(%rbp) 0xffffffff810bb7b8 <+55>: je 0xffffffff810bba9f and reduce_timer() contains: 0xffffffff810bbaed <+31>: cmpq $0x0,0x8(%rdi) 0xffffffff810bbaf2 <+36>: mov %rsi,%r13 0xffffffff810bbaf5 <+39>: mov %rdi,%rbx 0xffffffff810bbaf8 <+42>: je 0xffffffff810bbb9d 0xffffffff810bbafe <+48>: cmp 0x10(%rdi),%rsi 0xffffffff810bbb02 <+52>: mov $0x1,%r14d 0xffffffff810bbb08 <+58>: jns 0xffffffff810bbe23 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 0xffffffff810bbafe <+48>: mov 0x10(%rdi),%rax 0xffffffff810bbb02 <+52>: mov $0x1,%r14d 0xffffffff810bbb08 <+58>: cmp %rax,%rsi 0xffffffff810bbb0b <+61>: je 0xffffffff810bbe2f 0xffffffff810bbb11 <+67>: cmp %rax,%rsi 0xffffffff810bbb14 <+70>: jns 0xffffffff810bbe2f 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 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 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