Re: [sched/rt] Optimization of function pull_rt_task()
16.11.2012, 00:36, "Steven Rostedt" :
> Doing my INBOX maintenance (clean up), I've stumbled on this thread
> again. I'm not sure the changes here are hopeless.
>
> On Mon, 2012-06-04 at 13:27 +0800, Yong Zhang wrote:
>
>> On Fri, Jun 01, 2012 at 08:45:16PM +0400, Kirill Tkhai wrote:
>>> 19.04.2012, 12:54, "Yong Zhang" :
On Wed, Apr 18, 2012 at 05:16:55PM -0400, Steven Rostedt wrote:
> ?On Wed, 2012-04-18 at 14:32 -0400, Steven Rostedt wrote:
>> ?On Mon, 2012-04-16 at 12:06 -0400, Steven Rostedt wrote:
>>> ?On Sun, 2012-04-15 at 23:45 +0400, Kirill Tkhai wrote:
?The condition (src_rq->rt.rt_nr_running) is weak because it doesn't
?consider the cases when src_rq has only processes bound to it (when
?single cpu is allowed). It may be running kernel thread like
?migration/x etc.
?So it's better to use more stronger condition which is able to
exclude
?above conditions. The function has_pushable_tasks() complitely does
?this. A task may be pullable for another cpu rq only if he is
pushable
?for his own queue.
>>> ?I considered this before, and for some reason I never did the change.
>>> ?I'll have to think about it. It seems like this would be the obvious
>>> ?case, but I think there was something not so obvious that caused
>>> issues.
>>> ?But I don't remember what it was.
>>>
>>> ?I'll have to rethink this again.
>> ?I can't find anything wrong with this change. Maybe things change, or I
>> ?was thinking of another change.
>>
>> ?I'll apply it and start running my tests against it.
> ?Not only does this seem to work fine, I took it one step further :-)
Hmm... throttle doesn't handle the pushable list, so we may find a
throttled task by pick_next_pushable_task().
Thanks,
Yong
>>> I don't complitelly understand throttle logic.
>>>
>>> Is the source patch not-appliable the same reason?
>> I guess so.
>>
>> Your patch will change the semantic of pick_next_pushable_task().
>
> Looking at the original patch, I don't see how it changes the semantics
> (although mine may have). The original patch was:
>
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1729,7 +1729,7 @@ static int pull_rt_task(struct rq *this_rq)
> /*
> * Are there still pullable RT tasks?
> */
> - if (src_rq->rt.rt_nr_running <= 1)
> + if (!has_pushable_tasks(src_rq))
> goto skip;
>
> p = pick_next_highest_task_rt(src_rq, this_cpu);
>
> And I still don't see a problem with this. If a rq has no pushable
> tasks, then we shouldn't bother trying to pull from it (no task can
> migrate).
>
> Thus, the original patch, I believe should be applied without question.
>
> Now, about my patch, the one that made pick_next_highest_task_rt into
> just:
>
> static struct task_struct *pick_next_highest_task_rt(struct rq *rq, int cpu)
> {
> struct plist_head *head = >rt.pushable_tasks;
> struct task_struct *next;
>
> plist_for_each_entry(next, head, pushable_tasks) {
> if (pick_rt_task(rq, next, cpu))
> return next;
> }
>
> return NULL;
> }
>
> You said could pick a task from a throttled rq. I'm not sure that is
> different than what we have now. As the current
> pick_next_highest_task_rt() just does a loop over the leaf_rt_rqs which
> includes throttled rqs. That's because a throttled rq will not dequeue
> the rt_rq from the leaf_rt_rq list if the rt_rq has rt_nr_running != 0.
Yes, there is no connection between logic of pushable tasks and throttling at
the moment.
These activities are independent. ( I tried to connect them at the patch:
http://lkml.indiana.edu/hypermail/linux/kernel/1211.2/03750.html )
I think, there is no problem.
Kirill
>
> I'm still thinking about adding both patches.
>
> -- Steve
>
>> Thanks,
>> Yong
>>> Kirill
> ?Peter, do you see anything wrong with this patch?
>
> ?-- Steve
>
> ?diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> ?index 61e3086..b44fd1b 100644
> ?--- a/kernel/sched/rt.c
> ?+++ b/kernel/sched/rt.c
> ?@@ -1416,39 +1416,15 @@ static int pick_rt_task(struct rq *rq, struct
> task_struct *p, int cpu)
> ??/* Return the second highest RT task, NULL otherwise */
> ??static struct task_struct *pick_next_highest_task_rt(struct rq *rq,
> int cpu)
> ??{
> ?- struct task_struct *next = NULL;
> ?- struct sched_rt_entity *rt_se;
> ?- struct rt_prio_array *array;
> ?- struct rt_rq *rt_rq;
> ?- int idx;
> ?+ struct plist_head *head = >rt.pushable_tasks;
> ?+ struct task_struct *next;
>
> ?- for_each_leaf_rt_rq(rt_rq, rq) {
> ?- array = _rq->active;
> ?- idx = sched_find_first_bit(array->bitmap);
Re: [sched/rt] Optimization of function pull_rt_task()
16.11.2012, 00:36, Steven Rostedt rost...@goodmis.org:
Doing my INBOX maintenance (clean up), I've stumbled on this thread
again. I'm not sure the changes here are hopeless.
On Mon, 2012-06-04 at 13:27 +0800, Yong Zhang wrote:
On Fri, Jun 01, 2012 at 08:45:16PM +0400, Kirill Tkhai wrote:
19.04.2012, 12:54, Yong Zhang yong.zha...@gmail.com:
On Wed, Apr 18, 2012 at 05:16:55PM -0400, Steven Rostedt wrote:
?On Wed, 2012-04-18 at 14:32 -0400, Steven Rostedt wrote:
?On Mon, 2012-04-16 at 12:06 -0400, Steven Rostedt wrote:
?On Sun, 2012-04-15 at 23:45 +0400, Kirill Tkhai wrote:
?The condition (src_rq-rt.rt_nr_running) is weak because it doesn't
?consider the cases when src_rq has only processes bound to it (when
?single cpu is allowed). It may be running kernel thread like
?migration/x etc.
?So it's better to use more stronger condition which is able to
exclude
?above conditions. The function has_pushable_tasks() complitely does
?this. A task may be pullable for another cpu rq only if he is
pushable
?for his own queue.
?I considered this before, and for some reason I never did the change.
?I'll have to think about it. It seems like this would be the obvious
?case, but I think there was something not so obvious that caused
issues.
?But I don't remember what it was.
?I'll have to rethink this again.
?I can't find anything wrong with this change. Maybe things change, or I
?was thinking of another change.
?I'll apply it and start running my tests against it.
?Not only does this seem to work fine, I took it one step further :-)
Hmm... throttle doesn't handle the pushable list, so we may find a
throttled task by pick_next_pushable_task().
Thanks,
Yong
I don't complitelly understand throttle logic.
Is the source patch not-appliable the same reason?
I guess so.
Your patch will change the semantic of pick_next_pushable_task().
Looking at the original patch, I don't see how it changes the semantics
(although mine may have). The original patch was:
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1729,7 +1729,7 @@ static int pull_rt_task(struct rq *this_rq)
/*
* Are there still pullable RT tasks?
*/
- if (src_rq-rt.rt_nr_running = 1)
+ if (!has_pushable_tasks(src_rq))
goto skip;
p = pick_next_highest_task_rt(src_rq, this_cpu);
And I still don't see a problem with this. If a rq has no pushable
tasks, then we shouldn't bother trying to pull from it (no task can
migrate).
Thus, the original patch, I believe should be applied without question.
Now, about my patch, the one that made pick_next_highest_task_rt into
just:
static struct task_struct *pick_next_highest_task_rt(struct rq *rq, int cpu)
{
struct plist_head *head = rq-rt.pushable_tasks;
struct task_struct *next;
plist_for_each_entry(next, head, pushable_tasks) {
if (pick_rt_task(rq, next, cpu))
return next;
}
return NULL;
}
You said could pick a task from a throttled rq. I'm not sure that is
different than what we have now. As the current
pick_next_highest_task_rt() just does a loop over the leaf_rt_rqs which
includes throttled rqs. That's because a throttled rq will not dequeue
the rt_rq from the leaf_rt_rq list if the rt_rq has rt_nr_running != 0.
Yes, there is no connection between logic of pushable tasks and throttling at
the moment.
These activities are independent. ( I tried to connect them at the patch:
http://lkml.indiana.edu/hypermail/linux/kernel/1211.2/03750.html )
I think, there is no problem.
Kirill
I'm still thinking about adding both patches.
-- Steve
Thanks,
Yong
Kirill
?Peter, do you see anything wrong with this patch?
?-- Steve
?diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
?index 61e3086..b44fd1b 100644
?--- a/kernel/sched/rt.c
?+++ b/kernel/sched/rt.c
?@@ -1416,39 +1416,15 @@ static int pick_rt_task(struct rq *rq, struct
task_struct *p, int cpu)
??/* Return the second highest RT task, NULL otherwise */
??static struct task_struct *pick_next_highest_task_rt(struct rq *rq,
int cpu)
??{
?- struct task_struct *next = NULL;
?- struct sched_rt_entity *rt_se;
?- struct rt_prio_array *array;
?- struct rt_rq *rt_rq;
?- int idx;
?+ struct plist_head *head = rq-rt.pushable_tasks;
?+ struct task_struct *next;
?- for_each_leaf_rt_rq(rt_rq, rq) {
?- array = rt_rq-active;
?- idx = sched_find_first_bit(array-bitmap);
?-next_idx:
?- if (idx = MAX_RT_PRIO)
?- continue;
?- if (next next-prio = idx)
?- continue;
?- list_for_each_entry(rt_se, array-queue + idx, run_list) {
?- struct task_struct *p;
?-
?- if (!rt_entity_is_task(rt_se))
?- continue;
?-
?- p = rt_task_of(rt_se);
?- if (pick_rt_task(rq, p, cpu)) {
?- next = p;
?- break;
?- }
?- }
?- if (!next) {
Re: [sched/rt] Optimization of function pull_rt_task()
Doing my INBOX maintenance (clean up), I've stumbled on this thread
again. I'm not sure the changes here are hopeless.
On Mon, 2012-06-04 at 13:27 +0800, Yong Zhang wrote:
> On Fri, Jun 01, 2012 at 08:45:16PM +0400, Kirill Tkhai wrote:
> >
> >
> > 19.04.2012, 12:54, "Yong Zhang" :
> > > On Wed, Apr 18, 2012 at 05:16:55PM -0400, Steven Rostedt wrote:
> > >
> > >> ?On Wed, 2012-04-18 at 14:32 -0400, Steven Rostedt wrote:
> > >>> ?On Mon, 2012-04-16 at 12:06 -0400, Steven Rostedt wrote:
> > ?On Sun, 2012-04-15 at 23:45 +0400, Kirill Tkhai wrote:
> > > ?The condition (src_rq->rt.rt_nr_running) is weak because it doesn't
> > > ?consider the cases when src_rq has only processes bound to it (when
> > > ?single cpu is allowed). It may be running kernel thread like
> > > ?migration/x etc.
> > >
> > > ?So it's better to use more stronger condition which is able to
> > > exclude
> > > ?above conditions. The function has_pushable_tasks() complitely does
> > > ?this. A task may be pullable for another cpu rq only if he is
> > > pushable
> > > ?for his own queue.
> > ?I considered this before, and for some reason I never did the change.
> > ?I'll have to think about it. It seems like this would be the obvious
> > ?case, but I think there was something not so obvious that caused
> > issues.
> > ?But I don't remember what it was.
> >
> > ?I'll have to rethink this again.
> > >>> ?I can't find anything wrong with this change. Maybe things change, or I
> > >>> ?was thinking of another change.
> > >>>
> > >>> ?I'll apply it and start running my tests against it.
> > >> ?Not only does this seem to work fine, I took it one step further :-)
> > >
> > > Hmm... throttle doesn't handle the pushable list, so we may find a
> > > throttled task by pick_next_pushable_task().
> > >
> > > Thanks,
> > > Yong
> >
> > I don't complitelly understand throttle logic.
> >
> > Is the source patch not-appliable the same reason?
>
> I guess so.
>
> Your patch will change the semantic of pick_next_pushable_task().
Looking at the original patch, I don't see how it changes the semantics
(although mine may have). The original patch was:
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1729,7 +1729,7 @@ static int pull_rt_task(struct rq *this_rq)
/*
* Are there still pullable RT tasks?
*/
- if (src_rq->rt.rt_nr_running <= 1)
+ if (!has_pushable_tasks(src_rq))
goto skip;
p = pick_next_highest_task_rt(src_rq, this_cpu);
And I still don't see a problem with this. If a rq has no pushable
tasks, then we shouldn't bother trying to pull from it (no task can
migrate).
Thus, the original patch, I believe should be applied without question.
Now, about my patch, the one that made pick_next_highest_task_rt into
just:
static struct task_struct *pick_next_highest_task_rt(struct rq *rq, int cpu)
{
struct plist_head *head = >rt.pushable_tasks;
struct task_struct *next;
plist_for_each_entry(next, head, pushable_tasks) {
if (pick_rt_task(rq, next, cpu))
return next;
}
return NULL;
}
You said could pick a task from a throttled rq. I'm not sure that is
different than what we have now. As the current
pick_next_highest_task_rt() just does a loop over the leaf_rt_rqs which
includes throttled rqs. That's because a throttled rq will not dequeue
the rt_rq from the leaf_rt_rq list if the rt_rq has rt_nr_running != 0.
I'm still thinking about adding both patches.
-- Steve
>
> Thanks,
> Yong
>
> >
> > Kirill
> >
> > >
> > >> ?Peter, do you see anything wrong with this patch?
> > >>
> > >> ?-- Steve
> > >>
> > >> ?diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> > >> ?index 61e3086..b44fd1b 100644
> > >> ?--- a/kernel/sched/rt.c
> > >> ?+++ b/kernel/sched/rt.c
> > >> ?@@ -1416,39 +1416,15 @@ static int pick_rt_task(struct rq *rq, struct
> > >> task_struct *p, int cpu)
> > >> ??/* Return the second highest RT task, NULL otherwise */
> > >> ??static struct task_struct *pick_next_highest_task_rt(struct rq *rq,
> > >> int cpu)
> > >> ??{
> > >> ?- struct task_struct *next = NULL;
> > >> ?- struct sched_rt_entity *rt_se;
> > >> ?- struct rt_prio_array *array;
> > >> ?- struct rt_rq *rt_rq;
> > >> ?- int idx;
> > >> ?+ struct plist_head *head = >rt.pushable_tasks;
> > >> ?+ struct task_struct *next;
> > >>
> > >> ?- for_each_leaf_rt_rq(rt_rq, rq) {
> > >> ?- array = _rq->active;
> > >> ?- idx = sched_find_first_bit(array->bitmap);
> > >> ?-next_idx:
> > >> ?- if (idx >= MAX_RT_PRIO)
> > >> ?- continue;
> > >> ?- if (next && next->prio <= idx)
> > >> ?- continue;
> > >> ?- list_for_each_entry(rt_se, array->queue + idx, run_list) {
> > >> ?- struct task_struct *p;
> > >> ?-
> > >> ?- if (!rt_entity_is_task(rt_se))
> > >> ?- continue;
> > >> ?-
> > >> ?- p = rt_task_of(rt_se);
>
Re: [sched/rt] Optimization of function pull_rt_task()
Doing my INBOX maintenance (clean up), I've stumbled on this thread
again. I'm not sure the changes here are hopeless.
On Mon, 2012-06-04 at 13:27 +0800, Yong Zhang wrote:
On Fri, Jun 01, 2012 at 08:45:16PM +0400, Kirill Tkhai wrote:
19.04.2012, 12:54, Yong Zhang yong.zha...@gmail.com:
On Wed, Apr 18, 2012 at 05:16:55PM -0400, Steven Rostedt wrote:
?On Wed, 2012-04-18 at 14:32 -0400, Steven Rostedt wrote:
?On Mon, 2012-04-16 at 12:06 -0400, Steven Rostedt wrote:
?On Sun, 2012-04-15 at 23:45 +0400, Kirill Tkhai wrote:
?The condition (src_rq-rt.rt_nr_running) is weak because it doesn't
?consider the cases when src_rq has only processes bound to it (when
?single cpu is allowed). It may be running kernel thread like
?migration/x etc.
?So it's better to use more stronger condition which is able to
exclude
?above conditions. The function has_pushable_tasks() complitely does
?this. A task may be pullable for another cpu rq only if he is
pushable
?for his own queue.
?I considered this before, and for some reason I never did the change.
?I'll have to think about it. It seems like this would be the obvious
?case, but I think there was something not so obvious that caused
issues.
?But I don't remember what it was.
?I'll have to rethink this again.
?I can't find anything wrong with this change. Maybe things change, or I
?was thinking of another change.
?I'll apply it and start running my tests against it.
?Not only does this seem to work fine, I took it one step further :-)
Hmm... throttle doesn't handle the pushable list, so we may find a
throttled task by pick_next_pushable_task().
Thanks,
Yong
I don't complitelly understand throttle logic.
Is the source patch not-appliable the same reason?
I guess so.
Your patch will change the semantic of pick_next_pushable_task().
Looking at the original patch, I don't see how it changes the semantics
(although mine may have). The original patch was:
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1729,7 +1729,7 @@ static int pull_rt_task(struct rq *this_rq)
/*
* Are there still pullable RT tasks?
*/
- if (src_rq-rt.rt_nr_running = 1)
+ if (!has_pushable_tasks(src_rq))
goto skip;
p = pick_next_highest_task_rt(src_rq, this_cpu);
And I still don't see a problem with this. If a rq has no pushable
tasks, then we shouldn't bother trying to pull from it (no task can
migrate).
Thus, the original patch, I believe should be applied without question.
Now, about my patch, the one that made pick_next_highest_task_rt into
just:
static struct task_struct *pick_next_highest_task_rt(struct rq *rq, int cpu)
{
struct plist_head *head = rq-rt.pushable_tasks;
struct task_struct *next;
plist_for_each_entry(next, head, pushable_tasks) {
if (pick_rt_task(rq, next, cpu))
return next;
}
return NULL;
}
You said could pick a task from a throttled rq. I'm not sure that is
different than what we have now. As the current
pick_next_highest_task_rt() just does a loop over the leaf_rt_rqs which
includes throttled rqs. That's because a throttled rq will not dequeue
the rt_rq from the leaf_rt_rq list if the rt_rq has rt_nr_running != 0.
I'm still thinking about adding both patches.
-- Steve
Thanks,
Yong
Kirill
?Peter, do you see anything wrong with this patch?
?-- Steve
?diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
?index 61e3086..b44fd1b 100644
?--- a/kernel/sched/rt.c
?+++ b/kernel/sched/rt.c
?@@ -1416,39 +1416,15 @@ static int pick_rt_task(struct rq *rq, struct
task_struct *p, int cpu)
??/* Return the second highest RT task, NULL otherwise */
??static struct task_struct *pick_next_highest_task_rt(struct rq *rq,
int cpu)
??{
?- struct task_struct *next = NULL;
?- struct sched_rt_entity *rt_se;
?- struct rt_prio_array *array;
?- struct rt_rq *rt_rq;
?- int idx;
?+ struct plist_head *head = rq-rt.pushable_tasks;
?+ struct task_struct *next;
?- for_each_leaf_rt_rq(rt_rq, rq) {
?- array = rt_rq-active;
?- idx = sched_find_first_bit(array-bitmap);
?-next_idx:
?- if (idx = MAX_RT_PRIO)
?- continue;
?- if (next next-prio = idx)
?- continue;
?- list_for_each_entry(rt_se, array-queue + idx, run_list) {
?- struct task_struct *p;
?-
?- if (!rt_entity_is_task(rt_se))
?- continue;
?-
?- p = rt_task_of(rt_se);
?- if (pick_rt_task(rq, p, cpu)) {
?- next = p;
?- break;
?- }
?- }
?- if (!next) {
?- idx = find_next_bit(array-bitmap, MAX_RT_PRIO, idx+1);
?- goto next_idx;
?- }
?+ plist_for_each_entry(next, head, pushable_tasks) {
?+ if (pick_rt_task(rq, next, cpu))
?+ return next;
??}
?- return next;
?+
