Messages in this thread | | | Date | Tue, 22 Aug 2017 18:06:30 +1000 | From | Nicholas Piggin <> | Subject | Re: [PATCH resend] timers: Fix excessive granularity of new timers after a nohz idle |
| |
On Tue, 22 Aug 2017 09:45:46 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de> 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
| |