linux-kernel - Re: [PATCH v4 01/36] timers: Use static keys for migrate_enable/nohz

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180111042504.GB11633@lerouge>
Date:   Thu, 11 Jan 2018 05:25:05 +0100
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Anna-Maria Gleixner <anna-maria@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, keescook@...omium.org,
        Christoph Hellwig <hch@....de>,
        John Stultz <john.stultz@...aro.org>
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@...utronix.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@...utronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@...utronix.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!