diff mbox

sched: generalize CONFIG_IRQ_TIME_ACCOUNTING for X86 and ARM

Message ID 1329717859-19378-1-git-send-email-dmitry.antipov@linaro.org
State New
Headers show

Commit Message

Dmitry Antipov Feb. 20, 2012, 6:04 a.m. UTC 
Generalize CONFIG_IRQ_TIME_ACCOUNTING  between X86 and
ARM, move "noirqtime=" option to common debugging code.
For a bit of backward compatibility, X86-specific option
"tsc=noirqtime" is preserved, but issues a warning.

Suggested-by: Yong Zhang <yong.zhang0@gmail.com>
Suggested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Suggested-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Venkatesh Pallipadi <venki@google.com>
Signed-off-by: Dmitry Antipov <dmitry.antipov@linaro.org>
---
 Documentation/kernel-parameters.txt |    9 +++++----
 arch/arm/kernel/sched_clock.c       |    2 ++
 arch/x86/Kconfig                    |   11 -----------
 arch/x86/kernel/tsc.c               |   12 ++++++------
 include/linux/sched.h               |   17 ++++++++++-------
 kernel/sched/core.c                 |   21 ++++++++++++---------
 lib/Kconfig.debug                   |   12 ++++++++++++
 7 files changed, 47 insertions(+), 37 deletions(-)

Comments

Peter Zijlstra Feb. 27, 2012, 10:12 a.m. UTC | #1 
On Mon, 2012-02-20 at 10:04 +0400, Dmitry Antipov wrote:
> Generalize CONFIG_IRQ_TIME_ACCOUNTING  between X86 and
> ARM, move "noirqtime=" option to common debugging code.
> For a bit of backward compatibility, X86-specific option
> "tsc=noirqtime" is preserved, but issues a warning.
> 
> Suggested-by: Yong Zhang <yong.zhang0@gmail.com>
> Suggested-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Suggested-by: Ingo Molnar <mingo@elte.hu>
> Acked-by: Venkatesh Pallipadi <venki@google.com>
> Signed-off-by: Dmitry Antipov <dmitry.antipov@linaro.org>

Thanks for Cc'ing me (maintainer) and Venki (original author of the
stuff) :-)


> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 7d379a6..9b13f79 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1961,13 +1961,16 @@ extern void sched_clock_idle_wakeup_event(u64 delta_ns);
>  #endif
>  
>  #ifdef CONFIG_IRQ_TIME_ACCOUNTING
> -/*
> - * An i/f to runtime opt-in for irq time accounting based off of sched_clock.
> - * The reason for this explicit opt-in is not to have perf penalty with
> - * slow sched_clocks.
> - */
> -extern void enable_sched_clock_irqtime(void);
> -extern void disable_sched_clock_irqtime(void);
> +extern int sched_clock_irqtime;
> +static inline void enable_sched_clock_irqtime(void)
> +{
> +	if (sched_clock_irqtime == -1)
> +		sched_clock_irqtime = 1;
> +}
> +static inline void disable_sched_clock_irqtime(void)
> +{
> +	sched_clock_irqtime = 0;
> +}
>  #else
>  static inline void enable_sched_clock_irqtime(void) {}
>  static inline void disable_sched_clock_irqtime(void) {}

Please keep them out-of-line, its not a fast path and it avoids having
to expose the state variable.

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 5255c9d..4e7a197 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -757,18 +757,21 @@ static DEFINE_PER_CPU(u64, cpu_hardirq_time);
>  static DEFINE_PER_CPU(u64, cpu_softirq_time);
>  
>  static DEFINE_PER_CPU(u64, irq_start_time);
> -static int sched_clock_irqtime;
>  
> -void enable_sched_clock_irqtime(void)
> -{
> -	sched_clock_irqtime = 1;
> -}
> +/*
> + * -1 if not initialized, 0 if disabled with "noirqtime" kernel option
> + * or after unstable clock was detected, 1 if enabled and active.
> + */

You forgot to explain what you need the tri-state for.

> +__read_mostly int sched_clock_irqtime = -1;
>  
> -void disable_sched_clock_irqtime(void)
> +static int __init irqtime_setup(char *str)
>  {
>  	sched_clock_irqtime = 0;
> +	return 1;
>  }
>  
> +__setup("noirqtime", irqtime_setup);
> +
>  #ifndef CONFIG_64BIT
>  static DEFINE_PER_CPU(seqcount_t, irq_time_seq);

Other than that no real objections.. I guess.
Dmitry Antipov Feb. 28, 2012, 6:19 a.m. UTC | #2 
On 02/27/2012 02:12 PM, Peter Zijlstra wrote:

>> -extern void enable_sched_clock_irqtime(void);
>> -extern void disable_sched_clock_irqtime(void);
>> +extern int sched_clock_irqtime;
>> +static inline void enable_sched_clock_irqtime(void)
>> +{
>> +	if (sched_clock_irqtime == -1)
>> +		sched_clock_irqtime = 1;
>> +}
>> +static inline void disable_sched_clock_irqtime(void)
>> +{
>> +	sched_clock_irqtime = 0;
>> +}
>>   #else
>>   static inline void enable_sched_clock_irqtime(void) {}
>>   static inline void disable_sched_clock_irqtime(void) {}
>
> Please keep them out-of-line, its not a fast path and it avoids having
> to expose the state variable.

OK

>> +/*
>> + * -1 if not initialized, 0 if disabled with "noirqtime" kernel option
>> + * or after unstable clock was detected, 1 if enabled and active.
>> + */
>
> You forgot to explain what you need the tri-state for.
>
>> +__read_mostly int sched_clock_irqtime = -1;

The comment above should be a sufficient explanation, isn't it?

It's a tri-state just because it "merges" two variables: internal state (enabled/disabled)
and the value passed by "noirqtime" option (turn it on, default/turn it off). It can be
enabled only if it was not turned off explicitly, i.e. -1 => 1 transition is possible,
but 0 -> 1 is not. The same rule applies to a situation when an unstable clock is detected.

Dmitry
diff mbox

Patch

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 033d4e6..a5da255 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1790,6 +1790,9 @@  bytes respectively. Such letter suffixes can also be entirely omitted.
 	noirqdebug	[X86-32] Disables the code which attempts to detect and
 			disable unhandled interrupt sources.
 
+	noirqtime	[X86,ARM] Run time disables IRQ_TIME_ACCOUNTING and
+			eliminates the timestamping on irq/softirq entry/exit.
+
 	no_timer_check	[X86,APIC] Disables the code which tests for
 			broken timer IRQ sources.
 
@@ -2636,10 +2639,8 @@  bytes respectively. Such letter suffixes can also be entirely omitted.
 			as the stability checks done at bootup.	Used to enable
 			high-resolution timer mode on older hardware, and in
 			virtualized environment.
-			[x86] noirqtime: Do not use TSC to do irq accounting.
-			Used to run time disable IRQ_TIME_ACCOUNTING on any
-			platforms where RDTSC is slow and this accounting
-			can add overhead.
+			[x86] noirqtime: obsoleted by "noirqtime" generic option,
+			see it's documentation for details.
 
 	turbografx.map[2|3]=	[HW,JOY]
 			TurboGraFX parallel port interface
diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index 5416c7c..30b5f89 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -144,6 +144,8 @@  void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
 	 */
 	cd.epoch_ns = 0;
 
+	enable_sched_clock_irqtime();
+
 	pr_debug("Registered %pF as sched_clock source\n", read);
 }
 
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5bed94e..4759676 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -805,17 +805,6 @@  config SCHED_MC
 	  making when dealing with multi-core CPU chips at a cost of slightly
 	  increased overhead in some places. If unsure say N here.
 
-config IRQ_TIME_ACCOUNTING
-	bool "Fine granularity task level IRQ time accounting"
-	default n
-	---help---
-	  Select this option to enable fine granularity task irq time
-	  accounting. This is done by reading a timestamp on each
-	  transitions between softirq and hardirq state, so there can be a
-	  small performance impact.
-
-	  If in doubt, say N here.
-
 source "kernel/Kconfig.preempt"
 
 config X86_UP_APIC
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index a62c201..f1b2b63 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -103,14 +103,15 @@  int __init notsc_setup(char *str)
 
 __setup("notsc", notsc_setup);
 
-static int no_sched_irq_time;
-
 static int __init tsc_setup(char *str)
 {
 	if (!strcmp(str, "reliable"))
 		tsc_clocksource_reliable = 1;
-	if (!strncmp(str, "noirqtime", 9))
-		no_sched_irq_time = 1;
+	if (!strncmp(str, "noirqtime", 9)) {
+		printk(KERN_WARNING "tsc: tsc=noirqtime is "
+		       "obsolete, use noirqtime instead\n");
+		disable_sched_clock_irqtime();
+	}
 	return 1;
 }
 
@@ -978,8 +979,7 @@  void __init tsc_init(void)
 	/* now allow native_sched_clock() to use rdtsc */
 	tsc_disabled = 0;
 
-	if (!no_sched_irq_time)
-		enable_sched_clock_irqtime();
+	enable_sched_clock_irqtime();
 
 	lpj = ((u64)tsc_khz * 1000);
 	do_div(lpj, HZ);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7d379a6..9b13f79 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1961,13 +1961,16 @@  extern void sched_clock_idle_wakeup_event(u64 delta_ns);
 #endif
 
 #ifdef CONFIG_IRQ_TIME_ACCOUNTING
-/*
- * An i/f to runtime opt-in for irq time accounting based off of sched_clock.
- * The reason for this explicit opt-in is not to have perf penalty with
- * slow sched_clocks.
- */
-extern void enable_sched_clock_irqtime(void);
-extern void disable_sched_clock_irqtime(void);
+extern int sched_clock_irqtime;
+static inline void enable_sched_clock_irqtime(void)
+{
+	if (sched_clock_irqtime == -1)
+		sched_clock_irqtime = 1;
+}
+static inline void disable_sched_clock_irqtime(void)
+{
+	sched_clock_irqtime = 0;
+}
 #else
 static inline void enable_sched_clock_irqtime(void) {}
 static inline void disable_sched_clock_irqtime(void) {}
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5255c9d..4e7a197 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -757,18 +757,21 @@  static DEFINE_PER_CPU(u64, cpu_hardirq_time);
 static DEFINE_PER_CPU(u64, cpu_softirq_time);
 
 static DEFINE_PER_CPU(u64, irq_start_time);
-static int sched_clock_irqtime;
 
-void enable_sched_clock_irqtime(void)
-{
-	sched_clock_irqtime = 1;
-}
+/*
+ * -1 if not initialized, 0 if disabled with "noirqtime" kernel option
+ * or after unstable clock was detected, 1 if enabled and active.
+ */
+__read_mostly int sched_clock_irqtime = -1;
 
-void disable_sched_clock_irqtime(void)
+static int __init irqtime_setup(char *str)
 {
 	sched_clock_irqtime = 0;
+	return 1;
 }
 
+__setup("noirqtime", irqtime_setup);
+
 #ifndef CONFIG_64BIT
 static DEFINE_PER_CPU(seqcount_t, irq_time_seq);
 
@@ -822,7 +825,7 @@  void account_system_vtime(struct task_struct *curr)
 	s64 delta;
 	int cpu;
 
-	if (!sched_clock_irqtime)
+	if (sched_clock_irqtime < 1)
 		return;
 
 	local_irq_save(flags);
@@ -2853,7 +2856,7 @@  void account_process_tick(struct task_struct *p, int user_tick)
 	cputime_t one_jiffy_scaled = cputime_to_scaled(cputime_one_jiffy);
 	struct rq *rq = this_rq();
 
-	if (sched_clock_irqtime) {
+	if (sched_clock_irqtime > 0) {
 		irqtime_account_process_tick(p, user_tick, rq);
 		return;
 	}
@@ -2887,7 +2890,7 @@  void account_steal_ticks(unsigned long ticks)
 void account_idle_ticks(unsigned long ticks)
 {
 
-	if (sched_clock_irqtime) {
+	if (sched_clock_irqtime > 0) {
 		irqtime_account_idle_ticks(ticks);
 		return;
 	}
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 8745ac7..236e814 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -299,6 +299,18 @@  config SCHEDSTATS
 	  application, you can say N to avoid the very slight overhead
 	  this adds.
 
+config IRQ_TIME_ACCOUNTING
+	bool "Fine granularity task level IRQ time accounting"
+	depends on X86 || ARM
+	default n
+	---help---
+	  Select this option to enable fine granularity task irq time
+	  accounting. This is done by reading a timestamp on each
+	  transitions between softirq and hardirq state, so there can be a
+	  small performance impact.
+
+	  If in doubt, say N here.
+
 config TIMER_STATS
 	bool "Collect kernel timers statistics"
 	depends on DEBUG_KERNEL && PROC_FS