Messages in this thread | | | Date | Wed, 20 Aug 2014 09:52:38 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH v4 2/6] sched: Wrapper for checking task_struct::on_rq |
| |
* Kirill Tkhai <ktkhai@parallels.com> wrote:
> > Implement task_queued() and use it everywhere instead of on_rq check. > No functional changes. > > The only exception is we do not use the wrapper in check_for_tasks(), > because it requires to export task_queued() in global header files. > Next patch in series would return it back, so it doesn't matter. > > Signed-off-by: Kirill Tkhai <ktkhai@parallels.com> > --- > kernel/sched/core.c | 82 +++++++++++++++++++++++----------------------- > kernel/sched/deadline.c | 14 ++++---- > kernel/sched/fair.c | 22 ++++++------ > kernel/sched/rt.c | 16 ++++----- > kernel/sched/sched.h | 7 ++++ > kernel/sched/stop_task.c | 2 + > 6 files changed, 75 insertions(+), 68 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 1211575..67e8d1e 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -1043,7 +1043,7 @@ void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags) > * A queue event has occurred, and we're going to schedule. In > * this case, we can save a useless back to back clock update. > */ > - if (rq->curr->on_rq && test_tsk_need_resched(rq->curr)) > + if (task_queued(rq->curr) && test_tsk_need_resched(rq->curr)) > rq->skip_clock_update = 1;
> - p->on_rq = 1; > + p->on_rq = ONRQ_QUEUED;
> --- a/kernel/sched/sched.h > +++ b/kernel/sched/sched.h > @@ -15,6 +15,9 @@ > > struct rq; > > +/* task_struct::on_rq states: */ > +#define ONRQ_QUEUED 1 > + > extern __read_mostly int scheduler_running; > > extern unsigned long calc_load_update; > @@ -942,6 +945,10 @@ static inline int task_running(struct rq *rq, struct task_struct *p) > #endif > } > > +static inline int task_queued(struct task_struct *p) > +{ > + return p->on_rq == ONRQ_QUEUED; > +}
So I agree with splitting p->on_rq into more states, but the new naming used looks pretty random, we can and should do better.
For example 'task_queued()' gives very little clue that it's all about the p->on_rq state. The 'ONRQ_QUEUED' name does not signal that this is a task's scheduler internal state, etc.
So I'd suggest a more structured naming scheme, something along the lines of:
TASK_ON_RQ_QUEUED TASK_ON_RQ_MIGRATING
task_on_rq_queued() task_on_rq_migrating()
etc.
It's a bit longer, but also more logical and thus easier to read and maintain.
Thanks,
Ingo
| |