From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-x241.google.com (mail-pf0-x241.google.com [IPv6:2607:f8b0:400e:c00::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3xc3721xQVzDqjm for ; Tue, 22 Aug 2017 18:06:54 +1000 (AEST) Received: by mail-pf0-x241.google.com with SMTP id r62so2856439pfj.4 for ; Tue, 22 Aug 2017 01:06:54 -0700 (PDT) Date: Tue, 22 Aug 2017 18:06:30 +1000 From: Nicholas Piggin To: Thomas Gleixner Cc: akpm@linux-foundation.org, John Stultz , Stephen Boyd , Jonathan.Cameron@huawei.com, paulmck@linux.vnet.ibm.com, mpe@ellerman.id.au, dzickus@redhat.com, sfr@canb.auug.org.au, linuxarm@huawei.com, abdhalee@linux.vnet.ibm.com, sparclinux@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, torvalds@linux-foundation.org Subject: Re: [PATCH resend] timers: Fix excessive granularity of new timers after a nohz idle Message-ID: <20170822180630.6e1d7db5@roar.ozlabs.ibm.com> In-Reply-To: References: <20170822015439.11263-1-npiggin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, 22 Aug 2017 09:45:46 +0200 (CEST) Thomas Gleixner wrote: > On Tue, 22 Aug 2017, Nicholas Piggin wrote: > > I would have preferred to get comments from the timer maintainers, but > > they've been busy or away for the past copule of weeks. Perhaps you > > would consider carrying it until then? > > Yes, I was on vacation, but that patch never hit my LKML inbox .... Hmm okay. Well that's fine, we're just getting done testing it here. > > > diff --git a/kernel/time/timer.c b/kernel/time/timer.c > > index 8f5d1bf18854..dd7be9fe6839 100644 > > --- a/kernel/time/timer.c > > +++ b/kernel/time/timer.c > > @@ -203,6 +203,7 @@ struct timer_base { > > bool migration_enabled; > > bool nohz_active; > > bool is_idle; > > + bool was_idle; /* was it idle since last run/fwded */ > > Please do not use tail comments. They are a horrible habit and disturb the > reading flow. Sure. > I'm not fond of that name either. Something like forward_clk > or a similar self explaining name would be appreciated. Well I actually had that initially (must_forward). But on the other hand that's less explanatory about the state of the timer base I thought. Anyway I could go either way and you're going to be looking at this code more than me in future :) > > > /* > > @@ -938,6 +945,13 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) > > * same array bucket then just return: > > */ > > if (timer_pending(timer)) { > > + /* > > + * The downside of this optimization is that it can result in > > + * larger granularity than you would get from adding a new > > + * timer with this expiry. Would a timer flag for networking > > + * be appropriate, then we can try to keep expiry of general > > + * timers within ~1/8th of their interval? > > This really depends on the timer usage type. > > If you use it merily for TCP timeouts or similar things, then this does not > matter at all because then the timer is the safety net in case something > goes wrong. If you lose packets on the transport the larger granularity is > the least of your worries. > > From earlier discussions with the networking folks these timeouts are the > reason for this optimization because you avoid the expensive > dequeue/requeue operation if the successful communication is fast. > > If the timer has a short relative expiry and is used for sending packets or > similar purposes, then it usually sits in the first bucket and has no > granularity issues anyway. But from staring at trace data provided by > google and facebook these timer are not rearmed while pending, they fire, > do networking stuff and then get rearmed. > > So I rather avoid that kind of misleading comment. The first sentence > surely can stay. Right. The question was actually there for you to answer, so thanks. I can certainly understand the use-case and importance of keeping it light. > Other than that, nice detective work! Thanks for taking a look (and thanks everyone who's been testing and hacking on it). Do you want me to re-send with the changes? Thanks, Nick