[PATCH 0/2] cond_resched() optimization

[PATCH 0/2] cond_resched() optimization

[Peter Zijlstra]

Matt suggested, whereupon Linus sayeth:

 "This looks like a good patch. Please make it so"

Compile and boot tested on x86_64 only.

[PATCH 1/2] sched: INIT_PREEMPT_COUNT

[Peter Zijlstra]

[-- Attachment #1: sched_init_preempt_count.patch --]
[-- Type: text/plain, Size: 16250 bytes --]

Pull the initial preempt_count value into a single
definition site.

Maintainers for: alpha, ia64 and m68k, please have a look,
your arch code is funny.

The header magic is a bit odd, but similar to the KERNEL_DS
one, CPP waits with expanding these macros until the
INIT_THREAD_INFO macro itself is expanded, which is in
arch/*/kernel/init_task.c where we've already included
sched.h so we're good.

Cc: tony.luck@intel.com
Cc: rth@twiddle.net
Cc: geert@linux-m68k.org
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 arch/alpha/include/asm/thread_info.h      |    1 +
 arch/arm/include/asm/thread_info.h        |    2 +-
 arch/avr32/include/asm/thread_info.h      |    2 +-
 arch/blackfin/include/asm/thread_info.h   |    2 +-
 arch/cris/include/asm/thread_info.h       |    4 +---
 arch/frv/include/asm/thread_info.h        |    4 +---
 arch/h8300/include/asm/thread_info.h      |    2 +-
 arch/ia64/include/asm/thread_info.h       |    2 +-
 arch/m32r/include/asm/thread_info.h       |    4 +---
 arch/m68k/include/asm/thread_info_mm.h    |    1 +
 arch/m68k/include/asm/thread_info_no.h    |    1 +
 arch/microblaze/include/asm/thread_info.h |    4 +---
 arch/mips/include/asm/thread_info.h       |    4 +---
 arch/mn10300/include/asm/thread_info.h    |    4 +---
 arch/parisc/include/asm/thread_info.h     |    2 +-
 arch/powerpc/include/asm/thread_info.h    |    4 +---
 arch/s390/include/asm/thread_info.h       |    2 +-
 arch/sh/include/asm/thread_info.h         |    2 +-
 arch/sparc/include/asm/thread_info_32.h   |    4 +---
 arch/sparc/include/asm/thread_info_64.h   |    4 +---
 arch/um/include/asm/thread_info.h         |    2 +-
 arch/x86/include/asm/thread_info.h        |    2 +-
 arch/xtensa/include/asm/thread_info.h     |    4 +---
 include/linux/sched.h                     |    6 ++++++
 24 files changed, 29 insertions(+), 40 deletions(-)

Index: linux-2.6/arch/alpha/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/alpha/include/asm/thread_info.h
+++ linux-2.6/arch/alpha/include/asm/thread_info.h
@@ -37,6 +37,7 @@ struct thread_info {
 	.task		= &tsk,			\
 	.exec_domain	= &default_exec_domain,	\
 	.addr_limit	= KERNEL_DS,		\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/arm/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/arm/include/asm/thread_info.h
+++ linux-2.6/arch/arm/include/asm/thread_info.h
@@ -73,7 +73,7 @@ struct thread_info {
 	.task		= &tsk,						\
 	.exec_domain	= &default_exec_domain,				\
 	.flags		= 0,						\
-	.preempt_count	= 1,						\
+	.preempt_count	= INIT_PREEMPT_COUNT,				\
 	.addr_limit	= KERNEL_DS,					\
 	.cpu_domain	= domain_val(DOMAIN_USER, DOMAIN_MANAGER) |	\
 			  domain_val(DOMAIN_KERNEL, DOMAIN_MANAGER) |	\
Index: linux-2.6/arch/avr32/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/avr32/include/asm/thread_info.h
+++ linux-2.6/arch/avr32/include/asm/thread_info.h
@@ -40,7 +40,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,				\
 	.flags		= 0,						\
 	.cpu		= 0,						\
-	.preempt_count	= 1,						\
+	.preempt_count	= INIT_PREEMPT_COUNT,				\
 	.restart_block	= {						\
 		.fn	= do_no_restart_syscall				\
 	}								\
Index: linux-2.6/arch/blackfin/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/blackfin/include/asm/thread_info.h
+++ linux-2.6/arch/blackfin/include/asm/thread_info.h
@@ -77,7 +77,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count  = 1,                    \
+	.preempt_count  = INIT_PREEMPT_COUNT,   \
 	.restart_block	= {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/cris/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/cris/include/asm/thread_info.h
+++ linux-2.6/arch/cris/include/asm/thread_info.h
@@ -50,8 +50,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #ifndef __ASSEMBLY__
 #define INIT_THREAD_INFO(tsk)				\
@@ -60,7 +58,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,		\
 	.flags		= 0,				\
 	.cpu		= 0,				\
-	.preempt_count	= 1,				\
+	.preempt_count	= INIT_PREEMPT_COUNT,		\
 	.addr_limit	= KERNEL_DS,			\
 	.restart_block = {				\
 		       .fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/frv/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/frv/include/asm/thread_info.h
+++ linux-2.6/arch/frv/include/asm/thread_info.h
@@ -56,8 +56,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #ifndef __ASSEMBLY__
 
@@ -67,7 +65,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/h8300/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/h8300/include/asm/thread_info.h
+++ linux-2.6/arch/h8300/include/asm/thread_info.h
@@ -36,7 +36,7 @@ struct thread_info {
 	.exec_domain =	&default_exec_domain,	\
 	.flags =	0,			\
 	.cpu =		0,			\
-	.preempt_count = 1,			\
+	.preempt_count = INIT_PREEMPT_COUNT,	\
 	.restart_block	= {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/ia64/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/ia64/include/asm/thread_info.h
+++ linux-2.6/arch/ia64/include/asm/thread_info.h
@@ -48,7 +48,7 @@ struct thread_info {
 	.flags		= 0,			\
 	.cpu		= 0,			\
 	.addr_limit	= KERNEL_DS,		\
-	.preempt_count	= 0,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/m32r/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/m32r/include/asm/thread_info.h
+++ linux-2.6/arch/m32r/include/asm/thread_info.h
@@ -57,8 +57,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #ifndef __ASSEMBLY__
 
@@ -68,7 +66,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/m68k/include/asm/thread_info_mm.h
===================================================================
--- linux-2.6.orig/arch/m68k/include/asm/thread_info_mm.h
+++ linux-2.6/arch/m68k/include/asm/thread_info_mm.h
@@ -19,6 +19,7 @@ struct thread_info {
 {						\
 	.task		= &tsk,			\
 	.exec_domain	= &default_exec_domain,	\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/m68k/include/asm/thread_info_no.h
===================================================================
--- linux-2.6.orig/arch/m68k/include/asm/thread_info_no.h
+++ linux-2.6/arch/m68k/include/asm/thread_info_no.h
@@ -49,6 +49,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.restart_block	= {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/microblaze/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/microblaze/include/asm/thread_info.h
+++ linux-2.6/arch/microblaze/include/asm/thread_info.h
@@ -75,8 +75,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #define INIT_THREAD_INFO(tsk)			\
 {						\
@@ -84,7 +82,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/mips/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/mips/include/asm/thread_info.h
+++ linux-2.6/arch/mips/include/asm/thread_info.h
@@ -39,8 +39,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #define INIT_THREAD_INFO(tsk)			\
 {						\
@@ -48,7 +46,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= _TIF_FIXADE,		\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block	= {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/mn10300/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/mn10300/include/asm/thread_info.h
+++ linux-2.6/arch/mn10300/include/asm/thread_info.h
@@ -65,8 +65,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #ifndef __ASSEMBLY__
 
@@ -76,7 +74,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/parisc/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/parisc/include/asm/thread_info.h
+++ linux-2.6/arch/parisc/include/asm/thread_info.h
@@ -23,7 +23,7 @@ struct thread_info {
 	.flags		= 0,			\
 	.cpu		= 0,			\
 	.addr_limit	= KERNEL_DS,		\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
   	.restart_block	= {			\
 		.fn = do_no_restart_syscall	\
 	}					\
Index: linux-2.6/arch/powerpc/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/powerpc/include/asm/thread_info.h
+++ linux-2.6/arch/powerpc/include/asm/thread_info.h
@@ -46,15 +46,13 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #define INIT_THREAD_INFO(tsk)			\
 {						\
 	.task =		&tsk,			\
 	.exec_domain =	&default_exec_domain,	\
 	.cpu =		0,			\
-	.preempt_count = 1,			\
+	.preempt_count = INIT_PREEMPT_COUNT,	\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/s390/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/s390/include/asm/thread_info.h
+++ linux-2.6/arch/s390/include/asm/thread_info.h
@@ -61,7 +61,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.restart_block	= {			\
 		.fn = do_no_restart_syscall,	\
 	},					\
Index: linux-2.6/arch/sh/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/sh/include/asm/thread_info.h
+++ linux-2.6/arch/sh/include/asm/thread_info.h
@@ -51,7 +51,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block	= {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/sparc/include/asm/thread_info_32.h
===================================================================
--- linux-2.6.orig/arch/sparc/include/asm/thread_info_32.h
+++ linux-2.6/arch/sparc/include/asm/thread_info_32.h
@@ -54,8 +54,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #define INIT_THREAD_INFO(tsk)				\
 {							\
@@ -64,7 +62,7 @@ struct thread_info {
 	.exec_domain	=	&default_exec_domain,	\
 	.flags		=	0,			\
 	.cpu		=	0,			\
-	.preempt_count	=	1,			\
+	.preempt_count	=	INIT_PREEMPT_COUNT,	\
 	.restart_block	= {				\
 		.fn	=	do_no_restart_syscall,	\
 	},						\
Index: linux-2.6/arch/sparc/include/asm/thread_info_64.h
===================================================================
--- linux-2.6.orig/arch/sparc/include/asm/thread_info_64.h
+++ linux-2.6/arch/sparc/include/asm/thread_info_64.h
@@ -125,8 +125,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 #ifndef __ASSEMBLY__
 
@@ -135,7 +133,7 @@ struct thread_info {
 	.task		=	&tsk,			\
 	.flags		= ((unsigned long)ASI_P) << TI_FLAG_CURRENT_DS_SHIFT,	\
 	.exec_domain	=	&default_exec_domain,	\
-	.preempt_count	=	1,			\
+	.preempt_count	=	INIT_PREEMPT_COUNT,	\
 	.restart_block	= {				\
 		.fn	=	do_no_restart_syscall,	\
 	},						\
Index: linux-2.6/arch/um/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/um/include/asm/thread_info.h
+++ linux-2.6/arch/um/include/asm/thread_info.h
@@ -32,7 +32,7 @@ struct thread_info {
 	.exec_domain =	&default_exec_domain,	\
 	.flags =		0,		\
 	.cpu =		0,			\
-	.preempt_count =	1,		\
+	.preempt_count = INIT_PREEMPT_COUNT,	\
 	.addr_limit =	KERNEL_DS,		\
 	.restart_block =  {			\
 		.fn =  do_no_restart_syscall,	\
Index: linux-2.6/arch/x86/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/thread_info.h
+++ linux-2.6/arch/x86/include/asm/thread_info.h
@@ -49,7 +49,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/arch/xtensa/include/asm/thread_info.h
===================================================================
--- linux-2.6.orig/arch/xtensa/include/asm/thread_info.h
+++ linux-2.6/arch/xtensa/include/asm/thread_info.h
@@ -80,8 +80,6 @@ struct thread_info {
 
 /*
  * macros/functions for gaining access to the thread information structure
- *
- * preempt_count needs to be 1 initially, until the scheduler is functional.
  */
 
 #ifndef __ASSEMBLY__
@@ -92,7 +90,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -500,6 +500,12 @@ struct task_cputime {
 		.sum_exec_runtime = 0,				\
 	}
 
+/*
+ * Disable preemption until the scheduler is running.
+ * Reset by start_kernel()->sched_init()->init_idle().
+ */
+#define INIT_PREEMPT_COUNT	(1)
+
 /**
  * struct thread_group_cputimer - thread group interval timer counts
  * @cputime:		thread group interval timers.

-- 

[PATCH 2/2] sched: optimize cond_resched()

[Peter Zijlstra]

[-- Attachment #1: sched_opt_cond_resched.patch --]
[-- Type: text/plain, Size: 2450 bytes --]

Optimize cond_resched() by removing one conditional.

Currently cond_resched() checks system_state ==
SYSTEM_RUNNING in order to avoid scheduling before the
scheduler is running.

We can however, as per suggestion of Matt, use
PREEMPT_ACTIVE to accomplish that very same.

Suggested-by: Matt Mackall <mpm@selenic.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/sched.h |    5 ++++-
 kernel/sched.c        |   14 +++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -503,8 +503,11 @@ struct task_cputime {
 /*
  * Disable preemption until the scheduler is running.
  * Reset by start_kernel()->sched_init()->init_idle().
+ *
+ * We include PREEMPT_ACTIVE to avoid cond_resched() from working
+ * before the scheduler is active -- see should_resched().
  */
-#define INIT_PREEMPT_COUNT	(1)
+#define INIT_PREEMPT_COUNT	(1 + PREEMPT_ACTIVE)
 
 /**
  * struct thread_group_cputimer - thread group interval timer counts
Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -6580,6 +6580,11 @@ SYSCALL_DEFINE0(sched_yield)
 	return 0;
 }
 
+static inline int should_resched(void)
+{
+	return need_resched() && !(preempt_count() & PREEMPT_ACTIVE);
+}
+
 static void __cond_resched(void)
 {
 #ifdef CONFIG_DEBUG_SPINLOCK_SLEEP
@@ -6599,8 +6604,7 @@ static void __cond_resched(void)
 
 int __sched _cond_resched(void)
 {
-	if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
-					system_state == SYSTEM_RUNNING) {
+	if (should_resched()) {
 		__cond_resched();
 		return 1;
 	}
@@ -6618,12 +6622,12 @@ EXPORT_SYMBOL(_cond_resched);
  */
 int cond_resched_lock(spinlock_t *lock)
 {
-	int resched = need_resched() && system_state == SYSTEM_RUNNING;
+	int resched = should_resched();
 	int ret = 0;
 
 	if (spin_needbreak(lock) || resched) {
 		spin_unlock(lock);
-		if (resched && need_resched())
+		if (resched)
 			__cond_resched();
 		else
 			cpu_relax();
@@ -6638,7 +6642,7 @@ int __sched cond_resched_softirq(void)
 {
 	BUG_ON(!in_softirq());
 
-	if (need_resched() && system_state == SYSTEM_RUNNING) {
+	if (should_resched()) {
 		local_bh_enable();
 		__cond_resched();
 		local_bh_disable();

-- 

Re: [PATCH 1/2] sched: INIT_PREEMPT_COUNT

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Fri, 10 Jul 2009 14:57:56 +0200, Peter Zijlstra said:

> +/*
> + * Disable preemption until the scheduler is running.
> + * Reset by start_kernel()->sched_init()->init_idle().
> + */
> +#define INIT_PREEMPT_COUNT	(1)
> +

I had to look at this for quite some time before it sank in that it wasn't
a reset of a #define, or a reset of (1) (anybody else remember changing the
value of '5' in a Fortran program?).  Especially when stuck in with a bunch
of cputimer defines. Would have taken even longer if I was looking in sched.h
for something and not looking at this patch at the same time.

Can we fix this comment to mention it's thread_info.preempt_count that
needs the reset?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 1/2] sched: INIT_PREEMPT_COUNT

[Peter Zijlstra]

On Fri, 2009-07-10 at 11:42 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Fri, 10 Jul 2009 14:57:56 +0200, Peter Zijlstra said:
> 
> > +/*
> > + * Disable preemption until the scheduler is running.
> > + * Reset by start_kernel()->sched_init()->init_idle().
> > + */
> > +#define INIT_PREEMPT_COUNT	(1)
> > +
> 
> I had to look at this for quite some time before it sank in that it wasn't
> a reset of a #define, or a reset of (1) (anybody else remember changing the
> value of '5' in a Fortran program?).  Especially when stuck in with a bunch
> of cputimer defines. Would have taken even longer if I was looking in sched.h
> for something and not looking at this patch at the same time.
> 
> Can we fix this comment to mention it's thread_info.preempt_count that
> needs the reset?

Something along the lines of the below?

---
Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -502,7 +502,9 @@ struct task_cputime {
 
 /*
  * Disable preemption until the scheduler is running.
- * Reset by start_kernel()->sched_init()->init_idle().
+ *
+ * We reset this initial offset of init_thread_info.preempt_count in:
+ *     start_kernel()->sched_init()->init_idle().
  */
 #define INIT_PREEMPT_COUNT	(1)
 

Re: [PATCH 2/2] sched: optimize cond_resched()

[Matt Mackall]

On Fri, 2009-07-10 at 14:57 +0200, Peter Zijlstra wrote:
> plain text document attachment (sched_opt_cond_resched.patch)
> Optimize cond_resched() by removing one conditional.
> 
> Currently cond_resched() checks system_state ==
> SYSTEM_RUNNING in order to avoid scheduling before the
> scheduler is running.
> 
> We can however, as per suggestion of Matt, use
> PREEMPT_ACTIVE to accomplish that very same.

Pedantically, introducing should_resched should be its own patch, but
other than that, these two patches look good.

Acked-by: Matt Mackall <mpm@selenic.com>

> Suggested-by: Matt Mackall <mpm@selenic.com>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  include/linux/sched.h |    5 ++++-
>  kernel/sched.c        |   14 +++++++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> Index: linux-2.6/include/linux/sched.h
> ===================================================================
> --- linux-2.6.orig/include/linux/sched.h
> +++ linux-2.6/include/linux/sched.h
> @@ -503,8 +503,11 @@ struct task_cputime {
>  /*
>   * Disable preemption until the scheduler is running.
>   * Reset by start_kernel()->sched_init()->init_idle().
> + *
> + * We include PREEMPT_ACTIVE to avoid cond_resched() from working
> + * before the scheduler is active -- see should_resched().
>   */
> -#define INIT_PREEMPT_COUNT	(1)
> +#define INIT_PREEMPT_COUNT	(1 + PREEMPT_ACTIVE)
>  
>  /**
>   * struct thread_group_cputimer - thread group interval timer counts
> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c
> +++ linux-2.6/kernel/sched.c
> @@ -6580,6 +6580,11 @@ SYSCALL_DEFINE0(sched_yield)
>  	return 0;
>  }
>  
> +static inline int should_resched(void)
> +{
> +	return need_resched() && !(preempt_count() & PREEMPT_ACTIVE);
> +}
> +
>  static void __cond_resched(void)
>  {
>  #ifdef CONFIG_DEBUG_SPINLOCK_SLEEP
> @@ -6599,8 +6604,7 @@ static void __cond_resched(void)
>  
>  int __sched _cond_resched(void)
>  {
> -	if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
> -					system_state == SYSTEM_RUNNING) {
> +	if (should_resched()) {
>  		__cond_resched();
>  		return 1;
>  	}
> @@ -6618,12 +6622,12 @@ EXPORT_SYMBOL(_cond_resched);
>   */
>  int cond_resched_lock(spinlock_t *lock)
>  {
> -	int resched = need_resched() && system_state == SYSTEM_RUNNING;
> +	int resched = should_resched();
>  	int ret = 0;
>  
>  	if (spin_needbreak(lock) || resched) {
>  		spin_unlock(lock);
> -		if (resched && need_resched())
> +		if (resched)
>  			__cond_resched();
>  		else
>  			cpu_relax();
> @@ -6638,7 +6642,7 @@ int __sched cond_resched_softirq(void)
>  {
>  	BUG_ON(!in_softirq());
>  
> -	if (need_resched() && system_state == SYSTEM_RUNNING) {
> +	if (should_resched()) {
>  		local_bh_enable();
>  		__cond_resched();
>  		local_bh_disable();
> 

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH 1/2] sched: INIT_PREEMPT_COUNT

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Fri, 10 Jul 2009 17:51:06 +0200, Peter Zijlstra said:

> Something along the lines of the below?
> 
> ---
> Index: linux-2.6/include/linux/sched.h
> ===================================================================
> --- linux-2.6.orig/include/linux/sched.h
> +++ linux-2.6/include/linux/sched.h
> @@ -502,7 +502,9 @@ struct task_cputime {
>  
>  /*
>   * Disable preemption until the scheduler is running.
> - * Reset by start_kernel()->sched_init()->init_idle().
> + *
> + * We reset this initial offset of init_thread_info.preempt_count in:
> + *     start_kernel()->sched_init()->init_idle().
>   */
>  #define INIT_PREEMPT_COUNT	(1)

Works for me, thanks...

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 1/2] sched: INIT_PREEMPT_COUNT

[Bjorn Helgaas]

On Friday 10 July 2009 06:57:56 am Peter Zijlstra wrote:
> +#define INIT_PREEMPT_COUNT     (1)

If the parentheses are useful or necessary, I need to be
educated about why.  They look like superfluous paranoia to me.

Re: [PATCH 0/2] cond_resched() optimization

[Anton Vorontsov]

On Fri, Jul 10, 2009 at 02:57:55PM +0200, Peter Zijlstra wrote:
> Matt suggested, whereupon Linus sayeth:
> 
>  "This looks like a good patch. Please make it so"
> 
> Compile and boot tested on x86_64 only.

Tested on ppc32, boots fine.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/2] sched: INIT_PREEMPT_COUNT

[Peter Zijlstra]

On Fri, 2009-07-10 at 15:18 -0600, Bjorn Helgaas wrote:
> On Friday 10 July 2009 06:57:56 am Peter Zijlstra wrote:
> > +#define INIT_PREEMPT_COUNT     (1)
> 
> If the parentheses are useful or necessary, I need to be
> educated about why.  They look like superfluous paranoia to me.

They are strictly speaking superfluous, but since I already knew I'd be
adding a term I added them.

Re: [PATCH 1/2] sched: INIT_PREEMPT_COUNT

[Matthew Wilcox]

On Fri, Jul 10, 2009 at 02:57:56PM +0200, Peter Zijlstra wrote:
> Pull the initial preempt_count value into a single
> definition site.
> 
> Maintainers for: alpha, ia64 and m68k, please have a look,
> your arch code is funny.

Goodness.  I remember sending patches to fix the initial value of
preempt_count on these architectures about four-five years ago.
Guess they were ignored.  It just shows that we need to keep pulling
stuff out of arch/.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

-tip: ACPICA: Do not schedule during early init

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> Matt suggested, whereupon Linus sayeth:
> 
>  "This looks like a good patch. Please make it so"
> 
> Compile and boot tested on x86_64 only.

I wanted to wait with his for .32, but Linus applied it yesterday so 
lets hope it's all fine on all architectures ;-)

There's one new test failure on x86 caused by this commit: a 
scheduling-while-atomic assert during early ACPI init - fixed by the 
patch below. It triggers on two separate test-systems.

Btw., that ACPI_PREEMPTION_POINT() wrapper is both superfluous (we 
should not wrap something as obvious as a cond_resched()) and shows 
signs of problems:

#define ACPI_PREEMPTION_POINT() \
        do { \
                if (!irqs_disabled()) \
                        cond_resched(); \
        } while (0)

that should have been 1) an inline function 2) should not check for 
whether irqs are off. If we need to check for irqs-off like this 
then this is a sign that either the code flow is too unbalanced 
(mixing different things into the same function), or that the 
preemption point has been applied at a too low level.

So a followup cleanup would likely be in order, especially that this 
was the last (and only) call site of ACPI_PREEMPTION_POINT(). I'd 
suggest to remove it.

( I'm not sure what prompted the addition of this rescheduling point 
  - if there was a strong reason for it then we should probably add
  back the preemption point to the place that is causing the
  latency. )

	Ingo

-------------------->
>From 2b2b96115287177600c0158c95e87c5ab44f8379 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sat, 11 Jul 2009 13:15:04 +0200
Subject: [PATCH] ACPICA: Do not schedule during early init

-tip testing found a test failure on x86, a scheduling-while-atomic
bug during early ACPI init:

[    0.048083] ACPI: Core revision 20090521
[    0.051854] BUG: scheduling while atomic: swapper/0/0x10000002
[    0.052076] no locks held by swapper/0.
[    0.053000] Pid: 0, comm: swapper Not tainted 2.6.31-rc2-tip-01240-gd51b6ef-dirty #69901
[    0.053997] Call Trace:
[    0.055006]  [<ffffffff8107afe2>] __schedule_bug+0x80/0x9c
[    0.057001]  [<ffffffff81f1160a>] schedule+0xe6/0x5de
[    0.058000]  [<ffffffff8151531f>] ? acpi_os_release_object+0x1c/0x34
[    0.059002]  [<ffffffff8154d16c>] ? acpi_ut_exit+0x40/0x5c
[    0.060020]  [<ffffffff8107e1dc>] __cond_resched+0x37/0x69
[    0.060998]  [<ffffffff81f11d55>] _cond_resched+0x37/0x56
[    0.061999]  [<ffffffff81546748>] acpi_ps_complete_op+0x412/0x457
[    0.062998]  [<ffffffff8154588d>] ? acpi_ps_next_parse_state+0x14a/0x16b
[    0.064019]  [<ffffffff81546e87>] acpi_ps_parse_loop+0x47e/0x513
[    0.064998]  [<ffffffff81545418>] acpi_ps_parse_aml+0x177/0x4a2
[    0.065998]  [<ffffffff8154a83b>] ? acpi_get_table_by_index+0x138/0x159
[    0.066998]  [<ffffffff81543c0d>] acpi_ns_one_complete_parse+0x1d1/0x220
[    0.068019]  [<ffffffff81543cd7>] acpi_ns_parse_table+0x7b/0x148
[    0.068997]  [<ffffffff8154b229>] ? acpi_tb_allocate_owner_id+0x95/0xb4
[    0.069997]  [<ffffffff8153e62d>] acpi_ns_load_table+0xc5/0x1b8
[    0.070998]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.072018]  [<ffffffff8154aa03>] acpi_tb_load_namespace+0x9d/0x1ba
[    0.072996]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.073996]  [<ffffffff8154ab5a>] acpi_load_tables+0x3a/0x99
[    0.074997]  [<ffffffff828bef2e>] acpi_early_init+0x71/0x11a
[    0.076997]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.077996]  [<ffffffff8288b0eb>] start_kernel+0x39f/0x3c1
[    0.078995]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.080016]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.080995]  [<ffffffff8288a2ce>] x86_64_start_reservations+0xb9/0xd4
[    0.081995]  [<ffffffff8288a000>] ? __init_begin+0x0/0x140
[    0.082995]  [<ffffffff8288a3ed>] x86_64_start_kernel+0x104/0x127
[    0.088045] BUG: scheduling while atomic: swapper/0/0x10000002
[    0.089999] no locks held by swapper/0.
[    0.090993] Pid: 0, comm: swapper Not tainted 2.6.31-rc2-tip-01240-gd51b6ef-dirty #69901

The problem is that drivers/acpi/acpica/psloop.c has this line:

        ACPI_PREEMPTION_POINT();

Which does not mix well with this early init stage - we have 
preemption disabled and the init task has not started up yet, so we 
really should not schedule.

Remove this explicit preemption point.

Cc: Len Brown <len.brown@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/acpi/acpica/psloop.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/psloop.c b/drivers/acpi/acpica/psloop.c
index c5f6ce1..28cd67a 100644
--- a/drivers/acpi/acpica/psloop.c
+++ b/drivers/acpi/acpica/psloop.c
@@ -720,8 +720,6 @@ acpi_ps_complete_op(struct acpi_walk_state *walk_state,
 		*op = NULL;
 	}
 
-	ACPI_PREEMPTION_POINT();
-
 	return_ACPI_STATUS(AE_OK);
 }
 

-tip: ACPICA: Do not schedule during early init

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> Matt suggested, whereupon Linus sayeth:
> 
>  "This looks like a good patch. Please make it so"
> 
> Compile and boot tested on x86_64 only.

I wanted to wait with his for .32, but Linus applied it yesterday so 
lets hope it's all fine on all architectures ;-)

There's one new test failure on x86 caused by this commit: a 
scheduling-while-atomic assert during early ACPI init - fixed by the 
patch below. It triggers on two separate test-systems.

Btw., that ACPI_PREEMPTION_POINT() wrapper is both superfluous (we 
should not wrap something as obvious as a cond_resched()) and shows 
signs of problems:

#define ACPI_PREEMPTION_POINT() \
        do { \
                if (!irqs_disabled()) \
                        cond_resched(); \
        } while (0)

that should have been 1) an inline function 2) should not check for 
whether irqs are off. If we need to check for irqs-off like this 
then this is a sign that either the code flow is too unbalanced 
(mixing different things into the same function), or that the 
preemption point has been applied at a too low level.

So a followup cleanup would likely be in order, especially that this 
was the last (and only) call site of ACPI_PREEMPTION_POINT(). I'd 
suggest to remove it.

( I'm not sure what prompted the addition of this rescheduling point 
  - if there was a strong reason for it then we should probably add
  back the preemption point to the place that is causing the
  latency. )

	Ingo

-------------------->
From 2b2b96115287177600c0158c95e87c5ab44f8379 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sat, 11 Jul 2009 13:15:04 +0200
Subject: [PATCH] ACPICA: Do not schedule during early init

-tip testing found a test failure on x86, a scheduling-while-atomic
bug during early ACPI init:

[    0.048083] ACPI: Core revision 20090521
[    0.051854] BUG: scheduling while atomic: swapper/0/0x10000002
[    0.052076] no locks held by swapper/0.
[    0.053000] Pid: 0, comm: swapper Not tainted 2.6.31-rc2-tip-01240-gd51b6ef-dirty #69901
[    0.053997] Call Trace:
[    0.055006]  [<ffffffff8107afe2>] __schedule_bug+0x80/0x9c
[    0.057001]  [<ffffffff81f1160a>] schedule+0xe6/0x5de
[    0.058000]  [<ffffffff8151531f>] ? acpi_os_release_object+0x1c/0x34
[    0.059002]  [<ffffffff8154d16c>] ? acpi_ut_exit+0x40/0x5c
[    0.060020]  [<ffffffff8107e1dc>] __cond_resched+0x37/0x69
[    0.060998]  [<ffffffff81f11d55>] _cond_resched+0x37/0x56
[    0.061999]  [<ffffffff81546748>] acpi_ps_complete_op+0x412/0x457
[    0.062998]  [<ffffffff8154588d>] ? acpi_ps_next_parse_state+0x14a/0x16b
[    0.064019]  [<ffffffff81546e87>] acpi_ps_parse_loop+0x47e/0x513
[    0.064998]  [<ffffffff81545418>] acpi_ps_parse_aml+0x177/0x4a2
[    0.065998]  [<ffffffff8154a83b>] ? acpi_get_table_by_index+0x138/0x159
[    0.066998]  [<ffffffff81543c0d>] acpi_ns_one_complete_parse+0x1d1/0x220
[    0.068019]  [<ffffffff81543cd7>] acpi_ns_parse_table+0x7b/0x148
[    0.068997]  [<ffffffff8154b229>] ? acpi_tb_allocate_owner_id+0x95/0xb4
[    0.069997]  [<ffffffff8153e62d>] acpi_ns_load_table+0xc5/0x1b8
[    0.070998]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.072018]  [<ffffffff8154aa03>] acpi_tb_load_namespace+0x9d/0x1ba
[    0.072996]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.073996]  [<ffffffff8154ab5a>] acpi_load_tables+0x3a/0x99
[    0.074997]  [<ffffffff828bef2e>] acpi_early_init+0x71/0x11a
[    0.076997]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.077996]  [<ffffffff8288b0eb>] start_kernel+0x39f/0x3c1
[    0.078995]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.080016]  [<ffffffff8288a140>] ? early_idt_handler+0x0/0x71
[    0.080995]  [<ffffffff8288a2ce>] x86_64_start_reservations+0xb9/0xd4
[    0.081995]  [<ffffffff8288a000>] ? __init_begin+0x0/0x140
[    0.082995]  [<ffffffff8288a3ed>] x86_64_start_kernel+0x104/0x127
[    0.088045] BUG: scheduling while atomic: swapper/0/0x10000002
[    0.089999] no locks held by swapper/0.
[    0.090993] Pid: 0, comm: swapper Not tainted 2.6.31-rc2-tip-01240-gd51b6ef-dirty #69901

The problem is that drivers/acpi/acpica/psloop.c has this line:

        ACPI_PREEMPTION_POINT();

Which does not mix well with this early init stage - we have 
preemption disabled and the init task has not started up yet, so we 
really should not schedule.

Remove this explicit preemption point.

Cc: Len Brown <len.brown@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/acpi/acpica/psloop.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/psloop.c b/drivers/acpi/acpica/psloop.c
index c5f6ce1..28cd67a 100644
--- a/drivers/acpi/acpica/psloop.c
+++ b/drivers/acpi/acpica/psloop.c
@@ -720,8 +720,6 @@ acpi_ps_complete_op(struct acpi_walk_state *walk_state,
 		*op = NULL;
 	}
 
-	ACPI_PREEMPTION_POINT();
-
 	return_ACPI_STATUS(AE_OK);
 }