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

Groups > linux.kernel > #1312195 > unrolled thread

Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick

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 1/4] sched: Don't account tickless CPU load on tick Peter Zijlstra <peterz@infradead.org> - 2016-01-19 14:10 +0100
    Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Frederic Weisbecker <fweisbec@gmail.com> - 2016-01-19 17:30 +0100
      Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Peter Zijlstra <peterz@infradead.org> - 2016-01-19 20:00 +0100
        Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Frederic Weisbecker <fweisbec@gmail.com> - 2016-01-19 23:40 +0100
          Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Byungchul Park <byungchul.park@lge.com> - 2016-01-20 06:50 +0100
            Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Byungchul Park <byungchul.park@lge.com> - 2016-01-20 11:30 +0100
    Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Thomas Gleixner <tglx@linutronix.de> - 2016-01-20 09:50 +0100
      Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Frederic Weisbecker <fweisbec@gmail.com> - 2016-01-20 18:40 +0100
      Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Byungchul Park <byungchul.park@lge.com> - 2016-01-22 09:50 +0100

#1312195 — Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick

On Wed, Jan 13, 2016 at 05:01:28PM +0100, Frederic Weisbecker wrote:
> The cpu load update on tick doesn't care about dynticks and as such is
> buggy when occuring on nohz ticks (including idle ticks) as it resets
> the jiffies snapshot that was recorded on nohz entry. We eventually
> ignore the potentially long tickless load that happened before the
> tick.

I don't get it, how can we call scheduler_tick() while
tick_nohz_tick_stopped() ?

[toc] | [next] | [standalone]

#1312367

On Tue, Jan 19, 2016 at 02:08:57PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 13, 2016 at 05:01:28PM +0100, Frederic Weisbecker wrote:
> > The cpu load update on tick doesn't care about dynticks and as such is
> > buggy when occuring on nohz ticks (including idle ticks) as it resets
> > the jiffies snapshot that was recorded on nohz entry. We eventually
> > ignore the potentially long tickless load that happened before the
> > tick.
> 
> I don't get it, how can we call scheduler_tick() while
> tick_nohz_tick_stopped() ?

tick_nohz_tick_stopped() (which is ts->tick_stopped == 1) doesn't actually
mean that the tick is really stopped. It just means that the tick fires only
when it's really needed (timer list expired, RCU stuff, irq_work, ...). So
if you're lucky, the tick is really stopped because there is nothing that needs
the tick in the future, otherwise (and it's probably most of the time) there is
some tick that may well be programmed some time ahead in the future.

So that's the kind of tick that can break the accounting of a whole long tickless
load that just ran.

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

#1312447

On Tue, Jan 19, 2016 at 05:22:11PM +0100, Frederic Weisbecker wrote:
> On Tue, Jan 19, 2016 at 02:08:57PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 13, 2016 at 05:01:28PM +0100, Frederic Weisbecker wrote:
> > > The cpu load update on tick doesn't care about dynticks and as such is
> > > buggy when occuring on nohz ticks (including idle ticks) as it resets
> > > the jiffies snapshot that was recorded on nohz entry. We eventually
> > > ignore the potentially long tickless load that happened before the
> > > tick.
> > 
> > I don't get it, how can we call scheduler_tick() while
> > tick_nohz_tick_stopped() ?
> 
> tick_nohz_tick_stopped() (which is ts->tick_stopped == 1) doesn't actually
> mean that the tick is really stopped. It just means that the tick fires only
> when it's really needed (timer list expired, RCU stuff, irq_work, ...).

That's insane and broken. Fix _that_.

If RCU, irq_work etc.. needs the tick, do not stop the tick.

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

#1312564

On Tue, Jan 19, 2016 at 07:56:47PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 19, 2016 at 05:22:11PM +0100, Frederic Weisbecker wrote:
> > On Tue, Jan 19, 2016 at 02:08:57PM +0100, Peter Zijlstra wrote:
> > > On Wed, Jan 13, 2016 at 05:01:28PM +0100, Frederic Weisbecker wrote:
> > > > The cpu load update on tick doesn't care about dynticks and as such is
> > > > buggy when occuring on nohz ticks (including idle ticks) as it resets
> > > > the jiffies snapshot that was recorded on nohz entry. We eventually
> > > > ignore the potentially long tickless load that happened before the
> > > > tick.
> > > 
> > > I don't get it, how can we call scheduler_tick() while
> > > tick_nohz_tick_stopped() ?
> > 
> > tick_nohz_tick_stopped() (which is ts->tick_stopped == 1) doesn't actually
> > mean that the tick is really stopped. It just means that the tick fires only
> > when it's really needed (timer list expired, RCU stuff, irq_work, ...).
> 
> That's insane and broken. Fix _that_.
> 
> If RCU, irq_work etc.. needs the tick, do not stop the tick.

This is not the first time we have this conversation :-)

RCU/irq_work/foo_needs_tick() are treated just like any timer that expire in one
tick, although RCU is some more tunable there.

And timers that expire in 1 jiffy can be treated in two ways:

* If the tick is still periodic (ts->tick_stopped = 0), we don't stop the
  tick: we don't enter dynticks mode.

* If the tick is already stopped (or rather in dynticks mode to be more exact:
  ts->tick_stopped == 1) we just program the tick one jiffy ahead. This is
  an optimization and a simplification, if we were to restart the tick everytime
  we see a tick one jiffy ahead in the middle of a dynticks frame, we would have
  to perform all the accounting in tick_nohz_idle_exit() as well, including
  update_cpu_load_nohz() that locks rq->lock. Having a bunch of jiffies subsequently
  ticking in the middle of a dynticks frame is a common and frequent scenario and
  removing that optimization would have a bad visible impact.

Certainly the issue here is that "tick_stopped" can be misunderstood. ts->dynticks_active
would be better but we already have ts->nohz_active which reflects something very
different. I guess we need a cascading rename.

Anyway whether the next tick is one jiffy ahead or more doesn't really matter here.
The issue is that ticks can fire while dynticks-idle or dyntick-buzy and
update_cpu_load_active() treats them in a broken way.

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

#1312894

On Tue, Jan 19, 2016 at 11:33:22PM +0100, Frederic Weisbecker wrote:
> On Tue, Jan 19, 2016 at 07:56:47PM +0100, Peter Zijlstra wrote:
> > On Tue, Jan 19, 2016 at 05:22:11PM +0100, Frederic Weisbecker wrote:
> > > On Tue, Jan 19, 2016 at 02:08:57PM +0100, Peter Zijlstra wrote:
> > > > On Wed, Jan 13, 2016 at 05:01:28PM +0100, Frederic Weisbecker wrote:
> > > > > The cpu load update on tick doesn't care about dynticks and as such is
> > > > > buggy when occuring on nohz ticks (including idle ticks) as it resets
> > > > > the jiffies snapshot that was recorded on nohz entry. We eventually
> > > > > ignore the potentially long tickless load that happened before the
> > > > > tick.
> > > > 
> > > > I don't get it, how can we call scheduler_tick() while
> > > > tick_nohz_tick_stopped() ?
> > > 
> > > tick_nohz_tick_stopped() (which is ts->tick_stopped == 1) doesn't actually
> > > mean that the tick is really stopped. It just means that the tick fires only
> > > when it's really needed (timer list expired, RCU stuff, irq_work, ...).
> > 
> > That's insane and broken. Fix _that_.
> > 
> > If RCU, irq_work etc.. needs the tick, do not stop the tick.
> 
> This is not the first time we have this conversation :-)
> 
> RCU/irq_work/foo_needs_tick() are treated just like any timer that expire in one
> tick, although RCU is some more tunable there.
> 
> And timers that expire in 1 jiffy can be treated in two ways:
> 
> * If the tick is still periodic (ts->tick_stopped = 0), we don't stop the
>   tick: we don't enter dynticks mode.
> 
> * If the tick is already stopped (or rather in dynticks mode to be more exact:
>   ts->tick_stopped == 1) we just program the tick one jiffy ahead. This is
>   an optimization and a simplification, if we were to restart the tick everytime
>   we see a tick one jiffy ahead in the middle of a dynticks frame, we would have
>   to perform all the accounting in tick_nohz_idle_exit() as well, including
>   update_cpu_load_nohz() that locks rq->lock. Having a bunch of jiffies subsequently
>   ticking in the middle of a dynticks frame is a common and frequent scenario and
>   removing that optimization would have a bad visible impact.
> 
> Certainly the issue here is that "tick_stopped" can be misunderstood. ts->dynticks_active
> would be better but we already have ts->nohz_active which reflects something very
> different. I guess we need a cascading rename.
> 
> Anyway whether the next tick is one jiffy ahead or more doesn't really matter here.
> The issue is that ticks can fire while dynticks-idle or dyntick-buzy and
> update_cpu_load_active() treats them in a broken way.

It looks very tricky. I have a question. Do we have to call the
scheduler_tick() even while the tick is stopped? IMHO, it seems to be
ok even if we won't call it while the tick is stopped. Wrong? I mean,

---

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index bbc5d11..774adc2 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1422,7 +1422,8 @@ void update_process_times(int user_tick)
 	if (in_irq())
 		irq_work_tick();
 #endif
-	scheduler_tick();
+	if (!tick_nohz_tick_stopped())
+		scheduler_tick();
 	run_posix_cpu_timers(p);
 }
 
---

hm ???

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

#1313087

On Wed, Jan 20, 2016 at 02:43:35PM +0900, Byungchul Park wrote:
> 
> It looks very tricky. I have a question. Do we have to call the
> scheduler_tick() even while the tick is stopped? IMHO, it seems to be
> ok even if we won't call it while the tick is stopped. Wrong? I mean,
> 

The reason why I asked is that, scheduler_tick() looks to be a
scheduler callback for *periodic tick*. IMHO, we need to choose one of
these two.

1) Make scheduler_tick() can handle it, not only for the periodic tick
but also for the tick-like event during tick-stopped. But I am not sure
if this is the right way.

2) Distinguish the periodic tick from the tick-like event by which we
can handle rcu callback, irq work and so on, so that the periodic tick
handler only handles periodic stuff either locally or remotely, while
the tick-like event handler only does its purpose. I think this is
better, I am sure though.

> ---
> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index bbc5d11..774adc2 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1422,7 +1422,8 @@ void update_process_times(int user_tick)
>  	if (in_irq())
>  		irq_work_tick();
>  #endif
> -	scheduler_tick();
> +	if (!tick_nohz_tick_stopped())
> +		scheduler_tick();
>  	run_posix_cpu_timers(p);
>  }
>  
> ---
> 
> hm ???

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

#1312997

On Tue, 19 Jan 2016, Peter Zijlstra wrote:

> On Wed, Jan 13, 2016 at 05:01:28PM +0100, Frederic Weisbecker wrote:
> > The cpu load update on tick doesn't care about dynticks and as such is
> > buggy when occuring on nohz ticks (including idle ticks) as it resets
> > the jiffies snapshot that was recorded on nohz entry. We eventually
> > ignore the potentially long tickless load that happened before the
> > tick.
> 
> I don't get it, how can we call scheduler_tick() while
> tick_nohz_tick_stopped() ?

tick->nohz_stopped is merily indicating that we switched from periodic mode to
tickless mode. That's probably a misnomer, but it still has that meaning.

You really need to look at it from the history of that code which was designed
for tickless idle. The nohz full stuff was bolted on it.

So if we stop the tick in idle - or for that matter in full nohz - we look
ahead when the next tick is required. That can be:

      - a timer wheel timer expiring

      - other stuff which prevents the cpu from going "tickless" like rcu,
        irqwork

So lets assume rcu and irqwork are silent, but we have a timer expiring 100ms
from now, then we program the tick timer to 100ms from now. When it fires it
invokes the normal tick_sched() timer machinery:

     - timekeeping update
     - update_process_times
     - profile_tick

I have no idea why that is a problem. If update_process_times() is invoked
then it will account the elapsed time to the idle task in case of tickless
idle. In case of nohz full it should simply account the time to the task which
was busy on the cpu in user space.

The above changelog is just crap and doesnt make any sense at all. And the
patch is fixing symptoms not the root cause.

Thanks,

	tglx

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

#1313405

On Wed, Jan 20, 2016 at 09:42:16AM +0100, Thomas Gleixner wrote:
> On Tue, 19 Jan 2016, Peter Zijlstra wrote:
> 
> > On Wed, Jan 13, 2016 at 05:01:28PM +0100, Frederic Weisbecker wrote:
> > > The cpu load update on tick doesn't care about dynticks and as such is
> > > buggy when occuring on nohz ticks (including idle ticks) as it resets
> > > the jiffies snapshot that was recorded on nohz entry. We eventually
> > > ignore the potentially long tickless load that happened before the
> > > tick.
> > 
> > I don't get it, how can we call scheduler_tick() while
> > tick_nohz_tick_stopped() ?
> 
> tick->nohz_stopped is merily indicating that we switched from periodic mode to
> tickless mode. That's probably a misnomer, but it still has that meaning.
> 
> You really need to look at it from the history of that code which was designed
> for tickless idle. The nohz full stuff was bolted on it.
> 
> So if we stop the tick in idle - or for that matter in full nohz - we look
> ahead when the next tick is required. That can be:
> 
>       - a timer wheel timer expiring
> 
>       - other stuff which prevents the cpu from going "tickless" like rcu,
>         irqwork
> 
> So lets assume rcu and irqwork are silent, but we have a timer expiring 100ms
> from now, then we program the tick timer to 100ms from now. When it fires it
> invokes the normal tick_sched() timer machinery:
> 
>      - timekeeping update
>      - update_process_times
>      - profile_tick
> 
> I have no idea why that is a problem. If update_process_times() is invoked
> then it will account the elapsed time to the idle task in case of tickless
> idle. In case of nohz full it should simply account the time to the task which
> was busy on the cpu in user space.
> 
> The above changelog is just crap and doesnt make any sense at all. And the
> patch is fixing symptoms not the root cause.

So the other way to fix this is to account properly the tickless load and avoid
to account some newly awoken task load. We could record the weighted_cpuload()
on nohz entry (or 0 in the case of idle) and then account that on idle exit.

I think it's a better solution.

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

#1314829

On Wed, Jan 20, 2016 at 09:42:16AM +0100, Thomas Gleixner wrote:
> 
> The above changelog is just crap and doesnt make any sense at all. And the
> patch is fixing symptoms not the root cause.

IMHO, the root cause is the "tick" definition. Am I only one confused?
I am confused now. What is tick? The timer interrupt for handling rcu
callback or irq work while the (perioid) tick was stoped, is a tick?

If it is true, then many code including scheduler assuming tick happens
periodically must be fixed to be able to handle the non-periodic tick, esp.
scheduler_tick(). And we have to focus that. Then we don't need something
accounting a cpu load remotely and periodically, because the local tick
handler can account it well locally.

If it is not true, that is, the timer interrupt for handling rcu callback
or irq work during tick-stopped, is not a tick but just a interrupt by which
we want something to be done, then I think we need to make the handler do
only its purpose. In this case, tick related handling has to be deferred to
the tick-restart point. And we have to focus it, so that it can be done.

Regardless of the answer, true or not true, if there is something the
housekeeper must do periodically, then it should be done even remotely. But
I think accounting cpu load is not the case. The way to implement it
depends on the above answer. IMHO, the letter direction is better.

Just a my opinion, and I maybe lack kernel knowledge compared with you.
Please let me know if I am wrong. Or please let me know your opinions.

> 
> Thanks,
> 
> 	tglx
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web