true vs. system idle cputime

RFC PATCH In review Martin Schwidefsky 4 participants 13 messages Oct 8, 2008 – Oct 16, 2008

Subsystems (8)

x86 PowerPC Scheduler s390 +4 more subsystems

lore

Share & tools

lore

Martin Schwidefsky

author

2008-10-08 16:30 UTC #0 lore raw

cover true vs. system idle cputime

Greetings,
while working on the analysis of a mismatch between the cputime accounting numbers of z/VM as the host and Linux as the guest I started to wonder about the accounting of idle time. z/VM showed more cpu time for the guest as the guest itself. With the current code everything that the idle process does is accounted as idle time. If idle is sleeping that is fine, but if idle is actually using cpu cycles this is wrong.

The question is how wrong? To find out I've implemented really precise accounting of true idle vs. system idle cputime for s390. A really simple test that wakes up 100 times per second to do some minimal work before going back to sleep showed 0.35% of system idle time. If you are dealing with lots of virtual penguins this quickly becomes significant.

There are four patches in this series:
Patch #1: Cleanup scaled / unscaled cputime accounting
Patch #2: Change the accounting interface to allow the architectures to do
precise idle time accounting
Patch #3: s390 patch to improve the precision of the idle_time_us value Patch #4: s390 patch to implement improved idle time accounting

There is one change in patch #2 that might require a change on powerpc and/or ia64. The generic TICK_ONESHOT/NO_HZ code calculates the number of ticks spent with a disabled HZ timer and accounts this as idle time. For a configuration for VIRT_CPU_ACCOUNTING=y this is horribly wrong. Either you have precise accounting or you don't. Patch #2 just removes the calculation for VIRT_CPU_ACCOUNTING=y. The architectures which support precise accounting have to deal with it on their own. This is where the powerpc and ia64 maintainer come into play. Would you look at patch #2 please ?

To make it clearer what happens in tick_nohz_restart_sched_tick I've added a new function account_idle_ticks(). And for good measure another one named account_steal_ticks() for xen where "interesting" things have been done with the account_steal_time interface.

Signature · 4 lines

blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Martin Schwidefsky

author

2008-10-08 16:29 UTC #1 lore raw

PATCH 1/4 fix scaled & unscaled cputime accounting

From: Martin Schwidefsky <[email protected]>

The utimescaled / stimescaled fields in the task structure and the global cpustat should be set on all architectures. On s390 the calls to account_user_time_scaled and account_system_time_scaled never have been added. In addition system time that is accounted as guest time to the user time of a process is accounted to the scaled system time instead of the scaled user time.
To fix the bugs and to prevent future forgetfulness this patch merges account_system_time_scaled into account_system_time and account_user_time_scaled into account_user_time.

Signed-off-by Martin Schwidefsky <[email protected]>

---

 arch/ia64/kernel/time.c     |   12 ++++--------
 arch/powerpc/kernel/time.c  |    7 ++-----
 arch/s390/kernel/vtime.c    |   10 +++++-----
 include/linux/kernel_stat.h |    6 ++----
 kernel/sched.c              |   41 ++++++++++++++++-------------------------
 kernel/time/tick-sched.c    |    5 +++--
 kernel/timer.c              |   12 +++++-------
 7 files changed, 37 insertions(+), 56 deletions(-)

arch/ia64/kernel/time.c +4 -8

Index: linux-idle/arch/ia64/kernel/time.c
===================================================================
@@ -93,13 +93,11 @@ void ia64_account_on_switch(struct task_
 	now = ia64_get_itc();
 
 	delta_stime = cycle_to_cputime(pi->ac_stime + (now - pi->ac_stamp));
-	account_system_time(prev, 0, delta_stime);
-	account_system_time_scaled(prev, delta_stime);
+	account_system_time(prev, 0, delta_stime, delta_stime);
 
 	if (pi->ac_utime) {
 		delta_utime = cycle_to_cputime(pi->ac_utime);
-		account_user_time(prev, delta_utime);
-		account_user_time_scaled(prev, delta_utime);
+		account_user_time(prev, delta_utime, delta_utime);
 	}
 
 	pi->ac_stamp = ni->ac_stamp = now;

@@ -122,8 +120,7 @@ void account_system_vtime(struct task_st
 	now = ia64_get_itc();
 
 	delta_stime = cycle_to_cputime(ti->ac_stime + (now - ti->ac_stamp));
-	account_system_time(tsk, 0, delta_stime);
-	account_system_time_scaled(tsk, delta_stime);
+	account_system_time(tsk, 0, delta_stime, delta_stime);
 	ti->ac_stime = 0;
 
 	ti->ac_stamp = now;

@@ -143,8 +140,7 @@ void account_process_tick(struct task_st
 
 	if (ti->ac_utime) {
 		delta_utime = cycle_to_cputime(ti->ac_utime);
-		account_user_time(p, delta_utime);
-		account_user_time_scaled(p, delta_utime);
+		account_user_time(p, delta_utime, delta_utime);
 		ti->ac_utime = 0;
 	}
 }

arch/powerpc/kernel/time.c +2 -5

Index: linux-idle/arch/powerpc/kernel/time.c
===================================================================
@@ -258,8 +258,7 @@ void account_system_vtime(struct task_st
 		delta += sys_time;
 		get_paca()->system_time = 0;
 	}
-	account_system_time(tsk, 0, delta);
-	account_system_time_scaled(tsk, deltascaled);
+	account_system_time(tsk, 0, delta, deltascaled);
 	per_cpu(cputime_last_delta, smp_processor_id()) = delta;
 	per_cpu(cputime_scaled_last_delta, smp_processor_id()) = deltascaled;
 	local_irq_restore(flags);

@@ -277,10 +276,8 @@ void account_process_tick(struct task_st
 
 	utime = get_paca()->user_time;
 	get_paca()->user_time = 0;
-	account_user_time(tsk, utime);
-
 	utimescaled = cputime_to_scaled(utime);
-	account_user_time_scaled(tsk, utimescaled);
+	account_user_time(tsk, utime, utimescaled);
 }
 
 /*

arch/s390/kernel/vtime.c +5 -5

Index: linux-idle/arch/s390/kernel/vtime.c
===================================================================
@@ -51,12 +51,12 @@ void account_process_tick(struct task_st
 	rcu_user_flag = cputime != 0;
 	S390_lowcore.user_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_user_time(tsk, cputime);
+	account_user_time(tsk, cputime, cputime);
 
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_system_time(tsk, HARDIRQ_OFFSET, cputime);
+	account_system_time(tsk, HARDIRQ_OFFSET, cputime, cputime);
 
 	cputime = S390_lowcore.steal_clock;
 	if ((__s64) cputime > 0) {

@@ -83,12 +83,12 @@ void account_vtime(struct task_struct *t
 	cputime = S390_lowcore.user_timer >> 12;
 	S390_lowcore.user_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_user_time(tsk, cputime);
+	account_user_time(tsk, cputime, cputime);
 
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_system_time(tsk, 0, cputime);
+	account_system_time(tsk, 0, cputime, cputime);
 }
 
 /*

@@ -108,7 +108,7 @@ void account_system_vtime(struct task_st
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_system_time(tsk, 0, cputime);
+	account_system_time(tsk, 0, cputime, cputime);
 }
 EXPORT_SYMBOL_GPL(account_system_vtime);

include/linux/kernel_stat.h +2 -4

Index: linux-idle/include/linux/kernel_stat.h
===================================================================
@@ -52,10 +52,8 @@ static inline int kstat_irqs(int irq)
 	return sum;
 }
 
-extern void account_user_time(struct task_struct *, cputime_t);
-extern void account_user_time_scaled(struct task_struct *, cputime_t);
-extern void account_system_time(struct task_struct *, int, cputime_t);
-extern void account_system_time_scaled(struct task_struct *, cputime_t);
+extern void account_user_time(struct task_struct *, cputime_t, cputime_t);
+extern void account_system_time(struct task_struct *, int, cputime_t, cputime_t);
 extern void account_steal_time(struct task_struct *, cputime_t);
 
 #endif /* _LINUX_KERNEL_STAT_H */

kernel/sched.c +16 ~~-25~~

96 lines not on this page · raw message

kernel/time/tick-sched.c +3 -2

Index: linux-idle/kernel/time/tick-sched.c
===================================================================
@@ -378,6 +378,7 @@ void tick_nohz_restart_sched_tick(void)
 	int cpu = smp_processor_id();
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 	unsigned long ticks;
+	cputime_t cputime;
 	ktime_t now;
 
 	local_irq_disable();

@@ -410,8 +411,8 @@ void tick_nohz_restart_sched_tick(void)
 	 */
 	if (ticks && ticks < LONG_MAX) {
 		add_preempt_count(HARDIRQ_OFFSET);
-		account_system_time(current, HARDIRQ_OFFSET,
-				    jiffies_to_cputime(ticks));
+		cputime = jiffies_to_cputime(ticks);
+		account_system_time(current, HARDIRQ_OFFSET, cputime, cputime);
 		sub_preempt_count(HARDIRQ_OFFSET);
 	}

kernel/timer.c +5 -7

Index: linux-idle/kernel/timer.c
===================================================================
@@ -954,13 +954,11 @@ void account_process_tick(struct task_st
 {
 	cputime_t one_jiffy = jiffies_to_cputime(1);
 
-	if (user_tick) {
-		account_user_time(p, one_jiffy);
-		account_user_time_scaled(p, cputime_to_scaled(one_jiffy));
-	} else {
-		account_system_time(p, HARDIRQ_OFFSET, one_jiffy);
-		account_system_time_scaled(p, cputime_to_scaled(one_jiffy));
-	}
+	if (user_tick)
+		account_user_time(p, one_jiffy, cputime_to_scaled(one_jiffy));
+	else
+		account_system_time(p, HARDIRQ_OFFSET, one_jiffy,
+				    cputime_to_scaled(one_jiffy));
 }
 #endif

Signature · 4 lines

blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Paul Mackerras

Acked-by on v1 1/4

2008-10-16 04:31 UTC #2 lore raw

Re: [patch 1/4] fix scaled & unscaled cputime accounting

Quoted 11 lines · Martin Schwidefsky

The utimescaled / stimescaled fields in the task structure and the global cpustat should be set on all architectures. On s390 the calls to account_user_time_scaled and account_system_time_scaled never have been added. In addition system time that is accounted as guest time to the user time of a process is accounted to the scaled system time instead of the scaled user time.
To fix the bugs and to prevent future forgetfulness this patch merges account_system_time_scaled into account_system_time and account_user_time_scaled into account_user_time.
Signed-off-by Martin Schwidefsky <[email protected]>

Looks fine to me.

Acked-by Paul Mackerras <[email protected]>

Martin Schwidefsky

author

2008-10-08 16:29 UTC #3 lore raw

PATCH 2/4 idle cputime accounting

In reply to Martin Schwidefsky

From: Martin Schwidefsky <[email protected]>

The cpu time spent by the idle process actually doing something is currently accounted as idle time. This is plain wrong, the architectures that support VIRT_CPU_ACCOUNTING=y can do better: distinguish between the time spent doing nothing and the time spent by idle doing work. The first is accounted with account_steal_time and the second with account_idle_time. To improve the tick based accounting as well we would need an architecture primitive that can tell us if the pt_regs of the interrupted context points to the magic instruction that halts the cpu.

In addition idle time is no more added to the stime of the idle process. This field now contains the system time of the idle process as it should be. On systems without VIRT_CPU_ACCOUNTING this will always be zero as every tick that occurs while idle is running will be accounted as idle time.

This patch contains the necessary common code changes to be able to distinguish idle system time and true idle time. The architectures with support for VIRT_CPU_ACCOUNTING need some changes to exploit this.

Signed-off-by Martin Schwidefsky <[email protected]>

---

 arch/ia64/kernel/time.c     |   10 ++++-
 arch/powerpc/kernel/time.c  |   13 +++++--
 arch/s390/kernel/vtime.c    |   20 ++++++++---
 arch/x86/xen/time.c         |   10 ++---
 include/linux/kernel_stat.h |    7 +++
 include/linux/sched.h       |    1 
 kernel/sched.c              |   80 ++++++++++++++++++++++++++++++++++----------
 kernel/time/tick-sched.c    |   11 ++----
 kernel/timer.c              |   13 -------
 9 files changed, 111 insertions(+), 54 deletions(-)

arch/ia64/kernel/time.c +8 -2

Index: linux-idle/arch/ia64/kernel/time.c
===================================================================
@@ -93,7 +93,10 @@ void ia64_account_on_switch(struct task_
 	now = ia64_get_itc();
 
 	delta_stime = cycle_to_cputime(pi->ac_stime + (now - pi->ac_stamp));
-	account_system_time(prev, 0, delta_stime, delta_stime);
+	if (idle_task(smp_processor_id()) != prev)
+		account_system_time(prev, 0, delta_stime, delta_stime);
+	else
+		account_idle_time(delta_stime);
 
 	if (pi->ac_utime) {
 		delta_utime = cycle_to_cputime(pi->ac_utime);

@@ -120,7 +123,10 @@ void account_system_vtime(struct task_st
 	now = ia64_get_itc();
 
 	delta_stime = cycle_to_cputime(ti->ac_stime + (now - ti->ac_stamp));
-	account_system_time(tsk, 0, delta_stime, delta_stime);
+	if (irq_count() || idle_task(smp_processor_id()) != tsk)
+		account_system_time(tsk, 0, delta_stime, delta_stime);
+	else
+		account_idle_time(delta_stime);
 	ti->ac_stime = 0;
 
 	ti->ac_stamp = now;

arch/powerpc/kernel/time.c +10 -3

Index: linux-idle/arch/powerpc/kernel/time.c
===================================================================
@@ -258,7 +258,10 @@ void account_system_vtime(struct task_st
 		delta += sys_time;
 		get_paca()->system_time = 0;
 	}
-	account_system_time(tsk, 0, delta, deltascaled);
+	if (in_irq() || idle_task(smp_processor_id()) != tsk)
+		account_system_time(tsk, 0, delta, deltascaled);
+	else
+		account_idle_time(delta);
 	per_cpu(cputime_last_delta, smp_processor_id()) = delta;
 	per_cpu(cputime_scaled_last_delta, smp_processor_id()) = deltascaled;
 	local_irq_restore(flags);

@@ -337,8 +340,12 @@ void calculate_steal_time(void)
 	tb = mftb();
 	purr = mfspr(SPRN_PURR);
 	stolen = (tb - pme->tb) - (purr - pme->purr);
-	if (stolen > 0)
-		account_steal_time(current, stolen);
+	if (stolen > 0) {
+		if (idle_task(smp_processor_id()) != current)
+			account_steal_time(stolen);
+		else
+			account_idle_time(stolen);
+	}
 	pme->tb = tb;
 	pme->purr = purr;
 }

arch/s390/kernel/vtime.c +16 -4

Index: linux-idle/arch/s390/kernel/vtime.c
===================================================================
@@ -56,13 +56,19 @@ void account_process_tick(struct task_st
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_system_time(tsk, HARDIRQ_OFFSET, cputime, cputime);
+	if (idle_task(smp_processor_id()) != current)
+		account_system_time(tsk, HARDIRQ_OFFSET, cputime, cputime);
+	else
+		account_idle_time(cputime);
 
 	cputime = S390_lowcore.steal_clock;
 	if ((__s64) cputime > 0) {
 		cputime >>= 12;
 		S390_lowcore.steal_clock -= cputime << 12;
-		account_steal_time(tsk, cputime);
+		if (idle_task(smp_processor_id()) != current)
+			account_steal_time(cputime);
+		else
+			account_idle_time(cputime);
 	}
 }

@@ -88,7 +94,10 @@ void account_vtime(struct task_struct *t
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_system_time(tsk, 0, cputime, cputime);
+	if (idle_task(smp_processor_id()) != current)
+		account_system_time(tsk, 0, cputime, cputime);
+	else
+		account_idle_time(cputime);
 }
 
 /*

@@ -108,7 +117,10 @@ void account_system_vtime(struct task_st
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	account_system_time(tsk, 0, cputime, cputime);
+	if (in_irq() || idle_task(smp_processor_id()) != current)
+		account_system_time(tsk, 0, cputime, cputime);
+	else
+		account_idle_time(cputime);
 }
 EXPORT_SYMBOL_GPL(account_system_vtime);

arch/x86/xen/time.c +4 -6

Index: linux-idle/arch/x86/xen/time.c
===================================================================
@@ -134,8 +134,7 @@ static void do_stolen_accounting(void)
 	*snap = state;
 
 	/* Add the appropriate number of ticks of stolen time,
-	   including any left-overs from last time.  Passing NULL to
-	   account_steal_time accounts the time as stolen. */
+	   including any left-overs from last time. */
 	stolen = runnable + offline + __get_cpu_var(residual_stolen);
 
 	if (stolen < 0)

@@ -143,11 +142,10 @@ static void do_stolen_accounting(void)
 
 	ticks = iter_div_u64_rem(stolen, NS_PER_TICK, &stolen);
 	__get_cpu_var(residual_stolen) = stolen;
-	account_steal_time(NULL, ticks);
+	account_steal_ticks(ticks);
 
 	/* Add the appropriate number of ticks of blocked time,
-	   including any left-overs from last time.  Passing idle to
-	   account_steal_time accounts the time as idle/wait. */
+	   including any left-overs from last time. */
 	blocked += __get_cpu_var(residual_blocked);
 
 	if (blocked < 0)

@@ -155,7 +153,7 @@ static void do_stolen_accounting(void)
 
 	ticks = iter_div_u64_rem(blocked, NS_PER_TICK, &blocked);
 	__get_cpu_var(residual_blocked) = blocked;
-	account_steal_time(idle_task(smp_processor_id()), ticks);
+	account_idle_ticks(ticks);
 }
 
 /*

include/linux/kernel_stat.h +6 -1

Index: linux-idle/include/linux/kernel_stat.h
===================================================================
@@ -54,6 +54,11 @@ static inline int kstat_irqs(int irq)
 
 extern void account_user_time(struct task_struct *, cputime_t, cputime_t);
 extern void account_system_time(struct task_struct *, int, cputime_t, cputime_t);
-extern void account_steal_time(struct task_struct *, cputime_t);
+extern void account_steal_time(cputime_t);
+extern void account_idle_time(cputime_t);
+
+extern void account_process_tick(struct task_struct *, int user);
+extern void account_steal_ticks(unsigned long ticks);
+extern void account_idle_ticks(unsigned long ticks);
 
 #endif /* _LINUX_KERNEL_STAT_H */

include/linux/sched.h +0 -1

Index: linux-idle/include/linux/sched.h
===================================================================
@@ -284,7 +284,6 @@ long io_schedule_timeout(long timeout);
 
 extern void cpu_init (void);
 extern void trap_init(void);
-extern void account_process_tick(struct task_struct *task, int user);
 extern void update_process_times(int user);
 extern void scheduler_tick(void);
 extern void hrtick_resched(void);

kernel/sched.c +63 ~~-17~~

107 lines not on this page · raw message

kernel/time/tick-sched.c +4 -7

Index: linux-idle/kernel/time/tick-sched.c
===================================================================
@@ -378,7 +378,6 @@ void tick_nohz_restart_sched_tick(void)
 	int cpu = smp_processor_id();
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 	unsigned long ticks;
-	cputime_t cputime;
 	ktime_t now;
 
 	local_irq_disable();

@@ -400,6 +399,7 @@ void tick_nohz_restart_sched_tick(void)
 	tick_do_update_jiffies64(now);
 	cpu_clear(cpu, nohz_cpu_mask);
 
+#ifndef CONFIG_VIRT_CPU_ACCOUNTING
 	/*
 	 * We stopped the tick in idle. Update process times would miss the
 	 * time we slept as update_process_times does only a 1 tick

@@ -409,12 +409,9 @@ void tick_nohz_restart_sched_tick(void)
 	/*
 	 * We might be one off. Do not randomly account a huge number of ticks!
 	 */
-	if (ticks && ticks < LONG_MAX) {
-		add_preempt_count(HARDIRQ_OFFSET);
-		cputime = jiffies_to_cputime(ticks);
-		account_system_time(current, HARDIRQ_OFFSET, cputime, cputime);
-		sub_preempt_count(HARDIRQ_OFFSET);
-	}
+	if (ticks && ticks < LONG_MAX)
+		account_idle_ticks(ticks);
+#endif
 
 	touch_softlockup_watchdog();
 	/*

kernel/timer.c +0 ~~-13~~

Index: linux-idle/kernel/timer.c
===================================================================
@@ -949,19 +949,6 @@ unsigned long get_next_timer_interrupt(u
 }
 #endif
 
-#ifndef CONFIG_VIRT_CPU_ACCOUNTING
-void account_process_tick(struct task_struct *p, int user_tick)
-{
-	cputime_t one_jiffy = jiffies_to_cputime(1);
-
-	if (user_tick)
-		account_user_time(p, one_jiffy, cputime_to_scaled(one_jiffy));
-	else
-		account_system_time(p, HARDIRQ_OFFSET, one_jiffy,
-				    cputime_to_scaled(one_jiffy));
-}
-#endif
-
 /*
  * Called from the timer interrupt handler to charge one tick to the current
  * process.  user_tick is 1 if the tick is user time, 0 for system.

Signature · 4 lines

blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Paul Mackerras

2008-10-16 04:59 UTC #4 lore raw

Re: [patch 2/4] idle cputime accounting

Quoted 5 lines · Martin Schwidefsky

The cpu time spent by the idle process actually doing something is currently accounted as idle time. This is plain wrong, the architectures that support VIRT_CPU_ACCOUNTING=y can do better: distinguish between the time spent doing nothing and the time spent by idle doing work. The first is accounted with account_steal_time and the second with account_idle_time.

I don't think what you said in that last sentence is correct. If the hypervisor takes cpu time away from us while we're doing nothing, that's still idle time, not stolen time. Otherwise, imagine the situation where we have one process running periodically and using say 2% of the cpu, and nothing else (except the idle tasks) running. At the moment the kernel will report 98% idle and 0% stolen, which makes sense. If we do what you say above, then the kernel will report close to 0% idle and 98% stolen time. That looks like this partition is fully loaded and the machine as a whole is quite overloaded, which is incorrect.

However, that doesn't seem to be what your patch does, at least as far as powerpc is concerned. What you seem to have done in the patch is to move the logic that says "stolen time is idle time if stolen from the idle task" from generic code into arch-specific code. Which is fine, but it isn't mentioned in your patch description.

Quoted 3 lines

This patch contains the necessary common code changes to be able to distinguish idle system time and true idle time. The architectures with support for VIRT_CPU_ACCOUNTING need some changes to exploit this.

I can't see how you can make that distinction without adding hooks into the idle task code so it can say "now we're really idle" and "now we're executing idle task stuff". I don't see that this patch does that for any architecture. Is that what your last sentence above is saying?

Finally, with this patch I get the following compilation error on powerpc:

arch/powerpc/kernel/process.c: In function '__switch_to':
arch/powerpc/kernel/process.c:400: error: implicit declaration of function 'account_process_tick' make[2]: *** [arch/powerpc/kernel/process.o] Error 1
make[1]: *** [arch/powerpc/kernel] Error 2

Overall, most of the patch looks OK, but the patch description needs to explain things more clearly.

Paul.

Martin Schwidefsky

author

2008-10-16 06:47 UTC #5 lore raw

Re: [patch 2/4] idle cputime accounting

Quoted 8 lines · Paul Mackerras

Quoted 5 lines · Martin Schwidefsky
The cpu time spent by the idle process actually doing something is currently accounted as idle time. This is plain wrong, the architectures that support VIRT_CPU_ACCOUNTING=y can do better: distinguish between the time spent doing nothing and the time spent by idle doing work. The first is accounted with account_steal_time and the second with account_idle_time.

Quoted 10 lines

I don't think what you said in that last sentence is correct. If the hypervisor takes cpu time away from us while we're doing nothing, that's still idle time, not stolen time. Otherwise, imagine the situation where we have one process running periodically and using say 2% of the cpu, and nothing else (except the idle tasks) running. At the moment the kernel will report 98% idle and 0% stolen, which makes sense. If we do what you say above, then the kernel will report close to 0% idle and 98% stolen time. That looks like this partition is fully loaded and the machine as a whole is quite overloaded, which is incorrect.

True, that is nonsense. It should be "The first is accounted with account_idle_time and the second with account_system_time." I'll fix the description, good catch. The point I'm trying to make is that it is a difference if idle is actively using the cpu vs it is truly idle. If it is using the cpu and the hypervisor steals cpu then this has to be added as steal time.

Quoted 5 lines

However, that doesn't seem to be what your patch does, at least as far as powerpc is concerned. What you seem to have done in the patch is to move the logic that says "stolen time is idle time if stolen from the idle task" from generic code into arch-specific code. Which is fine, but it isn't mentioned in your patch description.

That is what is done for powerpc until you can come up with an improved method to measure true idle time.

Quoted 9 lines

Quoted 3 lines
This patch contains the necessary common code changes to be able to distinguish idle system time and true idle time. The architectures with support for VIRT_CPU_ACCOUNTING need some changes to exploit this.
I can't see how you can make that distinction without adding hooks into the idle task code so it can say "now we're really idle" and "now we're executing idle task stuff". I don't see that this patch does that for any architecture. Is that what your last sentence above is saying?

You need to look at patch #3 and patch #4. They are s390 specific and what they do is to add the required hooks. We already have code that measures the true idle time which is the time the cpu has an enabled wait psw loaded. With the two s390 patches the code does a store-clock right before loading the enabled wait psw and another store-clock as the first instruction on the asynchronous interrupt paths in entry[64].S. The difference between these two time stamps is the true idle time. Is is possible to pull of this scheme on powerpc as well ?

Quoted 7 lines

Finally, with this patch I get the following compilation error on powerpc:
arch/powerpc/kernel/process.c: In function '__switch_to':
arch/powerpc/kernel/process.c:400: error: implicit declaration of function 'account_process_tick' make[2]: *** [arch/powerpc/kernel/process.o] Error 1
make[1]: *** [arch/powerpc/kernel] Error 2

This is with CONFIG_VIRT_CPU_ACCOUNTING=y?

Quoted 2 lines

Overall, most of the patch looks OK, but the patch description needs to explain things more clearly.

Yes, I'll fix it.

Signature · 4 lines

blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Martin Schwidefsky

author

2008-10-16 09:13 UTC #6 lore raw

Re: [patch 2/4] idle cputime accounting

Quoted 10 lines · Martin Schwidefsky

Quoted 5 lines
Finally, with this patch I get the following compilation error on powerpc:
arch/powerpc/kernel/process.c: In function '__switch_to':
arch/powerpc/kernel/process.c:400: error: implicit declaration of
function 'account_process_tick'
Quoted 2 lines
make[2]: *** [arch/powerpc/kernel/process.o] Error 1 make[1]: *** [arch/powerpc/kernel] Error 2
This is with CONFIG_VIRT_CPU_ACCOUNTING=y?

Ok, the include for kernel_stat.h has been missing in arch/powerpc/kernel/process.c. All the other call sites already have the include.

Signature · 4 lines

blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Martin Schwidefsky

author

2008-10-08 16:29 UTC #7 lore raw

PATCH 3/4 improve precision of idle accounting.

In reply to Martin Schwidefsky

From: Martin Schwidefsky <[email protected]>

Increase the precision of the idle time calculation that is exported to user space via /sys/devices/system/cpu/cpu<x>/idle_time_us

Signed-off-by Martin Schwidefsky <[email protected]>

---

 arch/s390/include/asm/cpu.h |    3 -
 arch/s390/kernel/process.c  |   67 +++++++++++++++++++++++++++++---------------
 arch/s390/kernel/smp.c      |   25 ++++++++--------
 3 files changed, 59 insertions(+), 36 deletions(-)

arch/s390/include/asm/cpu.h +1 -2

Index: linux-idle/arch/s390/include/asm/cpu.h
===================================================================
@@ -14,7 +14,6 @@
 
 struct s390_idle_data {
 	spinlock_t lock;
-	unsigned int in_idle;
 	unsigned long long idle_count;
 	unsigned long long idle_enter;
 	unsigned long long idle_time;

@@ -26,7 +25,7 @@ void s390_idle_leave(void);
 
 static inline void s390_idle_check(void)
 {
-	if ((&__get_cpu_var(s390_idle))->in_idle)
+	if ((&__get_cpu_var(s390_idle))->idle_enter != 0ULL)
 		s390_idle_leave();
 }

arch/s390/kernel/process.c +45 ~~-22~~

106 lines not on this page · raw message

arch/s390/kernel/smp.c +13 ~~-12~~

Index: linux-idle/arch/s390/kernel/smp.c
===================================================================
@@ -992,9 +992,11 @@ static ssize_t show_idle_count(struct sy
 	unsigned long long idle_count;
 
 	idle = &per_cpu(s390_idle, dev->id);
-	spin_lock_irq(&idle->lock);
+	spin_lock(&idle->lock);
 	idle_count = idle->idle_count;
-	spin_unlock_irq(&idle->lock);
+	if (idle->idle_enter)
+		idle_count++;
+	spin_unlock(&idle->lock);
 	return sprintf(buf, "%llu\n", idle_count);
 }
 static SYSDEV_ATTR(idle_count, 0444, show_idle_count, NULL);

@@ -1003,18 +1005,17 @@ static ssize_t show_idle_time(struct sys
 				struct sysdev_attribute *attr, char *buf)
 {
 	struct s390_idle_data *idle;
-	unsigned long long new_time;
+	unsigned long long now, idle_time, idle_enter;
 
 	idle = &per_cpu(s390_idle, dev->id);
-	spin_lock_irq(&idle->lock);
-	if (idle->in_idle) {
-		new_time = get_clock();
-		idle->idle_time += new_time - idle->idle_enter;
-		idle->idle_enter = new_time;
-	}
-	new_time = idle->idle_time;
-	spin_unlock_irq(&idle->lock);
-	return sprintf(buf, "%llu\n", new_time >> 12);
+	spin_lock(&idle->lock);
+	now = get_clock();
+	idle_time = idle->idle_time;
+	idle_enter = idle->idle_enter;
+	if (idle_enter != 0ULL && idle_enter < now)
+		idle_time += now - idle_enter;
+	spin_unlock(&idle->lock);
+	return sprintf(buf, "%llu\n", idle_time >> 12);
 }
 static SYSDEV_ATTR(idle_time_us, 0444, show_idle_time, NULL);

Signature · 4 lines

blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Martin Schwidefsky

author

2008-10-08 16:29 UTC #8 lore raw

PATCH 4/4 improve idle cputime accounting

In reply to Martin Schwidefsky

From: Martin Schwidefsky <[email protected]>

Distinguish the cputime of the idle process where idle is actually using cpu cycles from the cputime where idle is sleeping on an enabled wait psw. The former is acconuted as system time, the later as idle time.

Signed-off-by Martin Schwidefsky <[email protected]>

---

 arch/s390/include/asm/cpu.h |    1 +
 arch/s390/kernel/process.c  |    7 +++++++
 arch/s390/kernel/s390_ext.c |    2 +-
 arch/s390/kernel/vtime.c    |   24 ++++++------------------
 drivers/s390/cio/cio.c      |    2 +-
 5 files changed, 16 insertions(+), 20 deletions(-)

arch/s390/include/asm/cpu.h +1 -0

Index: linux-idle/arch/s390/include/asm/cpu.h
===================================================================
@@ -15,6 +15,7 @@
 struct s390_idle_data {
 	spinlock_t lock;
 	unsigned long long idle_count;
+	unsigned long long idle_delta;
 	unsigned long long idle_enter;
 	unsigned long long idle_time;
 };

arch/s390/kernel/process.c +7 -0

Index: linux-idle/arch/s390/kernel/process.c
===================================================================
@@ -87,6 +87,13 @@ void s390_idle_leave(void)
 
 	idle = &__get_cpu_var(s390_idle);
 	idle_time = S390_lowcore.int_clock - idle->idle_enter;
+#ifdef CONFIG_VIRT_CPU_ACCOUNTING
+	idle_time += idle->idle_delta;
+	idle->idle_delta = idle_time & 4095;
+	idle_time -= idle->idle_delta;
+	account_idle_time(idle_time >> 12);
+	S390_lowcore.last_update_clock = S390_lowcore.int_clock;
+#endif
 	spin_lock(&idle->lock);
 	idle->idle_time += idle_time;
 	idle->idle_enter = 0ULL;

arch/s390/kernel/s390_ext.c +1 -1

Index: linux-idle/arch/s390/kernel/s390_ext.c
===================================================================
@@ -119,8 +119,8 @@ void do_extint(struct pt_regs *regs, uns
 	struct pt_regs *old_regs;
 
 	old_regs = set_irq_regs(regs);
-	irq_enter();
 	s390_idle_check();
+	irq_enter();
 	if (S390_lowcore.int_clock >= S390_lowcore.clock_comparator)
 		/* Serve timer interrupts first. */
 		clock_comparator_work();

arch/s390/kernel/vtime.c +6 ~~-18~~

Index: linux-idle/arch/s390/kernel/vtime.c
===================================================================
@@ -56,19 +56,13 @@ void account_process_tick(struct task_st
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	if (idle_task(smp_processor_id()) != current)
-		account_system_time(tsk, HARDIRQ_OFFSET, cputime, cputime);
-	else
-		account_idle_time(cputime);
+	account_system_time(tsk, HARDIRQ_OFFSET, cputime, cputime);
 
 	cputime = S390_lowcore.steal_clock;
 	if ((__s64) cputime > 0) {
 		cputime >>= 12;
 		S390_lowcore.steal_clock -= cputime << 12;
-		if (idle_task(smp_processor_id()) != current)
-			account_steal_time(cputime);
-		else
-			account_idle_time(cputime);
+		account_steal_time(cputime);
 	}
 }

@@ -94,10 +88,7 @@ void account_vtime(struct task_struct *t
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	if (idle_task(smp_processor_id()) != current)
-		account_system_time(tsk, 0, cputime, cputime);
-	else
-		account_idle_time(cputime);
+	account_system_time(tsk, 0, cputime, cputime);
 }
 
 /*

@@ -117,10 +108,7 @@ void account_system_vtime(struct task_st
 	cputime =  S390_lowcore.system_timer >> 12;
 	S390_lowcore.system_timer -= cputime << 12;
 	S390_lowcore.steal_clock -= cputime << 12;
-	if (in_irq() || idle_task(smp_processor_id()) != current)
-		account_system_time(tsk, 0, cputime, cputime);
-	else
-		account_idle_time(cputime);
+	account_system_time(tsk, 0, cputime, cputime);
 }
 EXPORT_SYMBOL_GPL(account_system_vtime);

@@ -158,8 +146,8 @@ void vtime_start_cpu_timer(void)
 	if (vt_list->idle & 1LL<<63)
 		return;
 
-	if (!list_empty(&vt_list->list))
-		set_vtimer(vt_list->idle);
+	S390_lowcore.last_update_timer = S390_lowcore.async_enter_timer;
+	set_vtimer(vt_list->idle);
 }
 
 void vtime_stop_cpu_timer(void)

drivers/s390/cio/cio.c +1 -1

Index: linux-idle/drivers/s390/cio/cio.c
===================================================================
@@ -626,8 +626,8 @@ do_IRQ (struct pt_regs *regs)
 	struct pt_regs *old_regs;
 
 	old_regs = set_irq_regs(regs);
-	irq_enter();
 	s390_idle_check();
+	irq_enter();
 	if (S390_lowcore.int_clock >= S390_lowcore.clock_comparator)
 		/* Serve timer interrupts first. */
 		clock_comparator_work();

Signature · 4 lines

blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Luck, Tony

2008-10-08 21:23 UTC #9 lore raw

In reply to Martin Schwidefsky

Quoted 3 lines

There is one change in patch #2 that might require a change on powerpc and/or ia64. The generic TICK_ONESHOT/NO_HZ code calculates the number of ticks spent with a disabled HZ timer and accounts this as idle time.

Your patches apply, build and boot cleanly on ia64.

At the moment ia64 doesn't do NO_HZ (exisiting cpus only support a C1 state, which we can enter/leave pretty quickly ... so power saving for NO_HZ is negligible) ... so your code change isn't an issue for ia64.

-Tony

Martin Schwidefsky

author

2008-10-09 08:11 UTC #10 lore raw

Quoted 10 lines · Tony

Quoted 3 lines
There is one change in patch #2 that might require a change on powerpc and/or ia64. The generic TICK_ONESHOT/NO_HZ code calculates the number of ticks spent with a disabled HZ timer and accounts this as idle time.
Your patches apply, build and boot cleanly on ia64.
At the moment ia64 doesn't do NO_HZ (exisiting cpus only support a C1 state, which we can enter/leave pretty quickly ... so power saving for NO_HZ is negligible) ... so your code change isn't an issue for ia64.

Thanks for checking.

Signature · 4 lines

blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Martin Schwidefsky

author

2008-10-15 14:04 UTC #11 lore raw

In reply to Martin Schwidefsky

Quoted 31 lines · Martin Schwidefsky

Greetings,
while working on the analysis of a mismatch between the cputime accounting numbers of z/VM as the host and Linux as the guest I started to wonder about the accounting of idle time. z/VM showed more cpu time for the guest as the guest itself. With the current code everything that the idle process does is accounted as idle time. If idle is sleeping that is fine, but if idle is actually using cpu cycles this is wrong.
The question is how wrong? To find out I've implemented really precise accounting of true idle vs. system idle cputime for s390. A really simple test that wakes up 100 times per second to do some minimal work before going back to sleep showed 0.35% of system idle time. If you are dealing with lots of virtual penguins this quickly becomes significant.
There are four patches in this series:
Patch #1: Cleanup scaled / unscaled cputime accounting
Patch #2: Change the accounting interface to allow the architectures to do
precise idle time accounting
Patch #3: s390 patch to improve the precision of the idle_time_us value Patch #4: s390 patch to implement improved idle time accounting
There is one change in patch #2 that might require a change on powerpc and/or ia64. The generic TICK_ONESHOT/NO_HZ code calculates the number of ticks spent with a disabled HZ timer and accounts this as idle time. For a configuration for VIRT_CPU_ACCOUNTING=y this is horribly wrong. Either you have precise accounting or you don't. Patch #2 just removes the calculation for VIRT_CPU_ACCOUNTING=y. The architectures which support precise accounting have to deal with it on their own. This is where the powerpc and ia64 maintainer come into play. Would you look at patch #2 please ?

Quoted 4 lines

To make it clearer what happens in tick_nohz_restart_sched_tick I've added a new function account_idle_ticks(). And for good measure another one named account_steal_ticks() for xen where "interesting" things have been done with the account_steal_time interface.

Any news about powerpc? Do these patches break anything or does it work?

Signature · 4 lines

blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Benjamin Herrenschmidt

2008-10-15 20:57 UTC #12 lore raw

Quoted 10 lines · Martin Schwidefsky

Quoted 10 lines
There is one change in patch #2 that might require a change on powerpc and/or ia64. The generic TICK_ONESHOT/NO_HZ code calculates the number of ticks spent with a disabled HZ timer and accounts this as idle time. For a configuration for VIRT_CPU_ACCOUNTING=y this is horribly wrong. Either you have precise accounting or you don't. Patch #2 just removes the calculation for VIRT_CPU_ACCOUNTING=y. The architectures which support precise accounting have to deal with it on their own. This is where the powerpc and ia64 maintainer come into play. Would you look at patch #2 please ?

Quoted 6 lines

Quoted 4 lines
To make it clearer what happens in tick_nohz_restart_sched_tick I've added a new function account_idle_ticks(). And for good measure another one named account_steal_ticks() for xen where "interesting" things have been done with the account_steal_time interface.
Any news about powerpc? Do these patches break anything or does it work?

I didn't have a chance to look at it yet. I'll try to get that looked at today.

Cheers,
Ben.