linux-kernel - Re: [PATCH 3/3] sched: update blocked load when newly idle

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:   Wed, 14 Feb 2018 15:43:30 +0100
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Morten Rasmussen <morten.rasmussen@...s.arm.com>,
        Brendan Jackman <brendan.jackman@....com>,
        Dietmar Eggemann <dietmar.eggemann@....com>
Subject: Re: [PATCH 3/3] sched: update blocked load when newly idle

On 14 February 2018 at 15:40, Valentin Schneider
<valentin.schneider@....com> wrote:
> On 02/13/2018 10:31 AM, Vincent Guittot wrote:
>> When NEWLY_IDLE load balance is not triggered, we might need to update the
>> blocked load anyway. We can kick an ilb so an idle CPU will take care of
>> updating blocked load or we can try to update them locally before entering
>> idle. In the latter case, we reuse part of the nohz_idle_balance.
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@...aro.org>
>> ---
>>  kernel/sched/fair.c | 324 +++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 193 insertions(+), 131 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 9183fee..cb1ab5c 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>>
>> [...]
>>
>>  /*
>> + * idle_balance is called by schedule() if this_cpu is about to become
>> + * idle. Attempts to pull tasks from other CPUs.
>> + */
>> +static int idle_balance(struct rq *this_rq, struct rq_flags *rf)
>> +{
>> +     unsigned long next_balance = jiffies + HZ;
>> +     int this_cpu = this_rq->cpu;
>> +     struct sched_domain *sd;
>> +     int pulled_task = 0;
>> +     u64 curr_cost = 0;
>> +
>> +     /*
>> +      * We must set idle_stamp _before_ calling idle_balance(), such that we
>> +      * measure the duration of idle_balance() as idle time.
>> +      */
>> +     this_rq->idle_stamp = rq_clock(this_rq);
>> +
>> +     /*
>> +      * Do not pull tasks towards !active CPUs...
>> +      */
>> +     if (!cpu_active(this_cpu))
>> +             return 0;
>> +
>> +     /*
>> +      * This is OK, because current is on_cpu, which avoids it being picked
>> +      * for load-balance and preemption/IRQs are still disabled avoiding
>> +      * further scheduler activity on it and we're being very careful to
>> +      * re-start the picking loop.
>> +      */
>> +     rq_unpin_lock(this_rq, rf);
>> +
>> +     if (this_rq->avg_idle < sysctl_sched_migration_cost ||
>> +         !this_rq->rd->overload) {
>> +#ifdef CONFIG_NO_HZ_COMMON
>> +             unsigned long has_blocked = READ_ONCE(nohz.has_blocked);
>> +             unsigned long next_blocked = READ_ONCE(nohz.next_blocked);
>> +#endif
>> +             rcu_read_lock();
>> +             sd = rcu_dereference_check_sched_domain(this_rq->sd);
>> +             if (sd)
>> +                     update_next_balance(sd, &next_balance);
>> +             rcu_read_unlock();
>> +
>> +#ifdef CONFIG_NO_HZ_COMMON
>> +             /*
>> +              * This CPU doesn't want to be disturbed by scheduler
>> +              * houskeeping
>
> Typo here (houskeeping)
>
>> +              */
>> +             if (!housekeeping_cpu(this_cpu, HK_FLAG_SCHED))
>> +                     goto out;
>> +
>> +             /* Will wake up very soon. No time for fdoing anything else*/
>
> Typo here (fdoing)
>
>> +             if (this_rq->avg_idle < sysctl_sched_migration_cost)
>> +                     goto out;
>> +
>> +             /* Don't need to update blocked load of idle CPUs*/
>> +             if (!has_blocked || time_after_eq(jiffies, next_blocked))
>> +                     goto out;
>
> My "stats update via NEWLY_IDLE" test case started misbehaving with this
> version: we skip most NEWLY_IDLE stats updates. AFAICT this is the culprit.
>
> I believe this time check should be time_before(jiffies, next_blocked)
> (or time_before_eq depending on what you want to guarantee with the jiffy
> interval stuff).

argh.. I have completely mess up the conditions when reordering them

Thanks for spotting this

>
>> +
>> +             raw_spin_unlock(&this_rq->lock);
>> +             /*
>> +              * This CPU is going to be idle and blocked load of idle CPUs
>> +              * need to be updated. Run the ilb locally as it is a good
>> +              * candidate for ilb instead of waking up another idle CPU.
>> +              * Kick an normal ilb if we failed to do the update.
>> +              */
>> +             if (!_nohz_idle_balance(this_rq, NOHZ_STATS_KICK, CPU_NEWLY_IDLE))
>> +                     kick_ilb(NOHZ_STATS_KICK);
>> +             raw_spin_lock(&this_rq->lock);
>> +#endif
>> +             goto out;
>> +     }
>> +