| 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
| ||
|
Date: Mon, 22 Jan 2018 11:23:12 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Dietmar Eggemann <dietmar.eggemann@....com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Brendan Jackman <brendan.jackman@....com>,
Morten Rasmussen <morten.rasmussen@....com>
Subject: Re: [RFC PATCH 2/5] sched: Add NOHZ_STATS_KICK
On 22 January 2018 at 10:40, Dietmar Eggemann <dietmar.eggemann@....com> wrote:
> On 01/15/2018 08:26 AM, Vincent Guittot wrote:
>>
>> Le Wednesday 03 Jan 2018 à 10:16:00 (+0100), Vincent Guittot a écrit :
>>>
>>> Hi Peter,
>>>
>>> On 22 December 2017 at 21:42, Peter Zijlstra <peterz@...radead.org>
>>> wrote:
>>>>
>>>> On Fri, Dec 22, 2017 at 07:56:29PM +0100, Peter Zijlstra wrote:
>>>>>
>>>>> Right; but I figured we'd try and do it 'right' and see how horrible it
>>>>> is before we try and do funny things.
>>>>
>>>>
>>>> So now it should have a 32ms tick for up to .5s when the system goes
>>>> completely idle.
>>>>
>>>> No idea how bad that is..
>>>
>>>
>>> I have tested your branch but the timer doesn't seem to fire correctly
>>> because i can still see blocked load in the use case i have run.
>>> I haven't found the reason yet
>
>
> Isn't the issue with the timer based implementation that
> rq->has_blocked_load is never set to 1 ?
Yes that's what I suggested to Peter on IRC
>
> Something you changed in your implementation by adding a
> rq->has_blocked_load = 1 into nohz_balance_enter_idle().
>
>
>>
>> Hi Peter,
>>
>> With the patch below on top of your branch, the blocked loads are updated
>> and
>> decayed regularly. The main differences are:
>> - It doesn't use a timer to trig ilb but the tick and when a cpu becomes
>> idle.
>> The main drawback of this solution is that the load is blocked when the
>> system is fully idle with the advantage of not waking up a fully idle
>> system. We have to wait for the next tick or newly idle event for
>> updating
>> blocked load when the system leaves idle stat which can be up to a tick
>> long.
>> If this is too long, we can check for kicking ilb when task wakes up so
>> the
>> blocked load will be updated as soon as the system leaves idle state.
>> The main advantage is that we don't wake up a fully idle system every
>> 32ms to
>> update blocked load that will be not used.
>> - I'm working on one more improvement to use nohz_idle_balance in the
>> newly
>> idle case when the system is not overloaded and
>> (this_rq->avg_idle > sysctl_sched_migration_cost). In this case, we can
>> try to
>> use nohz_idle_balance with NOHZ_STATS_KICK and abort as soon as it
>> exceed
>> this_rq->avg_idle. This will remove some calls to kick_ilb and some
>> wake up
>> of an idle cpus.
>>
>> ---
>> kernel/sched/fair.c | 72
>> +++++++++++++++++++++++++----------------------------
>> 1 file changed, 34 insertions(+), 38 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 52114c6..898785d 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5386,7 +5386,6 @@ static struct {
>> atomic_t stats_state;
>> unsigned long next_balance; /* in jiffy units */
>> unsigned long next_stats;
>> - struct timer_list timer;
>> } nohz ____cacheline_aligned;
>> #endif /* CONFIG_NO_HZ_COMMON */
>> @@ -8004,8 +8003,9 @@ static inline void update_sd_lb_stats(struct lb_env
>> *env, struct sd_lb_stats *sd
>> prefer_sibling = 1;
>> #ifdef CONFIG_NO_HZ_COMMON
>> - if (env->idle == CPU_NEWLY_IDLE)
>> + if (env->idle == CPU_NEWLY_IDLE && atomic_read(&nohz.stats_state))
>> {
>> env->flags |= LBF_NOHZ_STATS;
>> + }
>> #endif
>> load_idx = get_sd_load_idx(env->sd, env->idle);
>> @@ -8818,6 +8818,8 @@ update_next_balance(struct sched_domain *sd,
>> unsigned long *next_balance)
>> *next_balance = next;
>> }
>> +static void kick_ilb(unsigned int flags);
>> +
>> /*
>> * idle_balance is called by schedule() if this_cpu is about to become
>> * idle. Attempts to pull tasks from other CPUs.
>> @@ -8852,12 +8854,16 @@ static int idle_balance(struct rq *this_rq, struct
>> rq_flags *rf)
>> if (this_rq->avg_idle < sysctl_sched_migration_cost ||
>> !this_rq->rd->overload) {
>> + unsigned long next = READ_ONCE(nohz.next_stats);
>> rcu_read_lock();
>> sd = rcu_dereference_check_sched_domain(this_rq->sd);
>> if (sd)
>> update_next_balance(sd, &next_balance);
>> rcu_read_unlock();
>> + if (time_after(jiffies, next) &&
>> atomic_read(&nohz.stats_state))
>> + kick_ilb(NOHZ_STATS_KICK);
>> +
>> goto out;
>> }
>> @@ -9075,18 +9081,6 @@ static void kick_ilb(unsigned int flags)
>> smp_send_reschedule(ilb_cpu);
>> }
>> -void nohz_balance_timer(struct timer_list *timer)
>> -{
>> - unsigned long next = READ_ONCE(nohz.next_stats);
>> -
>> - if (time_before(jiffies, next)) {
>> - mod_timer(timer, next);
>> - return;
>> - }
>> -
>> - kick_ilb(NOHZ_STATS_KICK);
>> -}
>> -
>> /*
>> * Current heuristic for kicking the idle load balancer in the presence
>> * of an idle cpu in the system.
>> @@ -9122,6 +9116,9 @@ static void nohz_balancer_kick(struct rq *rq)
>> if (likely(!atomic_read(&nohz.nr_cpus)))
>> return;
>> + if (time_after(now, nohz.next_stats) &&
>> atomic_read(&nohz.stats_state))
>> + flags = NOHZ_STATS_KICK;
>> +
>> if (time_before(now, nohz.next_balance))
>> goto out;
>> @@ -9227,7 +9224,6 @@ static void set_cpu_sd_state_idle(int cpu)
>> void nohz_balance_enter_idle(int cpu)
>> {
>> struct rq *rq = cpu_rq(cpu);
>> - unsigned int val, new;
>> SCHED_WARN_ON(cpu != smp_processor_id());
>> @@ -9251,6 +9247,7 @@ void nohz_balance_enter_idle(int cpu)
>> return;
>> rq->nohz_tick_stopped = 1;
>> + rq->has_blocked_load = 1;
>> cpumask_set_cpu(cpu, nohz.idle_cpus_mask);
>> atomic_inc(&nohz.nr_cpus);
>> @@ -9258,21 +9255,11 @@ void nohz_balance_enter_idle(int cpu)
>> set_cpu_sd_state_idle(cpu);
>> /*
>> - * implies a barrier such that if the stats_state update is
>> observed
>> - * the above updates are also visible. Pairs with stuff in
>> - * update_sd_lb_stats() and nohz_idle_balance().
>> + * Each time a cpu enter idle, we assume that it has blocked load
>> and
>> + * enable the periodic update of the load of idle cpus
>> */
>> - val = atomic_read(&nohz.stats_state);
>> - do {
>> - new = val + 2;
>> - new |= 1;
>> - } while (!atomic_try_cmpxchg(&nohz.stats_state, &val, new));
>> + atomic_set(&nohz.stats_state, 1);
>> - /*
>> - * If the timer was stopped, restart the thing.
>> - */
>> - if (!(val & 1))
>> - mod_timer(&nohz.timer, jiffies +
>> msecs_to_jiffies(LOAD_AVG_PERIOD));
>> }
>> #else
>> static inline void nohz_balancer_kick(struct rq *rq) { }
>> @@ -9409,7 +9396,6 @@ static bool nohz_idle_balance(struct rq *this_rq,
>> enum cpu_idle_type idle)
>> bool has_blocked_load = false;
>> int update_next_balance = 0;
>> int this_cpu = this_rq->cpu;
>> - unsigned int stats_seq;
>> unsigned int flags;
>> int balance_cpu;
>> struct rq *rq;
>> @@ -9422,7 +9408,6 @@ static bool nohz_idle_balance(struct rq *this_rq,
>> enum cpu_idle_type idle)
>> return false;
>> }
>> - stats_seq = atomic_read(&nohz.stats_state);
>> /*
>> * barrier, pairs with nohz_balance_enter_idle(), ensures ...
>> */
>> @@ -9432,6 +9417,16 @@ static bool nohz_idle_balance(struct rq *this_rq,
>> enum cpu_idle_type idle)
>> SCHED_WARN_ON((flags & NOHZ_KICK_MASK) == NOHZ_BALANCE_KICK);
>> + /*
>> + * We assume there will be no idle load after this update and
>> clear
>> + * the stats state. If a cpu enters idle in the mean time, it will
>> + * set the stats state and trig another update of idle load.
>> + * Because a cpu that becomes idle, is added to idle_cpus_mask
>> before
>> + * setting the stats state, we are sure to not clear the state and
>> not
>> + * check the load of an idle cpu.
>> + */
>> + atomic_set(&nohz.stats_state, 0);
>> +
>> for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {
>> if (balance_cpu == this_cpu || !idle_cpu(balance_cpu))
>> continue;
>> @@ -9441,8 +9436,10 @@ static bool nohz_idle_balance(struct rq *this_rq,
>> enum cpu_idle_type idle)
>> * work being done for other cpus. Next load
>> * balancing owner will pick it up.
>> */
>> - if (need_resched())
>> + if (need_resched()) {
>> + has_blocked_load = true;
>> break;
>> + }
>> rq = cpu_rq(balance_cpu);
>> @@ -9477,12 +9474,12 @@ static bool nohz_idle_balance(struct rq
>> *this_rq, enum cpu_idle_type idle)
>> if (flags & NOHZ_BALANCE_KICK)
>> rebalance_domains(this_rq, CPU_IDLE);
>> - if (has_blocked_load ||
>> - !atomic_try_cmpxchg(&nohz.stats_state, &stats_seq, 0)) {
>> - WRITE_ONCE(nohz.next_stats,
>> - now + msecs_to_jiffies(LOAD_AVG_PERIOD));
>> - mod_timer(&nohz.timer, nohz.next_stats);
>> - }
>> + WRITE_ONCE(nohz.next_stats,
>> + now + msecs_to_jiffies(LOAD_AVG_PERIOD));
>> +
>> + /* There is still blocked load, enable periodic update */
>> + if (has_blocked_load)
>> + atomic_set(&nohz.stats_state, 1);
>> /*
>> * next_balance will be updated only when there is a need.
>> @@ -10115,7 +10112,6 @@ __init void init_sched_fair_class(void)
>> nohz.next_balance = jiffies;
>> nohz.next_stats = jiffies;
>> zalloc_cpumask_var(&nohz.idle_cpus_mask, GFP_NOWAIT);
>> - timer_setup(&nohz.timer, nohz_balance_timer, 0);
>> #endif
>> #endif /* SMP */
>>
>
>
Powered by blists - more mailing lists