linux-kernel - Re: [PATCH] sched: add heuristic logic to pick idle peers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 17 Jun 2013 13:08:12 +0800
From:	Lei Wen <adrian.wenl@...il.com>
To:	Michael Wang <wangyun@...ux.vnet.ibm.com>
Cc:	Lei Wen <leiwen@...vell.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...e.hu>, mingo@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched: add heuristic logic to pick idle peers

Hi Michael,

On Mon, Jun 17, 2013 at 11:27 AM, Michael Wang
<wangyun@...ux.vnet.ibm.com> wrote:
> Hi, Lei
>
> On 06/17/2013 10:21 AM, Lei Wen wrote:
>> nr_busy_cpus in sched_group_power structure cannot present the purpose
>> for judging below statement:
>> "this cpu's scheduler group has multiple busy cpu's exceeding
>>  the group's power."
>>
>> But only could tell how many cpus is doing their jobs for currently.
>
> AFAIK, this nr_busy_cpus presents how many cpus in local group are not
> idle, the logical here in nohz_kick_needed() is:
>
>         if domain cpus share resources and at least 2 cpus in
>         local group are not idle, prefer to do balance.
>

Seems reasonable for me. But this comment is conflicted with current documented
one. Do we need to modify the comment anyway, as previous says nr_busy>1 is
"scheduler group has multiple busy cpu;s exceeding the group's power"?

> And the idea behind is, we catch the timing when there are idle-cpu and
> busy-group and task-moving may cost low.

Since there is only one task over runqueue now, then why we could need the
load balance any way?...

>
> Your change will remove this timing for balance, I think you may need
> some test to prove that this patch will make things better.

I see. Yes, test data is always good. :)
Do you have any suggestion like using what kind of test program to
collect this data?

Thanks,
Lei

>
> Regards,
> Michael Wang
>
>>
>> However, the original purpose to add this logic still looks good.
>> So we move this kind of logic to find_new_ilb, so that we could pick
>> out peer from our sharing resource domain whenever possible.
>>
>> Signed-off-by: Lei Wen <leiwen@...vell.com>
>> ---
>>  kernel/sched/fair.c |   28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index c61a614..64f9120 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5368,10 +5368,31 @@ static struct {
>>       unsigned long next_balance;     /* in jiffy units */
>>  } nohz ____cacheline_aligned;
>>
>> +/*
>> + * Add the heuristic logic to try waking up idle cpu from
>> + * those peers who share resources with us, so that the
>> + * cost would be brought to minimum.
>> + */
>>  static inline int find_new_ilb(int call_cpu)
>>  {
>> -     int ilb = cpumask_first(nohz.idle_cpus_mask);
>> +     int ilb = nr_cpu_ids;
>> +     struct sched_domain *sd;
>> +
>> +     rcu_read_lock();
>> +     for_each_domain(call_cpu, sd) {
>> +             /* We loop till sched_domain no longer share resource */
>> +             if (!(sd->flags & SD_SHARE_PKG_RESOURCES)) {
>> +                     ilb = cpumask_first(nohz.idle_cpus_mask);
>> +                     break;
>> +             }
>>
>> +             /* else, we would try to pick the idle cpu from peers first */
>> +             ilb = cpumask_first_and(nohz.idle_cpus_mask,
>> +                             sched_domain_span(sd));
>> +             if (ilb < nr_cpu_ids)
>> +                     break;
>> +     }
>> +     rcu_read_unlock();
>>       if (ilb < nr_cpu_ids && idle_cpu(ilb))
>>               return ilb;
>>
>> @@ -5620,8 +5641,6 @@ end:
>>   * Current heuristic for kicking the idle load balancer in the presence
>>   * of an idle cpu is the system.
>>   *   - This rq has more than one task.
>> - *   - At any scheduler domain level, this cpu's scheduler group has multiple
>> - *     busy cpu's exceeding the group's power.
>>   *   - For SD_ASYM_PACKING, if the lower numbered cpu's in the scheduler
>>   *     domain span are idle.
>>   */
>> @@ -5659,9 +5678,6 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
>>               struct sched_group_power *sgp = sg->sgp;
>>               int nr_busy = atomic_read(&sgp->nr_busy_cpus);
>>
>> -             if (sd->flags & SD_SHARE_PKG_RESOURCES && nr_busy > 1)
>> -                     goto need_kick_unlock;
>> -
>>               if (sd->flags & SD_ASYM_PACKING && nr_busy != sg->group_weight
>>                   && (cpumask_first_and(nohz.idle_cpus_mask,
>>                                         sched_domain_span(sd)) < cpu))
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/