[Devel] [PATCH 07/17] Port diff-sched-introduce-cond_resched_may_throttle

[Kirill Tkhai]

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 <vdavydov at parallels.com>
> 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 <ktkhai at odin.com>
> Signed-off-by: Vladimir Davydov <vdavydov at parallels.com>

Acked-by: Kirill Tkhai <ktkhai at odin.com>

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