[PATCH] add i386 idle notifier (take 3)

[PATCH] add i386 idle notifier (take 3)

[Stephane Eranian]

Hello,

Here is the latest version of the idle notifier for i386.
This patch is against 2.6.20-rc1 (GIT). In this kernel, the idle
loop code was modified such that the lowest level idle
routines do not have loops anymore (e.g., poll_idle). As such,
we do not need to call enter_idle() in all the interrupt handlers.

This patch also duplicates the x86-64 bug fix for a race condition
as posted by Venkatesh Pallipadi from Intel.

changelog:
	- add idle notification mechanism to i386

signed-off-by: stephane eranian <eranian@hpl.hp.com>

diff --exclude=.git -urNp linux-2.6.orig/arch/i386/kernel/apic.c linux-2.6.base/arch/i386/kernel/apic.c
--- linux-2.6.orig/arch/i386/kernel/apic.c	2006-12-13 16:22:10.000000000 -0800
+++ linux-2.6.base/arch/i386/kernel/apic.c	2006-12-18 04:51:35.000000000 -0800
@@ -36,6 +36,7 @@
 #include <asm/hpet.h>
 #include <asm/i8253.h>
 #include <asm/nmi.h>
+#include <asm/idle.h>
 
 #include <mach_apic.h>
 #include <mach_apicdef.h>
@@ -1255,6 +1256,7 @@ fastcall void smp_apic_timer_interrupt(s
 	 * Besides, if we don't timer interrupts ignore the global
 	 * interrupt lock, which is the WrongThing (tm) to do.
 	 */
+	exit_idle();
 	irq_enter();
 	smp_local_timer_interrupt();
 	irq_exit();
@@ -1305,6 +1307,7 @@ fastcall void smp_spurious_interrupt(str
 {
 	unsigned long v;
 
+	exit_idle();
 	irq_enter();
 	/*
 	 * Check if this really is a spurious interrupt and ACK it
@@ -1329,6 +1332,7 @@ fastcall void smp_error_interrupt(struct
 {
 	unsigned long v, v1;
 
+	exit_idle();
 	irq_enter();
 	/* First tickle the hardware, only then report what went on. -- REW */
 	v = apic_read(APIC_ESR);
diff --exclude=.git -urNp linux-2.6.orig/arch/i386/kernel/cpu/mcheck/p4.c linux-2.6.base/arch/i386/kernel/cpu/mcheck/p4.c
--- linux-2.6.orig/arch/i386/kernel/cpu/mcheck/p4.c	2006-10-17 05:33:35.000000000 -0700
+++ linux-2.6.base/arch/i386/kernel/cpu/mcheck/p4.c	2006-12-18 04:52:37.000000000 -0800
@@ -12,6 +12,7 @@
 #include <asm/system.h>
 #include <asm/msr.h>
 #include <asm/apic.h>
+#include <asm/idle.h>
 
 #include <asm/therm_throt.h>
 
@@ -59,6 +60,7 @@ static void (*vendor_thermal_interrupt)(
 
 fastcall void smp_thermal_interrupt(struct pt_regs *regs)
 {
+	exit_idle();
 	irq_enter();
 	vendor_thermal_interrupt(regs);
 	irq_exit();
diff --exclude=.git -urNp linux-2.6.orig/arch/i386/kernel/irq.c linux-2.6.base/arch/i386/kernel/irq.c
--- linux-2.6.orig/arch/i386/kernel/irq.c	2006-10-26 14:12:56.000000000 -0700
+++ linux-2.6.base/arch/i386/kernel/irq.c	2006-12-18 04:52:42.000000000 -0800
@@ -19,6 +19,8 @@
 #include <linux/cpu.h>
 #include <linux/delay.h>
 
+#include <asm/idle.h>
+
 DEFINE_PER_CPU(irq_cpustat_t, irq_stat) ____cacheline_internodealigned_in_smp;
 EXPORT_PER_CPU_SYMBOL(irq_stat);
 
@@ -61,6 +63,7 @@ fastcall unsigned int do_IRQ(struct pt_r
 	union irq_ctx *curctx, *irqctx;
 	u32 *isp;
 #endif
+	exit_idle();
 
 	if (unlikely((unsigned)irq >= NR_IRQS)) {
 		printk(KERN_EMERG "%s: cannot handle IRQ %d\n",
diff --exclude=.git -urNp linux-2.6.orig/arch/i386/kernel/process.c linux-2.6.base/arch/i386/kernel/process.c
--- linux-2.6.orig/arch/i386/kernel/process.c	2006-12-13 16:22:10.000000000 -0800
+++ linux-2.6.base/arch/i386/kernel/process.c	2006-12-18 04:54:36.000000000 -0800
@@ -48,6 +48,7 @@
 #include <asm/i387.h>
 #include <asm/desc.h>
 #include <asm/vm86.h>
+#include <asm/idle.h>
 #ifdef CONFIG_MATH_EMULATION
 #include <asm/math_emu.h>
 #endif
@@ -80,6 +81,43 @@ void (*pm_idle)(void);
 EXPORT_SYMBOL(pm_idle);
 static DEFINE_PER_CPU(unsigned int, cpu_idle_state);
 
+static ATOMIC_NOTIFIER_HEAD(idle_notifier);
+
+void idle_notifier_register(struct notifier_block *n)
+{
+	atomic_notifier_chain_register(&idle_notifier, n);
+}
+EXPORT_SYMBOL_GPL(idle_notifier_register);
+
+void idle_notifier_unregister(struct notifier_block *n)
+{
+	atomic_notifier_chain_unregister(&idle_notifier, n);
+}
+EXPORT_SYMBOL(idle_notifier_unregister);
+
+static DEFINE_PER_CPU(volatile unsigned long, idle_state);
+
+void enter_idle(void)
+{
+	__get_cpu_var(idle_state) = 1;
+	atomic_notifier_call_chain(&idle_notifier, IDLE_START, NULL);
+}
+
+static void __exit_idle()
+{
+	/* needs to be atomic w.r.t. interrupts, not against other CPUs */
+	if (__test_and_clear_bit(0, &__get_cpu_var(idle_state)) == 0)
+		return;
+	atomic_notifier_call_chain(&idle_notifier, IDLE_END, NULL);
+}
+
+void exit_idle(void)
+{
+	if (current->pid)
+		return;
+	__exit_idle();
+}
+
 void disable_hlt(void)
 {
 	hlt_counter++;
@@ -125,6 +163,7 @@ EXPORT_SYMBOL(default_idle);
  */
 static void poll_idle (void)
 {
+	local_irq_enable();
 	cpu_relax();
 }
 
@@ -184,7 +223,16 @@ void cpu_idle(void)
 				play_dead();
 
 			__get_cpu_var(irq_stat).idle_timestamp = jiffies;
+
+			/*
+			 * Idle routines should keep interrupts disabled
+			 * from here on, until they go to idle.
+			 * Otherwise, idle callbacks can misfire.
+			 */
+			local_irq_disable();
+			enter_idle();
 			idle();
+			__exit_idle();
 		}
 		preempt_enable_no_resched();
 		schedule();
@@ -238,7 +286,11 @@ void mwait_idle_with_hints(unsigned long
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		smp_mb();
 		if (!need_resched())
-			__mwait(eax, ecx);
+			__sti_mwait(eax, ecx);
+		else
+			local_irq_enable();
+	} else {
+		local_irq_enable();
 	}
 }
 
diff --exclude=.git -urNp linux-2.6.orig/arch/i386/kernel/smp.c linux-2.6.base/arch/i386/kernel/smp.c
--- linux-2.6.orig/arch/i386/kernel/smp.c	2006-12-13 16:22:10.000000000 -0800
+++ linux-2.6.base/arch/i386/kernel/smp.c	2006-12-18 04:56:30.000000000 -0800
@@ -23,6 +23,7 @@
 
 #include <asm/mtrr.h>
 #include <asm/tlbflush.h>
+#include <asm/idle.h>
 #include <mach_apic.h>
 
 /*
@@ -624,6 +625,7 @@ fastcall void smp_call_function_interrup
 	/*
 	 * At this point the info structure may be out of scope unless wait==1
 	 */
+	exit_idle();
 	irq_enter();
 	(*func)(info);
 	irq_exit();
diff --exclude=.git -urNp linux-2.6.orig/include/asm-i386/idle.h linux-2.6.base/include/asm-i386/idle.h
--- linux-2.6.orig/include/asm-i386/idle.h	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.6.base/include/asm-i386/idle.h	2006-12-18 04:49:27.000000000 -0800
@@ -0,0 +1,14 @@
+#ifndef _ASM_I386_IDLE_H
+#define _ASM_I386_IDLE_H 1
+
+#define IDLE_START 1
+#define IDLE_END 2
+
+struct notifier_block;
+void idle_notifier_register(struct notifier_block *n);
+void idle_notifier_unregister(struct notifier_block *n);
+
+void exit_idle(void);
+void enter_idle(void);
+
+#endif
diff --exclude=.git -urNp linux-2.6.orig/include/asm-i386/processor.h linux-2.6.base/include/asm-i386/processor.h
--- linux-2.6.orig/include/asm-i386/processor.h	2006-12-13 16:22:22.000000000 -0800
+++ linux-2.6.base/include/asm-i386/processor.h	2006-12-13 16:38:11.000000000 -0800
@@ -257,6 +257,14 @@ static inline void __mwait(unsigned long
 		: :"a" (eax), "c" (ecx));
 }
 
+static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
+{
+	/* "mwait %eax,%ecx;" */
+	asm volatile(
+		"sti; .byte 0x0f,0x01,0xc9;"
+		: :"a" (eax), "c" (ecx));
+}
+
 extern void mwait_idle_with_hints(unsigned long eax, unsigned long ecx);
 
 /* from system description table in BIOS.  Mostly for MCA use, but

Re: [PATCH] add i386 idle notifier (take 3)

[Andrew Morton]

On Wed, 20 Dec 2006 06:05:00 -0800
Stephane Eranian <eranian@hpl.hp.com> wrote:

> Hello,
> 
> Here is the latest version of the idle notifier for i386.
> This patch is against 2.6.20-rc1 (GIT). In this kernel, the idle
> loop code was modified such that the lowest level idle
> routines do not have loops anymore (e.g., poll_idle). As such,
> we do not need to call enter_idle() in all the interrupt handlers.
> 
> This patch also duplicates the x86-64 bug fix for a race condition
> as posted by Venkatesh Pallipadi from Intel.
> 
> changelog:
> 	- add idle notification mechanism to i386
> 

None of the above text is actually usable as a changelog entry.  We are
left wondering:

- why is this patch needed?

- what does it do?

- how does it do it?

The three questions which all changelogs should answer ;)

Re: [PATCH] add i386 idle notifier (take 3)

[Stephane Eranian]

Andrew,

On Wed, Dec 20, 2006 at 09:05:14PM -0800, Andrew Morton wrote:
> On Wed, 20 Dec 2006 06:05:00 -0800
> Stephane Eranian <eranian@hpl.hp.com> wrote:
> 
> > Hello,
> > 
> > Here is the latest version of the idle notifier for i386.
> > This patch is against 2.6.20-rc1 (GIT). In this kernel, the idle
> > loop code was modified such that the lowest level idle
> > routines do not have loops anymore (e.g., poll_idle). As such,
> > we do not need to call enter_idle() in all the interrupt handlers.
> > 
> > This patch also duplicates the x86-64 bug fix for a race condition
> > as posted by Venkatesh Pallipadi from Intel.
> > 
> > changelog:
> > 	- add idle notification mechanism to i386
> > 
> 
> None of the above text is actually usable as a changelog entry.  We are
> left wondering:
> 
> - why is this patch needed?
> 
> - what does it do?
> 
> - how does it do it?
> 
> The three questions which all changelogs should answer ;)

Sorry about that. Here is a new changelog:

changelog:
	- add a notifier mechanism to the low level idle loop. You can
	  register a callback function which gets invoked on entry and exit
	  from the low level idle loop. The low level idle loop is defined as
	  the polling loop, low-power call, or the mwait instruction. Interrupts
	  processed by the idle thread are not considered part of the low level
	  loop. The notifier can be used to measure precisely how much is spent
	  in useless execution (or low power mode). The perfmon subsystem uses it
	  to turn on/off monitoring.

-- 
-Stephane

Re: [PATCH] add i386 idle notifier (take 3)

[Adrian Bunk]

On Thu, Dec 21, 2006 at 01:12:42AM -0800, Stephane Eranian wrote:
> Andrew,
> 
> On Wed, Dec 20, 2006 at 09:05:14PM -0800, Andrew Morton wrote:
> > On Wed, 20 Dec 2006 06:05:00 -0800
> > Stephane Eranian <eranian@hpl.hp.com> wrote:
> > 
> > > Hello,
> > > 
> > > Here is the latest version of the idle notifier for i386.
> > > This patch is against 2.6.20-rc1 (GIT). In this kernel, the idle
> > > loop code was modified such that the lowest level idle
> > > routines do not have loops anymore (e.g., poll_idle). As such,
> > > we do not need to call enter_idle() in all the interrupt handlers.
> > > 
> > > This patch also duplicates the x86-64 bug fix for a race condition
> > > as posted by Venkatesh Pallipadi from Intel.
> > > 
> > > changelog:
> > > 	- add idle notification mechanism to i386
> > > 
> > 
> > None of the above text is actually usable as a changelog entry.  We are
> > left wondering:
> > 
> > - why is this patch needed?
> > 
> > - what does it do?
> > 
> > - how does it do it?
> > 
> > The three questions which all changelogs should answer ;)
> 
> Sorry about that. Here is a new changelog:
> 
> changelog:
> 	- add a notifier mechanism to the low level idle loop. You can
> 	  register a callback function which gets invoked on entry and exit
> 	  from the low level idle loop. The low level idle loop is defined as
> 	  the polling loop, low-power call, or the mwait instruction. Interrupts
> 	  processed by the idle thread are not considered part of the low level
> 	  loop. The notifier can be used to measure precisely how much is spent
> 	  in useless execution (or low power mode). The perfmon subsystem uses it
> 	  to turn on/off monitoring.


Why is this patch not submitted as part of the perfmon patch that also 
adds a user of this code?

And why does it bloat the kernel with EXPORT_SYMBOL's although even your 
perfmon-new-base-061204 doesn't seem to add any modular user?


> -Stephane

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] add i386 idle notifier (take 3)

[Stephane Eranian]

Andrian,

On Fri, Dec 22, 2006 at 02:06:41AM +0100, Adrian Bunk wrote:
> > changelog:
> > 	- add a notifier mechanism to the low level idle loop. You can
> > 	  register a callback function which gets invoked on entry and exit
> > 	  from the low level idle loop. The low level idle loop is defined as
> > 	  the polling loop, low-power call, or the mwait instruction. Interrupts
> > 	  processed by the idle thread are not considered part of the low level
> > 	  loop. The notifier can be used to measure precisely how much is spent
> > 	  in useless execution (or low power mode). The perfmon subsystem uses it
> > 	  to turn on/off monitoring.
> 
> 
> Why is this patch not submitted as part of the perfmon patch that also 
> adds a user of this code?
> 

If you look at the perfmon-new-base patch, you'll see a base.diff patch which
includes this one. I am slowly getting rid of this requirement by pushing
those "infrastructure patches" to mainline so that the perfmon patch gets
smaller over time. Submitting smaller patches makes it easier for maintainers
to integrate.

> And why does it bloat the kernel with EXPORT_SYMBOL's although even your 
> perfmon-new-base-061204 doesn't seem to add any modular user?
> 

I have tried to stay as close as possible from the x86-64 implementation
of this mechanism. The registration entry points are exported to modules,
just like they are for x86-64. Also note that the x86-64 idle notifier does
not have a user at this point, yet it is in the kernel. Perfmon will become
the first user of this mechanism.

-- 
-Stephane

Re: [PATCH] add i386 idle notifier (take 3)

[Adrian Bunk]

On Fri, Dec 22, 2006 at 02:07:00AM -0800, Stephane Eranian wrote:
> Andrian,
> 
> On Fri, Dec 22, 2006 at 02:06:41AM +0100, Adrian Bunk wrote:
> > > changelog:
> > > 	- add a notifier mechanism to the low level idle loop. You can
> > > 	  register a callback function which gets invoked on entry and exit
> > > 	  from the low level idle loop. The low level idle loop is defined as
> > > 	  the polling loop, low-power call, or the mwait instruction. Interrupts
> > > 	  processed by the idle thread are not considered part of the low level
> > > 	  loop. The notifier can be used to measure precisely how much is spent
> > > 	  in useless execution (or low power mode). The perfmon subsystem uses it
> > > 	  to turn on/off monitoring.
> > 
> > 
> > Why is this patch not submitted as part of the perfmon patch that also 
> > adds a user of this code?
> 
> If you look at the perfmon-new-base patch, you'll see a base.diff patch which
> includes this one. I am slowly getting rid of this requirement by pushing
> those "infrastructure patches" to mainline so that the perfmon patch gets
> smaller over time. Submitting smaller patches makes it easier for maintainers
> to integrate.

No, the preferred way is to start with getting both the infrastructure 
and the users into -mm.

Adding infrastructure without users doesn't fit into the kernel 
development model.

The unused x86-64 idle notifiers are now bloating the kernel since 
nearly one year.

> > And why does it bloat the kernel with EXPORT_SYMBOL's although even your 
> > perfmon-new-base-061204 doesn't seem to add any modular user?
> 
> I have tried to stay as close as possible from the x86-64 implementation
> of this mechanism. The registration entry points are exported to modules,
> just like they are for x86-64. Also note that the x86-64 idle notifier does
> not have a user at this point, yet it is in the kernel. Perfmon will become
> the first user of this mechanism.

Where does the perfmon code use the EXPORT_SYMBOL's?

And having added bloat on one architecture is not an excuse for adding 
bloat on other architectures.

> -Stephane

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] add i386 idle notifier (take 3)

[Stephane Eranian]

Adrian,

On Sat, Dec 23, 2006 at 12:40:15PM +0100, Adrian Bunk wrote:
> > 
> > If you look at the perfmon-new-base patch, you'll see a base.diff patch which
> > includes this one. I am slowly getting rid of this requirement by pushing
> > those "infrastructure patches" to mainline so that the perfmon patch gets
> > smaller over time. Submitting smaller patches makes it easier for maintainers
> > to integrate.
> 
> No, the preferred way is to start with getting both the infrastructure 
> and the users into -mm.
> 
> Adding infrastructure without users doesn't fit into the kernel 
> development model.
> 

I am hearing conflicting opinions on this one.

Perfmon is a fairly big patch. It is hard to take it as one. I have tried to
split it up in smaller, more manageable pieces as requested by top-level
maintainers. This process implies that I supply small patches which may not
necessarily have users just yet.

> The unused x86-64 idle notifiers are now bloating the kernel since 
> nearly one year.
> 
> > > And why does it bloat the kernel with EXPORT_SYMBOL's although even your 
> > > perfmon-new-base-061204 doesn't seem to add any modular user?
> > 
> Where does the perfmon code use the EXPORT_SYMBOL's?

The perfmon patch includes several kernel modules which make use of
the exported entry points. The following symbols are exported:

pfm_pmu_register/pfm_pmu_unregister:
	* PMU description module registration.
	* Used to describe PMU model.
	* Used by perfmon_p4.c, perfmon_core.c, perfmon_mckinley.c, and others

pfm_fmt_register/pfm_fmt_unregister:
	* Sampling format module registration
	* Used by perfmon_dfl_smpl.c, perfmon_pebs_smpl.c

pfm_interrupt_handler:
	* PMU interrupt handler
	* Used by MIPS-specific perfmon code

pfm_pmu_conf/pfm_controls:
	* global state/control variable

All exported symbols are currently used. Why are you saying this adds bloat?

-- 
-Stephane

Re: [PATCH] add i386 idle notifier (take 3)

[Adrian Bunk]

On Wed, Jan 03, 2007 at 05:20:15AM -0800, Stephane Eranian wrote:
> Adrian,
> 
> On Sat, Dec 23, 2006 at 12:40:15PM +0100, Adrian Bunk wrote:
> > > 
> > > If you look at the perfmon-new-base patch, you'll see a base.diff patch which
> > > includes this one. I am slowly getting rid of this requirement by pushing
> > > those "infrastructure patches" to mainline so that the perfmon patch gets
> > > smaller over time. Submitting smaller patches makes it easier for maintainers
> > > to integrate.
> > 
> > No, the preferred way is to start with getting both the infrastructure 
> > and the users into -mm.
> > 
> > Adding infrastructure without users doesn't fit into the kernel 
> > development model.
> 
> I am hearing conflicting opinions on this one.
> 
> Perfmon is a fairly big patch. It is hard to take it as one. I have tried to
> split it up in smaller, more manageable pieces as requested by top-level
> maintainers. This process implies that I supply small patches which may not
> necessarily have users just yet.

There should be a big patchset consisting of manageable pieces, if 
possible all of it in -mm.

> > The unused x86-64 idle notifiers are now bloating the kernel since 
> > nearly one year.
> > 
> > > > And why does it bloat the kernel with EXPORT_SYMBOL's although even your 
> > > > perfmon-new-base-061204 doesn't seem to add any modular user?
> > > 
> > Where does the perfmon code use the EXPORT_SYMBOL's?
> 
> The perfmon patch includes several kernel modules which make use of
> the exported entry points. The following symbols are exported:
> 
> pfm_pmu_register/pfm_pmu_unregister:
> 	* PMU description module registration.
> 	* Used to describe PMU model.
> 	* Used by perfmon_p4.c, perfmon_core.c, perfmon_mckinley.c, and others
> 
> pfm_fmt_register/pfm_fmt_unregister:
> 	* Sampling format module registration
> 	* Used by perfmon_dfl_smpl.c, perfmon_pebs_smpl.c
> 
> pfm_interrupt_handler:
> 	* PMU interrupt handler
> 	* Used by MIPS-specific perfmon code
> 
> pfm_pmu_conf/pfm_controls:
> 	* global state/control variable
> 
> All exported symbols are currently used. Why are you saying this adds bloat?

Which module uses idle_notifier_register/idle_notifier_unregister?

> -Stephane

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] add i386 idle notifier (take 3)

[Stephane Eranian]

Adrian,

On Thu, Jan 04, 2007 at 12:07:08AM +0100, Adrian Bunk wrote:
> > I am hearing conflicting opinions on this one.
> > 
> > Perfmon is a fairly big patch. It is hard to take it as one. I have tried to
> > split it up in smaller, more manageable pieces as requested by top-level
> > maintainers. This process implies that I supply small patches which may not
> > necessarily have users just yet.
> 
> There should be a big patchset consisting of manageable pieces, if 
> possible all of it in -mm.
> 

I have already split up the pieces: generic vs. per-arch. I have also
divided it between modified vs. new files. It becomes harder to go
much beyond that without creating also one patch per modified file.

> > > The unused x86-64 idle notifiers are now bloating the kernel since 
> > > nearly one year.
> > > 
> > > > > And why does it bloat the kernel with EXPORT_SYMBOL's although even your 
> > > > > perfmon-new-base-061204 doesn't seem to add any modular user?
> > > > 
> > > Where does the perfmon code use the EXPORT_SYMBOL's?
> > 
> > The perfmon patch includes several kernel modules which make use of
> > the exported entry points. The following symbols are exported:
> > 
> > pfm_pmu_register/pfm_pmu_unregister:
> > 	* PMU description module registration.
> > 	* Used to describe PMU model.
> > 	* Used by perfmon_p4.c, perfmon_core.c, perfmon_mckinley.c, and others
> > 
> > pfm_fmt_register/pfm_fmt_unregister:
> > 	* Sampling format module registration
> > 	* Used by perfmon_dfl_smpl.c, perfmon_pebs_smpl.c
> > 
> > pfm_interrupt_handler:
> > 	* PMU interrupt handler
> > 	* Used by MIPS-specific perfmon code
> > 
> > pfm_pmu_conf/pfm_controls:
> > 	* global state/control variable
> > 
> > All exported symbols are currently used. Why are you saying this adds bloat?
> 
> Which module uses idle_notifier_register/idle_notifier_unregister?
> 
None.

I have no issue with removing the EXPORT_SYMBOL on i386 and x86_64 if you
think that would help.

-- 
-Stephane

Re: [PATCH] add i386 idle notifier (take 3)

[Adrian Bunk]

On Fri, Jan 05, 2007 at 02:55:14AM -0800, Stephane Eranian wrote:
> Adrian,
> 
> On Thu, Jan 04, 2007 at 12:07:08AM +0100, Adrian Bunk wrote:
> > > I am hearing conflicting opinions on this one.
> > > 
> > > Perfmon is a fairly big patch. It is hard to take it as one. I have tried to
> > > split it up in smaller, more manageable pieces as requested by top-level
> > > maintainers. This process implies that I supply small patches which may not
> > > necessarily have users just yet.
> > 
> > There should be a big patchset consisting of manageable pieces, if 
> > possible all of it in -mm.
> 
> I have already split up the pieces: generic vs. per-arch. I have also
> divided it between modified vs. new files. It becomes harder to go
> much beyond that without creating also one patch per modified file.

That's not my point.

My point is that while big changes should come in manageable pieces, 
it's also important to have the whole.

Is there any reason against getting all your patches into -mm?

> > > > The unused x86-64 idle notifiers are now bloating the kernel since 
> > > > nearly one year.
> > > > 
> > > > > > And why does it bloat the kernel with EXPORT_SYMBOL's although even your 
> > > > > > perfmon-new-base-061204 doesn't seem to add any modular user?
> > > > > 
> > > > Where does the perfmon code use the EXPORT_SYMBOL's?
> > > 
> > > The perfmon patch includes several kernel modules which make use of
> > > the exported entry points. The following symbols are exported:
> > > 
> > > pfm_pmu_register/pfm_pmu_unregister:
> > > 	* PMU description module registration.
> > > 	* Used to describe PMU model.
> > > 	* Used by perfmon_p4.c, perfmon_core.c, perfmon_mckinley.c, and others
> > > 
> > > pfm_fmt_register/pfm_fmt_unregister:
> > > 	* Sampling format module registration
> > > 	* Used by perfmon_dfl_smpl.c, perfmon_pebs_smpl.c
> > > 
> > > pfm_interrupt_handler:
> > > 	* PMU interrupt handler
> > > 	* Used by MIPS-specific perfmon code
> > > 
> > > pfm_pmu_conf/pfm_controls:
> > > 	* global state/control variable
> > > 
> > > All exported symbols are currently used. Why are you saying this adds bloat?
> > 
> > Which module uses idle_notifier_register/idle_notifier_unregister?
> > 
> None.
> 
> I have no issue with removing the EXPORT_SYMBOL on i386 and x86_64 if you
> think that would help.

OK, thanks.

> -Stephane

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] add i386 idle notifier (take 3)

[Stephane Eranian]

Hello,

On Fri, Jan 05, 2007 at 02:36:53PM +0100, Adrian Bunk wrote:
> > > > > Where does the perfmon code use the EXPORT_SYMBOL's?
> > > > 
> > > > The perfmon patch includes several kernel modules which make use of
> > > > the exported entry points. The following symbols are exported:
> > > > 
> > > > pfm_pmu_register/pfm_pmu_unregister:
> > > > 	* PMU description module registration.
> > > > 	* Used to describe PMU model.
> > > > 	* Used by perfmon_p4.c, perfmon_core.c, perfmon_mckinley.c, and others
> > > > 
> > > > pfm_fmt_register/pfm_fmt_unregister:
> > > > 	* Sampling format module registration
> > > > 	* Used by perfmon_dfl_smpl.c, perfmon_pebs_smpl.c
> > > > 
> > > > pfm_interrupt_handler:
> > > > 	* PMU interrupt handler
> > > > 	* Used by MIPS-specific perfmon code
> > > > 
> > > > pfm_pmu_conf/pfm_controls:
> > > > 	* global state/control variable
> > > > 
> > > > All exported symbols are currently used. Why are you saying this adds bloat?
> > > 
> > > Which module uses idle_notifier_register/idle_notifier_unregister?
> > > 
> > None.
> > 
> > I have no issue with removing the EXPORT_SYMBOL on i386 and x86_64 if you
> > think that would help.
> 

As a follow-up to this discussion, here is a patch that remove unused
EXPORT_SYMBOL by the i386 idle notifier. It also make it more explicit
that the enter_idle() store must be atomic.

changelog:
	- do not export i386 idle notification registration entry points
	  because there is currently no user for this
	- make it more explicit that the store to idle_state must be atomic

signed-off-by: stephane eranian <eranian@hpl.hp.com>

--- linux-2.6.20-rc3-mm1.orig/arch/i386/kernel/process.c	2007-01-08 01:58:08.000000000 -0800
+++ linux-2.6.20-rc3-mm1/arch/i386/kernel/process.c	2007-01-08 02:01:59.000000000 -0800
@@ -87,19 +87,18 @@ void idle_notifier_register(struct notif
 {
 	atomic_notifier_chain_register(&idle_notifier, n);
 }
-EXPORT_SYMBOL_GPL(idle_notifier_register);
 
 void idle_notifier_unregister(struct notifier_block *n)
 {
 	atomic_notifier_chain_unregister(&idle_notifier, n);
 }
-EXPORT_SYMBOL(idle_notifier_unregister);
 
 static DEFINE_PER_CPU(volatile unsigned long, idle_state);
 
 void enter_idle(void)
 {
-	__get_cpu_var(idle_state) = 1;
+	/* needs to be atomic w.r.t. interrupts, not against other CPUs */
+	__set_bit(0, &__get_cpu_var(idle_state));
 	atomic_notifier_call_chain(&idle_notifier, IDLE_START, NULL);
 }