[PATCH 1/3] Reduce get_current() to the asm-generic implementation where possible

[PATCH 1/3] Reduce get_current() to the asm-generic implementation where possible

[David Howells]

Reduce get_current() to the asm-generic implementation where possible to remove
duplicate cases.

Note that this will lose the const attribute on get_current() for ARM and
MN10300.  This will be added back in a later patch for all architectures.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/alpha/include/asm/current.h   |   10 +---------
 arch/arm/include/asm/current.h     |   16 +---------------
 arch/avr32/include/asm/current.h   |   16 +---------------
 arch/cris/include/asm/current.h    |   16 +---------------
 arch/h8300/include/asm/current.h   |   26 +-------------------------
 arch/m32r/include/asm/current.h    |   16 +---------------
 arch/m68k/include/asm/current.h    |   13 ++-----------
 arch/mn10300/include/asm/current.h |    8 +-------
 arch/parisc/include/asm/current.h  |   16 +---------------
 arch/sparc/include/asm/current.h   |    7 +------
 arch/xtensa/include/asm/current.h  |   13 +------------
 11 files changed, 12 insertions(+), 145 deletions(-)

diff --git a/arch/alpha/include/asm/current.h b/arch/alpha/include/asm/current.h
index 094d285..4c51401 100644
--- a/arch/alpha/include/asm/current.h
+++ b/arch/alpha/include/asm/current.h
@@ -1,9 +1 @@
-#ifndef _ALPHA_CURRENT_H
-#define _ALPHA_CURRENT_H
-
-#include <linux/thread_info.h>
-
-#define get_current()	(current_thread_info()->task)
-#define current		get_current()
-
-#endif /* _ALPHA_CURRENT_H */
+#include <asm-generic/current.h>
diff --git a/arch/arm/include/asm/current.h b/arch/arm/include/asm/current.h
index 75d21e2..4c51401 100644
--- a/arch/arm/include/asm/current.h
+++ b/arch/arm/include/asm/current.h
@@ -1,15 +1 @@
-#ifndef _ASMARM_CURRENT_H
-#define _ASMARM_CURRENT_H
-
-#include <linux/thread_info.h>
-
-static inline struct task_struct *get_current(void) __attribute_const__;
-
-static inline struct task_struct *get_current(void)
-{
-	return current_thread_info()->task;
-}
-
-#define current (get_current())
-
-#endif /* _ASMARM_CURRENT_H */
+#include <asm-generic/current.h>
diff --git a/arch/avr32/include/asm/current.h b/arch/avr32/include/asm/current.h
index c7b0549..4c51401 100644
--- a/arch/avr32/include/asm/current.h
+++ b/arch/avr32/include/asm/current.h
@@ -1,15 +1 @@
-#ifndef __ASM_AVR32_CURRENT_H
-#define __ASM_AVR32_CURRENT_H
-
-#include <linux/thread_info.h>
-
-struct task_struct;
-
-inline static struct task_struct * get_current(void)
-{
-	return current_thread_info()->task;
-}
-
-#define current get_current()
-
-#endif /* __ASM_AVR32_CURRENT_H */
+#include <asm-generic/current.h>
diff --git a/arch/cris/include/asm/current.h b/arch/cris/include/asm/current.h
index 5f5c0ef..4c51401 100644
--- a/arch/cris/include/asm/current.h
+++ b/arch/cris/include/asm/current.h
@@ -1,15 +1 @@
-#ifndef _CRIS_CURRENT_H
-#define _CRIS_CURRENT_H
-
-#include <linux/thread_info.h>
-
-struct task_struct;
-
-static inline struct task_struct * get_current(void)
-{
-        return current_thread_info()->task;
-}
- 
-#define current get_current()
-
-#endif /* !(_CRIS_CURRENT_H) */
+#include <asm-generic/current.h>
diff --git a/arch/h8300/include/asm/current.h b/arch/h8300/include/asm/current.h
index 57d74ee..4c51401 100644
--- a/arch/h8300/include/asm/current.h
+++ b/arch/h8300/include/asm/current.h
@@ -1,25 +1 @@
-#ifndef _H8300_CURRENT_H
-#define _H8300_CURRENT_H
-/*
- *	current.h
- *	(C) Copyright 2000, Lineo, David McCullough <davidm@lineo.com>
- *	(C) Copyright 2002, Greg Ungerer (gerg@snapgear.com)
- *
- *	rather than dedicate a register (as the m68k source does), we
- *	just keep a global,  we should probably just change it all to be
- *	current and lose _current_task.
- */
-
-#include <linux/thread_info.h>
-#include <asm/thread_info.h>
-
-struct task_struct;
-
-static inline struct task_struct *get_current(void)
-{
-	return(current_thread_info()->task);
-}
-
-#define	current	get_current()
-
-#endif /* _H8300_CURRENT_H */
+#include <asm-generic/current.h>
diff --git a/arch/m32r/include/asm/current.h b/arch/m32r/include/asm/current.h
index 7859d86..4c51401 100644
--- a/arch/m32r/include/asm/current.h
+++ b/arch/m32r/include/asm/current.h
@@ -1,15 +1 @@
-#ifndef _ASM_M32R_CURRENT_H
-#define _ASM_M32R_CURRENT_H
-
-#include <linux/thread_info.h>
-
-struct task_struct;
-
-static __inline__ struct task_struct *get_current(void)
-{
-	return current_thread_info()->task;
-}
-
-#define current	(get_current())
-
-#endif	/* _ASM_M32R_CURRENT_H */
+#include <asm-generic/current.h>
diff --git a/arch/m68k/include/asm/current.h b/arch/m68k/include/asm/current.h
index 91fcc53..e279667 100644
--- a/arch/m68k/include/asm/current.h
+++ b/arch/m68k/include/asm/current.h
@@ -12,17 +12,8 @@ register struct task_struct *current __asm__("%a2");
  *	just keep a global,  we should probably just change it all to be
  *	current and lose _current_task.
  */
-#include <linux/thread_info.h>
+#include <asm-generic/current.h>
 
-struct task_struct;
-
-static inline struct task_struct *get_current(void)
-{
-	return(current_thread_info()->task);
-}
-
-#define	current	get_current()
-
-#endif /* CONFNIG_MMU */
+#endif /* CONFIG_MMU */
 
 #endif /* !(_M68K_CURRENT_H) */
diff --git a/arch/mn10300/include/asm/current.h b/arch/mn10300/include/asm/current.h
index ca6027d..c1ee370 100644
--- a/arch/mn10300/include/asm/current.h
+++ b/arch/mn10300/include/asm/current.h
@@ -25,13 +25,7 @@ register struct task_struct *const current asm("e2") __attribute__((used));
 extern struct task_struct *__current;
 
 #else
-static inline __attribute__((const))
-struct task_struct *get_current(void)
-{
-	return current_thread_info()->task;
-}
-
-#define current get_current()
+#include <asm-generic/current.h>
 #endif
 
 #endif /* _ASM_CURRENT_H */
diff --git a/arch/parisc/include/asm/current.h b/arch/parisc/include/asm/current.h
index 0fb9338..4c51401 100644
--- a/arch/parisc/include/asm/current.h
+++ b/arch/parisc/include/asm/current.h
@@ -1,15 +1 @@
-#ifndef _PARISC_CURRENT_H
-#define _PARISC_CURRENT_H
-
-#include <linux/thread_info.h>
-
-struct task_struct;
-
-static inline struct task_struct * get_current(void)
-{
-	return current_thread_info()->task;
-}
- 
-#define current get_current()
-
-#endif /* !(_PARISC_CURRENT_H) */
+#include <asm-generic/current.h>
diff --git a/arch/sparc/include/asm/current.h b/arch/sparc/include/asm/current.h
index 10a0df5..f04fd6c 100644
--- a/arch/sparc/include/asm/current.h
+++ b/arch/sparc/include/asm/current.h
@@ -23,12 +23,7 @@ register struct task_struct *current asm("g4");
  * Two stage process (inline + #define) for type-checking.
  * We also obfuscate get_current() to check if anyone used that by mistake.
  */
-struct task_struct;
-static inline struct task_struct *__get_current(void)
-{
-	return current_thread_info()->task;
-}
-#define current __get_current()
+#include <asm-generic/current.h>
 #endif
 
 #endif /* !(_SPARC_CURRENT_H) */
diff --git a/arch/xtensa/include/asm/current.h b/arch/xtensa/include/asm/current.h
index 8d1eb5d..c3398ce 100644
--- a/arch/xtensa/include/asm/current.h
+++ b/arch/xtensa/include/asm/current.h
@@ -12,18 +12,7 @@
 #define _XTENSA_CURRENT_H
 
 #ifndef __ASSEMBLY__
-
-#include <linux/thread_info.h>
-
-struct task_struct;
-
-static inline struct task_struct *get_current(void)
-{
-	return current_thread_info()->task;
-}
-
-#define current get_current()
-
+#include <asm-generic/current.h>
 #else
 
 #define CURRENT_SHIFT 13

[PATCH 2/3] Mark the 'current' pointer register read-only when such a thing exists

[David Howells]

Where the value of current is kept in a register, that register can be marked
such that the pointer value is read only, e.g.:

	register struct task_struct *const current asm("e2");

This prevents inadvertent assignment outside of assembly code.

This has been available on the MN10300 arch for a while now, and I've also
tested it on the FRV arch.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/frv/include/asm/current.h        |    2 +-
 arch/microblaze/include/asm/current.h |    2 +-
 arch/powerpc/include/asm/current.h    |    2 +-
 arch/sparc/include/asm/current.h      |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/frv/include/asm/current.h b/arch/frv/include/asm/current.h
index 86b0274..51bdaf2 100644
--- a/arch/frv/include/asm/current.h
+++ b/arch/frv/include/asm/current.h
@@ -17,7 +17,7 @@
 /*
  * dedicate GR29 to keeping the current task pointer
  */
-register struct task_struct *current asm("gr29");
+register struct task_struct *const current asm("gr29");
 
 #define get_current() current
 
diff --git a/arch/microblaze/include/asm/current.h b/arch/microblaze/include/asm/current.h
index 29303ed..b824847 100644
--- a/arch/microblaze/include/asm/current.h
+++ b/arch/microblaze/include/asm/current.h
@@ -21,7 +21,7 @@
 /*
  * Dedicate r31 to keeping the current task pointer
  */
-register struct task_struct *current asm("r31");
+register struct task_struct *const current asm("r31");
 
 # define get_current()	current
 # endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/include/asm/current.h b/arch/powerpc/include/asm/current.h
index e2c7f06..4ca5126 100644
--- a/arch/powerpc/include/asm/current.h
+++ b/arch/powerpc/include/asm/current.h
@@ -32,7 +32,7 @@ static inline struct task_struct *get_current(void)
 /*
  * We keep `current' in r2 for speed.
  */
-register struct task_struct *current asm ("r2");
+register struct task_struct *const current asm ("r2");
 
 #endif
 
diff --git a/arch/sparc/include/asm/current.h b/arch/sparc/include/asm/current.h
index f04fd6c..9ae8826 100644
--- a/arch/sparc/include/asm/current.h
+++ b/arch/sparc/include/asm/current.h
@@ -14,7 +14,7 @@
 #include <linux/thread_info.h>
 
 #ifdef CONFIG_SPARC64
-register struct task_struct *current asm("g4");
+register struct task_struct *const current asm("g4");
 #endif
 
 #ifdef CONFIG_SPARC32

[PATCH 3/3] Make get_current() __attribute__((const))

[David Howells]

get_current() can be made __attribute__((const)) as it's not allowed to change
under you, except in switch_to() (it's fine to violate the constness there
since this switches stacks too, and will almost certainly do this in assembly
code).  This allows the compiler more choices about when it can cache this
pointer.

This doesn't break cached copies of current, whether they're cached by gcc in
registers or on the stack.  switch_to() will save all registers on the stack
before actually switching, then when it switches current, it will also switch
the stack and then pop back what was stored in the 'unclobbered' registers for
the now active task and stack.  Thus the copies of current that were cached
just work.

Note that does not affect all arches under all circumstances.  Some arches, at
least some of the time, keep current in a register, and some use thread_info as
the source of current.

This has been the norm on ARM and MN10300 arches (the latter only when not
using E2 to hold current) for a while now.  I've also tested this on x86_64.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/powerpc/include/asm/current.h |    3 ++-
 arch/x86/include/asm/current.h     |    3 ++-
 include/asm-generic/current.h      |    9 ++++++++-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/current.h b/arch/powerpc/include/asm/current.h
index 4ca5126..affcbbe 100644
--- a/arch/powerpc/include/asm/current.h
+++ b/arch/powerpc/include/asm/current.h
@@ -15,7 +15,8 @@ struct task_struct;
 #include <linux/stddef.h>
 #include <asm/paca.h>
 
-static inline struct task_struct *get_current(void)
+static inline __attribute_const__
+struct task_struct *get_current(void)
 {
 	struct task_struct *task;
 
diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h
index 4d447b7..defb13c 100644
--- a/arch/x86/include/asm/current.h
+++ b/arch/x86/include/asm/current.h
@@ -9,7 +9,8 @@ struct task_struct;
 
 DECLARE_PER_CPU(struct task_struct *, current_task);
 
-static __always_inline struct task_struct *get_current(void)
+static __always_inline __attribute_const__
+struct task_struct *get_current(void)
 {
 	return percpu_read_stable(current_task);
 }
diff --git a/include/asm-generic/current.h b/include/asm-generic/current.h
index 5e86f6a..8deb67e 100644
--- a/include/asm-generic/current.h
+++ b/include/asm-generic/current.h
@@ -3,7 +3,14 @@
 
 #include <linux/thread_info.h>
 
-#define get_current() (current_thread_info()->task)
+struct task_struct;
+
+static inline __attribute_const__
+struct task_struct *get_current(void)
+{
+	return current_thread_info()->task;
+}
+
 #define current get_current()
 
 #endif /* __ASM_GENERIC_CURRENT_H */

Re: [PATCH 1/3] Reduce get_current() to the asm-generic implementation where possible

[Kyle McMartin]

On Tue, May 18, 2010 at 05:45:37PM +0100, David Howells wrote:
> Reduce get_current() to the asm-generic implementation where possible to remove
> duplicate cases.
> 
> Note that this will lose the const attribute on get_current() for ARM and
> MN10300.  This will be added back in a later patch for all architectures.
> 

Acked-by: Kyle McMartin <kyle@mcmartin.ca>

> diff --git a/arch/parisc/include/asm/current.h b/arch/parisc/include/asm/current.h
> index 0fb9338..4c51401 100644
> --- a/arch/parisc/include/asm/current.h
> +++ b/arch/parisc/include/asm/current.h
> @@ -1,15 +1 @@
> -#ifndef _PARISC_CURRENT_H
> -#define _PARISC_CURRENT_H
> -
> -#include <linux/thread_info.h>
> -
> -struct task_struct;
> -
> -static inline struct task_struct * get_current(void)
> -{
> -	return current_thread_info()->task;
> -}
> - 
> -#define current get_current()
> -
> -#endif /* !(_PARISC_CURRENT_H) */
> +#include <asm-generic/current.h>

Re: [PATCH 1/3] Reduce get_current() to the asm-generic implementation where possible

[Jamie Lokier]

David Howells wrote:
> Reduce get_current() to the asm-generic implementation where
> possible to remove duplicate cases.
> 
> Note that this will lose the const attribute on get_current() for ARM and
> MN10300.  This will be added back in a later patch for all architectures.
[...]
> -static inline struct task_struct *get_current(void) __attribute_const__;
> -
> -static inline struct task_struct *get_current(void)
> -{
> -	return current_thread_info()->task;
> -}


Last time I looked, GCC didn't seem to do anything useful with the
const attribute on inline functions.  I.e. no elimination of duplicate calls.

(It does eliminate them when out-of-line.)

So there is probably no point putting the const attribute back, unless
GCC has changed at this.

It might be able to eliminate some duplicates if the asm inside
current_thread_info() is consty enough, and duplicate ->task
dereferences might be eliminated by strict-aliasing in some cases.

-- Jamie

Re: [PATCH 2/3] Mark the 'current' pointer register read-only when such a thing exists

[David Miller]

From: David Howells <dhowells@redhat.com>
Date: Tue, 18 May 2010 17:45:42 +0100

> Where the value of current is kept in a register, that register can be marked
> such that the pointer value is read only, e.g.:
> 
> 	register struct task_struct *const current asm("e2");
> 
> This prevents inadvertent assignment outside of assembly code.
> 
> This has been available on the MN10300 arch for a while now, and I've also
> tested it on the FRV arch.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))

[Oleg Nesterov]

On 05/18, David Howells wrote:
>
> This doesn't break cached copies of current, whether they're cached by gcc in
> registers or on the stack.  switch_to() will save all registers on the stack
> before actually switching, then when it switches current, it will also switch
> the stack and then pop back what was stored in the 'unclobbered' registers for
> the now active task and stack.  Thus the copies of current that were cached
> just work.
>
> [...snip...]
>
> --- a/include/asm-generic/current.h
> +++ b/include/asm-generic/current.h
> @@ -3,7 +3,14 @@
>
>  #include <linux/thread_info.h>
>
> -#define get_current() (current_thread_info()->task)
> +struct task_struct;
> +
> +static inline __attribute_const__
> +struct task_struct *get_current(void)
> +{
> +	return current_thread_info()->task;
> +}

Can't ack this patch, but it looks correct to me.


And, looking at this patch I think that schedule() can be simplified
a little bit.

"sched: Reassign prev and switch_count when reacquire_kernel_lock() fail"
commit 6d558c3ac9b6508d26fd5cadccce51fc9d726b1c says:

	Assume A->B schedule is processing,
	...
	Then on B's context,
	...
	prev and switch_count are related to A

How so? switch_count - yes, we should change it. But prev must be
equal to B, and it must be equal to current. When we return from
switch_to() registers/stack should be restored correctly, so we
can do

	--- x/kernel/sched.c
	+++ x/kernel/sched.c
	@@ -3729,8 +3729,7 @@ need_resched_nonpreemptible:
	 
		post_schedule(rq);
	 
	-	if (unlikely(reacquire_kernel_lock(current) < 0)) {
	-		prev = rq->curr;
	+	if (unlikely(reacquire_kernel_lock(prev) < 0)) {
			switch_count = &prev->nivcsw;
			goto need_resched_nonpreemptible;
		}

and in fact we can simplify this even more, no need to reassign
switch_count, we can just move the initial assignment down, under
"need_resched_nonpreemptible" label.

switch_to(prev, next, prev) changes "prev" inside context_switch()
but it should be stable inside of schedule().

Oleg.

--- x/kernel/sched.c
+++ x/kernel/sched.c
@@ -3708,7 +3708,6 @@ need_resched:
 	rq = cpu_rq(cpu);
 	rcu_sched_qs(cpu);
 	prev = rq->curr;
-	switch_count = &prev->nivcsw;
 
 	release_kernel_lock(prev);
 need_resched_nonpreemptible:
@@ -3722,6 +3721,7 @@ need_resched_nonpreemptible:
 	update_rq_clock(rq);
 	clear_tsk_need_resched(prev);
 
+	switch_count = &prev->nivcsw;
 	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
 		if (unlikely(signal_pending_state(prev->state, prev)))
 			prev->state = TASK_RUNNING;
@@ -3758,11 +3758,8 @@ need_resched_nonpreemptible:
 
 	post_schedule(rq);
 
-	if (unlikely(reacquire_kernel_lock(current) < 0)) {
-		prev = rq->curr;
-		switch_count = &prev->nivcsw;
+	if (unlikely(reacquire_kernel_lock(prev))
 		goto need_resched_nonpreemptible;
-	}
 
 	preempt_enable_no_resched();
 	if (need_resched())

Re: [PATCH 1/3] Reduce get_current() to the asm-generic implementation where possible

[Nick Piggin]

On Tue, May 18, 2010 at 08:47:53PM +0100, Jamie Lokier wrote:
> David Howells wrote:
> > Reduce get_current() to the asm-generic implementation where
> > possible to remove duplicate cases.
> > 
> > Note that this will lose the const attribute on get_current() for ARM and
> > MN10300.  This will be added back in a later patch for all architectures.
> [...]
> > -static inline struct task_struct *get_current(void) __attribute_const__;
> > -
> > -static inline struct task_struct *get_current(void)
> > -{
> > -	return current_thread_info()->task;
> > -}
> 
> 
> Last time I looked, GCC didn't seem to do anything useful with the
> const attribute on inline functions.  I.e. no elimination of duplicate calls.

Probably child functions should be marked const as well.

 
> (It does eliminate them when out-of-line.)
> 
> So there is probably no point putting the const attribute back, unless
> GCC has changed at this.
> 
> It might be able to eliminate some duplicates if the asm inside
> current_thread_info() is consty enough, and duplicate ->task
> dereferences might be eliminated by strict-aliasing in some cases.

I don't think it hurts to give more information even if the compiler
does not use it yet.

Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))

[Peter Zijlstra]

On Tue, 2010-05-18 at 23:22 +0200, Oleg Nesterov wrote:

> And, looking at this patch I think that schedule() can be simplified
> a little bit.
> 
> "sched: Reassign prev and switch_count when reacquire_kernel_lock() fail"
> commit 6d558c3ac9b6508d26fd5cadccce51fc9d726b1c says:
> 
> 	Assume A->B schedule is processing,
> 	...
> 	Then on B's context,
> 	...
> 	prev and switch_count are related to A
> 
> How so? switch_count - yes, we should change it. But prev must be
> equal to B, and it must be equal to current. When we return from
> switch_to() registers/stack should be restored correctly, so we
> can do

What if schedule() got called at a different stack depth than we are
now?

I don't think we can assume anything about the stack context we just
switched to.

Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))

[Oleg Nesterov]

On 05/19, Peter Zijlstra wrote:
>
> On Tue, 2010-05-18 at 23:22 +0200, Oleg Nesterov wrote:
>
> > And, looking at this patch I think that schedule() can be simplified
> > a little bit.
> >
> > "sched: Reassign prev and switch_count when reacquire_kernel_lock() fail"
> > commit 6d558c3ac9b6508d26fd5cadccce51fc9d726b1c says:
> >
> > 	Assume A->B schedule is processing,
> > 	...
> > 	Then on B's context,
> > 	...
> > 	prev and switch_count are related to A
> >
> > How so? switch_count - yes, we should change it. But prev must be
> > equal to B, and it must be equal to current. When we return from
> > switch_to() registers/stack should be restored correctly, so we
> > can do
>
> What if schedule() got called at a different stack depth than we are
> now?
>
> I don't think we can assume anything about the stack context we just
> switched to.

Not sure I understand...

OK. Firstly, we shouldn't worry about the freshly forked tasks, they
never "return" from switch_to() but call ret_from_fork()->schedule_tail(),
right?

Now suppose that A calls schedule() and we switch to B. When switch_to()
returns on B's context, this context (register/stack) matches the previous
context which was used by B when it in turn called schedule(), correct?

IOW. B calls schedule, prev == B. schedule() picks another task, prev
is saved on B's stck after switch_to(). A calls schedule(), prev == A
before context_switch(A, B), but after that switch_to() switches to
B's stack and prev == B.

No?


I am looking into the git history now... and I guess I understand why
reacquire_kernel_lock() uses current. Because schedule() did something
like

	prev = context_switch(prev, next);	// prev == last

	finish_task_switch(prev);

	reacquire_kernel_lock(current);		// prev != current

Oleg.

Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))

[Peter Zijlstra]

On Wed, 2010-05-19 at 12:27 +0200, Oleg Nesterov wrote:

> Now suppose that A calls schedule() and we switch to B. When switch_to()
> returns on B's context, this context (register/stack) matches the previous
> context which was used by B when it in turn called schedule(), correct?

Ah, right, you always schedule back to where you came from,... I
shouldn't try and write emails before the morning juice hits ;-)

Re: [PATCH 1/3] Reduce get_current() to the asm-generic implementation where possible

[Oleg Nesterov]

On 05/19, Nick Piggin wrote:
>
> On Tue, May 18, 2010 at 08:47:53PM +0100, Jamie Lokier wrote:
> >
> > So there is probably no point putting the const attribute back, unless
> > GCC has changed at this.
> >
> > It might be able to eliminate some duplicates if the asm inside
> > current_thread_info() is consty enough, and duplicate ->task
> > dereferences might be eliminated by strict-aliasing in some cases.
>
> I don't think it hurts to give more information even if the compiler
> does not use it yet.

Completely agreed.

Personally, I think this patch makes sense even as documentation. And
hopefully some time later gcc will be improved.

Oleg.

[PATCH] schedule: simplify the reacquire_kernel_lock() logic

[Oleg Nesterov]

- Contrary to what 6d558c3a says, there is no need to reload
  prev = rq->curr after the context switch. You always schedule
  back to where you came from, prev must be equal to current
  even if cpu/rq was changed.

- This also means reacquire_kernel_lock() can use prev instead
  of current.

- No need to reassign switch_count if reacquire_kernel_lock()
  reports need_resched(), we can just move the initial assignment
  down, under the "need_resched_nonpreemptible:" label.

- Try to update the comment after context_switch().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/sched.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

--- 34-rc1/kernel/sched.c~SCHEDULE_PREV_EQ_TO_CURRENT	2010-05-18 23:32:50.000000000 +0200
+++ 34-rc1/kernel/sched.c	2010-05-19 14:32:57.000000000 +0200
@@ -3679,7 +3679,6 @@ need_resched:
 	rq = cpu_rq(cpu);
 	rcu_sched_qs(cpu);
 	prev = rq->curr;
-	switch_count = &prev->nivcsw;
 
 	release_kernel_lock(prev);
 need_resched_nonpreemptible:
@@ -3693,6 +3692,7 @@ need_resched_nonpreemptible:
 	update_rq_clock(rq);
 	clear_tsk_need_resched(prev);
 
+	switch_count = &prev->nivcsw;
 	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
 		if (unlikely(signal_pending_state(prev->state, prev)))
 			prev->state = TASK_RUNNING;
@@ -3719,8 +3719,10 @@ need_resched_nonpreemptible:
 
 		context_switch(rq, prev, next); /* unlocks the rq */
 		/*
-		 * the context switch might have flipped the stack from under
-		 * us, hence refresh the local variables.
+		 * The context switch have flipped the stack from under us
+		 * and restored the local variables which were saved when
+		 * this task called schedule() in the past. prev == current
+		 * is still correct, but it can be moved to another cpu/rq.
 		 */
 		cpu = smp_processor_id();
 		rq = cpu_rq(cpu);
@@ -3729,11 +3731,8 @@ need_resched_nonpreemptible:
 
 	post_schedule(rq);
 
-	if (unlikely(reacquire_kernel_lock(current) < 0)) {
-		prev = rq->curr;
-		switch_count = &prev->nivcsw;
+	if (unlikely(reacquire_kernel_lock(prev)))
 		goto need_resched_nonpreemptible;
-	}
 
 	preempt_enable_no_resched();
 	if (need_resched())

Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))

[Yong Zhang]

On Wed, May 19, 2010 at 12:27:43PM +0200, Oleg Nesterov wrote:
> On 05/19, Peter Zijlstra wrote:
> >
> > On Tue, 2010-05-18 at 23:22 +0200, Oleg Nesterov wrote:
> >
> > > And, looking at this patch I think that schedule() can be simplified
> > > a little bit.
> > >
> > > "sched: Reassign prev and switch_count when reacquire_kernel_lock() fail"
> > > commit 6d558c3ac9b6508d26fd5cadccce51fc9d726b1c says:
> > >
> > > 	Assume A->B schedule is processing,
> > > 	...
> > > 	Then on B's context,
> > > 	...
> > > 	prev and switch_count are related to A
> > >
> > > How so? switch_count - yes, we should change it. But prev must be
> > > equal to B, and it must be equal to current. When we return from
> > > switch_to() registers/stack should be restored correctly, so we
> > > can do
> >
> > What if schedule() got called at a different stack depth than we are
> > now?
> >
> > I don't think we can assume anything about the stack context we just
> > switched to.
> 
> Not sure I understand...
> 
> OK. Firstly, we shouldn't worry about the freshly forked tasks, they
> never "return" from switch_to() but call ret_from_fork()->schedule_tail(),
> right?
> 
> Now suppose that A calls schedule() and we switch to B. When switch_to()
> returns on B's context, this context (register/stack) matches the previous
> context which was used by B when it in turn called schedule(), correct?
> 
> IOW. B calls schedule, prev == B. schedule() picks another task, prev
> is saved on B's stck after switch_to(). A calls schedule(), prev == A
> before context_switch(A, B), but after that switch_to() switches to
> B's stack and prev == B.
> 
> No?

I think you are right.

> 
> 
> I am looking into the git history now... and I guess I understand why
> reacquire_kernel_lock() uses current. Because schedule() did something
> like
> 
> 	prev = context_switch(prev, next);	// prev == last
> 
> 	finish_task_switch(prev);
> 
> 	reacquire_kernel_lock(current);		// prev != current

This is what I think when I wrote that patch. Now the task switch is
entirely finished in context_switch(). So commit log in 6d558c3a has
some flaw in it. The "prev" is also a churn.

Thanks,
Yong

Re: [PATCH] schedule: simplify the reacquire_kernel_lock() logic

[Yong Zhang]

On Wed, May 19, 2010 at 02:57:11PM +0200, Oleg Nesterov wrote:
> - Contrary to what 6d558c3a says, there is no need to reload
>   prev = rq->curr after the context switch. You always schedule
>   back to where you came from, prev must be equal to current
>   even if cpu/rq was changed.
> 
> - This also means reacquire_kernel_lock() can use prev instead
>   of current.
> 
> - No need to reassign switch_count if reacquire_kernel_lock()
>   reports need_resched(), we can just move the initial assignment
>   down, under the "need_resched_nonpreemptible:" label.
> 
> - Try to update the comment after context_switch().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

This make it more clear now. Thank you Oleg.

Acked-by: Yong Zhang <yong.zhang0@gmail.com>

> ---
> 
>  kernel/sched.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> --- 34-rc1/kernel/sched.c~SCHEDULE_PREV_EQ_TO_CURRENT	2010-05-18 23:32:50.000000000 +0200
> +++ 34-rc1/kernel/sched.c	2010-05-19 14:32:57.000000000 +0200
> @@ -3679,7 +3679,6 @@ need_resched:
>  	rq = cpu_rq(cpu);
>  	rcu_sched_qs(cpu);
>  	prev = rq->curr;
> -	switch_count = &prev->nivcsw;
>  
>  	release_kernel_lock(prev);
>  need_resched_nonpreemptible:
> @@ -3693,6 +3692,7 @@ need_resched_nonpreemptible:
>  	update_rq_clock(rq);
>  	clear_tsk_need_resched(prev);
>  
> +	switch_count = &prev->nivcsw;
>  	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
>  		if (unlikely(signal_pending_state(prev->state, prev)))
>  			prev->state = TASK_RUNNING;
> @@ -3719,8 +3719,10 @@ need_resched_nonpreemptible:
>  
>  		context_switch(rq, prev, next); /* unlocks the rq */
>  		/*
> -		 * the context switch might have flipped the stack from under
> -		 * us, hence refresh the local variables.
> +		 * The context switch have flipped the stack from under us
> +		 * and restored the local variables which were saved when
> +		 * this task called schedule() in the past. prev == current
> +		 * is still correct, but it can be moved to another cpu/rq.
>  		 */
>  		cpu = smp_processor_id();
>  		rq = cpu_rq(cpu);
> @@ -3729,11 +3731,8 @@ need_resched_nonpreemptible:
>  
>  	post_schedule(rq);
>  
> -	if (unlikely(reacquire_kernel_lock(current) < 0)) {
> -		prev = rq->curr;
> -		switch_count = &prev->nivcsw;
> +	if (unlikely(reacquire_kernel_lock(prev)))
>  		goto need_resched_nonpreemptible;
> -	}
>  
>  	preempt_enable_no_resched();
>  	if (need_resched())

Re: [PATCH 1/3] Reduce get_current() to the asm-generic implementation where possible

[David Howells]

Jamie Lokier <jamie@shareable.org> wrote:

> Last time I looked, GCC didn't seem to do anything useful with the
> const attribute on inline functions.  I.e. no elimination of duplicate calls.

That's something that may change in the future.  It doesn't hurt to have it as
far as I can see.

David

[tip:sched/core] sched: Simplify the reacquire_kernel_lock() logic

[tip-bot for Oleg Nesterov]

Commit-ID:  246d86b51845063e4b06b27579990492dc5fa317
Gitweb:     http://git.kernel.org/tip/246d86b51845063e4b06b27579990492dc5fa317
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Wed, 19 May 2010 14:57:11 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 9 Jun 2010 10:34:50 +0200

sched: Simplify the reacquire_kernel_lock() logic

- Contrary to what 6d558c3a says, there is no need to reload
  prev = rq->curr after the context switch. You always schedule
  back to where you came from, prev must be equal to current
  even if cpu/rq was changed.

- This also means reacquire_kernel_lock() can use prev instead
  of current.

- No need to reassign switch_count if reacquire_kernel_lock()
  reports need_resched(), we can just move the initial assignment
  down, under the "need_resched_nonpreemptible:" label.

- Try to update the comment after context_switch().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20100519125711.GA30199@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 3abd8f7..f37a961 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -3636,7 +3636,6 @@ need_resched:
 	rq = cpu_rq(cpu);
 	rcu_note_context_switch(cpu);
 	prev = rq->curr;
-	switch_count = &prev->nivcsw;
 
 	release_kernel_lock(prev);
 need_resched_nonpreemptible:
@@ -3649,6 +3648,7 @@ need_resched_nonpreemptible:
 	raw_spin_lock_irq(&rq->lock);
 	clear_tsk_need_resched(prev);
 
+	switch_count = &prev->nivcsw;
 	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
 		if (unlikely(signal_pending_state(prev->state, prev))) {
 			prev->state = TASK_RUNNING;
@@ -3689,8 +3689,10 @@ need_resched_nonpreemptible:
 
 		context_switch(rq, prev, next); /* unlocks the rq */
 		/*
-		 * the context switch might have flipped the stack from under
-		 * us, hence refresh the local variables.
+		 * The context switch have flipped the stack from under us
+		 * and restored the local variables which were saved when
+		 * this task called schedule() in the past. prev == current
+		 * is still correct, but it can be moved to another cpu/rq.
 		 */
 		cpu = smp_processor_id();
 		rq = cpu_rq(cpu);
@@ -3699,11 +3701,8 @@ need_resched_nonpreemptible:
 
 	post_schedule(rq);
 
-	if (unlikely(reacquire_kernel_lock(current) < 0)) {
-		prev = rq->curr;
-		switch_count = &prev->nivcsw;
+	if (unlikely(reacquire_kernel_lock(prev)))
 		goto need_resched_nonpreemptible;
-	}
 
 	preempt_enable_no_resched();
 	if (need_resched())