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

Groups > linux.kernel > #1320827 > 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 Frederic Weisbecker <fweisbec@gmail.com> - 2016-01-28 17:10 +0100
    Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Peter Zijlstra <peterz@infradead.org> - 2016-01-29 11:00 +0100
      Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Byungchul Park <byungchul.park@lge.com> - 2016-02-01 11:10 +0100
        [PATCH] sched: calculate sched_clock_cpu without tick handling during nohz Byungchul Park <byungchul.park@lge.com> - 2016-02-01 11:20 +0100
        Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Peter Zijlstra <peterz@infradead.org> - 2016-02-01 11:40 +0100
          Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Byungchul Park <byungchul.park@lge.com> - 2016-02-02 01:00 +0100
          Re: [PATCH 1/4] sched: Don't account tickless CPU load on tick Byungchul Park <byungchul.park@lge.com> - 2016-02-02 02:00 +0100

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

On Wed, Jan 20, 2016 at 07:26:14PM +0900, Byungchul Park wrote:
> 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.

So lets check all the things we call on scheduler_tick():

_ sched_clock_tick(): maybe it doesn't need to be called when idle. I'm not sure.
  Some code in idle (timers, irqs, ...) might need to call sched_clock().

_ update_rq_clock(), task_tick(): task_tick is empty for idle class, so we probably
  don't need an updated rq either.

_ update_cpu_load_active(): I was about to fix the issue properly and make it account
  correctly on idle ticks but we might as well want to spare it.

_ calc_global_load_tick(): no idea

_ perf_event_task_tick(): needed, some freq CPU events can trigger in idle and need
  adjustments

_ trigger_load_balance(): maybe needed, I see it triggers the softirq after some
  rebalance delay, regardless of the current CPU idleness.

_ rq_last_tick_reset(): not needed in idle

Here we are.


> 
> > ---
> > 
> > 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] | [next] | [standalone]

#1321621

On Thu, Jan 28, 2016 at 05:01:26PM +0100, Frederic Weisbecker wrote:
> So lets check all the things we call on scheduler_tick():
> 
> _ sched_clock_tick(): maybe it doesn't need to be called when idle. I'm not sure.
>   Some code in idle (timers, irqs, ...) might need to call sched_clock().

Only needed if you've got a shady TSC.

> _ update_rq_clock(), task_tick(): task_tick is empty for idle class, so we probably
>   don't need an updated rq either.

Right, for regular NOHZ we'll be running the idle task, and the idle
tick handler is empty. So for NOHZ you can ignore this.

For NOHZ_FULL you'll not be running the idle task and this gets
'interesting'.

The most important part would be tracking the task runtime, which is
used for a number of user visible things. This should be doable
remotely.

> _ update_cpu_load_active(): I was about to fix the issue properly and make it account
>   correctly on idle ticks but we might as well want to spare it.

Right, we've gone over this one in detail in other emails I think.

> _ calc_global_load_tick(): no idea

Can easily be done remote. However, this only records deltas of
nr_active (:= nr_running + nr_uninterruptible) and for NOHZ and
NOHZ_FULL this should not change, therefore the delta _should_ be 0 and
you can skip this.

> _ perf_event_task_tick(): needed, some freq CPU events can trigger in idle and need
>   adjustments

Right, this is a tricky one. Maybe I should look into moving this into a
hrtimer, but that too has 'fun' problems IIRC. I'll put it on the TODO
list somewhere.

> _ trigger_load_balance(): maybe needed, I see it triggers the softirq after some
>   rebalance delay, regardless of the current CPU idleness.

We already have NOHZ remote balancing, we could (and should) probably do
the same for NOHZ_FULL. Then again, I would expect the NOHZ_FULL cpus to
not actually be part of a balance domain, so we could probably detect
and short-circuit this.

> _ rq_last_tick_reset(): not needed in idle

Right, part of the NOHZ_FULL 'hack', once you fix all the remote
accounting stuff this could go away entirely think.

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

#1322958

On Fri, Jan 29, 2016 at 10:50:16AM +0100, Peter Zijlstra wrote:
> On Thu, Jan 28, 2016 at 05:01:26PM +0100, Frederic Weisbecker wrote:
> > So lets check all the things we call on scheduler_tick():
> > 
> > _ sched_clock_tick(): maybe it doesn't need to be called when idle. I'm not sure.
> >   Some code in idle (timers, irqs, ...) might need to call sched_clock().
> 
> Only needed if you've got a shady TSC.

Yeh.. IMO, this can be done without the tick handling during nohz, with the
patch I am attaching. Could you check the patch? Or we have to handle it
remotely, too. (for a crazy TSC)

> 
> > _ update_rq_clock(), task_tick(): task_tick is empty for idle class, so we probably
> >   don't need an updated rq either.
> 
> Right, for regular NOHZ we'll be running the idle task, and the idle
> tick handler is empty. So for NOHZ you can ignore this.
> 
> For NOHZ_FULL you'll not be running the idle task and this gets
> 'interesting'.
> 
> The most important part would be tracking the task runtime, which is
> used for a number of user visible things. This should be doable
> remotely.

Isn't there any way to show it to user at the time it's requested?

> 
> > _ update_cpu_load_active(): I was about to fix the issue properly and make it account
> >   correctly on idle ticks but we might as well want to spare it.
> 
> Right, we've gone over this one in detail in other emails I think.

Doing it remotely... hm...

> 
> > _ calc_global_load_tick(): no idea
> 
> Can easily be done remote. However, this only records deltas of
> nr_active (:= nr_running + nr_uninterruptible) and for NOHZ and
> NOHZ_FULL this should not change, therefore the delta _should_ be 0 and
> you can skip this.

It sounds good.

> 
> > _ perf_event_task_tick(): needed, some freq CPU events can trigger in idle and need
> >   adjustments
> 
> Right, this is a tricky one. Maybe I should look into moving this into a
> hrtimer, but that too has 'fun' problems IIRC. I'll put it on the TODO
> list somewhere.

Good luck. I'm sure you'll do well.

> 
> > _ trigger_load_balance(): maybe needed, I see it triggers the softirq after some
> >   rebalance delay, regardless of the current CPU idleness.
> 
> We already have NOHZ remote balancing, we could (and should) probably do

I think so.

> the same for NOHZ_FULL. Then again, I would expect the NOHZ_FULL cpus to
> not actually be part of a balance domain, so we could probably detect

Could not the NOHZ_FULL cpus be part of a balance domain? It sounds good.

> and short-circuit this.
> 
> > _ rq_last_tick_reset(): not needed in idle
> 
> Right, part of the NOHZ_FULL 'hack', once you fix all the remote
> accounting stuff this could go away entirely think.

I'm sure it can be removed eventually!

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

#1322962 — [PATCH] sched: calculate sched_clock_cpu without tick handling during nohz

Currently, unstable TSCs can be adjusted by sched_clock_tick(),
sched_clock_local() and sched_clock_remote() with the tick handling.
Thus without tick, it does not work properly.

This patch makes it possible to adjust it approximately without the
tick handling so that it would be ok even if the sched_clock_tick()
is not called during nohz.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/clock.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index caf4041..064b40a 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -128,6 +128,9 @@ struct sched_clock_data {
 	u64			tick_raw;
 	u64			tick_gtod;
 	u64			clock;
+#ifdef CONFIG_NO_HZ_COMMON
+	unsigned long		update;
+#endif
 };
 
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct sched_clock_data, sched_clock_data);
@@ -153,6 +156,9 @@ void sched_clock_init(void)
 		scd->tick_raw = 0;
 		scd->tick_gtod = ktime_now;
 		scd->clock = ktime_now;
+#ifdef CONFIG_NO_HZ_COMMON
+		scd->update = READ_ONCE(jiffies);
+#endif
 	}
 
 	sched_clock_running = 1;
@@ -186,6 +192,27 @@ static inline u64 wrap_max(u64 x, u64 y)
 	return (s64)(x - y) > 0 ? x : y;
 }
 
+#ifdef CONFIG_NO_HZ_COMMON
+static inline unsigned long tick_pending(struct sched_clock_data *scd)
+{
+	return READ_ONCE(jiffies) - scd->update;
+}
+
+static inline void sched_clock_tick_update(struct sched_clock_data *scd)
+{
+	scd->update = READ_ONCE(jiffies);
+}
+#else
+static inline unsigned long tick_pending(struct sched_clock_data *scd)
+{
+	return 0;
+}
+
+static inline void sched_clock_tick_update(struct sched_clock_data *scd)
+{
+}
+#endif
+
 /*
  * update the percpu scd from the raw @now value
  *
@@ -194,7 +221,7 @@ static inline u64 wrap_max(u64 x, u64 y)
  */
 static u64 sched_clock_local(struct sched_clock_data *scd)
 {
-	u64 now, clock, old_clock, min_clock, max_clock;
+	u64 now, clock, old_clock, min_clock, max_clock, next_gtod;
 	s64 delta;
 
 again:
@@ -211,9 +238,10 @@ again:
 	 *		      scd->tick_gtod + TICK_NSEC);
 	 */
 
+	next_gtod = scd->tick_gtod + (tick_pending(scd) + 1) * TICK_NSEC;
 	clock = scd->tick_gtod + delta;
 	min_clock = wrap_max(scd->tick_gtod, old_clock);
-	max_clock = wrap_max(old_clock, scd->tick_gtod + TICK_NSEC);
+	max_clock = wrap_max(old_clock, next_gtod);
 
 	clock = wrap_max(clock, min_clock);
 	clock = wrap_min(clock, max_clock);
@@ -333,6 +361,7 @@ void sched_clock_tick(void)
 
 	scd->tick_raw = now;
 	scd->tick_gtod = now_gtod;
+	sched_clock_tick_update(scd);
 	sched_clock_local(scd);
 }
 
-- 
1.9.1

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

#1322977

On Mon, Feb 01, 2016 at 07:05:13PM +0900, Byungchul Park wrote:
> On Fri, Jan 29, 2016 at 10:50:16AM +0100, Peter Zijlstra wrote:
> > On Thu, Jan 28, 2016 at 05:01:26PM +0100, Frederic Weisbecker wrote:
> > > So lets check all the things we call on scheduler_tick():
> > > 
> > > _ sched_clock_tick(): maybe it doesn't need to be called when idle. I'm not sure.
> > >   Some code in idle (timers, irqs, ...) might need to call sched_clock().
> > 
> > Only needed if you've got a shady TSC.
> 
> Yeh.. IMO, this can be done without the tick handling during nohz, with the
> patch I am attaching. Could you check the patch? Or we have to handle it
> remotely, too. (for a crazy TSC)

I think NOHZ_FULL already requires the TSC not to be wrecked.

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

#1323653

On Mon, Feb 01, 2016 at 11:34:33AM +0100, Peter Zijlstra wrote:
> On Mon, Feb 01, 2016 at 07:05:13PM +0900, Byungchul Park wrote:
> > On Fri, Jan 29, 2016 at 10:50:16AM +0100, Peter Zijlstra wrote:
> > > On Thu, Jan 28, 2016 at 05:01:26PM +0100, Frederic Weisbecker wrote:
> > > > So lets check all the things we call on scheduler_tick():
> > > > 
> > > > _ sched_clock_tick(): maybe it doesn't need to be called when idle. I'm not sure.
> > > >   Some code in idle (timers, irqs, ...) might need to call sched_clock().
> > > 
> > > Only needed if you've got a shady TSC.
> > 
> > Yeh.. IMO, this can be done without the tick handling during nohz, with the
> > patch I am attaching. Could you check the patch? Or we have to handle it
> > remotely, too. (for a crazy TSC)
> 
> I think NOHZ_FULL already requires the TSC not to be wrecked.

It sounds good!

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

#1323681

On Mon, Feb 01, 2016 at 11:34:33AM +0100, Peter Zijlstra wrote:
> On Mon, Feb 01, 2016 at 07:05:13PM +0900, Byungchul Park wrote:
> > On Fri, Jan 29, 2016 at 10:50:16AM +0100, Peter Zijlstra wrote:
> > > On Thu, Jan 28, 2016 at 05:01:26PM +0100, Frederic Weisbecker wrote:
> > > > So lets check all the things we call on scheduler_tick():
> > > > 
> > > > _ sched_clock_tick(): maybe it doesn't need to be called when idle. I'm not sure.
> > > >   Some code in idle (timers, irqs, ...) might need to call sched_clock().
> > > 
> > > Only needed if you've got a shady TSC.
> > 
> > Yeh.. IMO, this can be done without the tick handling during nohz, with the
> > patch I am attaching. Could you check the patch? Or we have to handle it
> > remotely, too. (for a crazy TSC)
> 
> I think NOHZ_FULL already requires the TSC not to be wrecked.

What about the regular NOHZ? Or does not any code in idle call a kind of
sched_lock_cpu() at all?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web