* schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))
[not found] ` <20100518164547.6194.94193.stgit@warthog.procyon.org.uk>
@ 2010-05-18 21:22 ` Oleg Nesterov
2010-05-19 6:21 ` Peter Zijlstra
0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2010-05-18 21:22 UTC (permalink / raw)
To: David Howells, Ingo Molnar, Peter Zijlstra, Yong Zhang
Cc: linux-arch, linux-kernel
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())
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))
2010-05-18 21:22 ` schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const))) Oleg Nesterov
@ 2010-05-19 6:21 ` Peter Zijlstra
2010-05-19 10:27 ` Oleg Nesterov
0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2010-05-19 6:21 UTC (permalink / raw)
To: Oleg Nesterov
Cc: David Howells, Ingo Molnar, Yong Zhang, linux-arch, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))
2010-05-19 6:21 ` Peter Zijlstra
@ 2010-05-19 10:27 ` Oleg Nesterov
2010-05-19 10:37 ` Peter Zijlstra
2010-05-19 13:07 ` schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const))) Yong Zhang
0 siblings, 2 replies; 8+ messages in thread
From: Oleg Nesterov @ 2010-05-19 10:27 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Howells, Ingo Molnar, Yong Zhang, linux-arch, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))
2010-05-19 10:27 ` Oleg Nesterov
@ 2010-05-19 10:37 ` Peter Zijlstra
2010-05-19 12:57 ` [PATCH] schedule: simplify the reacquire_kernel_lock() logic Oleg Nesterov
2010-05-19 13:07 ` schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const))) Yong Zhang
1 sibling, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2010-05-19 10:37 UTC (permalink / raw)
To: Oleg Nesterov
Cc: David Howells, Ingo Molnar, Yong Zhang, linux-arch, linux-kernel
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 ;-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] schedule: simplify the reacquire_kernel_lock() logic
2010-05-19 10:37 ` Peter Zijlstra
@ 2010-05-19 12:57 ` Oleg Nesterov
2010-05-19 13:11 ` Yong Zhang
2010-06-09 10:13 ` [tip:sched/core] sched: Simplify " tip-bot for Oleg Nesterov
0 siblings, 2 replies; 8+ messages in thread
From: Oleg Nesterov @ 2010-05-19 12:57 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Howells, Ingo Molnar, Yong Zhang, linux-arch, linux-kernel
- 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())
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const)))
2010-05-19 10:27 ` Oleg Nesterov
2010-05-19 10:37 ` Peter Zijlstra
@ 2010-05-19 13:07 ` Yong Zhang
1 sibling, 0 replies; 8+ messages in thread
From: Yong Zhang @ 2010-05-19 13:07 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Peter Zijlstra, David Howells, Ingo Molnar, linux-arch, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] schedule: simplify the reacquire_kernel_lock() logic
2010-05-19 12:57 ` [PATCH] schedule: simplify the reacquire_kernel_lock() logic Oleg Nesterov
@ 2010-05-19 13:11 ` Yong Zhang
2010-06-09 10:13 ` [tip:sched/core] sched: Simplify " tip-bot for Oleg Nesterov
1 sibling, 0 replies; 8+ messages in thread
From: Yong Zhang @ 2010-05-19 13:11 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Peter Zijlstra, David Howells, Ingo Molnar, linux-arch, linux-kernel
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())
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip:sched/core] sched: Simplify the reacquire_kernel_lock() logic
2010-05-19 12:57 ` [PATCH] schedule: simplify the reacquire_kernel_lock() logic Oleg Nesterov
2010-05-19 13:11 ` Yong Zhang
@ 2010-06-09 10:13 ` tip-bot for Oleg Nesterov
1 sibling, 0 replies; 8+ messages in thread
From: tip-bot for Oleg Nesterov @ 2010-06-09 10:13 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, a.p.zijlstra, oleg, tglx, mingo
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())
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-06-09 10:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20100518164537.6194.73366.stgit@warthog.procyon.org.uk>
[not found] ` <20100518164547.6194.94193.stgit@warthog.procyon.org.uk>
2010-05-18 21:22 ` schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const))) Oleg Nesterov
2010-05-19 6:21 ` Peter Zijlstra
2010-05-19 10:27 ` Oleg Nesterov
2010-05-19 10:37 ` Peter Zijlstra
2010-05-19 12:57 ` [PATCH] schedule: simplify the reacquire_kernel_lock() logic Oleg Nesterov
2010-05-19 13:11 ` Yong Zhang
2010-06-09 10:13 ` [tip:sched/core] sched: Simplify " tip-bot for Oleg Nesterov
2010-05-19 13:07 ` schedule() && prev/current (Was: [PATCH 3/3] Make get_current() __attribute__((const))) Yong Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).