Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/10/2013 01:52 PM, Namhyung Kim wrote: > On Wed, 09 Jan 2013 16:34:39 +0800, Alex Shi wrote: >> On 01/09/2013 03:54 PM, Namhyung Kim wrote: >>> Hi Alex, >>> >>> On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: On 01/09/2013 02:50 PM, Namhyung Kim wrote: > From: Namhyung Kim > > AFAICS @target cpu of select_idle_sibling() is always either prev_cpu > or this_cpu. So no need to check it again and the conditionals can be > consolidated. >>> [snip] Uh, we don't know if the target is this_cpu or previous cpu, If we just check the target idle status, we may miss another idle cpu. So this patch change the logical in this function. >>> >>> select_idle_sibling() is called only in select_task_rq_fair() if it >>> found a suitable affine_sd. The default target is the 'prev_cpu' of the >>> task but if wake_affine() returns true it'd be (this) 'cpu'. >>> >>> I cannot see where the prev_cpu or the cpu is set to another one before >>> calling select_idle_sibling. >> >> The old logical will return directly whenever prev_cpu or this cpu idle, >> but your new logical just has one chance. > > Sorry, I can't get your point. Could you elaborate on it a bit more? Sorry. I misunderstand this. Acked-by: Alex Shi > > Thanks, > Namhyung > -- Thanks Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/10/2013 11:19 AM, Namhyung Kim wrote:
> Hi Preeti,
>
> On Wed, 09 Jan 2013 13:51:00 +0530, Preeti U. Murthy wrote:
>> On 01/09/2013 12:20 PM, Namhyung Kim wrote:
>>> From: Namhyung Kim
>>>
>>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
>>> or this_cpu. So no need to check it again and the conditionals can be
>>> consolidated.
> [snip]
>> If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by
>
> I can't find those bits in the code. I've checked v3.8-rc2,
> next-20130110, tip/master and tip/numa/core but there's nothing like
> above. Which tree are you saying?
>
>
>> default),cpu/prev_cpu can be changed to be a random node_cpu(the node
>> that 'this_cpu' is on). In which case even if the node cpu is idle,it
>> would not be a viable target,looks like.Maybe that is why
>> select_idle_sibling() makes the check if the target is prev_cpu/this cpu.
>
> Looking into tip/numa/core, I can see that there's a code added for
> CONFIG_NUMA_BALANCING. But still, it seems nothing changed on a path
> from select_task_rq_fair() to select_idle_sibling() - i.e. if the
> select_idle_sibling called, the target would be either prev_cpu or this
> cpu. Am I missing something?
Hi Namhyung,
Sorry about this.I did a git pull on the tip/master.You are right this
piece of code is not there any more.So not an issue.
You can go ahead and add my Reviewed-by.I don't find any potential
problems now.
For your information however, the code piece which was earlier there, is
given below which aimed to pull the tasks towards the node of the waking
cpu,if NUMA_TTWU_BIAS is enabled.
if (sched_feat_numa(NUMA_TTWU_BIAS) && node != -1) {
/*
* For fork,exec find the idlest cpu in the home-node.
*/
if (sd_flag & (SD_BALANCE_FORK|SD_BALANCE_EXEC)) {
int node_cpu = numa_select_node_cpu(p, node);
if (node_cpu < 0)
goto find_sd;
new_cpu = cpu = node_cpu;
sd = per_cpu(sd_node, cpu);
goto pick_idlest;
}
/*
* For wake, pretend we were running in the home-node.
*/
if (cpu_to_node(prev_cpu) != node) {
int node_cpu = numa_select_node_cpu(p, node);
if (node_cpu < 0)
goto find_sd;
if (sched_feat_numa(NUMA_TTWU_TO))
cpu = node_cpu;
else
prev_cpu = node_cpu;
}
}
>
> Thanks,
> Namhyung
>
I apologise once again for the wrong feedback.
Thank you
Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/10/2013 01:52 PM, Namhyung Kim wrote: On Wed, 09 Jan 2013 16:34:39 +0800, Alex Shi wrote: On 01/09/2013 03:54 PM, Namhyung Kim wrote: Hi Alex, On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: On 01/09/2013 02:50 PM, Namhyung Kim wrote: From: Namhyung Kim namhyung@lge.com AFAICS @target cpu of select_idle_sibling() is always either prev_cpu or this_cpu. So no need to check it again and the conditionals can be consolidated. [snip] Uh, we don't know if the target is this_cpu or previous cpu, If we just check the target idle status, we may miss another idle cpu. So this patch change the logical in this function. select_idle_sibling() is called only in select_task_rq_fair() if it found a suitable affine_sd. The default target is the 'prev_cpu' of the task but if wake_affine() returns true it'd be (this) 'cpu'. I cannot see where the prev_cpu or the cpu is set to another one before calling select_idle_sibling. The old logical will return directly whenever prev_cpu or this cpu idle, but your new logical just has one chance. Sorry, I can't get your point. Could you elaborate on it a bit more? Sorry. I misunderstand this. Acked-by: Alex Shi Thanks, Namhyung -- Thanks Alex -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/10/2013 11:19 AM, Namhyung Kim wrote:
Hi Preeti,
On Wed, 09 Jan 2013 13:51:00 +0530, Preeti U. Murthy wrote:
On 01/09/2013 12:20 PM, Namhyung Kim wrote:
From: Namhyung Kim namhyung@lge.com
AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
or this_cpu. So no need to check it again and the conditionals can be
consolidated.
[snip]
If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by
I can't find those bits in the code. I've checked v3.8-rc2,
next-20130110, tip/master and tip/numa/core but there's nothing like
above. Which tree are you saying?
default),cpu/prev_cpu can be changed to be a random node_cpu(the node
that 'this_cpu' is on). In which case even if the node cpu is idle,it
would not be a viable target,looks like.Maybe that is why
select_idle_sibling() makes the check if the target is prev_cpu/this cpu.
Looking into tip/numa/core, I can see that there's a code added for
CONFIG_NUMA_BALANCING. But still, it seems nothing changed on a path
from select_task_rq_fair() to select_idle_sibling() - i.e. if the
select_idle_sibling called, the target would be either prev_cpu or this
cpu. Am I missing something?
Hi Namhyung,
Sorry about this.I did a git pull on the tip/master.You are right this
piece of code is not there any more.So not an issue.
You can go ahead and add my Reviewed-by.I don't find any potential
problems now.
For your information however, the code piece which was earlier there, is
given below which aimed to pull the tasks towards the node of the waking
cpu,if NUMA_TTWU_BIAS is enabled.
if (sched_feat_numa(NUMA_TTWU_BIAS) node != -1) {
/*
* For fork,exec find the idlest cpu in the home-node.
*/
if (sd_flag (SD_BALANCE_FORK|SD_BALANCE_EXEC)) {
int node_cpu = numa_select_node_cpu(p, node);
if (node_cpu 0)
goto find_sd;
new_cpu = cpu = node_cpu;
sd = per_cpu(sd_node, cpu);
goto pick_idlest;
}
/*
* For wake, pretend we were running in the home-node.
*/
if (cpu_to_node(prev_cpu) != node) {
int node_cpu = numa_select_node_cpu(p, node);
if (node_cpu 0)
goto find_sd;
if (sched_feat_numa(NUMA_TTWU_TO))
cpu = node_cpu;
else
prev_cpu = node_cpu;
}
}
Thanks,
Namhyung
I apologise once again for the wrong feedback.
Thank you
Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On Wed, 09 Jan 2013 16:34:39 +0800, Alex Shi wrote: > On 01/09/2013 03:54 PM, Namhyung Kim wrote: >> Hi Alex, >> >> On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: >>> On 01/09/2013 02:50 PM, Namhyung Kim wrote: From: Namhyung Kim AFAICS @target cpu of select_idle_sibling() is always either prev_cpu or this_cpu. So no need to check it again and the conditionals can be consolidated. >> [snip] >>> Uh, we don't know if the target is this_cpu or previous cpu, If we just >>> check the target idle status, we may miss another idle cpu. So this >>> patch change the logical in this function. >> >> select_idle_sibling() is called only in select_task_rq_fair() if it >> found a suitable affine_sd. The default target is the 'prev_cpu' of the >> task but if wake_affine() returns true it'd be (this) 'cpu'. >> >> I cannot see where the prev_cpu or the cpu is set to another one before >> calling select_idle_sibling. > > The old logical will return directly whenever prev_cpu or this cpu idle, > but your new logical just has one chance. Sorry, I can't get your point. Could you elaborate on it a bit more? Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
Hi Preeti, On Wed, 09 Jan 2013 13:51:00 +0530, Preeti U. Murthy wrote: > On 01/09/2013 12:20 PM, Namhyung Kim wrote: >> From: Namhyung Kim >> >> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu >> or this_cpu. So no need to check it again and the conditionals can be >> consolidated. [snip] > If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by I can't find those bits in the code. I've checked v3.8-rc2, next-20130110, tip/master and tip/numa/core but there's nothing like above. Which tree are you saying? > default),cpu/prev_cpu can be changed to be a random node_cpu(the node > that 'this_cpu' is on). In which case even if the node cpu is idle,it > would not be a viable target,looks like.Maybe that is why > select_idle_sibling() makes the check if the target is prev_cpu/this cpu. Looking into tip/numa/core, I can see that there's a code added for CONFIG_NUMA_BALANCING. But still, it seems nothing changed on a path from select_task_rq_fair() to select_idle_sibling() - i.e. if the select_idle_sibling called, the target would be either prev_cpu or this cpu. Am I missing something? Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/09/2013 03:54 PM, Namhyung Kim wrote: > Hi Alex, > > On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: >> On 01/09/2013 02:50 PM, Namhyung Kim wrote: >>> From: Namhyung Kim >>> >>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu >>> or this_cpu. So no need to check it again and the conditionals can be >>> consolidated. > [snip] >> Uh, we don't know if the target is this_cpu or previous cpu, If we just >> check the target idle status, we may miss another idle cpu. So this >> patch change the logical in this function. > > select_idle_sibling() is called only in select_task_rq_fair() if it > found a suitable affine_sd. The default target is the 'prev_cpu' of the > task but if wake_affine() returns true it'd be (this) 'cpu'. > > I cannot see where the prev_cpu or the cpu is set to another one before > calling select_idle_sibling. The old logical will return directly whenever prev_cpu or this cpu idle, but your new logical just has one chance. > > Thanks, > Namhyung > -- Thanks Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/09/2013 12:20 PM, Namhyung Kim wrote:
> From: Namhyung Kim
>
> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
> or this_cpu. So no need to check it again and the conditionals can be
> consolidated.
>
> Cc: Mike Galbraith
> Cc: Preeti U Murthy
> Cc: Vincent Guittot
> Cc: Alex Shi
> Signed-off-by: Namhyung Kim
> ---
> kernel/sched/fair.c | 17 -
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 5eea8707234a..af665814c216 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct
> task_struct *p, int this_cpu)
> */
> static int select_idle_sibling(struct task_struct *p, int target)
> {
> - int cpu = smp_processor_id();
> - int prev_cpu = task_cpu(p);
> struct sched_domain *sd;
> struct sched_group *sg;
> int i;
>
> /*
> - * If the task is going to be woken-up on this cpu and if it is
> - * already idle, then it is the right target.
> - */
> - if (target == cpu && idle_cpu(cpu))
> - return cpu;
> -
> - /*
> - * If the task is going to be woken-up on the cpu where it previously
> - * ran and if it is currently idle, then it the right target.
> + * If the task is going to be woken-up on this cpu or the cpu where it
> + * previously ran and it is already idle, then it is the right target.
>*/
> - if (target == prev_cpu && idle_cpu(prev_cpu))
> - return prev_cpu;
> + if (idle_cpu(target))
> + return target;
>
> /*
>* Otherwise, iterate the domains and find an elegible idle cpu.
>
If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by
default),cpu/prev_cpu can be changed to be a random node_cpu(the node
that 'this_cpu' is on). In which case even if the node cpu is idle,it
would not be a viable target,looks like.Maybe that is why
select_idle_sibling() makes the check if the target is prev_cpu/this cpu.
Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On Wed, 09 Jan 2013 15:38:11 +0800, Alex Shi wrote: >> >> Uh, we don't know if the target is this_cpu or previous cpu, If we just >> check the target idle status, we may miss another idle cpu. So this >> patch change the logical in this function. > > But, you can fold wake_affine into select_idle_sibling(). that will save > a complicate calculation whichever cpu idle. :) Looks like a good idea. :) Will send v2. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On Wed, 09 Jan 2013 15:38:11 +0800, Alex Shi wrote: Uh, we don't know if the target is this_cpu or previous cpu, If we just check the target idle status, we may miss another idle cpu. So this patch change the logical in this function. But, you can fold wake_affine into select_idle_sibling(). that will save a complicate calculation whichever cpu idle. :) Looks like a good idea. :) Will send v2. Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/09/2013 12:20 PM, Namhyung Kim wrote:
From: Namhyung Kim namhyung@lge.com
AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
or this_cpu. So no need to check it again and the conditionals can be
consolidated.
Cc: Mike Galbraith efa...@gmx.de
Cc: Preeti U Murthy pre...@linux.vnet.ibm.com
Cc: Vincent Guittot vincent.guit...@linaro.org
Cc: Alex Shi alex@intel.com
Signed-off-by: Namhyung Kim namhy...@kernel.org
---
kernel/sched/fair.c | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5eea8707234a..af665814c216 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct
task_struct *p, int this_cpu)
*/
static int select_idle_sibling(struct task_struct *p, int target)
{
- int cpu = smp_processor_id();
- int prev_cpu = task_cpu(p);
struct sched_domain *sd;
struct sched_group *sg;
int i;
/*
- * If the task is going to be woken-up on this cpu and if it is
- * already idle, then it is the right target.
- */
- if (target == cpu idle_cpu(cpu))
- return cpu;
-
- /*
- * If the task is going to be woken-up on the cpu where it previously
- * ran and if it is currently idle, then it the right target.
+ * If the task is going to be woken-up on this cpu or the cpu where it
+ * previously ran and it is already idle, then it is the right target.
*/
- if (target == prev_cpu idle_cpu(prev_cpu))
- return prev_cpu;
+ if (idle_cpu(target))
+ return target;
/*
* Otherwise, iterate the domains and find an elegible idle cpu.
If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by
default),cpu/prev_cpu can be changed to be a random node_cpu(the node
that 'this_cpu' is on). In which case even if the node cpu is idle,it
would not be a viable target,looks like.Maybe that is why
select_idle_sibling() makes the check if the target is prev_cpu/this cpu.
Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/09/2013 03:54 PM, Namhyung Kim wrote: Hi Alex, On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: On 01/09/2013 02:50 PM, Namhyung Kim wrote: From: Namhyung Kim namhyung@lge.com AFAICS @target cpu of select_idle_sibling() is always either prev_cpu or this_cpu. So no need to check it again and the conditionals can be consolidated. [snip] Uh, we don't know if the target is this_cpu or previous cpu, If we just check the target idle status, we may miss another idle cpu. So this patch change the logical in this function. select_idle_sibling() is called only in select_task_rq_fair() if it found a suitable affine_sd. The default target is the 'prev_cpu' of the task but if wake_affine() returns true it'd be (this) 'cpu'. I cannot see where the prev_cpu or the cpu is set to another one before calling select_idle_sibling. The old logical will return directly whenever prev_cpu or this cpu idle, but your new logical just has one chance. Thanks, Namhyung -- Thanks Alex -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
Hi Preeti, On Wed, 09 Jan 2013 13:51:00 +0530, Preeti U. Murthy wrote: On 01/09/2013 12:20 PM, Namhyung Kim wrote: From: Namhyung Kim namhyung@lge.com AFAICS @target cpu of select_idle_sibling() is always either prev_cpu or this_cpu. So no need to check it again and the conditionals can be consolidated. [snip] If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by I can't find those bits in the code. I've checked v3.8-rc2, next-20130110, tip/master and tip/numa/core but there's nothing like above. Which tree are you saying? default),cpu/prev_cpu can be changed to be a random node_cpu(the node that 'this_cpu' is on). In which case even if the node cpu is idle,it would not be a viable target,looks like.Maybe that is why select_idle_sibling() makes the check if the target is prev_cpu/this cpu. Looking into tip/numa/core, I can see that there's a code added for CONFIG_NUMA_BALANCING. But still, it seems nothing changed on a path from select_task_rq_fair() to select_idle_sibling() - i.e. if the select_idle_sibling called, the target would be either prev_cpu or this cpu. Am I missing something? Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On Wed, 09 Jan 2013 16:34:39 +0800, Alex Shi wrote: On 01/09/2013 03:54 PM, Namhyung Kim wrote: Hi Alex, On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: On 01/09/2013 02:50 PM, Namhyung Kim wrote: From: Namhyung Kim namhyung@lge.com AFAICS @target cpu of select_idle_sibling() is always either prev_cpu or this_cpu. So no need to check it again and the conditionals can be consolidated. [snip] Uh, we don't know if the target is this_cpu or previous cpu, If we just check the target idle status, we may miss another idle cpu. So this patch change the logical in this function. select_idle_sibling() is called only in select_task_rq_fair() if it found a suitable affine_sd. The default target is the 'prev_cpu' of the task but if wake_affine() returns true it'd be (this) 'cpu'. I cannot see where the prev_cpu or the cpu is set to another one before calling select_idle_sibling. The old logical will return directly whenever prev_cpu or this cpu idle, but your new logical just has one chance. Sorry, I can't get your point. Could you elaborate on it a bit more? Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
Hi Alex, On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: > On 01/09/2013 02:50 PM, Namhyung Kim wrote: >> From: Namhyung Kim >> >> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu >> or this_cpu. So no need to check it again and the conditionals can be >> consolidated. [snip] > Uh, we don't know if the target is this_cpu or previous cpu, If we just > check the target idle status, we may miss another idle cpu. So this > patch change the logical in this function. select_idle_sibling() is called only in select_task_rq_fair() if it found a suitable affine_sd. The default target is the 'prev_cpu' of the task but if wake_affine() returns true it'd be (this) 'cpu'. I cannot see where the prev_cpu or the cpu is set to another one before calling select_idle_sibling. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
> > Uh, we don't know if the target is this_cpu or previous cpu, If we just > check the target idle status, we may miss another idle cpu. So this > patch change the logical in this function. But, you can fold wake_affine into select_idle_sibling(). that will save a complicate calculation whichever cpu idle. :) -- Thanks Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/09/2013 02:50 PM, Namhyung Kim wrote:
> From: Namhyung Kim
>
> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
> or this_cpu. So no need to check it again and the conditionals can be
> consolidated.
>
> Cc: Mike Galbraith
> Cc: Preeti U Murthy
> Cc: Vincent Guittot
> Cc: Alex Shi
> Signed-off-by: Namhyung Kim
> ---
> kernel/sched/fair.c | 17 -
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 5eea8707234a..af665814c216 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct
> task_struct *p, int this_cpu)
> */
> static int select_idle_sibling(struct task_struct *p, int target)
> {
> - int cpu = smp_processor_id();
> - int prev_cpu = task_cpu(p);
> struct sched_domain *sd;
> struct sched_group *sg;
> int i;
>
> /*
> - * If the task is going to be woken-up on this cpu and if it is
> - * already idle, then it is the right target.
> - */
> - if (target == cpu && idle_cpu(cpu))
> - return cpu;
> -
> - /*
> - * If the task is going to be woken-up on the cpu where it previously
> - * ran and if it is currently idle, then it the right target.
> + * If the task is going to be woken-up on this cpu or the cpu where it
> + * previously ran and it is already idle, then it is the right target.
>*/
> - if (target == prev_cpu && idle_cpu(prev_cpu))
> - return prev_cpu;
> + if (idle_cpu(target))
> + return target;
Uh, we don't know if the target is this_cpu or previous cpu, If we just
check the target idle status, we may miss another idle cpu. So this
patch change the logical in this function.
>
> /*
>* Otherwise, iterate the domains and find an elegible idle cpu.
>
--
Thanks Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
From: Namhyung Kim
AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
or this_cpu. So no need to check it again and the conditionals can be
consolidated.
Cc: Mike Galbraith
Cc: Preeti U Murthy
Cc: Vincent Guittot
Cc: Alex Shi
Signed-off-by: Namhyung Kim
---
kernel/sched/fair.c | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5eea8707234a..af665814c216 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct
task_struct *p, int this_cpu)
*/
static int select_idle_sibling(struct task_struct *p, int target)
{
- int cpu = smp_processor_id();
- int prev_cpu = task_cpu(p);
struct sched_domain *sd;
struct sched_group *sg;
int i;
/*
-* If the task is going to be woken-up on this cpu and if it is
-* already idle, then it is the right target.
-*/
- if (target == cpu && idle_cpu(cpu))
- return cpu;
-
- /*
-* If the task is going to be woken-up on the cpu where it previously
-* ran and if it is currently idle, then it the right target.
+* If the task is going to be woken-up on this cpu or the cpu where it
+* previously ran and it is already idle, then it is the right target.
*/
- if (target == prev_cpu && idle_cpu(prev_cpu))
- return prev_cpu;
+ if (idle_cpu(target))
+ return target;
/*
* Otherwise, iterate the domains and find an elegible idle cpu.
--
1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
From: Namhyung Kim namhyung@lge.com
AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
or this_cpu. So no need to check it again and the conditionals can be
consolidated.
Cc: Mike Galbraith efa...@gmx.de
Cc: Preeti U Murthy pre...@linux.vnet.ibm.com
Cc: Vincent Guittot vincent.guit...@linaro.org
Cc: Alex Shi alex@intel.com
Signed-off-by: Namhyung Kim namhy...@kernel.org
---
kernel/sched/fair.c | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5eea8707234a..af665814c216 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct
task_struct *p, int this_cpu)
*/
static int select_idle_sibling(struct task_struct *p, int target)
{
- int cpu = smp_processor_id();
- int prev_cpu = task_cpu(p);
struct sched_domain *sd;
struct sched_group *sg;
int i;
/*
-* If the task is going to be woken-up on this cpu and if it is
-* already idle, then it is the right target.
-*/
- if (target == cpu idle_cpu(cpu))
- return cpu;
-
- /*
-* If the task is going to be woken-up on the cpu where it previously
-* ran and if it is currently idle, then it the right target.
+* If the task is going to be woken-up on this cpu or the cpu where it
+* previously ran and it is already idle, then it is the right target.
*/
- if (target == prev_cpu idle_cpu(prev_cpu))
- return prev_cpu;
+ if (idle_cpu(target))
+ return target;
/*
* Otherwise, iterate the domains and find an elegible idle cpu.
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
On 01/09/2013 02:50 PM, Namhyung Kim wrote:
From: Namhyung Kim namhyung@lge.com
AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
or this_cpu. So no need to check it again and the conditionals can be
consolidated.
Cc: Mike Galbraith efa...@gmx.de
Cc: Preeti U Murthy pre...@linux.vnet.ibm.com
Cc: Vincent Guittot vincent.guit...@linaro.org
Cc: Alex Shi alex@intel.com
Signed-off-by: Namhyung Kim namhy...@kernel.org
---
kernel/sched/fair.c | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5eea8707234a..af665814c216 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct
task_struct *p, int this_cpu)
*/
static int select_idle_sibling(struct task_struct *p, int target)
{
- int cpu = smp_processor_id();
- int prev_cpu = task_cpu(p);
struct sched_domain *sd;
struct sched_group *sg;
int i;
/*
- * If the task is going to be woken-up on this cpu and if it is
- * already idle, then it is the right target.
- */
- if (target == cpu idle_cpu(cpu))
- return cpu;
-
- /*
- * If the task is going to be woken-up on the cpu where it previously
- * ran and if it is currently idle, then it the right target.
+ * If the task is going to be woken-up on this cpu or the cpu where it
+ * previously ran and it is already idle, then it is the right target.
*/
- if (target == prev_cpu idle_cpu(prev_cpu))
- return prev_cpu;
+ if (idle_cpu(target))
+ return target;
Uh, we don't know if the target is this_cpu or previous cpu, If we just
check the target idle status, we may miss another idle cpu. So this
patch change the logical in this function.
/*
* Otherwise, iterate the domains and find an elegible idle cpu.
--
Thanks Alex
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
Uh, we don't know if the target is this_cpu or previous cpu, If we just check the target idle status, we may miss another idle cpu. So this patch change the logical in this function. But, you can fold wake_affine into select_idle_sibling(). that will save a complicate calculation whichever cpu idle. :) -- Thanks Alex -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling
Hi Alex, On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote: On 01/09/2013 02:50 PM, Namhyung Kim wrote: From: Namhyung Kim namhyung@lge.com AFAICS @target cpu of select_idle_sibling() is always either prev_cpu or this_cpu. So no need to check it again and the conditionals can be consolidated. [snip] Uh, we don't know if the target is this_cpu or previous cpu, If we just check the target idle status, we may miss another idle cpu. So this patch change the logical in this function. select_idle_sibling() is called only in select_task_rq_fair() if it found a suitable affine_sd. The default target is the 'prev_cpu' of the task but if wake_affine() returns true it'd be (this) 'cpu'. I cannot see where the prev_cpu or the cpu is set to another one before calling select_idle_sibling. Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
