From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751846AbdJTMUo (ORCPT ); Fri, 20 Oct 2017 08:20:44 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:39834 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750936AbdJTMUj (ORCPT ); Fri, 20 Oct 2017 08:20:39 -0400 Date: Fri, 20 Oct 2017 14:20:27 +0200 (CEST) From: Thomas Gleixner To: David Howells cc: linux-afs@lists.infradead.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 04/11] Add a function to start/reduce a timer In-Reply-To: <150428047736.25051.11186891058355974569.stgit@warthog.procyon.org.uk> Message-ID: References: <150428045304.25051.1778333106306853298.stgit@warthog.procyon.org.uk> <150428047736.25051.11186891058355974569.stgit@warthog.procyon.org.uk> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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