From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751569AbdLUH7m (ORCPT ); Thu, 21 Dec 2017 02:59:42 -0500 Received: from mail-wr0-f195.google.com ([209.85.128.195]:45067 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751366AbdLUH7i (ORCPT ); Thu, 21 Dec 2017 02:59:38 -0500 X-Google-Smtp-Source: ACJfBouNhGDUfZVsEc0URgBZk17PqlaI4mGZ9Xa1mmPW1PQ14MBFPJzNR6oRhxdbJ9XOKjUdnX3YEw== Date: Thu, 21 Dec 2017 08:59:34 +0100 From: Vincent Guittot To: Peter Zijlstra Cc: Brendan Jackman , Dietmar Eggemann , Ingo Molnar , linux-kernel , Ingo Molnar , Morten Rasmussen Subject: Re: [PATCH v2 1/2] sched: force update of blocked load of idle cpus Message-ID: <20171221075934.GA20623@linaro.org> References: <20171201180157.18937-1-brendan.jackman@arm.com> <20171201180157.18937-2-brendan.jackman@arm.com> <20171220140306.egxub7sog4il2plm@hirez.programming.kicks-ass.net> <20171220150110.4iglytjqgqinehy6@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20171220150110.4iglytjqgqinehy6@hirez.programming.kicks-ass.net> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Le Wednesday 20 Dec 2017 à 16:01:10 (+0100), Peter Zijlstra a écrit : > On Wed, Dec 20, 2017 at 03:23:01PM +0100, Vincent Guittot wrote: > > On 20 December 2017 at 15:03, Peter Zijlstra wrote: > > > On Fri, Dec 01, 2017 at 06:01:56PM +0000, Brendan Jackman wrote: > > >> @@ -8955,8 +8964,20 @@ static void nohz_balancer_kick(void) > > >> if (ilb_cpu >= nr_cpu_ids) > > >> return; > > >> > > >> - if (test_and_set_bit(NOHZ_BALANCE_KICK, nohz_flags(ilb_cpu))) > > >> + if (test_and_set_bit(NOHZ_BALANCE_KICK, nohz_flags(ilb_cpu))) { > > >> + if (!only_update) { > > >> + /* > > >> + * There's a pending/ongoing nohz kick/balance. If it's > > >> + * just for stats, convert it to a proper load balance. > > >> + */ > > >> + clear_bit(NOHZ_STATS_KICK, nohz_flags(ilb_cpu)); > > >> + } > > >> return; > > >> + } > > >> + > > >> + if (only_update) > > >> + set_bit(NOHZ_STATS_KICK, nohz_flags(ilb_cpu)); > > >> + > > >> /* > > >> * Use smp_send_reschedule() instead of resched_cpu(). > > >> * This way we generate a sched IPI on the target cpu which > > > > > > This looks racy.. if its not we don't need atomic ops, if it is but is > > > still fine it needs a comment. > > > > NOHZ_STATS_KICK modification is protected by test_and_set_bit(NOHZ_BALANCE_KICK. > > You're right that we don't need atomics ops and __set_bit() is enough > > Well, you shouldn't mix atomic and non-atomic ops to the same word, > that's asking for trouble. In fact, the atomic operation is needed, I forgot that set/clear of NOHZ_TICK_STOPPED can happen simultaneously on the ilb_cpu so we must keep set_bit(NOHZ_STATS_KICK, nohz_flags(ilb_cpu)); As comment we can add: /* * Update of NOHZ_STATS_KICK itself is protected by test_and_set_biti but * clear/set of NOHZ_TICK_STOPPED can happen simultaneously so we need * atomic operation */ > > But why don't you do something like: > > nohz_kick() > > flags = NOHZ_STAT; > if (!only_update) > flags |= NOHZ_BALANCE; > > atomic_long_or(flags, &nohz_cpu(cpu)); > > > nohz_idle_balance() > > unsigned long do_flags = atomic_long_fetch_andnot(NOHZ_BALANCE|NOHZ_STAT, &nohz_flags(cpu)); > > if (do_flags & NOHZ_STAT) > update_blocked_stuff(); > > if (do_flags & NOHZ_BALANCE) > rebalance_domains(); > > That way its far more readable. >