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

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

[Hidetoshi Seto]

> [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 <seto.hidetoshi@jp.fujitsu.com>

---
 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__ */

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

[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:

> +         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

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

[Hidetoshi Seto]

Thank you for your nice words.

Tony Luck wrote:
> 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:
> 
>> +         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.

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

 >> -- 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)

> 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

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

[Luck, Tony]

> 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

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

[Kenji Kaneshige]

> 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&m\x119303432628668&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

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

[Simon Horman]

On Tue, Oct 16, 2007 at 10:33:59PM +0900, Hidetoshi Seto wrote:
> > [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.

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'

-- 
Horms

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

[Hidetoshi Seto]

Simon Horman wrote:
> 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.

> commit fa13a5a1f25f671d084d8884be96fc48d9b68275
> Author: Paul Mackerras <paulus@samba.org>
> 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().
> 
>     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 <paulus@samba.org>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>

Thanks,
H.Seto