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

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: 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 ;-)

[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: [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())

[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())

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