> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
> ---
>   arch/ia64/include/asm/topology.h |    1 +
>   arch/tile/include/asm/topology.h |    1 +
>   include/linux/sched.h            |    1 +
>   include/linux/topology.h         |    3 +++
>   kernel/sched/core.c              |    5 +++++
>   5 files changed, 11 insertions(+)

> diff --git a/arch/ia64/include/asm/topology.h b/arch/ia64/include/asm/topology.h
> index a2496e4..065c720 100644
> --- a/arch/ia64/include/asm/topology.h
> +++ b/arch/ia64/include/asm/topology.h
> @@ -65,6 +65,7 @@ void build_cpu_to_node_map(void);
>                            | SD_BALANCE_EXEC       \
>                            | SD_BALANCE_FORK       \
>                            | SD_WAKE_AFFINE,       \
> +                          | arch_sd_share_power_line()            \
>    .last_balance           = jiffies,              \
>    .balance_interval       = 1,                    \
>    .nr_balance_failed      = 0,                    \
> diff --git a/arch/tile/include/asm/topology.h b/arch/tile/include/asm/topology.h
> index 7a7ce39..d39ed0b 100644
> --- a/arch/tile/include/asm/topology.h
> +++ b/arch/tile/include/asm/topology.h
> @@ -72,6 +72,7 @@ static inline const struct cpumask *cpumask_of_node(int node)
>                            | 0*SD_PREFER_LOCAL                     \
>                            | 0*SD_SHARE_CPUPOWER                   \
>                            | 0*SD_SHARE_PKG_RESOURCES              \
> +                          | arch_sd_share_power_line()            \
>                            | 0*SD_SERIALIZE                        \
>                            ,                                       \
>    .last_balance           = jiffies,                              \
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 4786b20..74f2daf 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -862,6 +862,7 @@ enum cpu_idle_type {
>   #define SD_WAKE_AFFINE           0x0020  /* Wake task to waking CPU */
>   #define SD_PREFER_LOCAL          0x0040  /* Prefer to keep tasks local to this domain */
>   #define SD_SHARE_CPUPOWER        0x0080  /* Domain members share cpu power */
> +#define SD_SHARE_POWERLINE        0x0100  /* Domain members share power domain */

> On a system that share the powerline at all level, the buddy is set to -1

> On a dual clusters / dual cores system which can powergate each core and
> cluster independantly, the buddy configuration will be :
>        | CPU0 | CPU1 | CPU2 | CPU3 |
> -----------------------------------
> buddy | CPU0 | CPU0 | CPU0 | CPU2 |

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index dab7908..70cadbe 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6131,6 +6131,7 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu)
>    rcu_assign_pointer(rq->sd, sd);
>    destroy_sched_domains(tmp, cpu);

> +  update_packing_domain(cpu);
>    update_top_cache_domain(cpu);
>   }

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 4f4a4f6..8c9d3ed 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -157,6 +157,63 @@ void sched_init_granularity(void)
>    update_sysctl();
>   }

> +
> +/*
> + * Save the id of the optimal CPU that should be used to pack small tasks
> + * The value -1 is used when no buddy has been found
> + */
> +DEFINE_PER_CPU(int, sd_pack_buddy);
> +
> +/* Look for the best buddy CPU that can be used to pack small tasks
> + * We make the assumption that it doesn't wort to pack on CPU that share the
s/wort/worth
> + * same powerline. We looks for the 1st sched_domain without the
> + * SD_SHARE_POWERLINE flag. Then We look for the sched_group witht the lowest
> + * power per core based on the assumption that their power efficiency is
> + * better */

> +static inline bool is_buddy_busy(int cpu)
> +{
> +  struct rq *rq = cpu_rq(cpu);
> +
> +  /*
> +   * A busy buddy is a CPU with a high load or a small load with a lot of
> +   * running tasks.
> +   */
> +  return ((rq->avg.usage_avg_sum << rq->nr_running) >
> +                  rq->avg.runnable_avg_period);

> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
> ---
>   kernel/sched/fair.c |   18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 6df53b5..f76acdc 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5158,7 +5158,25 @@ static struct {

>   static inline int find_new_ilb(int call_cpu)
>   {
> +  struct sched_domain *sd;
>    int ilb = cpumask_first(nohz.idle_cpus_mask);
> +  int buddy = per_cpu(sd_pack_buddy, call_cpu);
> +
> +  /*
> +   * If we have a pack buddy CPU, we try to run load balance on a CPU
> +   * that is close to the buddy.
> +   */
> +  if (buddy != -1)
> +          for_each_domain(buddy, sd) {
> +                  if (sd->flags & SD_SHARE_CPUPOWER)
> +                          continue;

>    if (ilb < nr_cpu_ids && idle_cpu(ilb))
>            return ilb;

> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
> ---
>   kernel/sched/fair.c |   19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8c9d3ed..6df53b5 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3133,13 +3133,28 @@ static int select_idle_sibling(struct task_struct *p, int target)
>   static inline bool is_buddy_busy(int cpu)
>   {
>    struct rq *rq = cpu_rq(cpu);
> +  volatile u32 *psum = &rq->avg.runnable_avg_sum;
> +  volatile u32 *pperiod = &rq->avg.runnable_avg_period;
> +  u32 sum, new_sum, period, new_period;
> +  int timeout = 10;

> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
> ---
>   arch/arm/kernel/topology.c |    5 +++++
>   1 file changed, 5 insertions(+)

> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 26c12c6..00511d0 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -226,6 +226,11 @@ static inline void update_cpu_power(unsigned int cpuid, unsigned int mpidr) {}
>    */
>   struct cputopo_arm cpu_topology[NR_CPUS];

> +int arch_sd_share_power_line(void)
> +{
> +  return 0*SD_SHARE_POWERLINE;
> +}

> Few comments/questions.

> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

>> This new flag SD SHARE_POWERLINE reflects the sharing of the power rail
>> between the members of a domain. As this is the current assumption of the
>> scheduler, the flag is added to all sched_domain

>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>> ---
>>   arch/ia64/include/asm/topology.h |    1 +
>>   arch/tile/include/asm/topology.h |    1 +
>>   include/linux/sched.h            |    1 +
>>   include/linux/topology.h         |    3 +++
>>   kernel/sched/core.c              |    5 +++++
>>   5 files changed, 11 insertions(+)

>> diff --git a/arch/ia64/include/asm/topology.h
>> b/arch/ia64/include/asm/topology.h
>> index a2496e4..065c720 100644
>> --- a/arch/ia64/include/asm/topology.h
>> +++ b/arch/ia64/include/asm/topology.h
>> @@ -65,6 +65,7 @@ void build_cpu_to_node_map(void);
>>                                 | SD_BALANCE_EXEC       \
>>                                 | SD_BALANCE_FORK       \
>>                                 | SD_WAKE_AFFINE,       \
>> +                               | arch_sd_share_power_line()            \
>>         .last_balance           = jiffies,              \
>>         .balance_interval       = 1,                    \
>>         .nr_balance_failed      = 0,                    \
>> diff --git a/arch/tile/include/asm/topology.h
>> b/arch/tile/include/asm/topology.h
>> index 7a7ce39..d39ed0b 100644
>> --- a/arch/tile/include/asm/topology.h
>> +++ b/arch/tile/include/asm/topology.h
>> @@ -72,6 +72,7 @@ static inline const struct cpumask *cpumask_of_node(int
>> node)
>>                                 | 0*SD_PREFER_LOCAL                     \
>>                                 | 0*SD_SHARE_CPUPOWER                   \
>>                                 | 0*SD_SHARE_PKG_RESOURCES              \
>> +                               | arch_sd_share_power_line()            \
>>                                 | 0*SD_SERIALIZE                        \
>>                                 ,                                       \
>>         .last_balance           = jiffies,                              \
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 4786b20..74f2daf 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -862,6 +862,7 @@ enum cpu_idle_type {
>>   #define SD_WAKE_AFFINE                0x0020  /* Wake task to waking CPU
>> */
>>   #define SD_PREFER_LOCAL               0x0040  /* Prefer to keep tasks
>> local to this domain */
>>   #define SD_SHARE_CPUPOWER     0x0080  /* Domain members share cpu power
>> */
>> +#define SD_SHARE_POWERLINE     0x0100  /* Domain members share power
>> domain */

> If you ignore the current use of SD_SHARE_CPUPOWER, isn't the meaning of
> CPUPOWER and POWERLINE is same here. Just trying to understand the clear
> meaning of this new flag. Have you not considered SD_SHARE_CPUPOWER
> because it is being used for cpu_power and needs at least minimum two
> domains ? SD_PACKING would have been probably more appropriate based
> on the way it is being used in further series.

> Regards
> Santosh

>> Few comments/questions.

>> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

>>> This new flag SD SHARE_POWERLINE reflects the sharing of the power rail
>>> between the members of a domain. As this is the current assumption of the
>>> scheduler, the flag is added to all sched_domain

>>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>>> ---
>>>   arch/ia64/include/asm/topology.h |    1 +
>>>   arch/tile/include/asm/topology.h |    1 +
>>>   include/linux/sched.h            |    1 +
>>>   include/linux/topology.h         |    3 +++
>>>   kernel/sched/core.c              |    5 +++++
>>>   5 files changed, 11 insertions(+)

>>> diff --git a/arch/ia64/include/asm/topology.h
>>> b/arch/ia64/include/asm/topology.h
>>> index a2496e4..065c720 100644
>>> --- a/arch/ia64/include/asm/topology.h
>>> +++ b/arch/ia64/include/asm/topology.h
>>> @@ -65,6 +65,7 @@ void build_cpu_to_node_map(void);
>>>                                 | SD_BALANCE_EXEC       \
>>>                                 | SD_BALANCE_FORK       \
>>>                                 | SD_WAKE_AFFINE,       \
>>> +                               | arch_sd_share_power_line()            \
>>>         .last_balance           = jiffies,              \
>>>         .balance_interval       = 1,                    \
>>>         .nr_balance_failed      = 0,                    \
>>> diff --git a/arch/tile/include/asm/topology.h
>>> b/arch/tile/include/asm/topology.h
>>> index 7a7ce39..d39ed0b 100644
>>> --- a/arch/tile/include/asm/topology.h
>>> +++ b/arch/tile/include/asm/topology.h
>>> @@ -72,6 +72,7 @@ static inline const struct cpumask *cpumask_of_node(int
>>> node)
>>>                                 | 0*SD_PREFER_LOCAL                     \
>>>                                 | 0*SD_SHARE_CPUPOWER                   \
>>>                                 | 0*SD_SHARE_PKG_RESOURCES              \
>>> +                               | arch_sd_share_power_line()            \
>>>                                 | 0*SD_SERIALIZE                        \
>>>                                 ,                                       \
>>>         .last_balance           = jiffies,                              \
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index 4786b20..74f2daf 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -862,6 +862,7 @@ enum cpu_idle_type {
>>>   #define SD_WAKE_AFFINE                0x0020  /* Wake task to waking CPU
>>> */
>>>   #define SD_PREFER_LOCAL               0x0040  /* Prefer to keep tasks
>>> local to this domain */
>>>   #define SD_SHARE_CPUPOWER     0x0080  /* Domain members share cpu power
>>> */
>>> +#define SD_SHARE_POWERLINE     0x0100  /* Domain members share power
>>> domain */

>> If you ignore the current use of SD_SHARE_CPUPOWER, isn't the meaning of
>> CPUPOWER and POWERLINE is same here. Just trying to understand the clear
>> meaning of this new flag. Have you not considered SD_SHARE_CPUPOWER
>> because it is being used for cpu_power and needs at least minimum two
>> domains ? SD_PACKING would have been probably more appropriate based
>> on the way it is being used in further series.

> CPUPOWER reflects the share of hw ressources between cores like for
> hyper threading. POWERLINE describes the fact that cores are sharing
> the same power line amore precisely the powergate.

>> Regards
>> Santosh

> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

>> The atomic update of runnable_avg_sum and runnable_avg_period are ensured
>> by their size and the toolchain. But we must ensure to not read an old
>> value
>> for one field and a newly updated value for the other field. As we don't
>> want to lock other CPU while reading these fields, we read twice each
>> fields
>> and check that no change have occured in the middle.

>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>> ---
>>   kernel/sched/fair.c |   19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)

>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 8c9d3ed..6df53b5 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -3133,13 +3133,28 @@ static int select_idle_sibling(struct task_struct
>> *p, int target)
>>   static inline bool is_buddy_busy(int cpu)
>>   {
>>         struct rq *rq = cpu_rq(cpu);
>> +       volatile u32 *psum = &rq->avg.runnable_avg_sum;
>> +       volatile u32 *pperiod = &rq->avg.runnable_avg_period;
>> +       u32 sum, new_sum, period, new_period;
>> +       int timeout = 10;

> So it can be 2 times read or more as well.

>> +
>> +       while (timeout) {
>> +               sum = *psum;
>> +               period = *pperiod;
>> +               new_sum = *psum;
>> +               new_period = *pperiod;
>> +
>> +               if ((sum == new_sum) && (period == new_period))
>> +                       break;
>> +
>> +               timeout--;
>> +       }

> Seems like you did notice incorrect pair getting read
> for rq runnable_avg_sum and runnable_avg_period. Seems
> like the fix is to update them together under some lock
> to avoid such issues.

> Regards
> Santosh

> Few comments/questions.

> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

>> During sched_domain creation, we define a pack buddy CPU if available.

>> On a system that share the powerline at all level, the buddy is set to -1

>> On a dual clusters / dual cores system which can powergate each core and
>> cluster independantly, the buddy configuration will be :
>>        | CPU0 | CPU1 | CPU2 | CPU3 |
>> -----------------------------------
>> buddy | CPU0 | CPU0 | CPU0 | CPU2 |

>                         ^
> Is that a typo ? Should it be CPU2 instead of
> CPU0 ?

>> Small tasks tend to slip out of the periodic load balance.
>> The best place to choose to migrate them is at their wake up.

> I have tried this series since I was looking at some of these packing
> bits. On Mobile workloads like OSIdle with Screen ON, MP3, gallary,
> I did see some additional filtering of threads with this series
> but its not making much difference in power. More on this below.

>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>> ---
>>   kernel/sched/core.c  |    1 +
>>   kernel/sched/fair.c  |  109
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   kernel/sched/sched.h |    1 +
>>   3 files changed, 111 insertions(+)

>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index dab7908..70cadbe 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -6131,6 +6131,7 @@ cpu_attach_domain(struct sched_domain *sd, struct
>> root_domain *rd, int cpu)
>>         rcu_assign_pointer(rq->sd, sd);
>>         destroy_sched_domains(tmp, cpu);

>> +       update_packing_domain(cpu);
>>         update_top_cache_domain(cpu);
>>   }

>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 4f4a4f6..8c9d3ed 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -157,6 +157,63 @@ void sched_init_granularity(void)
>>         update_sysctl();
>>   }

>> +
>> +/*
>> + * Save the id of the optimal CPU that should be used to pack small tasks
>> + * The value -1 is used when no buddy has been found
>> + */
>> +DEFINE_PER_CPU(int, sd_pack_buddy);
>> +
>> +/* Look for the best buddy CPU that can be used to pack small tasks
>> + * We make the assumption that it doesn't wort to pack on CPU that share
>> the

> s/wort/worth

>> + * same powerline. We looks for the 1st sched_domain without the
>> + * SD_SHARE_POWERLINE flag. Then We look for the sched_group witht the
>> lowest
>> + * power per core based on the assumption that their power efficiency is
>> + * better */

> Commenting style..
> /*
>  *
>  */

> Think about a scenario where you have quad core, ducal cluster system

>         |Cluster1|                      |cluster 2|
> | CPU0 | CPU1 | CPU2 | CPU3 |   | CPU0 | CPU1 | CPU2 | CPU3 |

> Both clusters run from same voltage rail and have same PLL
> clocking them. But the cluster have their own power domain
> and all CPU's can power gate them-self to low power states.
> Clusters also have their own level2 caches.

> In this case, you will still save power if you try to pack
> load on one cluster. No ?

>> +void update_packing_domain(int cpu)
>> +{
>> +       struct sched_domain *sd;
>> +       int id = -1;
>> +
>> +       sd = highest_flag_domain(cpu, SD_SHARE_POWERLINE);
>> +       if (!sd)
>> +               sd = rcu_dereference_check_sched_domain(cpu_rq(cpu)->sd);
>> +       else
>> +               sd = sd->parent;
>> +
>> +       while (sd) {
>> +               struct sched_group *sg = sd->groups;
>> +               struct sched_group *pack = sg;
>> +               struct sched_group *tmp = sg->next;
>> +
>> +               /* 1st CPU of the sched domain is a good candidate */
>> +               if (id == -1)
>> +                       id = cpumask_first(sched_domain_span(sd));
>> +
>> +               /* loop the sched groups to find the best one */
>> +               while (tmp != sg) {
>> +                       if (tmp->sgp->power * sg->group_weight <
>> +                                       sg->sgp->power *
>> tmp->group_weight)
>> +                               pack = tmp;
>> +                       tmp = tmp->next;
>> +               }
>> +
>> +               /* we have found a better group */
>> +               if (pack != sg)
>> +                       id = cpumask_first(sched_group_cpus(pack));
>> +
>> +               /* Look for another CPU than itself */
>> +               if ((id != cpu)
>> +                || ((sd->parent) && !(sd->parent->flags &&
>> SD_LOAD_BALANCE)))

> Is the condition "!(sd->parent->flags && SD_LOAD_BALANCE)" for
> big.LITTLE kind of system where SD_LOAD_BALANCE may not be used ?

>> +                       break;
>> +
>> +               sd = sd->parent;
>> +       }
>> +
>> +       pr_info(KERN_INFO "CPU%d packing on CPU%d\n", cpu, id);
>> +       per_cpu(sd_pack_buddy, cpu) = id;
>> +}
>> +
>>   #if BITS_PER_LONG == 32
>>   # define WMULT_CONST  (~0UL)
>>   #else
>> @@ -3073,6 +3130,55 @@ static int select_idle_sibling(struct task_struct
>> *p, int target)
>>         return target;
>>   }

>> +static inline bool is_buddy_busy(int cpu)
>> +{
>> +       struct rq *rq = cpu_rq(cpu);
>> +
>> +       /*
>> +        * A busy buddy is a CPU with a high load or a small load with a
>> lot of
>> +        * running tasks.
>> +        */
>> +       return ((rq->avg.usage_avg_sum << rq->nr_running) >
>> +                       rq->avg.runnable_avg_period);

> I agree busy CPU is the one with high load, but many small threads may
> not make CPU fully busy, right ? Should we just stick to the load
> parameter alone here ?

>> +}
>> +
>> +static inline bool is_light_task(struct task_struct *p)
>> +{
>> +       /* A light task runs less than 25% in average */
>> +       return ((p->se.avg.usage_avg_sum << 2) <
>> p->se.avg.runnable_avg_period);
>> +}

> Since the whole packing logic relies on the light threads only, the
> overall effectiveness is not significant. Infact with multiple tries on
> Dual core system, I didn't see any major improvement in power. I think
> we need to be more aggressive here. From the cover letter, I noticed
> that, you were concerned about any performance drop due to packing and
> may be that is the reason you chose the conservative threshold. But the
> fact is, if we want to save meaningful power, there will be slight
> performance drop which is expected.

>> +static int check_pack_buddy(int cpu, struct task_struct *p)
>> +{
>> +       int buddy = per_cpu(sd_pack_buddy, cpu);
>> +
>> +       /* No pack buddy for this CPU */
>> +       if (buddy == -1)
>> +               return false;
>> +
>> +       /*
>> +        * If a task is waiting for running on the CPU which is its own
>> buddy,
>> +        * let the default behavior to look for a better CPU if available
>> +        * The threshold has been set to 37.5%
>> +        */
>> +       if ((buddy == cpu)
>> +        && ((p->se.avg.usage_avg_sum << 3) < (p->se.avg.runnable_avg_sum
>> * 5)))
>> +               return false;

> I lost you here on better CPU , 37.5 % and last two conditions.
> Isn't the first condition 'buddy==cpu' enough to return since nothing really
> needs to be done in that case. Can you please expand this a bit?

>> +
>> +       /* buddy is not an allowed CPU */
>> +       if (!cpumask_test_cpu(buddy, tsk_cpus_allowed(p)))
>> +               return false;
>> +
>> +       /*
>> +        * If the task is a small one and the buddy is not overloaded,
>> +        * we use buddy cpu
>> +        */
>> +        if (!is_light_task(p) || is_buddy_busy(buddy))
>> +              

>> The ARM platforms take advantage of packing small tasks on few cores.
>> This is true even when the cores of a cluster can't be powergated
>> independently.

>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>> ---
>>   arch/arm/kernel/topology.c |    5 +++++
>>   1 file changed, 5 insertions(+)

>> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>> index 26c12c6..00511d0 100644
>> --- a/arch/arm/kernel/topology.c
>> +++ b/arch/arm/kernel/topology.c
>> @@ -226,6 +226,11 @@ static inline void update_cpu_power(unsigned int
>> cpuid, unsigned int mpidr) {}
>>    */
>>   struct cputopo_arm cpu_topology[NR_CPUS];

>> +int arch_sd_share_power_line(void)
>> +{
>> +       return 0*SD_SHARE_POWERLINE;
>> +}

> Making this selection of policy based on sched domain will better. Just
> gives the flexibility to choose a separate scheme for big and little
> systems which will be very convenient.

> Regards
> Santosh

>> Look for an idle CPU close the pack buddy CPU whenever possible.

> s/close/close to

>> The goal is to prevent the wake up of a CPU which doesn't share the power
>> line of the pack CPU

>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>> ---
>>   kernel/sched/fair.c |   18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)

>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 6df53b5..f76acdc 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5158,7 +5158,25 @@ static struct {

>>   static inline int find_new_ilb(int call_cpu)
>>   {
>> +       struct sched_domain *sd;
>>         int ilb = cpumask_first(nohz.idle_cpus_mask);
>> +       int buddy = per_cpu(sd_pack_buddy, call_cpu);
>> +
>> +       /*
>> +        * If we have a pack buddy CPU, we try to run load balance on a
>> CPU
>> +        * that is close to the buddy.
>> +        */
>> +       if (buddy != -1)
>> +               for_each_domain(buddy, sd) {
>> +                       if (sd->flags & SD_SHARE_CPUPOWER)
>> +                               continue;

> Do you mean SD_SHARE_POWERLINE here ?

>> +
>> +                       ilb = cpumask_first_and(sched_domain_span(sd),
>> +                                       nohz.idle_cpus_mask);
>> +
>> +                       if (ilb < nr_cpu_ids)
>> +                               break;
>> +               }

>>         if (ilb < nr_cpu_ids && idle_cpu(ilb))
>>                 return ilb;

> Can you please expand "idle CPU _close_ the pack buddy CPU" ?

> Regards
> santosh

> On 29 October 2012 10:40, Vincent Guittot <vincent.guit...@linaro.org> wrote:
>> On 24 October 2012 17:17, Santosh Shilimkar <santosh.shilim...@ti.com> wrote:
>>> Vincent,

>>> Few comments/questions.

>>> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

>>>> This new flag SD SHARE_POWERLINE reflects the sharing of the power rail
>>>> between the members of a domain. As this is the current assumption of the
>>>> scheduler, the flag is added to all sched_domain

>>>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>>>> ---
>>>>    arch/ia64/include/asm/topology.h |    1 +
>>>>    arch/tile/include/asm/topology.h |    1 +
>>>>    include/linux/sched.h            |    1 +
>>>>    include/linux/topology.h         |    3 +++
>>>>    kernel/sched/core.c              |    5 +++++
>>>>    5 files changed, 11 insertions(+)

>>>> diff --git a/arch/ia64/include/asm/topology.h
>>>> b/arch/ia64/include/asm/topology.h
>>>> index a2496e4..065c720 100644
>>>> --- a/arch/ia64/include/asm/topology.h
>>>> +++ b/arch/ia64/include/asm/topology.h
>>>> @@ -65,6 +65,7 @@ void build_cpu_to_node_map(void);
>>>>                                  | SD_BALANCE_EXEC       \
>>>>                                  | SD_BALANCE_FORK       \
>>>>                                  | SD_WAKE_AFFINE,       \
>>>> +                               | arch_sd_share_power_line()            \
>>>>          .last_balance           = jiffies,              \
>>>>          .balance_interval       = 1,                    \
>>>>          .nr_balance_failed      = 0,                    \
>>>> diff --git a/arch/tile/include/asm/topology.h
>>>> b/arch/tile/include/asm/topology.h
>>>> index 7a7ce39..d39ed0b 100644
>>>> --- a/arch/tile/include/asm/topology.h
>>>> +++ b/arch/tile/include/asm/topology.h
>>>> @@ -72,6 +72,7 @@ static inline const struct cpumask *cpumask_of_node(int
>>>> node)
>>>>                                  | 0*SD_PREFER_LOCAL                     \
>>>>                                  | 0*SD_SHARE_CPUPOWER                   \
>>>>                                  | 0*SD_SHARE_PKG_RESOURCES              \
>>>> +                               | arch_sd_share_power_line()            \
>>>>                                  | 0*SD_SERIALIZE                        \
>>>>                                  ,                                       \
>>>>          .last_balance           = jiffies,                              \
>>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>>> index 4786b20..74f2daf 100644
>>>> --- a/include/linux/sched.h
>>>> +++ b/include/linux/sched.h
>>>> @@ -862,6 +862,7 @@ enum cpu_idle_type {
>>>>    #define SD_WAKE_AFFINE                0x0020  /* Wake task to waking CPU
>>>> */
>>>>    #define SD_PREFER_LOCAL               0x0040  /* Prefer to keep tasks
>>>> local to this domain */
>>>>    #define SD_SHARE_CPUPOWER     0x0080  /* Domain members share cpu power
>>>> */
>>>> +#define SD_SHARE_POWERLINE     0x0100  /* Domain members share power
>>>> domain */

>>> If you ignore the current use of SD_SHARE_CPUPOWER, isn't the meaning of
>>> CPUPOWER and POWERLINE is same here. Just trying to understand the clear
>>> meaning of this new flag. Have you not considered SD_SHARE_CPUPOWER
>>> because it is being used for cpu_power and needs at least minimum two
>>> domains ? SD_PACKING would have been probably more appropriate based
>>> on the way it is being used in further series.

>> CPUPOWER reflects the share of hw ressources between cores like for
>> hyper threading. POWERLINE describes the fact that cores are sharing
>> the same power line amore precisely the powergate.

> Sorry, the mail has been sent too early while I was writing it

> CPUPOWER reflects the share of hw ressources between cores like for
> hyper threading. POWERLINE describes the fact that cores are sharing
> the same power line and more precisely the same power gating. It looks
> like I need to describe more precisely what i would mean with
> SHARE_POWERLINE.

>>> Look for an idle CPU close the pack buddy CPU whenever possible.

>> s/close/close to

> yes

>>> The goal is to prevent the wake up of a CPU which doesn't share the power
>>> line of the pack CPU

>>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>>> ---
>>>    kernel/sched/fair.c |   18 ++++++++++++++++++
>>>    1 file changed, 18 insertions(+)

>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 6df53b5..f76acdc 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -5158,7 +5158,25 @@ static struct {

>>>    static inline int find_new_ilb(int call_cpu)
>>>    {
>>> +       struct sched_domain *sd;
>>>          int ilb = cpumask_first(nohz.idle_cpus_mask);
>>> +       int buddy = per_cpu(sd_pack_buddy, call_cpu);
>>> +
>>> +       /*
>>> +        * If we have a pack buddy CPU, we try to run load balance on a
>>> CPU
>>> +        * that is close to the buddy.
>>> +        */
>>> +       if (buddy != -1)
>>> +               for_each_domain(buddy, sd) {
>>> +                       if (sd->flags & SD_SHARE_CPUPOWER)
>>> +                               continue;

>> Do you mean SD_SHARE_POWERLINE here ?

> No, I just don't want to take hyperthread level for ILB

>>> +
>>> +                       ilb = cpumask_first_and(sched_domain_span(sd),
>>> +                                       nohz.idle_cpus_mask);
>>> +
>>> +                       if (ilb < nr_cpu_ids)
>>> +                               break;
>>> +               }

>>>          if (ilb < nr_cpu_ids && idle_cpu(ilb))
>>>                  return ilb;

>> Can you please expand "idle CPU _close_ the pack buddy CPU" ?

> The goal is to packed  the tasks on the pack buddy CPU so when the
> scheduler needs to start ILB, I try to wake up a CPU that is close to
> the buddy and preferably in the same cluster

>> Few comments/questions.

>> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

>>> During sched_domain creation, we define a pack buddy CPU if available.

>>> On a system that share the powerline at all level, the buddy is set to -1

>>> On a dual clusters / dual cores system which can powergate each core and
>>> cluster independantly, the buddy configuration will be :
>>>         | CPU0 | CPU1 | CPU2 | CPU3 |
>>> -----------------------------------
>>> buddy | CPU0 | CPU0 | CPU0 | CPU2 |

>>                          ^
>> Is that a typo ? Should it be CPU2 instead of
>> CPU0 ?

> No it's not a typo.
> The system packs at each scheduling level. It starts to pack in
> cluster because each core can power gate independently so CPU1 tries
> to pack its tasks in CPU0 and CPU3 in CPU2. Then, it packs at CPU
> level so CPU2 tries to pack in the cluster of CPU0 and CPU0 packs in
> itself

>>> Small tasks tend to slip out of the periodic load balance.
>>> The best place to choose to migrate them is at their wake up.

>> I have tried this series since I was looking at some of these packing
>> bits. On Mobile workloads like OSIdle with Screen ON, MP3, gallary,
>> I did see some additional filtering of threads with this series
>> but its not making much difference in power. More on this below.

> Can I ask you which configuration you have used ? how many cores and
> cluster ?  Can they be power gated independently ?

>>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>>> ---
>>>    kernel/sched/core.c  |    1 +
>>>    kernel/sched/fair.c  |  109
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    kernel/sched/sched.h |    1 +
>>>    3 files changed, 111 insertions(+)

>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>> index dab7908..70cadbe 100644
>>> --- a/kernel/sched/core.c
>>> +++ b/kernel/sched/core.c
>>> @@ -6131,6 +6131,7 @@ cpu_attach_domain(struct sched_domain *sd, struct
>>> root_domain *rd, int cpu)
>>>          rcu_assign_pointer(rq->sd, sd);
>>>          destroy_sched_domains(tmp, cpu);

>>> +       update_packing_domain(cpu);
>>>          update_top_cache_domain(cpu);
>>>    }

>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 4f4a4f6..8c9d3ed 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -157,6 +157,63 @@ void sched_init_granularity(void)
>>>          update_sysctl();
>>>    }

>>> +
>>> +/*
>>> + * Save the id of the optimal CPU that should be used to pack small tasks
>>> + * The value -1 is used when no buddy has been found
>>> + */
>>> +DEFINE_PER_CPU(int, sd_pack_buddy);
>>> +
>>> +/* Look for the best buddy CPU that can be used to pack small tasks
>>> + * We make the assumption that it doesn't wort to pack on CPU that share
>>> the

>> s/wort/worth

> yes

>>> + * same powerline. We looks for the 1st sched_domain without the
>>> + * SD_SHARE_POWERLINE flag. Then We look for the sched_group witht the
>>> lowest
>>> + * power per core based on the assumption that their power efficiency is
>>> + * better */

>> Commenting style..
>> /*
>>   *
>>   */

> yes

>> Can you please expand the why the assumption is right ?
>> "it doesn't wort to pack on CPU that share the same powerline"

> By "share the same power-line", I mean that the CPUs can't power off
> independently. So if some CPUs can't power off independently, it's
> worth to try to use most of them to race to idle.

>> Think about a scenario where you have quad core, ducal cluster system

>>          |Cluster1|                      |cluster 2|
>> | CPU0 | CPU1 | CPU2 | CPU3 |   | CPU0 | CPU1 | CPU2 | CPU3 |

>> Both clusters run from same voltage rail and have same PLL
>> clocking them. But the cluster have their own power domain
>> and all CPU's can power gate them-self to low power states.
>> Clusters also have their own level2 caches.

>> In this case, you will still save power if you try to pack
>> load on one cluster. No ?

> yes, I need to update the description of SD_SHARE_POWERLINE because
> I'm afraid I was not clear enough. SD_SHARE_POWERLINE includes the
> power gating capacity of each core. For your example above, the
> SD_SHARE_POWERLINE shoud be cleared at both MC and CPU level.

>>> +void update_packing_domain(int cpu)
>>> +{
>>> +       struct sched_domain *sd;
>>> +       int id = -1;
>>> +
>>> +       sd = highest_flag_domain(cpu, SD_SHARE_POWERLINE);
>>> +       if (!sd)
>>> +               sd = rcu_dereference_check_sched_domain(cpu_rq(cpu)->sd);
>>> +       else
>>> +               sd = sd->parent;
>>> +
>>> +       while (sd) {
>>> +               struct sched_group *sg = sd->groups;
>>> +               struct sched_group *pack = sg;
>>> +               struct sched_group *tmp = sg->next;
>>> +
>>> +               /* 1st CPU of the sched domain is a good candidate */
>>> +               if (id == -1)
>>> +                       id = cpumask_first(sched_domain_span(sd));
>>> +
>>> +               /* loop the sched groups to find the best one */
>>> +               while (tmp != sg) {
>>> +                       if (tmp->sgp->power * sg->group_weight <
>>> +                                       sg->sgp->power *
>>> tmp->group_weight)
>>> +                               pack = tmp;
>>> +                       tmp = tmp->next;
>>> +               }
>>> +
>>> +               /* we have found a better group */
>>> +               if (pack != sg)
>>> +                       id = cpumask_first(sched_group_cpus(pack));
>>> +
>>> +               /* Look for another CPU than itself */
>>> +               if ((id != cpu)
>>> +                || ((sd->parent) && !(sd->parent->flags &&
>>> SD_LOAD_BALANCE)))

>> Is the condition "!(sd->parent->flags && SD_LOAD_BALANCE)" for
>> big.LITTLE kind of system where SD_LOAD_BALANCE may not be used ?

> No, packing small tasks is part of the load balance so if the
> LOAD_BALANCE flag is cleared, we will not try to pack which is a kind
> of load balance. There is no link with big.LITTLE

>>> +                       break;
>>> +
>>> +               sd = sd->parent;
>>> +       }
>>> +
>>> +       pr_info(KERN_INFO "CPU%d packing on CPU%d\n", cpu, id);
>>> +       per_cpu(sd_pack_buddy, cpu) = id;
>>> +}
>>> +
>>>    #if BITS_PER_LONG == 32
>>>    # define WMULT_CONST  (~0UL)
>>>    #else
>>> @@ -3073,6 +3130,55 @@ static int select_idle_sibling(struct task_struct
>>> *p, int target)
>>>          return target;
>>>    }

>>> +static inline bool is_buddy_busy(int cpu)
>>> +{
>>> +       struct rq *rq = cpu_rq(cpu);
>>> +
>>> +       /*
>>> +        * A busy buddy is a CPU with a high load or a small load with a
>>> lot of
>>> +        * running tasks.
>>> +        */
>>> +       return ((rq->avg.usage_avg_sum << rq->nr_running) >
>>> +                       rq->avg.runnable_avg_period);

>> I agree busy CPU is the one with high load, but many small threads may
>> not make CPU fully busy, right ? Should we just stick to the load
>> parameter alone here ?

> IMO, the busy state of a CPU isn't only the load but also how many
> threads are waiting for running on it. This formula tries to take into
> account both inputs. If you have dozen of small tasks on a CPU, the
> latency can be large even if the tasks are small.

>>> +}
>>> +
>>> +static inline bool is_light_task(struct task_struct *p)
>>> +{
>>> +       /* A light task runs less than 25% in average */
>>> +       return ((p->se.avg.usage_avg_sum << 2) <
>>> p->se.avg.runnable_avg_period);
>>> +}

>> Since the whole packing logic relies on the light threads only, the
>> overall effectiveness is not significant. Infact with multiple tries on
>> Dual core system, I didn't see any major improvement in power. I think
>> we need to be more aggressive here. From the cover letter, I noticed
>> that, you were concerned about any performance drop due to packing and
>> may be that is the reason you chose the conservative threshold. But the
>> fact is, if we want to save meaningful power, there will be slight
>> performance drop which is expected.

> I think that everybody agrees that packing small tasks will save power
> whereas it seems to be not so obvious for heavy task. But I may have
> set the threshold a bit too low

>>> The ARM platforms take advantage of packing small tasks on few cores.
>>> This is true even when the cores of a cluster can't be powergated
>>> independently.

>>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>>> ---
>>>    arch/arm/kernel/topology.c |    5 +++++
>>>    1 file changed, 5 insertions(+)

>>> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>>> index 26c12c6..00511d0 100644
>>> --- a/arch/arm/kernel/topology.c
>>> +++ b/arch/arm/kernel/topology.c
>>> @@ -226,6 +226,11 @@ static inline void update_cpu_power(unsigned int
>>> cpuid, unsigned int mpidr) {}
>>>     */
>>>    struct cputopo_arm cpu_topology[NR_CPUS];

>>> +int arch_sd_share_power_line(void)
>>> +{
>>> +       return 0*SD_SHARE_POWERLINE;
>>> +}

>> Making this selection of policy based on sched domain will better. Just
>> gives the flexibility to choose a separate scheme for big and little
>> systems which will be very convenient.

> I agree that it would be more flexible to be able to set it for each level

> >> Few comments/questions.

> >> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

> >>> During sched_domain creation, we define a pack buddy CPU if available.

> >>> On a system that share the powerline at all level, the buddy is set to -1

> >>> On a dual clusters / dual cores system which can powergate each core and
> >>> cluster independantly, the buddy configuration will be :
> >>>         | CPU0 | CPU1 | CPU2 | CPU3 |
> >>> -----------------------------------
> >>> buddy | CPU0 | CPU0 | CPU0 | CPU2 |

> >>                          ^
> >> Is that a typo ? Should it be CPU2 instead of
> >> CPU0 ?

> > No it's not a typo.
> > The system packs at each scheduling level. It starts to pack in
> > cluster because each core can power gate independently so CPU1 tries
> > to pack its tasks in CPU0 and CPU3 in CPU2. Then, it packs at CPU
> > level so CPU2 tries to pack in the cluster of CPU0 and CPU0 packs in
> > itself

> I get it. Though in above example a task may migrate from say
> CPU3->CPU2->CPU0 as part of packing. I was just thinking whether
> moving such task from say CPU3 to CPU0 might be best instead.

> >>> Small tasks tend to slip out of the periodic load balance.
> >>> The best place to choose to migrate them is at their wake up.

> >> I have tried this series since I was looking at some of these packing
> >> bits. On Mobile workloads like OSIdle with Screen ON, MP3, gallary,
> >> I did see some additional filtering of threads with this series
> >> but its not making much difference in power. More on this below.

> > Can I ask you which configuration you have used ? how many cores and
> > cluster ?  Can they be power gated independently ?

> I have been trying with couple of setups. Dual Core ARM machine and
> Quad core X86 box with single package thought most of the mobile
> workload analysis I was doing on ARM machine. On both setups
> CPUs can be gated independently.

> >>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
> >>> ---
> >>>    kernel/sched/core.c  |    1 +
> >>>    kernel/sched/fair.c  |  109
> >>> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>    kernel/sched/sched.h |    1 +
> >>>    3 files changed, 111 insertions(+)

> >>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >>> index dab7908..70cadbe 100644
> >>> --- a/kernel/sched/core.c
> >>> +++ b/kernel/sched/core.c
> >>> @@ -6131,6 +6131,7 @@ cpu_attach_domain(struct sched_domain *sd, struct
> >>> root_domain *rd, int cpu)
> >>>          rcu_assign_pointer(rq->sd, sd);
> >>>          destroy_sched_domains(tmp, cpu);

> >>> +       update_packing_domain(cpu);
> >>>          update_top_cache_domain(cpu);
> >>>    }

> >>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >>> index 4f4a4f6..8c9d3ed 100644
> >>> --- a/kernel/sched/fair.c
> >>> +++ b/kernel/sched/fair.c
> >>> @@ -157,6 +157,63 @@ void sched_init_granularity(void)
> >>>          update_sysctl();
> >>>    }

> >>> +
> >>> +/*
> >>> + * Save the id of the optimal CPU that should be used to pack small tasks
> >>> + * The value -1 is used when no buddy has been found
> >>> + */
> >>> +DEFINE_PER_CPU(int, sd_pack_buddy);
> >>> +
> >>> +/* Look for the best buddy CPU that can be used to pack small tasks
> >>> + * We make the assumption that it doesn't wort to pack on CPU that share
> >>> the

> >> s/wort/worth

> > yes

> >>> + * same powerline. We looks for the 1st sched_domain without the
> >>> + * SD_SHARE_POWERLINE flag. Then We look for the sched_group witht the
> >>> lowest
> >>> + * power per core based on the assumption that their power efficiency is
> >>> + * better */

> >> Commenting style..
> >> /*
> >>   *
> >>   */

> > yes

> >> Can you please expand the why the assumption is right ?
> >> "it doesn't wort to pack on CPU that share the same powerline"

> > By "share the same power-line", I mean that the CPUs can't power off
> > independently. So if some CPUs can't power off independently, it's
> > worth to try to use most of them to race to idle.

> In that case I suggest we use different word here. Power line can be
> treated as voltage line, power domain.
> May be SD_SHARE_CPU_POWERDOMAIN ?

> >> Think about a scenario where you have quad core, ducal cluster system

> >>          |Cluster1|                      |cluster 2|
> >> | CPU0 | CPU1 | CPU2 | CPU3 |   | CPU0 | CPU1 | CPU2 | CPU3 |

> >> Both clusters run from same voltage rail and have same PLL
> >> clocking them. But the cluster have their own power domain
> >> and all CPU's can power gate them-self to low power states.
> >> Clusters also have their own level2 caches.

> >> In this case, you will still save power if you try to pack
> >> load on one cluster. No ?

> > yes, I need to update the description of SD_SHARE_POWERLINE because
> > I'm afraid I was not clear enough. SD_SHARE_POWERLINE includes the
> > power gating capacity of each core. For your example above, the
> > SD_SHARE_POWERLINE shoud be cleared at both MC and CPU level.

> Thanks for clarification.

> >>> +void update_packing_domain(int cpu)
> >>> +{
> >>> +       struct sched_domain *sd;
> >>> +       int id = -1;
> >>> +
> >>> +       sd = highest_flag_domain(cpu, SD_SHARE_POWERLINE);
> >>> +       if (!sd)
> >>> +               sd = rcu_dereference_check_sched_domain(cpu_rq(cpu)->sd);
> >>> +       else
> >>> +               sd = sd->parent;
> >>> +
> >>> +       while (sd) {
> >>> +               struct sched_group *sg = sd->groups;
> >>> +               struct sched_group *pack = sg;
> >>> +               struct sched_group *tmp = sg->next;
> >>> +
> >>> +               /* 1st CPU of the sched domain is a good candidate */
> >>> +               if (id == -1)
> >>> +                       id = cpumask_first(sched_domain_span(sd));
> >>> +
> >>> +               /* loop the sched groups to find the best one */
> >>> +               while (tmp != sg) {
> >>> +                       if (tmp->sgp->power * sg->group_weight <
> >>> +                                       sg->sgp->power *
> >>> tmp->group_weight)
> >>> +                               pack = tmp;
> >>> +                       tmp = tmp->next;
> >>> +               }
> >>> +
> >>> +               /* we have found a better group */
> >>> +               if (pack != sg)
> >>> +                       id = cpumask_first(sched_group_cpus(pack));
> >>> +
> >>> +               /* Look for another CPU than itself */
> >>> +               if ((id != cpu)
> >>> +                || ((sd->parent) && !(sd->parent->flags &&
> >>> SD_LOAD_BALANCE)))

> >> Is the condition "!(sd->parent->flags && SD_LOAD_BALANCE)" for
> >> big.LITTLE kind of system where SD_LOAD_BALANCE may not be used ?

> > No, packing small tasks is part of the load balance so if the
> > LOAD_BALANCE flag is cleared, we will not try to pack which is a kind
> > of load balance. There is no link with big.LITTLE

> Now it make sense to me.

> >>> +                       break;
> >>> +
> >>> +               sd = sd->parent;
> >>> +       }
> >>> +
> >>> +       pr_info(KERN_INFO "CPU%d packing on CPU%d\n", cpu, id);
> >>> +       per_cpu(sd_pack_buddy, cpu) = id;
> >>> +}
> >>> +
> >>>    #if BITS_PER_LONG == 32
> >>>    # define WMULT_CONST  (~0UL)
> >>>    #else
> >>> @@ -3073,6 +3130,55 @@ static int select_idle_sibling(struct task_struct
> >>> *p, int target)
> >>>          return target;
> >>>    }

> >>> +static inline bool is_buddy_busy(int cpu)
> >>> +{
> >>> +       struct rq *rq = cpu_rq(cpu);
> >>> +
> >>> +       /*
> >>> +        * A busy buddy is a CPU with a high load or a small load with a
> >>> lot of
> >>> +        * running tasks.
> >>> +        */
> >>> +       return ((rq->avg.usage_avg_sum << rq->nr_running) >
> >>> +                       rq->avg.runnable_avg_period);

> >> I agree busy CPU is the one with high load, but many small threads may
> >> not make CPU fully busy, right ? Should we just stick to the load
> >> parameter alone here ?

> > IMO, the busy state of a CPU isn't only the load but also how many
> > threads are waiting for running on it. This formula tries to take into
> > account both inputs. If you have dozen of small tasks on a CPU, the
> > latency can be large even if the tasks are small.

> Sure. Your point is to avoid throttling and probably use race for
> idle.

> >>> +}
> >>> +
> >>> +static inline bool is_light_task(struct task_struct *p)
> >>> +{
> >>> +       /* A light task runs less than 25% in average */
> >>> +       return ((p->se.avg.usage_avg_sum << 2) <
> >>> p->se.avg.runnable_avg_period);
> >>> +}

> >> Since the whole packing logic relies on the light threads only, the
> >> overall effectiveness is not significant. Infact with multiple tries on
> >> Dual core system, I didn't see any major improvement in power. I think
> >> we need to

> On a system that share the powerline at all level, the buddy is set to -1

> On a dual clusters / dual cores system which can powergate each core and
> cluster independantly, the buddy configuration will be :
>       | CPU0 | CPU1 | CPU2 | CPU3 |
> -----------------------------------
> buddy | CPU0 | CPU0 | CPU0 | CPU2 |

> Small tasks tend to slip out of the periodic load balance.
> The best place to choose to migrate them is at their wake up.

> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
> ---
>  kernel/sched/core.c  |    1 +
>  kernel/sched/fair.c  |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  kernel/sched/sched.h |    1 +
>  3 files changed, 111 insertions(+)

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index dab7908..70cadbe 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6131,6 +6131,7 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu)
>    rcu_assign_pointer(rq->sd, sd);
>    destroy_sched_domains(tmp, cpu);

> +  update_packing_domain(cpu);
>    update_top_cache_domain(cpu);
>  }

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 4f4a4f6..8c9d3ed 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -157,6 +157,63 @@ void sched_init_granularity(void)
>    update_sysctl();
>  }

> +
> +/*
> + * Save the id of the optimal CPU that should be used to pack small tasks
> + * The value -1 is used when no buddy has been found
> + */
> +DEFINE_PER_CPU(int, sd_pack_buddy);
> +
> +/* Look for the best buddy CPU that can be used to pack small tasks
> + * We make the assumption that it doesn't wort to pack on CPU that share the
> + * same powerline. We looks for the 1st sched_domain without the
> + * SD_SHARE_POWERLINE flag. Then We look for the sched_group witht the lowest
> + * power per core based on the assumption that their power efficiency is
> + * better */
> +void update_packing_domain(int cpu)
> +{
> +  struct sched_domain *sd;
> +  int id = -1;
> +
> +  sd = highest_flag_domain(cpu, SD_SHARE_POWERLINE);
> +  if (!sd)
> +          sd = rcu_dereference_check_sched_domain(cpu_rq(cpu)->sd);
> +  else
> +          sd = sd->parent;

> +static inline bool is_buddy_busy(int cpu)
> +{
> +  struct rq *rq = cpu_rq(cpu);
> +
> +  /*
> +   * A busy buddy is a CPU with a high load or a small load with a lot of
> +   * running tasks.
> +   */
> +  return ((rq->avg.usage_avg_sum << rq->nr_running) >
> +                  rq->avg.runnable_avg_period);
> +}
> +
> +static inline bool is_light_task(struct task_struct *p)
> +{
> +  /* A light task runs less than 25% in average */
> +  return ((p->se.avg.usage_avg_sum << 2) < p->se.avg.runnable_avg_period);
> +}
> +
> +static int check_pack_buddy(int cpu, struct task_struct *p)
> +{
> +  int buddy = per_cpu(sd_pack_buddy, cpu);
> +
> +  /* No pack buddy for this CPU */
> +  if (buddy == -1)
> +          return false;
> +
> +  /*
> +   * If a task is waiting for running on the CPU which is its own buddy,
> +   * let the default behavior to look for a better CPU if available
> +   * The threshold has been set to 37.5%
> +   */
> +  if ((buddy == cpu)
> +   && ((p->se.avg.usage_avg_sum << 3) < (p->se.avg.runnable_avg_sum * 5)))
> +          return false;
> +
> +  /* buddy is not an allowed CPU */
> +  if (!cpumask_test_cpu(buddy, tsk_cpus_allowed(p)))
> +          return false;
> +
> +  /*
> +   * If the task is a small one and the buddy is not overloaded,
> +   * we use buddy cpu
> +   */
> +   if (!is_light_task(p) || is_buddy_busy(buddy))
> +          return false;
> +
> +  return true;
> +}
> +
>  /*
>   * sched_balance_self: balance the current task (running on cpu) in domains
>   * that have the 'flag' flag set. In practice, this is SD_BALANCE_FORK and
> @@ -3098,6 +3204,9 @@ select_task_rq_fair(struct task_struct *p, int sd_flag, int wake_flags)
>    if (p->nr_cpus_allowed == 1)
>            return prev_cpu;

> +  if (check_pack_buddy(cpu, p))
> +          return per_cpu(sd_pack_buddy, cpu);
> +
>    if (sd_flag & SD_BALANCE_WAKE) {
>            if (cpumask_test_cpu(cpu, tsk_cpus_allowed(p)))
>                    want_affine = 1;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index a95d5c1..086d8bf 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -875,6 +875,7 @@ static inline void idle_balance(int cpu, struct rq *rq)

>  extern void sysrq_sched_debug_show(void);
>  extern void sched_init_granularity(void);
> +extern void update_packing_domain(int cpu);
>  extern void update_max_interval(void);
>  extern void update_group_power(struct sched_domain *sd, int cpu);
>  extern int update_runtime(struct notifier_block *nfb, unsigned long action, void *hcpu);
> --
> 1.7.9.5

> _______________________________________________
> linaro-dev mailing list
> linaro-...@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

>> On 24 October 2012 17:21, Santosh Shilimkar <santosh.shilim...@ti.com>
>> wrote:

>>> On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote:

>>>> The ARM platforms take advantage of packing small tasks on few cores.
>>>> This is true even when the cores of a cluster can't be powergated
>>>> independently.

>>>> Signed-off-by: Vincent Guittot <vincent.guit...@linaro.org>
>>>> ---
>>>>    arch/arm/kernel/topology.c |    5 +++++
>>>>    1 file changed, 5 insertions(+)

>>>> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>>>> index 26c12c6..00511d0 100644
>>>> --- a/arch/arm/kernel/topology.c
>>>> +++ b/arch/arm/kernel/topology.c
>>>> @@ -226,6 +226,11 @@ static inline void update_cpu_power(unsigned int
>>>> cpuid, unsigned int mpidr) {}
>>>>     */
>>>>    struct cputopo_arm cpu_topology[NR_CPUS];

>>>> +int arch_sd_share_power_line(void)
>>>> +{
>>>> +       return 0*SD_SHARE_POWERLINE;
>>>> +}

>>> Making this selection of policy based on sched domain will better. Just
>>> gives the flexibility to choose a separate scheme for big and little
>>> systems which will be very convenient.

>> I agree that it would be more flexible to be able to set it for each level

> Will you be addressing that in next version then ?

> Regards
> santosh