From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753619AbeAKEZK (ORCPT + 1 other); Wed, 10 Jan 2018 23:25:10 -0500 Received: from mail.kernel.org ([198.145.29.99]:39416 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752922AbeAKEZI (ORCPT ); Wed, 10 Jan 2018 23:25:08 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D5DEF2173F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=frederic@kernel.org Date: Thu, 11 Jan 2018 05:25:05 +0100 From: Frederic Weisbecker To: Anna-Maria Gleixner , Sebastian Andrzej Siewior Cc: LKML , Thomas Gleixner , Peter Zijlstra , Ingo Molnar , keescook@chromium.org, Christoph Hellwig , John Stultz Subject: Re: [PATCH v4 01/36] timers: Use static keys for migrate_enable/nohz_active Message-ID: <20180111042504.GB11633@lerouge> References: <20171221104205.7269-1-anna-maria@linutronix.de> <20171221104205.7269-2-anna-maria@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171221104205.7269-2-anna-maria@linutronix.de> 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 Return-Path: On Thu, Dec 21, 2017 at 11:41:30AM +0100, Anna-Maria Gleixner wrote: > From: Thomas Gleixner > > The members migrate_enable and nohz_active in the timer/hrtimer per CPU > bases have been introduced to avoid accessing global variables for these > decisions. > > Still that results in a (cache hot) load and conditional branch, which can > be avoided by using static keys. > > Implement it with static keys and optimize for the most critical case of > high performance networking which tends to disable the timer migration > functionality. > > Signed-off-by: Thomas Gleixner > Signed-off-by: Anna-Maria Gleixner > --- > include/linux/hrtimer.h | 4 -- > kernel/time/hrtimer.c | 17 +++------ > kernel/time/tick-internal.h | 19 ++++++---- > kernel/time/tick-sched.c | 2 +- > kernel/time/timer.c | 91 +++++++++++++++++++++++---------------------- > 5 files changed, 65 insertions(+), 68 deletions(-) > > diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h > index f8e1845aa464..4ac74dff59f0 100644 > --- a/kernel/time/tick-internal.h > +++ b/kernel/time/tick-internal.h > @@ -150,14 +150,19 @@ static inline void tick_nohz_init(void) { } > > #ifdef CONFIG_NO_HZ_COMMON > extern unsigned long tick_nohz_active; > -#else > +extern void timers_update_nohz(void); > +extern struct static_key_false timers_nohz_active; > +static inline bool is_timers_nohz_active(void) > +{ > + return static_branch_unlikely(&timers_nohz_active); Shouldn't we expect instead that timers_nohz_active is a likely scenario? I guess deactivating nohz is for specific workloads that can't stand the deep state wake up latency. Also perhaps the above symbols should be harmonized, something like: timers_nohz_update() timers_nohz_active_key timers_nohz_active() > diff --git a/kernel/time/timer.c b/kernel/time/timer.c > index ffebcf878fba..1e2140a23044 100644 > --- a/kernel/time/timer.c > +++ b/kernel/time/timer.c > @@ -200,8 +200,6 @@ struct timer_base { > unsigned long clk; > unsigned long next_expiry; > unsigned int cpu; > - bool migration_enabled; > - bool nohz_active; > bool is_idle; > bool must_forward_clk; > DECLARE_BITMAP(pending_map, WHEEL_SIZE); > @@ -210,45 +208,59 @@ struct timer_base { > > static DEFINE_PER_CPU(struct timer_base, timer_bases[NR_BASES]); > > -#if defined(CONFIG_SMP) && defined(CONFIG_NO_HZ_COMMON) > +#ifdef CONFIG_NO_HZ_COMMON > + > +DEFINE_STATIC_KEY_FALSE(timers_nohz_active); > +static DEFINE_MUTEX(timer_keys_mutex); > + > +static void timer_update_keys(struct work_struct *work); > +static DECLARE_WORK(timer_update_work, timer_update_keys); > + > +#ifdef CONFIG_SMP > unsigned int sysctl_timer_migration = 1; > > -void timers_update_migration(bool update_nohz) > +DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); > + > +static void timers_update_migration(void) > { > bool on = sysctl_timer_migration && tick_nohz_active; > - unsigned int cpu; > > - /* Avoid the loop, if nothing to update */ > - if (this_cpu_read(timer_bases[BASE_STD].migration_enabled) == on) > - return; > + if (on) You may as well put the condition directly ;) > + static_branch_enable(&timers_migration_enabled); > + else > + static_branch_disable(&timers_migration_enabled); > +} Thanks!