Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753708AbdLUQ46 (ORCPT ); Thu, 21 Dec 2017 11:56:58 -0500 Received: from mail-it0-f68.google.com ([209.85.214.68]:40940 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752118AbdLUQ4y (ORCPT ); Thu, 21 Dec 2017 11:56:54 -0500 X-Google-Smtp-Source: ACJfBouXPfo9PU8vTW9JsW+NTNUZVdW7JR3MwUMmm5LGrhJrHEqz4FIOcCen26+TUycvCWzSqd3BF6HSMvwwt3khyRE= MIME-Version: 1.0 In-Reply-To: References: <20171221102139.177253391@infradead.org> <20171221102235.088181011@infradead.org> From: Vincent Guittot Date: Thu, 21 Dec 2017 17:56:32 +0100 Message-ID: Subject: Re: [RFC PATCH 2/5] sched: Add NOHZ_STATS_KICK To: Peter Zijlstra Cc: Ingo Molnar , linux-kernel , Brendan Jackman , Dietmar Eggemann , Morten Rasmussen Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6997 Lines: 191 On 21 December 2017 at 17:23, Vincent Guittot wrote: > Hi Peter, > > I think that part of the proposal is missing. > > One goal of the patchset was to kick an update of the stats of idle > cpu when a task wake up on a cpu but the statistic has not been > updated for a while. > > That's why there where a call to nohz_kick_needed in the proposal to > kick ilb but only for updating blocked load and not a full idle load sorry a call to nohz_kick_needed (which becomes nohz_balancer_kick in yours patchset) in select_task_fair_rq_fair > balance > > I can't find this call any more in your patchset In fact, we can't only rely on the tick and newly_idle load balance to ensure a period update of the blocked load because they can never happen. So we need to find another place to kick for a periodic update which is when a task wake up > > On 21 December 2017 at 11:21, Peter Zijlstra wrote: >> >> Suggested-by: Vincent Guittot >> Signed-off-by: Peter Zijlstra (Intel) >> --- >> kernel/sched/core.c | 4 +-- >> kernel/sched/fair.c | 52 ++++++++++++++++++++++++++++++++++----------------- >> kernel/sched/sched.h | 4 +++ >> 3 files changed, 41 insertions(+), 19 deletions(-) >> >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -608,7 +608,7 @@ static inline bool got_nohz_idle_kick(vo >> { >> int cpu = smp_processor_id(); >> >> - if (!(atomic_read(nohz_flags(cpu)) & NOHZ_BALANCE_KICK)) >> + if (!(atomic_read(nohz_flags(cpu)) & NOHZ_KICK_MASK)) >> return false; >> >> if (idle_cpu(cpu) && !need_resched()) >> @@ -618,7 +618,7 @@ static inline bool got_nohz_idle_kick(vo >> * We can't run Idle Load Balance on this CPU for this time so we >> * cancel it and clear NOHZ_BALANCE_KICK >> */ >> - atomic_andnot(NOHZ_BALANCE_KICK, nohz_flags(cpu)); >> + atomic_andnot(NOHZ_KICK_MASK, nohz_flags(cpu)); >> return false; >> } >> >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -9001,8 +9001,8 @@ static void nohz_balancer_kick(void) >> if (ilb_cpu >= nr_cpu_ids) >> return; >> >> - flags = atomic_fetch_or(NOHZ_BALANCE_KICK, nohz_flags(ilb_cpu)); >> - if (flags & NOHZ_BALANCE_KICK) >> + flags = atomic_fetch_or(NOHZ_KICK_MASK, nohz_flags(ilb_cpu)); >> + if (flags & NOHZ_KICK_MASK) >> return; >> /* >> * Use smp_send_reschedule() instead of resched_cpu(). >> @@ -9125,8 +9125,6 @@ static void rebalance_domains(struct rq >> int need_serialize, need_decay = 0; >> u64 max_cost = 0; >> >> - update_blocked_averages(cpu); >> - >> rcu_read_lock(); >> for_each_domain(cpu, sd) { >> /* >> @@ -9221,20 +9219,27 @@ static void rebalance_domains(struct rq >> * In CONFIG_NO_HZ_COMMON case, the idle balance kickee will do the >> * rebalancing for all the cpus for whom scheduler ticks are stopped. >> */ >> -static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle) >> +static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle) >> { >> - int this_cpu = this_rq->cpu; >> - struct rq *rq; >> - int balance_cpu; >> /* Earliest time when we have to do rebalance again */ >> unsigned long next_balance = jiffies + 60*HZ; >> int update_next_balance = 0; >> + int this_cpu = this_rq->cpu; >> + unsigned int flags; >> + int balance_cpu; >> + struct rq *rq; >> >> - if (!(atomic_read(nohz_flags(this_cpu)) & NOHZ_BALANCE_KICK)) >> - return; >> + if (!(atomic_read(nohz_flags(this_cpu)) & NOHZ_KICK_MASK)) >> + return false; >> >> - if (idle != CPU_IDLE) >> - goto end; >> + if (idle != CPU_IDLE) { >> + atomic_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu)); >> + return false; >> + } >> + >> + flags = atomic_fetch_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu)); >> + >> + SCHED_WARN_ON((flags & NOHZ_KICK_MASK) == NOHZ_BALANCE_KICK); >> >> for_each_cpu(balance_cpu, nohz.idle_cpus_mask) { >> if (balance_cpu == this_cpu || !idle_cpu(balance_cpu)) >> @@ -9262,7 +9267,9 @@ static void nohz_idle_balance(struct rq >> cpu_load_update_idle(rq); >> rq_unlock_irq(rq, &rf); >> >> - rebalance_domains(rq, CPU_IDLE); >> + update_blocked_averages(rq->cpu); >> + if (flags & NOHZ_BALANCE_KICK) >> + rebalance_domains(rq, CPU_IDLE); >> } >> >> if (time_after(next_balance, rq->next_balance)) { >> @@ -9271,6 +9278,10 @@ static void nohz_idle_balance(struct rq >> } >> } >> >> + update_blocked_averages(this_cpu); >> + if (flags & NOHZ_BALANCE_KICK) >> + rebalance_domains(this_rq, CPU_IDLE); >> + >> /* >> * next_balance will be updated only when there is a need. >> * When the CPU is attached to null domain for ex, it will not be >> @@ -9278,8 +9289,8 @@ static void nohz_idle_balance(struct rq >> */ >> if (likely(update_next_balance)) >> nohz.next_balance = next_balance; >> -end: >> - atomic_andnot(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)); >> + >> + return true; >> } >> >> /* >> @@ -9366,7 +9377,10 @@ static inline bool nohz_kick_needed(stru >> return kick; >> } >> #else >> -static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle) { } >> +static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle) >> +{ >> + return false; >> +} >> #endif >> >> /* >> @@ -9387,7 +9401,11 @@ static __latent_entropy void run_rebalan >> * load balance only within the local sched_domain hierarchy >> * and abort nohz_idle_balance altogether if we pull some load. >> */ >> - nohz_idle_balance(this_rq, idle); >> + if (nohz_idle_balance(this_rq, idle)) >> + return; >> + >> + /* normal load balance */ >> + update_blocked_averages(this_rq->cpu); >> rebalance_domains(this_rq, idle); >> } >> >> --- a/kernel/sched/sched.h >> +++ b/kernel/sched/sched.h >> @@ -2005,9 +2005,13 @@ extern void cfs_bandwidth_usage_dec(void >> #ifdef CONFIG_NO_HZ_COMMON >> #define NOHZ_TICK_STOPPED_BIT 0 >> #define NOHZ_BALANCE_KICK_BIT 1 >> +#define NOHZ_STATS_KICK_BIT 2 >> >> #define NOHZ_TICK_STOPPED BIT(NOHZ_TICK_STOPPED_BIT) >> #define NOHZ_BALANCE_KICK BIT(NOHZ_BALANCE_KICK_BIT) >> +#define NOHZ_STATS_KICK BIT(NOHZ_STATS_KICK_BIT) >> + >> +#define NOHZ_KICK_MASK (NOHZ_BALANCE_KICK | NOHZ_STATS_KICK) >> >> #define nohz_flags(cpu) (&cpu_rq(cpu)->nohz_flags) >> >> >>