Messages in this thread |  | | Date | Thu, 11 Jan 2018 05:25:05 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH v4 01/36] timers: Use static keys for migrate_enable/nohz_active |
| |
On Thu, Dec 21, 2017 at 11:41:30AM +0100, Anna-Maria Gleixner wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > 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 <tglx@linutronix.de> > Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de> > --- > 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!
|  |