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

Groups > linux.kernel > #1420610 > unrolled thread

[patch 00/20] timer: Refactor the timer wheel

Back to article view | Back to linux.kernel

Contents

  [patch 00/20] timer: Refactor the timer wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 11/20] hlist: Add hlist_is_last_node() helper Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
      Re: [patch 11/20] hlist: Add hlist_is_last_node() helper Paolo Bonzini <pbonzini@redhat.com> - 2016-06-13 12:30 +0200
    [patch 07/20] drivers/tty/mips_ejtag: Initialize timer as pinned Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 01/20] timer: Make pinned a timer property Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 17/20] tick/sched: Remove pointless empty function Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 05/20] driver/net/ethernet/tile: Initialize timer as pinned Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 15/20] timer: Move __run_timers() function Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 12/20] timer: Give a few structs and members proper names Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 04/20] cpufreq/powernv: Initialize timer as pinned Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
      Re: [patch 04/20] cpufreq/powernv: Initialize timer as pinned Arjan van de Ven <arjanvandeven@gmail.com> - 2016-06-13 15:20 +0200
    [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
      Re: [patch 13/20] timer: Switch to a non cascading wheel Peter Zijlstra <peterz@infradead.org> - 2016-06-13 13:50 +0200
        Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 14:40 +0200
          Re: [patch 13/20] timer: Switch to a non cascading wheel Eric Dumazet <edumazet@google.com> - 2016-06-13 14:50 +0200
            Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 16:40 +0200
          Re: [patch 13/20] timer: Switch to a non cascading wheel Ingo Molnar <mingo@kernel.org> - 2016-06-14 12:20 +0200
            Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-14 18:40 +0200
              Re: [patch 13/20] timer: Switch to a non cascading wheel Arjan van de Ven <arjanvandeven@gmail.com> - 2016-06-14 19:20 +0200
                Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-14 20:10 +0200
                  Re: [patch 13/20] timer: Switch to a non cascading wheel Peter Zijlstra <peterz@infradead.org> - 2016-06-14 22:40 +0200
                  Re: [patch 13/20] timer: Switch to a non cascading wheel Peter Zijlstra <peterz@infradead.org> - 2016-06-14 22:50 +0200
                    Re: [patch 13/20] timer: Switch to a non cascading wheel Eric Dumazet <edumazet@google.com> - 2016-06-14 23:20 +0200
                      Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-15 17:00 +0200
                        Re: [patch 13/20] timer: Switch to a non cascading wheel Arjan van de Ven <arjanvandeven@gmail.com> - 2016-06-15 17:00 +0200
                          Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-15 18:50 +0200
                            Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-16 17:50 +0200
                              Re: [patch 13/20] timer: Switch to a non cascading wheel "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-06-16 18:10 +0200
                                Re: [patch 13/20] timer: Switch to a non cascading wheel Peter Zijlstra <peterz@infradead.org> - 2016-06-16 20:20 +0200
                                  Re: [patch 13/20] timer: Switch to a non cascading wheel "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-06-17 02:50 +0200
                                    Re: [patch 13/20] timer: Switch to a non cascading wheel "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-06-17 06:10 +0200
                              Re: [patch 13/20] timer: Switch to a non cascading wheel Arjan van de Ven <arjanvandeven@gmail.com> - 2016-06-16 18:10 +0200
                                Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-16 18:20 +0200
                        Re: [patch 13/20] timer: Switch to a non cascading wheel Eric Dumazet <edumazet@google.com> - 2016-06-15 17:10 +0200
      Re: [patch 13/20] timer: Switch to a non cascading wheel Richard Cochran <richardcochran@gmail.com> - 2016-06-13 16:40 +0200
        Re: [patch 13/20] timer: Switch to a non cascading wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 16:50 +0200
    [patch 14/20] timer: Remove slack leftovers Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
    [patch 18/20] timer: Forward wheel clock whenever possible Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 10:50 +0200
      Re: [patch 18/20] timer: Forward wheel clock whenever possible Richard Cochran <richardcochran@gmail.com> - 2016-06-13 17:20 +0200
        Re: [patch 18/20] timer: Forward wheel clock whenever possible Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 17:30 +0200
    [patch 06/20] drivers/tty/metag_da: Initialize timer as pinned Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 11:00 +0200
      Re: [patch 06/20] drivers/tty/metag_da: Initialize timer as pinned Arjan van de Ven <arjanvandeven@gmail.com> - 2016-06-13 15:20 +0200
    [patch 03/20] x86/mce: Initialize timer as pinned Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 11:00 +0200
    [patch 08/20] net/ipv4/inet: Initialize timers as pinned Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 11:00 +0200
    [patch 02/20] x86/apic/uv: Initialize timer as pinned Thomas Gleixner <tglx@linutronix.de> - 2016-06-13 11:00 +0200
    Re: [patch 00/20] timer: Refactor the timer wheel Eric Dumazet <edumazet@google.com> - 2016-06-13 16:20 +0200
    Re: [patch 00/20] timer: Refactor the timer wheel "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-06-13 18:20 +0200
      Re: [patch 00/20] timer: Refactor the timer wheel "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-06-15 17:20 +0200
        Re: [patch 00/20] timer: Refactor the timer wheel Thomas Gleixner <tglx@linutronix.de> - 2016-06-15 19:10 +0200
          Re: [patch 00/20] timer: Refactor the timer wheel "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-06-15 22:30 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1422312 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Tue, Jun 14, 2016 at 08:05:49PM +0200, Thomas Gleixner wrote:
> On Tue, 14 Jun 2016, Arjan van de Ven wrote:
> 
> > evaluating a 120 hours timer ever 37 hours to see if it should fire...
> > not too horrid.
> 
> Well that thing is doing weird stuff anyway:
> 
>    swapper     0 [001] 1789995.305532: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850639994 [timeout=108000000]

You're running a HZ=250 kernel, right?

inet/netfilter/nf_conntrack_proto_tcp.c:

#define SECS * HZ
#define MINS * 60 SECS
#define HOURS * 60 MINS
#define DAYS * 24 HOURS

static unsigned int tcp_timeouts[TCP_CONNTRACK_TIMEOUT_MAX] __read_mostly = {
	[TCP_CONNTRACK_SYN_SENT]	= 2 MINS,
	[TCP_CONNTRACK_SYN_RECV]	= 60 SECS,
	[TCP_CONNTRACK_ESTABLISHED]	= 5 DAYS,

					  ^^^^^^ that
ends up being 108000000 for HZ == 250.


	[TCP_CONNTRACK_FIN_WAIT]	= 2 MINS,
	[TCP_CONNTRACK_CLOSE_WAIT]	= 60 SECS,
	[TCP_CONNTRACK_LAST_ACK]	= 30 SECS,
	[TCP_CONNTRACK_TIME_WAIT]	= 2 MINS,
	[TCP_CONNTRACK_CLOSE]		= 10 SECS,
	[TCP_CONNTRACK_SYN_SENT2]	= 2 MINS,
/* RFC1122 says the R2 limit should be at least 100 seconds.
   Linux uses 15 packets as limit, which corresponds
   to ~13-30min depending on RTO. */
	[TCP_CONNTRACK_RETRANS]		= 5 MINS,
	[TCP_CONNTRACK_UNACK]		= 5 MINS,
};


>              ssh  3870 [001] 1790025.284704: timer:timer_cancel: timer=0xffff8800c8346920
>              ssh  3870 [001] 1790025.284707: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742722493 [timeout=75000]

And that one would then be one of the 5 MINS ones.

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

#1422325 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Tue, Jun 14, 2016 at 08:05:49PM +0200, Thomas Gleixner wrote:
> On Tue, 14 Jun 2016, Arjan van de Ven wrote:
> 
> > evaluating a 120 hours timer ever 37 hours to see if it should fire...
> > not too horrid.
> 
> Well that thing is doing weird stuff anyway:
> 
>    swapper     0 [001] 1789995.305532: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850639994 [timeout=108000000]
>              ssh  3870 [001] 1790025.284704: timer:timer_cancel: timer=0xffff8800c8346920
>              ssh  3870 [001] 1790025.284707: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742722493 [timeout=75000]
>          swapper     0 [001] 1790025.330514: timer:timer_cancel: timer=0xffff8800c8346920
>          swapper     0 [001] 1790025.330515: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850647504 [timeout=108000000]
>              ssh  3870 [001] 1790055.307058: timer:timer_cancel: timer=0xffff8800c8346920
>              ssh  3870 [001] 1790055.307060: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742730003 [timeout=75000]
>          swapper     0 [001] 1790055.352146: timer:timer_cancel: timer=0xffff8800c8346920
> 
> And that goes on forever. 2834 such sequences for this particular timer
> instance in 4.5 hours. 90000 sequences total for all timers related to
> death_by_timeout in 4.5 hours
> 
> No idea what this is doing and why the heck it nees a 120 hour timeout ....

So it moves that timer on every packet for that TCP connection stream,
provided the expiration is at least 1 second behind.

If the stream hasn't had a packet in 5 days (see previous email), then
the connection state is destroyed.

Its been too long since I've read the TCP RFCs, but I can imagine
changing this will upset people.

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

#1422348 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Tue, Jun 14, 2016 at 1:42 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 14, 2016 at 08:05:49PM +0200, Thomas Gleixner wrote:
>> On Tue, 14 Jun 2016, Arjan van de Ven wrote:
>>
>> > evaluating a 120 hours timer ever 37 hours to see if it should fire...
>> > not too horrid.
>>
>> Well that thing is doing weird stuff anyway:
>>
>>    swapper     0 [001] 1789995.305532: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850639994 [timeout=108000000]
>>              ssh  3870 [001] 1790025.284704: timer:timer_cancel: timer=0xffff8800c8346920
>>              ssh  3870 [001] 1790025.284707: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742722493 [timeout=75000]
>>          swapper     0 [001] 1790025.330514: timer:timer_cancel: timer=0xffff8800c8346920
>>          swapper     0 [001] 1790025.330515: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850647504 [timeout=108000000]
>>              ssh  3870 [001] 1790055.307058: timer:timer_cancel: timer=0xffff8800c8346920
>>              ssh  3870 [001] 1790055.307060: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742730003 [timeout=75000]
>>          swapper     0 [001] 1790055.352146: timer:timer_cancel: timer=0xffff8800c8346920
>>
>> And that goes on forever. 2834 such sequences for this particular timer
>> instance in 4.5 hours. 90000 sequences total for all timers related to
>> death_by_timeout in 4.5 hours
>>
>> No idea what this is doing and why the heck it nees a 120 hour timeout ....
>
> So it moves that timer on every packet for that TCP connection stream,
> provided the expiration is at least 1 second behind.
>
> If the stream hasn't had a packet in 5 days (see previous email), then
> the connection state is destroyed.
>
> Its been too long since I've read the TCP RFCs, but I can imagine
> changing this will upset people.
>

Original TCP RFCs tell timeout is infinite ;)

Practically, conntrack has a 5 days timeout, but I really doubt anyone
expects an idle TCP flow to stay 'alive' when nothing is sent for 5
days.

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

#1423106 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Tue, 14 Jun 2016, Eric Dumazet wrote:
> Original TCP RFCs tell timeout is infinite ;)
> 
> Practically, conntrack has a 5 days timeout, but I really doubt anyone
> expects an idle TCP flow to stay 'alive' when nothing is sent for 5
> days.

So would 37hrs ~= 1.5 days be a reasonable cutoff or will stuff fall apart and
people be surprised?

Thanks,

	tglx

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

#1423117 — Re: [patch 13/20] timer: Switch to a non cascading wheel

what would 1 more timer wheel do?

On Wed, Jun 15, 2016 at 7:53 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 14 Jun 2016, Eric Dumazet wrote:
>> Original TCP RFCs tell timeout is infinite ;)
>>
>> Practically, conntrack has a 5 days timeout, but I really doubt anyone
>> expects an idle TCP flow to stay 'alive' when nothing is sent for 5
>> days.
>
> So would 37hrs ~= 1.5 days be a reasonable cutoff or will stuff fall apart and
> people be surprised?
>
> Thanks,
>
>         tglx
>

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

#1423232 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Wed, 15 Jun 2016, Arjan van de Ven wrote:
> what would 1 more timer wheel do?

Waste storage space and make the collection of expired timers more expensive.

The selection of the timer wheel properties is combination of:

    1) Granularity 

    2) Storage space

    3) Number of levels to collect

#1 Influences the accuracy of the wheel levels. The worst case error is the
   ratio of the shortest timeout in a level and the granularity of that level.

   Currently I've chosen 12.5%. If we go to 25% worst case, then we increase
   the per level capacity by factor (1 << lvl).

#2 It strikes me a bit silly to waste storage space for esoteric
   outliers. According to the data I collected we have no timers at all
   between 2hrs and 5 days.

#3 The more levels we have the more steps we have when collecting expired
   timers or searching the next expiring timer. It's all bound, but it still
   adds up.

Thanks,

	tglx




   

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

#1424209 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Wed, 15 Jun 2016, Thomas Gleixner wrote:
> On Wed, 15 Jun 2016, Arjan van de Ven wrote:
> > what would 1 more timer wheel do?
> 
> Waste storage space and make the collection of expired timers more expensive.
> 
> The selection of the timer wheel properties is combination of:
> 
>     1) Granularity 
> 
>     2) Storage space
> 
>     3) Number of levels to collect

So I came up with a slightly different solution for this. The problem case is
HZ=1000 and again looking at the data, there is no reason why we need actual
1ms granularity for timer wheel timers. That's independent of the desired ms
based interfaces.

We can simply run the wheel internaly with 4ms base level resolution and
degrade from there. That gives us 6 days+ and a simple cutoff at the capacity
of the 7th level wheel.

 0     0        4 ms               0 ms -        255 ms		    
 1    64       32 ms             256 ms -       2047 ms (256ms - ~2s)
 2   128      256 ms            2048 ms -      16383 ms (~2s - ~16s) 
 3   192     2048 ms (~2s)     16384 ms -     131071 ms (~16s - ~2m)
 4   256    16384 ms (~16s)   131072 ms -    1048575 ms (~2m - ~17m)
 5   320   131072 ms (~2m)   1048576 ms -    8388607 ms (~17m - ~2h)
 6   384  1048576 ms (~17m)  8388608 ms -   67108863 ms (~2h - ~18h)
 7   448  8388608 ms (~2h)  67108864 ms -  536870911 ms (~18h - ~6d)

That works really nice and has the interesting side effect that we batch in
the first level wheel which helps networking. I'll repost the series with the
other review points addressed later tonight.

Btw, I also thought a bit more about the milliseconds interfaces. I think we
shouldn't invent new interfaces. The correct solution IMHO is to distangle the
scheduler tick frequency and jiffies. If we have that completely seperated
then we can do the following:

1) Force HZ=1000. That means jiffies and timer wheel units are 1ms. If the
   tick frequency is != 1000 we simply increment jiffies in the tick by the
   proper amount (4 @250 ticks/sec, 10 @100 ticks/sec).

   So all msec_to_jiffies() invocations compile out into nothing magically and
   we can remove them gradually over time.

2) When we do that right, we can make the tick frequency a command line option
   and just have a compiled in default.

Thoughts?

Thanks,

	tglx

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

#1424228 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Thu, Jun 16, 2016 at 05:43:36PM +0200, Thomas Gleixner wrote:
> On Wed, 15 Jun 2016, Thomas Gleixner wrote:
> > On Wed, 15 Jun 2016, Arjan van de Ven wrote:
> > > what would 1 more timer wheel do?
> > 
> > Waste storage space and make the collection of expired timers more expensive.
> > 
> > The selection of the timer wheel properties is combination of:
> > 
> >     1) Granularity 
> > 
> >     2) Storage space
> > 
> >     3) Number of levels to collect
> 
> So I came up with a slightly different solution for this. The problem case is
> HZ=1000 and again looking at the data, there is no reason why we need actual
> 1ms granularity for timer wheel timers. That's independent of the desired ms
> based interfaces.
> 
> We can simply run the wheel internaly with 4ms base level resolution and
> degrade from there. That gives us 6 days+ and a simple cutoff at the capacity
> of the 7th level wheel.
> 
>  0     0        4 ms               0 ms -        255 ms		    
>  1    64       32 ms             256 ms -       2047 ms (256ms - ~2s)
>  2   128      256 ms            2048 ms -      16383 ms (~2s - ~16s) 
>  3   192     2048 ms (~2s)     16384 ms -     131071 ms (~16s - ~2m)
>  4   256    16384 ms (~16s)   131072 ms -    1048575 ms (~2m - ~17m)
>  5   320   131072 ms (~2m)   1048576 ms -    8388607 ms (~17m - ~2h)
>  6   384  1048576 ms (~17m)  8388608 ms -   67108863 ms (~2h - ~18h)
>  7   448  8388608 ms (~2h)  67108864 ms -  536870911 ms (~18h - ~6d)
> 
> That works really nice and has the interesting side effect that we batch in
> the first level wheel which helps networking. I'll repost the series with the
> other review points addressed later tonight.
> 
> Btw, I also thought a bit more about the milliseconds interfaces. I think we
> shouldn't invent new interfaces. The correct solution IMHO is to distangle the
> scheduler tick frequency and jiffies. If we have that completely seperated
> then we can do the following:
> 
> 1) Force HZ=1000. That means jiffies and timer wheel units are 1ms. If the
>    tick frequency is != 1000 we simply increment jiffies in the tick by the
>    proper amount (4 @250 ticks/sec, 10 @100 ticks/sec).
> 
>    So all msec_to_jiffies() invocations compile out into nothing magically and
>    we can remove them gradually over time.

Some of RCU's heuristics assume that if scheduling-clock ticks happen,
they happen once per jiffy.  These would need to be adjusted, which would
not be a big deal, just a bit more use of HZ.

> 2) When we do that right, we can make the tick frequency a command line option
>    and just have a compiled in default.

As long as there is something that tells RCU what the tick frequency
actually is at runtime, this should not be a problem.  For example,
in rcu_implicit_dynticks_qs(), the following:

	rdp->rsp->jiffies_resched += 5;

Would instead need to be something like:

	rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;

Changing tick frequency at runtime would be a bit more tricky, as it would
be tough to avoid some oddball false positives during the transition.
But setting it at boot time would be fine.  ;-)

							Thanx, Paul

> Thoughts?
> 
> Thanks,
> 
> 	tglx
> 

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

#1424333 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Thu, Jun 16, 2016 at 09:02:15AM -0700, Paul E. McKenney wrote:
> > 2) When we do that right, we can make the tick frequency a command line option
> >    and just have a compiled in default.
> 
> As long as there is something that tells RCU what the tick frequency
> actually is at runtime, this should not be a problem.  For example,
> in rcu_implicit_dynticks_qs(), the following:
> 
> 	rdp->rsp->jiffies_resched += 5;
> 
> Would instead need to be something like:
> 
> 	rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;
> 
> Changing tick frequency at runtime would be a bit more tricky, as it would
> be tough to avoid some oddball false positives during the transition.

So the 'fun' part will be frequencies with non integer factors of 1000.
Like say HZ=300. For that we'll have to keep jiffies_remainder, and
add an extra jiffy every time that rolls over.

That would make your case slightly more interesting than you really
want I suspect.

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

#1424575 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Thu, Jun 16, 2016 at 08:14:02PM +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2016 at 09:02:15AM -0700, Paul E. McKenney wrote:
> > > 2) When we do that right, we can make the tick frequency a command line option
> > >    and just have a compiled in default.
> > 
> > As long as there is something that tells RCU what the tick frequency
> > actually is at runtime, this should not be a problem.  For example,
> > in rcu_implicit_dynticks_qs(), the following:
> > 
> > 	rdp->rsp->jiffies_resched += 5;
> > 
> > Would instead need to be something like:
> > 
> > 	rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;
> > 
> > Changing tick frequency at runtime would be a bit more tricky, as it would
> > be tough to avoid some oddball false positives during the transition.
> 
> So the 'fun' part will be frequencies with non integer factors of 1000.
> Like say HZ=300. For that we'll have to keep jiffies_remainder, and
> add an extra jiffy every time that rolls over.
> 
> That would make your case slightly more interesting than you really
> want I suspect.

My particular case is not that sensitive, so 1000/300 would be plenty
accurate.

							Thanx, Paul

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

#1424637 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Thu, Jun 16, 2016 at 05:40:51PM -0700, Paul E. McKenney wrote:
> On Thu, Jun 16, 2016 at 08:14:02PM +0200, Peter Zijlstra wrote:
> > On Thu, Jun 16, 2016 at 09:02:15AM -0700, Paul E. McKenney wrote:
> > > > 2) When we do that right, we can make the tick frequency a command line option
> > > >    and just have a compiled in default.
> > > 
> > > As long as there is something that tells RCU what the tick frequency
> > > actually is at runtime, this should not be a problem.  For example,
> > > in rcu_implicit_dynticks_qs(), the following:
> > > 
> > > 	rdp->rsp->jiffies_resched += 5;
> > > 
> > > Would instead need to be something like:
> > > 
> > > 	rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;
> > > 
> > > Changing tick frequency at runtime would be a bit more tricky, as it would
> > > be tough to avoid some oddball false positives during the transition.
> > 
> > So the 'fun' part will be frequencies with non integer factors of 1000.
> > Like say HZ=300. For that we'll have to keep jiffies_remainder, and
> > add an extra jiffy every time that rolls over.
> > 
> > That would make your case slightly more interesting than you really
> > want I suspect.
> 
> My particular case is not that sensitive, so 1000/300 would be plenty
> accurate.

That said...  If someone were to set HZ=501, now -that- could be a
bit problematic.  :-/

							Thanx, Paul

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

#1424237 — Re: [patch 13/20] timer: Switch to a non cascading wheel

I think there's 2 elements on the interface.

1) having a relative interface to the current time (avoid use of
absolute jiffies in drivers)

2) having wallclock units. Making HZ always be 1000 is effectively
doing that as well (1 msec after all)



On Thu, Jun 16, 2016 at 8:43 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 15 Jun 2016, Thomas Gleixner wrote:
>> On Wed, 15 Jun 2016, Arjan van de Ven wrote:
>> > what would 1 more timer wheel do?
>>
>> Waste storage space and make the collection of expired timers more expensive.
>>
>> The selection of the timer wheel properties is combination of:
>>
>>     1) Granularity
>>
>>     2) Storage space
>>
>>     3) Number of levels to collect
>
> So I came up with a slightly different solution for this. The problem case is
> HZ=1000 and again looking at the data, there is no reason why we need actual
> 1ms granularity for timer wheel timers. That's independent of the desired ms
> based interfaces.
>
> We can simply run the wheel internaly with 4ms base level resolution and
> degrade from there. That gives us 6 days+ and a simple cutoff at the capacity
> of the 7th level wheel.
>
>  0     0        4 ms               0 ms -        255 ms
>  1    64       32 ms             256 ms -       2047 ms (256ms - ~2s)
>  2   128      256 ms            2048 ms -      16383 ms (~2s - ~16s)
>  3   192     2048 ms (~2s)     16384 ms -     131071 ms (~16s - ~2m)
>  4   256    16384 ms (~16s)   131072 ms -    1048575 ms (~2m - ~17m)
>  5   320   131072 ms (~2m)   1048576 ms -    8388607 ms (~17m - ~2h)
>  6   384  1048576 ms (~17m)  8388608 ms -   67108863 ms (~2h - ~18h)
>  7   448  8388608 ms (~2h)  67108864 ms -  536870911 ms (~18h - ~6d)
>
> That works really nice and has the interesting side effect that we batch in
> the first level wheel which helps networking. I'll repost the series with the
> other review points addressed later tonight.
>
> Btw, I also thought a bit more about the milliseconds interfaces. I think we
> shouldn't invent new interfaces. The correct solution IMHO is to distangle the
> scheduler tick frequency and jiffies. If we have that completely seperated
> then we can do the following:
>
> 1) Force HZ=1000. That means jiffies and timer wheel units are 1ms. If the
>    tick frequency is != 1000 we simply increment jiffies in the tick by the
>    proper amount (4 @250 ticks/sec, 10 @100 ticks/sec).
>
>    So all msec_to_jiffies() invocations compile out into nothing magically and
>    we can remove them gradually over time.
>
> 2) When we do that right, we can make the tick frequency a command line option
>    and just have a compiled in default.
>
> Thoughts?
>
> Thanks,
>
>         tglx

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

#1424243 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Thu, 16 Jun 2016, Arjan van de Ven wrote:

> I think there's 2 elements on the interface.
> 
> 1) having a relative interface to the current time (avoid use of
> absolute jiffies in drivers)

That's the easy part :)
 
> 2) having wallclock units. Making HZ always be 1000 is effectively
> doing that as well (1 msec after all)

Right. And it allows us to switch over the whole code base w/o fiddling with
it in one go. Cleanups should obviously take place after that.

Thanks,

	tglx

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

#1423124 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Wed, Jun 15, 2016 at 7:53 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 14 Jun 2016, Eric Dumazet wrote:
>> Original TCP RFCs tell timeout is infinite ;)
>>
>> Practically, conntrack has a 5 days timeout, but I really doubt anyone
>> expects an idle TCP flow to stay 'alive' when nothing is sent for 5
>> days.
>
> So would 37hrs ~= 1.5 days be a reasonable cutoff or will stuff fall apart and
> people be surprised?
>

It seems very reasonable to me at least.

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

#1420936 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Mon, Jun 13, 2016 at 08:41:00AM -0000, Thomas Gleixner wrote:
> +static inline struct timer_base *get_timer_base(u32 tflags)
> +{
> +	return get_timer_cpu_base(tflags, tflags & TIMER_BASEMASK);
> +}

This should rather be (tflags & TIMER_CPUMASK) to avoid using
per_cpu_ptr() with the TIMER_MIGRATING bit set in the CPU index.

The one caller in this patch is okay, since it already checks that
TIMER_MIGRATING is clear:

>  static struct timer_base *lock_timer_base(struct timer_list *timer,
> -					unsigned long *flags)
> +					  unsigned long *flags)
>  	__acquires(timer->base->lock)
>  {
>  	for (;;) {
> -		u32 tf = timer->flags;
>  		struct timer_base *base;
> +		u32 tf = timer->flags;
>  
>  		if (!(tf & TIMER_MIGRATING)) {
> -			base = per_cpu_ptr(&timer_bases, tf & TIMER_CPUMASK);
> +			base = get_timer_base(tf);

However, in patch #20, we'll have this in __mod_timer();

		/*
		 * Take the current timer_jiffies of base, but without holding
		 * the lock!
		 */
		base = get_timer_base(timer->flags);

Thanks,
Richard

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

#1420943 — Re: [patch 13/20] timer: Switch to a non cascading wheel

On Mon, 13 Jun 2016, Richard Cochran wrote:
> On Mon, Jun 13, 2016 at 08:41:00AM -0000, Thomas Gleixner wrote:
> > +static inline struct timer_base *get_timer_base(u32 tflags)
> > +{
> > +	return get_timer_cpu_base(tflags, tflags & TIMER_BASEMASK);
> > +}
> 
> This should rather be (tflags & TIMER_CPUMASK) to avoid using
> per_cpu_ptr() with the TIMER_MIGRATING bit set in the CPU index.
> 
> The one caller in this patch is okay, since it already checks that
> TIMER_MIGRATING is clear:

Good catch!
 
Thanks,

	tglx

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

#1420622 — [patch 14/20] timer: Remove slack leftovers

We now have implicit batching in the timer wheel. The slack is not longer
used. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 block/genhd.c                   |    5 -----
 drivers/mmc/host/jz4740_mmc.c   |    2 --
 drivers/power/bq27xxx_battery.c |    5 +----
 drivers/usb/host/ohci-hcd.c     |    1 -
 drivers/usb/host/xhci.c         |    2 --
 include/linux/timer.h           |    4 ----
 kernel/time/timer.c             |   19 -------------------
 lib/random32.c                  |    1 -
 8 files changed, 1 insertion(+), 38 deletions(-)

--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1523,12 +1523,7 @@ static void __disk_unblock_events(struct
 	if (--ev->block)
 		goto out_unlock;
 
-	/*
-	 * Not exactly a latency critical operation, set poll timer
-	 * slack to 25% and kick event check.
-	 */
 	intv = disk_events_poll_jiffies(disk);
-	set_timer_slack(&ev->dwork.timer, intv / 4);
 	if (check_now)
 		queue_delayed_work(system_freezable_power_efficient_wq,
 				&ev->dwork, 0);
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -1068,8 +1068,6 @@ static int jz4740_mmc_probe(struct platf
 	jz4740_mmc_clock_disable(host);
 	setup_timer(&host->timeout_timer, jz4740_mmc_timeout,
 			(unsigned long)host);
-	/* It is not important when it times out, it just needs to timeout. */
-	set_timer_slack(&host->timeout_timer, HZ);
 
 	host->use_dma = true;
 	if (host->use_dma && jz4740_mmc_acquire_dma_channels(host) != 0)
--- a/drivers/power/bq27xxx_battery.c
+++ b/drivers/power/bq27xxx_battery.c
@@ -735,11 +735,8 @@ static void bq27xxx_battery_poll(struct
 
 	bq27xxx_battery_update(di);
 
-	if (poll_interval > 0) {
-		/* The timer does not have to be accurate. */
-		set_timer_slack(&di->work.timer, poll_interval * HZ / 4);
+	if (poll_interval > 0)
 		schedule_delayed_work(&di->work, poll_interval * HZ);
-	}
 }
 
 /*
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -500,7 +500,6 @@ static int ohci_init (struct ohci_hcd *o
 
 	setup_timer(&ohci->io_watchdog, io_watchdog_func,
 			(unsigned long) ohci);
-	set_timer_slack(&ohci->io_watchdog, msecs_to_jiffies(20));
 
 	ohci->hcca = dma_alloc_coherent (hcd->self.controller,
 			sizeof(*ohci->hcca), &ohci->hcca_dma, GFP_KERNEL);
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -490,8 +490,6 @@ static void compliance_mode_recovery_tim
 	xhci->comp_mode_recovery_timer.expires = jiffies +
 			msecs_to_jiffies(COMP_MODE_RCVRY_MSECS);
 
-	set_timer_slack(&xhci->comp_mode_recovery_timer,
-			msecs_to_jiffies(COMP_MODE_RCVRY_MSECS));
 	add_timer(&xhci->comp_mode_recovery_timer);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
 			"Compliance mode recovery timer initialized");
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -19,7 +19,6 @@ struct timer_list {
 	void			(*function)(unsigned long);
 	unsigned long		data;
 	u32			flags;
-	int			slack;
 
 #ifdef CONFIG_TIMER_STATS
 	int			start_pid;
@@ -73,7 +72,6 @@ struct timer_list {
 		.expires = (_expires),				\
 		.data = (_data),				\
 		.flags = (_flags),				\
-		.slack = -1,					\
 		__TIMER_LOCKDEP_MAP_INITIALIZER(		\
 			__FILE__ ":" __stringify(__LINE__))	\
 	}
@@ -193,8 +191,6 @@ extern int del_timer(struct timer_list *
 extern int mod_timer(struct timer_list *timer, unsigned long expires);
 extern int mod_timer_pending(struct timer_list *timer, unsigned long expires);
 
-extern void set_timer_slack(struct timer_list *time, int slack_hz);
-
 /*
  * The jiffies value which is added to now, when there is no timer
  * in the timer wheel:
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -432,24 +432,6 @@ unsigned long round_jiffies_up_relative(
 }
 EXPORT_SYMBOL_GPL(round_jiffies_up_relative);
 
-/**
- * set_timer_slack - set the allowed slack for a timer
- * @timer: the timer to be modified
- * @slack_hz: the amount of time (in jiffies) allowed for rounding
- *
- * Set the amount of time, in jiffies, that a certain timer has
- * in terms of slack. By setting this value, the timer subsystem
- * will schedule the actual timer somewhere between
- * the time mod_timer() asks for, and that time plus the slack.
- *
- * By setting the slack to -1, a percentage of the delay is used
- * instead.
- */
-void set_timer_slack(struct timer_list *timer, int slack_hz)
-{
-	timer->slack = slack_hz;
-}
-EXPORT_SYMBOL_GPL(set_timer_slack);
 
 static inline unsigned int timer_get_idx(struct timer_list *timer)
 {
@@ -753,7 +735,6 @@ static void do_init_timer(struct timer_l
 {
 	timer->entry.pprev = NULL;
 	timer->flags = flags | raw_smp_processor_id();
-	timer->slack = -1;
 #ifdef CONFIG_TIMER_STATS
 	timer->start_site = NULL;
 	timer->start_pid = -1;
--- a/lib/random32.c
+++ b/lib/random32.c
@@ -233,7 +233,6 @@ static void __prandom_timer(unsigned lon
 
 static void __init __prandom_start_seed_timer(void)
 {
-	set_timer_slack(&seed_timer, HZ);
 	seed_timer.expires = jiffies + msecs_to_jiffies(40 * MSEC_PER_SEC);
 	add_timer(&seed_timer);
 }

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

#1420624 — [patch 18/20] timer: Forward wheel clock whenever possible

The wheel clock is stale when a cpu goes into a long idle sleep. This has the
side effect, that timers which are queued end up in the outer wheel
levels. That results in coarser granularity.

To solve this, we keep track of the idle state and forward the wheel clock
whenever it's possible.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/tick-internal.h |    1 
 kernel/time/tick-sched.c    |   13 ++++
 kernel/time/timer.c         |  120 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 111 insertions(+), 23 deletions(-)

--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -164,3 +164,4 @@ static inline void timers_update_migrati
 DECLARE_PER_CPU(struct hrtimer_cpu_base, hrtimer_bases);
 
 extern u64 get_next_timer_interrupt(unsigned long basej, u64 basem);
+void timer_clear_idle(void);
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -700,6 +700,12 @@ static ktime_t tick_nohz_stop_sched_tick
 	delta = next_tick - basemono;
 	if (delta <= (u64)TICK_NSEC) {
 		tick.tv64 = 0;
+
+		/*
+		 * Tell the timer code that the base is not idle, i.e. undo
+		 * the effect of get_next_timer_interrupt().
+		 */
+		timer_clear_idle();
 		/*
 		 * We've not stopped the tick yet, and there's a timer in the
 		 * next period, so no point in stopping it either, bail.
@@ -809,6 +815,12 @@ static void tick_nohz_restart_sched_tick
 	tick_do_update_jiffies64(now);
 	cpu_load_update_nohz_stop();
 
+	/*
+	 * Clear the timer idle flag, so we avoid IPIs on remote queueing and
+	 * the clock forward checks in the enqueue path.
+	 */
+	timer_clear_idle();
+
 	calc_load_exit_idle();
 	touch_softlockup_watchdog_sched();
 	/*
@@ -1025,6 +1037,7 @@ void tick_nohz_idle_exit(void)
 		tick_nohz_stop_idle(ts, now);
 
 	if (ts->tick_stopped) {
+		timer_clear_idle();
 		tick_nohz_restart_sched_tick(ts, now);
 		tick_nohz_account_idle_ticks(ts);
 	}
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -181,9 +181,11 @@ struct timer_base {
 	spinlock_t		lock;
 	struct timer_list	*running_timer;
 	unsigned long		clk;
+	unsigned long		next_expiry;
 	unsigned int		cpu;
 	bool			migration_enabled;
 	bool			nohz_active;
+	bool			is_idle;
 	DECLARE_BITMAP(pending_map, WHEEL_SIZE);
 	struct hlist_head	vectors[WHEEL_SIZE];
 } ____cacheline_aligned;
@@ -498,23 +500,27 @@ static void internal_add_timer(struct ti
 	__internal_add_timer(base, timer);
 
 	/*
-	 * Check whether the other CPU is in dynticks mode and needs
-	 * to be triggered to reevaluate the timer wheel.  We are
-	 * protected against the other CPU fiddling with the timer by
-	 * holding the timer base lock. This also makes sure that a
-	 * CPU on the way to stop its tick can not evaluate the timer
-	 * wheel.
-	 *
-	 * Spare the IPI for deferrable timers on idle targets though.
-	 * The next busy ticks will take care of it. Except full dynticks
-	 * require special care against races with idle_cpu(), lets deal
-	 * with that later.
-	 */
-	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active) {
-		if (!(timer->flags & TIMER_DEFERRABLE) ||
-		    tick_nohz_full_cpu(base->cpu))
-			wake_up_nohz_cpu(base->cpu);
-	}
+	 * We might have to IPI the remote CPU if the base is idle and the
+	 * timer is not deferrable. If the other cpu is on the way to idle
+	 * then it can't set base->is_idle as we hold base lock.
+	 */
+	if (!IS_ENABLED(CONFIG_NO_HZ_COMMON) || !base->is_idle ||
+	    (timer->flags & TIMER_DEFERRABLE))
+		return;
+
+	/* Check whether this is the new first expiring timer */
+	if (time_after_eq(timer->expires, base->next_expiry))
+		return;
+	base->next_expiry = timer->expires;
+
+	/*
+	 * Check whether the other CPU is in dynticks mode and needs to be
+	 * triggered to reevaluate the timer wheel.  We are protected against
+	 * the other CPU fiddling with the timer by holding the timer base
+	 * lock.
+	 */
+	if (tick_nohz_full_cpu(base->cpu))
+		wake_up_nohz_cpu(base->cpu);
 }
 
 #ifdef CONFIG_TIMER_STATS
@@ -827,16 +833,50 @@ static inline struct timer_base *get_tim
 	return get_timer_cpu_base(tflags, tflags & TIMER_BASEMASK);
 }
 
-static inline struct timer_base *get_target_base(struct timer_base *base,
-						 unsigned tflags)
-{
 #ifdef CONFIG_NO_HZ_COMMON
+static inline struct timer_base *__get_target_base(struct timer_base *base,
+						   unsigned tflags)
+{
 	if ((tflags & TIMER_PINNED) || !base->migration_enabled)
 		return get_timer_this_cpu_base(tflags);
 	return get_timer_cpu_base(tflags, get_nohz_timer_target());
+}
+
+static inline void forward_timer_base(struct timer_base *base)
+{
+	/*
+	 * We only forward the base when it's idle and we have a delta between
+	 * base clock and jiffies.
+	 */
+	if (!base->is_idle || (jiffies - base->clk) < 2)
+		return;
+
+	/*
+	 * If the next expiry value is >= jiffies, then we fast forward to
+	 * jiffies otherwise we forward to the next expiry value.
+	 */
+	if (time_after_eq(base->next_expiry, jiffies))
+		base->clk = jiffies;
+	else
+		base->clk = base->next_expiry;
+}
 #else
+static inline struct timer_base *__get_target_base(struct timer_base *base,
+						   unsigned tflags)
+{
 	return get_timer_this_cpu_base(tflags);
+}
+
+static inline void forward_timer_base(struct timer_base *base) { }
 #endif
+
+static inline struct timer_base *get_target_base(struct timer_base *base,
+						 unsigned tflags)
+{
+	struct timer_base *target = __get_target_base(base, tflags);
+
+	forward_timer_base(target);
+	return target;
 }
 
 /*
@@ -1255,6 +1295,7 @@ static int __collect_expired_timers(stru
 		/* Is it time to look at the next level? */
 		if (clock & LVL_CLK_MASK)
 			return levels;
+
 		/* Shift clock for the next level granularity */
 		clock >>= LVL_CLK_SHIFT;
 	}
@@ -1426,16 +1467,49 @@ u64 get_next_timer_interrupt(unsigned lo
 
 	spin_lock(&base->lock);
 	nextevt = __next_timer_interrupt(base);
-	spin_unlock(&base->lock);
+	base->next_expiry = nextevt;
+	/*
+	 * We have a fresh next event. Check whether we can forward the base.
+	 */
+	if (time_after_eq(nextevt, jiffies))
+		base->clk = jiffies;
+	else if (time_after(nextevt, base->clk))
+		base->clk = nextevt;
 
-	if (time_before_eq(nextevt, basej))
+	if (time_before_eq(nextevt, basej)) {
 		expires = basem;
-	else
+		base->is_idle = false;
+	} else {
 		expires = basem + (nextevt - basej) * TICK_NSEC;
+		/*
+		 * If we expect to sleep more than a tick, mark the base idle.
+		 */
+		if ((expires - basem) > TICK_NSEC)
+			base->is_idle = true;
+	}
+	spin_unlock(&base->lock);
 
 	return cmp_next_hrtimer_event(basem, expires);
 }
 
+/**
+ * timer_clear_idle - Clear the idle state of the timer base
+ *
+ * Called with interrupts disabled
+ */
+void timer_clear_idle(void)
+{
+	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
+
+	/*
+	 * We do this unlocked. The worst outcome is a remote enqueue sending
+	 * a pointless IPI, but taking the lock would just make the window for
+	 * sending the IPI a few instructions smaller for the cost of taking
+	 * the lock in the exit from idle path.
+	 */
+	base->is_idle = false;
+}
+
 static int collect_expired_timers(struct timer_base *base,
 				  struct hlist_head *heads)
 {

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

#1420986 — Re: [patch 18/20] timer: Forward wheel clock whenever possible

Thomas,

On Mon, Jun 13, 2016 at 08:41:04AM -0000, Thomas Gleixner wrote:
> @@ -498,23 +500,27 @@ static void internal_add_timer(struct ti
>  	__internal_add_timer(base, timer);
>  
>  	/*
> -	 * Check whether the other CPU is in dynticks mode and needs
> -	 * to be triggered to reevaluate the timer wheel.  We are
> -	 * protected against the other CPU fiddling with the timer by
> -	 * holding the timer base lock. This also makes sure that a
> -	 * CPU on the way to stop its tick can not evaluate the timer
> -	 * wheel.
> -	 *
> -	 * Spare the IPI for deferrable timers on idle targets though.
> -	 * The next busy ticks will take care of it. Except full dynticks
> -	 * require special care against races with idle_cpu(), lets deal
> -	 * with that later.
> -	 */
> -	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active) {
> -		if (!(timer->flags & TIMER_DEFERRABLE) ||
> -		    tick_nohz_full_cpu(base->cpu))
> -			wake_up_nohz_cpu(base->cpu);
> -	}
> +	 * We might have to IPI the remote CPU if the base is idle and the
> +	 * timer is not deferrable. If the other cpu is on the way to idle
> +	 * then it can't set base->is_idle as we hold base lock.
> +	 */
> +	if (!IS_ENABLED(CONFIG_NO_HZ_COMMON) || !base->is_idle ||
> +	    (timer->flags & TIMER_DEFERRABLE))
> +		return;

The tests for is_idle and TIMER_DEFERRABLE are actually checking the
same thing.  I was looking at the usage of base.is_idle, and it is
only ever set for BASE_STD.

So the TIMER_DEFERRABLE test is redundant, but maybe this is on
purpose?  Anyhow, it did leave me scratching my head.

Thanks,
Richard

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

#1420995 — Re: [patch 18/20] timer: Forward wheel clock whenever possible

On Mon, 13 Jun 2016, Richard Cochran wrote:
> On Mon, Jun 13, 2016 at 08:41:04AM -0000, Thomas Gleixner wrote:
> > @@ -498,23 +500,27 @@ static void internal_add_timer(struct ti
> >  	__internal_add_timer(base, timer);
> >  
> >  	/*
> > -	 * Check whether the other CPU is in dynticks mode and needs
> > -	 * to be triggered to reevaluate the timer wheel.  We are
> > -	 * protected against the other CPU fiddling with the timer by
> > -	 * holding the timer base lock. This also makes sure that a
> > -	 * CPU on the way to stop its tick can not evaluate the timer
> > -	 * wheel.
> > -	 *
> > -	 * Spare the IPI for deferrable timers on idle targets though.
> > -	 * The next busy ticks will take care of it. Except full dynticks
> > -	 * require special care against races with idle_cpu(), lets deal
> > -	 * with that later.
> > -	 */
> > -	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active) {
> > -		if (!(timer->flags & TIMER_DEFERRABLE) ||
> > -		    tick_nohz_full_cpu(base->cpu))
> > -			wake_up_nohz_cpu(base->cpu);
> > -	}
> > +	 * We might have to IPI the remote CPU if the base is idle and the
> > +	 * timer is not deferrable. If the other cpu is on the way to idle
> > +	 * then it can't set base->is_idle as we hold base lock.
> > +	 */
> > +	if (!IS_ENABLED(CONFIG_NO_HZ_COMMON) || !base->is_idle ||
> > +	    (timer->flags & TIMER_DEFERRABLE))
> > +		return;
> 
> The tests for is_idle and TIMER_DEFERRABLE are actually checking the
> same thing.  I was looking at the usage of base.is_idle, and it is
> only ever set for BASE_STD.
> 
> So the TIMER_DEFERRABLE test is redundant, but maybe this is on
> purpose?  Anyhow, it did leave me scratching my head.

You're right. is_idle makes the deferrable check obsolete. We can remove it,
but it needs a big fat comment ....

Thanks

	tglx

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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web