Fix hardware IRQ time accounting problem.
PATCH v2 1/1 Fix hardware IRQ time accounting problem.
The commit fa13a5a1f25f671d084d8884be96fc48d9b68275 (sched: restore deterministic CPU accounting on powerpc), unconditionally calls update_process_tick() in system context. In the deterministic accounting case this is the correct thing to do. However, in the non-deterministic accounting case we need to not do this, and results in the time accounted as hardware irq time being artificially elevated.
Also this patch collapses 2 consecutive '#ifdef CONFIG_VIRT_CPU_ACCOUNTING' checks in time.h into one for neatness.
---
The problem was seen and reported by Johannes Berg and Frederik Himpe. Paul, I think this is good for 2.6.24.
Changes since v1:
- I noticed that the #define was explictly using "current" rather than the task passed in. Using tsk is the right thing to do.
- The whiteapce changes dirty-up the patch and are un-needed with the change above.
arch/powerpc/kernel/process.c | 2 +- include/asm-powerpc/time.h | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-)
arch/powerpc/kernel/process.c +1 -1
@@ -350,7 +350,7 @@ struct task_struct *__switch_to(struct task_struct *prev,local_irq_save (flags);account_system_vtime (current); -account_process_tick (current,0 ); +account_process_vtime (current);calculate_steal_time (); last =_switch (old_thread, new_thread);
include/asm-powerpc/time.h +2 -6
@@ -237,18 +237,14 @@ struct cpu_usage { DECLARE_PER_CPU(struct cpu_usage, cpu_usage_array); -#ifdef CONFIG_VIRT_CPU_ACCOUNTING -extern void account_process_vtime (struct task_struct *tsk); -#else -#define account_process_vtime (tsk) do { } while (0) -#endif -#if defined(CONFIG_VIRT_CPU_ACCOUNTING )extern void calculate_steal_time (void );extern void snapshot_timebases (void ); +#define account_process_vtime (tsk) account_process_tick(tsk, 0);#else #define calculate_steal_time () do { } while (0)#define snapshot_timebases () do { } while (0) +#define account_process_vtime (tsk) do { } while (0)#endif extern void secondary_cpu_time_init (void );
Signature · 1 line
1.5.3.6
Quoted 6 lines · Tony Breeds
The commit
fa13a5a1f25f671d084d8884be96fc48d9b68275(sched: restore deterministic CPU accounting on powerpc), unconditionally callsupdate_process_tick()in system context. In the deterministic accounting case this is the correct thing to do. However, in the non-deterministic accounting case we need to not do this, and results in the time accounted as hardware irq time being artificially elevated.
Cool, I wouldn't have stood a chance of tracking this down :) Thanks, I'll apply it to my testing tree later today.
Quoted 53 lines
Signed-off-by Tony Breeds <[email protected]>---
The problem was seen and reported by Johannes Berg and Frederik Himpe. Paul, I think this is good for 2.6.24.Changes since v1:
- I noticed that the #define was explictly using "current" rather than the task passed in. Using tsk is the right thing to do.
- The whiteapce changes dirty-up the patch and are un-needed with the change above.arch/powerpc/kernel/process.c | 2 +- include/asm-powerpc/time.h | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-)arch/powerpc/kernel/process.c +1
-1@@ -350,7 +350,7 @@ struct task_struct *__switch_to(struct task_struct *prev,local_irq_save (flags);account_system_vtime (current); -account_process_tick (current,0 ); +account_process_vtime (current);calculate_steal_time (); last =_switch (old_thread, new_thread);include/asm-powerpc/time.h +2
-6@@ -237,18 +237,14 @@ struct cpu_usage { DECLARE_PER_CPU(struct cpu_usage, cpu_usage_array); -#ifdef CONFIG_VIRT_CPU_ACCOUNTING -extern void account_process_vtime (struct task_struct *tsk); -#else -#define account_process_vtime (tsk) do { } while (0) -#endif -#if defined(CONFIG_VIRT_CPU_ACCOUNTING )extern void calculate_steal_time (void );extern void snapshot_timebases (void ); +#define account_process_vtime (tsk) account_process_tick(tsk, 0);#else #define calculate_steal_time () do { } while (0)#define snapshot_timebases () do { } while (0) +#define account_process_vtime (tsk) do { } while (0)#endif extern void secondary_cpu_time_init (void );
Hallo Tony,
Quoted 14 lines · Tony Breeds
The commit
fa13a5a1f25f671d084d8884be96fc48d9b68275(sched: restore deterministic CPU accounting on powerpc), unconditionally callsupdate_process_tick()in system context. In the deterministic accounting case this is the correct thing to do. However, in the non-deterministic accounting case we need to not do this, and results in the time accounted as hardware irq time being artificially elevated.Signed-off-by Tony Breeds <[email protected]>---
arch/powerpc/kernel/process.c +0
-0
arch/powerpc/kernel/process.c struct task_struct *__switch_to(struct task_struct *prev, · line 354
Quoted 6 lines
arch/powerpc/kernel/process.c +1
-1@@ -350,7 +350,7 @@ struct task_struct *__switch_to(struct task_struct *prev,local_irq_save (flags);account_system_vtime (current); -account_process_tick (current,0 ); +account_process_vtime (current);calculate_steal_time ();
This patch works for me. The high hardware interrupt load is gone. Thanks.
Bye, Jörg.
Signature · 1 line
Der Klügere gibt so lange nach bis er der Dumme ist.
Quoted 9 lines · Tony Breeds
The commit
fa13a5a1f25f671d084d8884be96fc48d9b68275(sched: restore deterministic CPU accounting on powerpc), unconditionally callsupdate_process_tick()in system context. In the deterministic accounting case this is the correct thing to do. However, in the non-deterministic accounting case we need to not do this, and results in the time accounted as hardware irq time being artificially elevated.Also this patch collapses 2 consecutive '#ifdef
CONFIG_VIRT_CPU_ACCOUNTING' checks intime.hinto one for neatness.
I can confirm this patch fixed the problem for me. Thank you!
Signature · 1 line
Frederik Himpe