Skip to contentSkip to conversation

ia64: VIRT_CPU_ACCOUNTING (accurate cpu time accounting)

PATCH In review Hidetoshi Seto 4 participants 7 messages Oct 16, 2007 – Nov 19, 2007

lore
Share & tools
lore

Hidetoshi Seto

author
#0 lore raw

PATCH 1/9 ia64: VIRT_CPU_ACCOUNTING (accurate cpu time accounting)

Quoted 1 line

[1/9] ia64_add_config_virt_cpu_accounting.patch

The VIRT_CPU_ACCOUNTING option is already implemented s390 and powerpc archs.

By enabling this option, the arch can hook a function named account_system_vtime() to irq_enter(), irq_exit(), and head and tail of do_softirq().

This patch just add the Kconfig option to ia64, and enable to hook ia64 specific account_system_vtime() function, currently nop as generic one.

Thanks,
H.Seto

Signed-off-by Hidetoshi Seto <[email protected]>
---
 arch/ia64/Kconfig         |   11 +++++++++++
 arch/ia64/kernel/time.c   |   13 +++++++++++++
 include/asm-ia64/system.h |    4 ++++
 3 files changed, 28 insertions(+)

Index: linux-2.6.23/arch/ia64/Kconfig =================================--- linux-2.6.23.orig/arch/ia64/Kconfig +++ linux-2.6.23/arch/ia64/Kconfig
@@ -257,6 +257,17 @@
default "17" if HUGETLB_PAGE
default "11"

+config VIRT_CPU_ACCOUNTING
+ bool "Deterministic task and CPU time accounting"
+ default y
+ help

+	  Select this option to enable more accurate task and CPU time
+	  accounting.  This is done by reading a CPU counter on each
+	  kernel entry and exit and on transitions within the kernel
+	  between system, softirq and hardirq state, so there is a
+	  small performance impact.
+	  If in doubt, say Y here.

+
config SMP
bool "Symmetric multi-processing support"
help
Index: linux-2.6.23/arch/ia64/kernel/time.c =================================--- linux-2.6.23.orig/arch/ia64/kernel/time.c +++ linux-2.6.23/arch/ia64/kernel/time.c
@@ -59,6 +59,19 @@
};
static struct clocksource *itc_clocksource;

+#ifdef CONFIG_VIRT_CPU_ACCOUNTING
+
+/*
+ * Account time for a transition between system, hard irq
+ * or soft irq state.
+ */
+void account_system_vtime(struct task_struct *tsk)
+{
+
+}
+
+#endif /* CONFIG_VIRT_CPU_ACCOUNTING */
+
static irqreturn_t
timer_interrupt (int irq, void *dev_id)
{
Index: linux-2.6.23/include/asm-ia64/system.h =================================--- linux-2.6.23.orig/include/asm-ia64/system.h +++ linux-2.6.23/include/asm-ia64/system.h
@@ -264,6 +264,10 @@

void default_idle(void);

+#ifdef CONFIG_VIRT_CPU_ACCOUNTING
+extern void account_system_vtime(struct task_struct *); +#endif
+
#endif /* __KERNEL__ */

#endif /* __ASSEMBLY__ */

#1 lore raw

Overall impression: nice! There have always been some pathalogical workloads that have confused the scheduler by executing synchronously with the timer tick ... this should end their tricks.

A couple of comments:

Review

Quoted 5 lines
(quoted diff) +5 -0
+         Select this option to enable more accurate task and CPU time
+         accounting.  This is done by reading a CPU counter on each
+         kernel entry and exit and on transitions within the kernel
+         between system, softirq and hardirq state, so there is a
+         small performance impact.

I haven't tried any macro-level benchmarks, but on a micro-benchmark to measure system call overhead I see an additional 19-20 cycles in the cached case. This doesn't look like a "small" performance impact (the execution time for a syscall in my benchmark is only ~61 cycles). But I'd like to see some real benchmarks to see whether the micro-level problem is measureable at the macro scale.

One warning generated by this patch:
kernel/posix-cpu-timers.c:41: warning: passing arg 1 of `timespec_to_cputime' discards qualifiers from pointer target type

-Tony

author
#2 lore raw
In reply to Hidetoshi Seto

Thank you for your nice words.

Quoted 12 lines · Tony Luck

Overall impression: nice! There have always been some pathalogical workloads that have confused the scheduler by executing synchronously with the timer tick ... this should end their tricks.

A couple of comments:

Review

Quoted 5 lines
(quoted diff) +5 -0
+         Select this option to enable more accurate task and CPU time
+         accounting.  This is done by reading a CPU counter on each
+         kernel entry and exit and on transitions within the kernel
+         between system, softirq and hardirq state, so there is a
+         small performance impact.
Quoted 6 lines

I haven't tried any macro-level benchmarks, but on a micro-benchmark to measure system call overhead I see an additional 19-20 cycles in the cached case. This doesn't look like a "small" performance impact (the execution time for a syscall in my benchmark is only ~61 cycles). But I'd like to see some real benchmarks to see whether the micro-level problem is measureable at the macro scale.

Now I'm preparing benchmarks...
I suppose there is still room for improvement(i.e. Peter's hint).

Quoted 3 lines
Quoted 3 lines

-- Adding ACCOUNT_SYS_ENTER adds around 40 cycles to the system
call path. If you wanted, you could move reading ITC earlier (it takes up to 36 cycles), and overlap with other work.

The impact of this option would not be 0, however it can be said that this feature is quite useful enough to be default option of some powerpc and s390 where that already have its implementation.
(ex.
arch/powerpc/configs/ps3_defconfig:18:CONFIG_VIRT_CPU_ACCOUNTING=y)

Quoted 3 lines

One warning generated by this patch:
kernel/posix-cpu-timers.c:41: warning: passing arg 1 of `timespec_to_cputime' discards qualifiers from pointer target type

There was a dropped "const", now fixed.

Thanks,
H.Seto

#3 lore raw
In reply to Hidetoshi Seto
Quoted 2 lines

Now I'm preparing benchmarks...
I suppose there is still room for improvement(i.e. Peter's hint).

We definitely need to reduce the overhead of this patch. David Mosberger, Ken chen and others battled for every cycle to make ia64 system calls fast ... so I'm not willing to add back 20 cycles in a single patch.

I did run the kernel some more today and noticed some odd "hangs". Some processes seemed to get stuck for many seconds and then continued again. Didn't have time to try and track what was happening. Have you seem snything like this?

-Tony

#4 lore raw
In reply to Hidetoshi Seto
Quoted 5 lines

I did run the kernel some more today and noticed some odd "hangs". Some processes seemed to get stuck for many seconds and then continued again. Didn't have time to try and track what was happening. Have you seem snything like this?

I've encountered the similar phenomenon several times. I have not investigated it very much, but I guess it is related to the problem reported at

http://marc.info/?l=linux-kernel&m9303432628668&w=2

The 'wa' field of 'top' command showed that one of cpus is under 100% iowait when I encountered the problem.

Thanks,
Kenji Kaneshige

#5 lore raw
In reply to Hidetoshi Seto
Quoted 5 lines · Hidetoshi Seto
Quoted 1 line

[1/9] ia64_add_config_virt_cpu_accounting.patch

The VIRT_CPU_ACCOUNTING option is already implemented s390 and powerpc archs.

Quoted 7 lines

By enabling this option, the arch can hook a function named account_system_vtime() to irq_enter(), irq_exit(), and head and tail of do_softirq().

This patch just add the Kconfig option to ia64, and enable to hook ia64 specific account_system_vtime() function, currently nop as generic one.

Hi,

When I apply this patch and enable VIRT_CPU_ACCOUNTING I get the following build error. Perhaps the fragment that creates account_process_tick in arch/ia64/kernel/time.c is missing?

kernel/built-in.o: In function update_process_times': undefined reference to account_process_tick'

Signature · 1 line
Horms
author
#6 lore raw
In reply to Hidetoshi Seto
Quoted 6 lines · Simon Horman

When I apply this patch and enable VIRT_CPU_ACCOUNTING I get the following build error. Perhaps the fragment that creates account_process_tick in arch/ia64/kernel/time.c is missing?

kernel/built-in.o: In function update_process_times': undefined reference to account_process_tick'

I found that this error is caused by change of update_process_times, i.e. the following commit.
Since my patches are based on 2.6.23, they should be rebased on 2.6.24-rcX and have some fixes.

Quoted 24 lines

commit fa13a5a1f25f671d084d8884be96fc48d9b68275

Author: Paul Mackerras <[email protected]>
Date:   Fri Nov 9 22:39:38 2007 +0100

sched: restore deterministic CPU accounting on powerpc

Since powerpc started using CONFIG_GENERIC_CLOCKEVENTS, the deterministic CPU accounting (CONFIG_VIRT_CPU_ACCOUNTING) has been broken on powerpc, because we end up counting user time twice: once in timer_interrupt() and once in update_process_times().

This fixes the problem by pulling the code in update_process_times that updates utime and stime into a separate function called account_process_tick. If CONFIG_VIRT_CPU_ACCOUNTING is not defined, there is a version of account_process_tick in kernel/timer.c that simply accounts a whole tick to either utime or stime as before. If CONFIG_VIRT_CPU_ACCOUNTING is defined, then arch code gets to implement account_process_tick.

This also lets us simplify the s390 code a bit; it means that the s390 timer interrupt can now call update_process_times even when CONFIG_VIRT_CPU_ACCOUNTING is turned on, and can just implement a suitable account_process_tick().

Quoted 5 lines

account_process_tick() now takes the task_struct * as an argument. Tested both with and without CONFIG_VIRT_CPU_ACCOUNTING.

Signed-off-by Paul Mackerras <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>

Thanks,
H.Seto