Re: CFS related question

[Cyrill Gorcunov <gorcunov@gmail.com>]

[Hiroshi Shimamoto - Mon, Oct 20, 2008 at 11:12:36AM -0700]
| Peter Zijlstra wrote:
| > On Sun, 2008-10-19 at 00:03 +0400, Cyrill Gorcunov wrote:
| >> Hi Ingo, Peter,
| >>
| >> I just curious, look we have the following
| >>
| >> static struct sched_entity *pick_next_entity(struct cfs_rq *cfs_rq)
| >> {
| >> 	struct sched_entity *se = NULL;
| >>
| >> 	if (first_fair(cfs_rq)) {
| >> 		se = __pick_next_entity(cfs_rq);
| >> 		se = pick_next(cfs_rq, se);
| >> 		set_next_entity(cfs_rq, se);
| >> 	}
| >>
| >> 	return se;
| >> }
| >>
| >> which I presume may return NULL so the following piece
| >> could fail
| >>
| >> static struct task_struct *pick_next_task_fair(struct rq *rq)
| >> {
| >> 	struct task_struct *p;
| >> 	struct cfs_rq *cfs_rq = &rq->cfs;
| >> 	struct sched_entity *se;
| >>
| >> 	if (unlikely(!cfs_rq->nr_running))
| >> 		return NULL;
| >>
| >> 	do {
| >> -->		se = pick_next_entity(cfs_rq);
| >> --> OOPs	cfs_rq = group_cfs_rq(se);
| >> 	} while (cfs_rq);
| >>
| >> 	p = task_of(se);
| >> 	hrtick_start_fair(rq, p);
| >>
| >> 	return p;
| >> }
| >>
| >> Did I miss something? Or it comepletely can NOT happen?
| > 
| > pick_next_entity() only returns NULL when !first_fair(), which is when !
| > nr_running.
| > 
| > So the initial !nr_running check in pick_next_task_fair() will catch
| > that. Further nested RQs will never have !nr_running because then they
| > get dequeued.
| 
| Hi Peter,
| 
| pick_next_entity() is used in pick_next_task_fair() only.
| So, checking first_fair() never fail, and if fails it means bug. Right?
| 
| How about the below patch?
| --------
| From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
| Subject: [PATCH] sched: replace check with BUG_ON in pick_next_entity()
| 
| BUG_ON instead of returning NULL in pick_next_entity() when !first_fair().
| Basically first_fair() is always true, and returning NULL will cause oops later.
| 
| Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
| ---
|  kernel/sched_fair.c |   12 ++++++------
|  1 files changed, 6 insertions(+), 6 deletions(-)
| 
| diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
| index 9573c33..3ce7c25 100644
| --- a/kernel/sched_fair.c
| +++ b/kernel/sched_fair.c
| @@ -758,13 +758,13 @@ pick_next(struct cfs_rq *cfs_rq, struct sched_entity *se)
|  
|  static struct sched_entity *pick_next_entity(struct cfs_rq *cfs_rq)
|  {
| -	struct sched_entity *se = NULL;
| +	struct sched_entity *se;
|  
| -	if (first_fair(cfs_rq)) {
| -		se = __pick_next_entity(cfs_rq);
| -		se = pick_next(cfs_rq, se);
| -		set_next_entity(cfs_rq, se);
| -	}
| +	BUG_ON(!first_fair(cfs_rq));
| +
| +	se = __pick_next_entity(cfs_rq);
| +	se = pick_next(cfs_rq, se);
| +	set_next_entity(cfs_rq, se);
|  
|  	return se;
|  }
| -- 
| 1.5.6
|

Technically it should not happen (as Peter explained). But it
seems it happens sometime -- and most probably 'cause of error
in another part of kernel (ie indirect error). So if Peter would
not object against -- I would really like to better have BUG_ON
check there :) Peter?

	http://lkml.org/lkml/2008/8/3/269
	http://lists.openwall.net/linux-kernel/2008/05/14/130
 
		- Cyrill -