On 03.09.2015 13:14, Vladimir Davydov wrote: > On Thu, Sep 03, 2015 at 11:56:57AM +0300, Kirill Tkhai wrote: > ... >>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>> index 167d0f66387b..49a93569b5e6 100644 >>> --- a/kernel/sched/fair.c >>> +++ b/kernel/sched/fair.c >>> @@ -965,7 +965,8 @@ static inline void update_entity_boost(struct >>> sched_entity *se) >>> p->woken_while_running; >>> p->woken_while_running = 0; >>> } else >>> - se->boosted = sched_feat(BOOST_PREEMPT); >>> + se->boosted = sched_feat(BOOST_PREEMPT) && >>> + !p->may_throttle; >> >> This function is unsafe, because there is case where put_prev_task() is made >> before dequeue_task() in scheduler. This lead to we clear boosted flag and >> leave >> se linked in boosted list, while it mustn't be there. > > AFAICS the only such a place is cpu offline handling, so this is an > extremely rare case. Anyway, it's a separate issue, which has to be > fixed separately.
Yeah, will wait. >> >> I suggest to unconditionally dequeue bootsted entities, where you do that, >> without checking for entity_boosted(). Of course, you need to use >> list_del_init() >> instead of plain list_del(). In this case you will be able to change >> se->boosted >> flag like you want in this function. > > I'd prefer to just skip updating ->boosted flag when p != current. In > fact, when this code was initially written I simply overlooked the fact > that update_entity_boost may be called from another task's context (like > from setprio). Doing this is completely wrong, because checking > preempt_count or altering a bit field on task_struct must only be done > from the current context. > > Here it goes: > --- > From: Vladimir Davydov <[email protected]> > Date: Thu, 3 Sep 2015 12:46:53 +0300 > Subject: [PATCH] sched/fair: only update current task boost > > put_prev_task, which calls update_entity_boost, may be invoked for prev > != current (e.g. on setprio). In rare cases (e.g. cpu offline), this is > done before dequeue_task. As a result, a task might not get removed from > the list of boosted entity on dequeue, leading to a crash. > > Anyway, it only makes sense to call update_entity_boost on current task, > because it relies on preempt_count and flips task_struct bit fields. So > insert the appropriate check to this function in order to fix this > issue. > > Reported-by: Kirill Tkhai <[email protected]> > Signed-off-by: Vladimir Davydov <[email protected]> Acked-by: Kirill Tkhai <[email protected]> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 49a93569b5e6..f7e8ba7df643 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -960,6 +960,9 @@ static inline void update_entity_boost(struct > sched_entity *se) > else { > struct task_struct *p = task_of(se); > > + if (unlikely(p != current)) > + return; > + > if (!(preempt_count() & PREEMPT_ACTIVE)) { > se->boosted = sched_feat(BOOST_WAKEUPS) && > p->woken_while_running; > _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
