Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448541 > unrolled thread

Re: [patch 4 15/22] timer: Remove slack leftovers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch 4 15/22] timer: Remove slack leftovers "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-07-22 13:40 +0200
    Re: [patch 4 15/22] timer: Remove slack leftovers Thomas Gleixner <tglx@linutronix.de> - 2016-07-22 15:10 +0200
      Re: [patch 4 15/22] timer: Remove slack leftovers "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-07-22 17:20 +0200
        Re: [patch 4 15/22] timer: Remove slack leftovers "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-07-23 01:00 +0200

#1448541 — Re: [patch 4 15/22] timer: Remove slack leftovers

Hi Thomas,

Thomas Gleixner <tglx@linutronix.de> writes:
> We now have implicit batching in the timer wheel. The slack is not longer
> used. Remove it.
> - set_timer_slack(&ev->dwork.timer, intv / 4);
> - set_timer_slack(&host->timeout_timer, HZ);
> - set_timer_slack(&di->work.timer, poll_interval * HZ / 4);
> - set_timer_slack(&ohci->io_watchdog, msecs_to_jiffies(20));
> - set_timer_slack(&xhci->comp_mode_recovery_timer,
> etc...

The current mod_timer implementation has this code:

        /*
         * This is a common optimization triggered by the
         * networking code - if the timer is re-modified
         * to be the same thing then just return:
         */
        if (timer_pending(timer) && timer->expires == expires)
                return 1;

In a (currently out-of-tree, but hopefully mergable soon) driver of
mine, I call mod_timer in the hot path. Every time some very frequent
event happens, I call mod_timer to move the timer back to be at
"jiffies + TIMEOUT".

From a brief look at timer.c, it looked like __mod_timer was rather
expensive. So, as an optimization, I wanted the "timer_pending(timer)
&& timer->expires == expires" condition to be hit in most of the
cases. I accomplished this by doing:

    set_timer_slack(timer, HZ / 4);

This ensured that we'd only wind up calling __mod_timer 4 times per
second, at most.

With the removal of the slack concept, I no longer can do this. I
haven't reviewed this series in depth, but I'm wondering if you'd
recommend a different optimization instead. Or, have things been
reworked so much, that calling mod_timer is now always inexpensive?

Thanks,
Jason

[toc] | [next] | [standalone]

#1448582

On Fri, 22 Jul 2016, Jason A. Donenfeld wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
> > We now have implicit batching in the timer wheel. The slack is not longer
> > used. Remove it.
> >From a brief look at timer.c, it looked like __mod_timer was rather
> expensive. So, as an optimization, I wanted the "timer_pending(timer)
> && timer->expires == expires" condition to be hit in most of the
> cases. I accomplished this by doing:
> 
>     set_timer_slack(timer, HZ / 4);
> 
> This ensured that we'd only wind up calling __mod_timer 4 times per
> second, at most.
>
> With the removal of the slack concept, I no longer can do this. I
> haven't reviewed this series in depth, but I'm wondering if you'd
> recommend a different optimization instead. Or, have things been

Well, this really depends on the TIMEOUT value you have. The code now does
implicit batching for larger timeouts by queueing the timers into wheels with
coarse grained granularity. As long as your new TIMEOUT value ends up in the
same bucket then that's equivalent to the slack thing.

Can you give me a ballpark of your TIMEOUT value?

> reworked so much, that calling mod_timer is now always inexpensive?

When you take the slow (queueing) path, it's still expensive, not as bad as
the previous one, but not really cheap either.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1448630

Hi Thomas,

On Fri, Jul 22, 2016 at 3:04 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Well, this really depends on the TIMEOUT value you have. The code now does
> implicit batching for larger timeouts by queueing the timers into wheels with
> coarse grained granularity. As long as your new TIMEOUT value ends up in the
> same bucket then that's equivalent to the slack thing.
>
> Can you give me a ballpark of your TIMEOUT value?

Generally either 5 seconds, 10 seconds, or 25 seconds.

Are these okay? Is the 25 case substantially different from the 5 case?


> When you take the slow (queueing) path, it's still expensive, not as bad as
> the previous one, but not really cheap either.

Hmm, okay.

[toc] | [prev] | [next] | [standalone]

#1448819

On Fri, Jul 22, 2016 at 5:18 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hi Thomas,
>
> On Fri, Jul 22, 2016 at 3:04 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> Well, this really depends on the TIMEOUT value you have. The code now does
>> implicit batching for larger timeouts by queueing the timers into wheels with
>> coarse grained granularity. As long as your new TIMEOUT value ends up in the
>> same bucket then that's equivalent to the slack thing.
>>
>> Can you give me a ballpark of your TIMEOUT value?
>
> Generally either 5 seconds, 10 seconds, or 25 seconds.
>
> Are these okay? Is the 25 case substantially different from the 5 case?

I suppose, anyway, it's easy enough just to provide my own coalescing
function. Actually, I'd rather have it batch timers to fire earlier
rather than later, in my case. So, I can do something like this to
round down to the nearest ~ quarter of a second:

    static inline unsigned long slack_time(unsigned long time)
    {
       return time & ~(BIT_MASK(ilog2(HZ / 4) + 1) - 1);
    }

    mod_timer(&timer, slack_time(jiffies + TIMEOUT));

This seems to do roughly what I want.

Jason

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web