Messages in this thread | | | Subject | Re: [PATCH v2 1/2] sched: schedule_tail() should disable preemption | From | Kirill Tkhai <> | Date | Thu, 09 Oct 2014 01:37:28 +0400 |
| |
В Ср, 08/10/2014 в 21:36 +0200, Oleg Nesterov пишет: > On 10/08, Oleg Nesterov wrote: > > > > Another problem is that finish_task_switch() itself runs with preempt > > enabled after finish_lock_switch(). If nothing else this means that > > ->sched_in() notifier can't trust its "cpu" arg. > > OOPS, this obviously can't happen, ->preempt_notifiers must be empty. > Remove this part from the changelog, please see v2. > > I am not sure about finish_arch_post_lock_switch() ... but probably > it should be fine without preempt_disable. > > ------------------------------------------------------------------------------- > Subject: [PATCH v2 1/2] sched: schedule_tail() should disable preemption > > finish_task_switch() enables preemption, so post_schedule(rq) can be > called on the wrong (and even dead) CPU. Afaics, nothing really bad > can happen, but in this case we can wrongly clear rq->post_schedule > on that CPU. And this simply looks wrong in any case. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > kernel/sched/core.c | 11 +++++------ > 1 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 703c7e6..3f267e8 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2277,15 +2277,14 @@ static inline void post_schedule(struct rq *rq) > asmlinkage __visible void schedule_tail(struct task_struct *prev) > __releases(rq->lock) > { > - struct rq *rq = this_rq(); > + struct rq *rq; > > + /* finish_task_switch() drops rq->lock and enables preemtion */ > + preempt_disable();
Maybe, the code would look simpler if we change init_task_preempt_count() and create new tasks with preempt_count() == 2, so this preempt_disable() won't be necessary. But it's more or less subjectively.
Reviewed-by: Kirill Tkhai <ktkhai@parallels.com>
> + rq = this_rq(); > finish_task_switch(rq, prev); > - > - /* > - * FIXME: do we need to worry about rq being invalidated by the > - * task_switch? > - */ > post_schedule(rq); > + preempt_enable(); > > if (current->set_child_tid) > put_user(task_pid_vnr(current), current->set_child_tid);
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |