On Thu, Dec 21, 2017 at 11:41:30AM +0100, Anna-Maria Gleixner wrote:
> From: Thomas Gleixner <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Anna-Maria Gleixner <[email protected]>
> ---
> 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!
