Re: [PATCH] genirq: change force_irqthreads from bool test to static key

[Kees Cook]

On Wed, Jun 02, 2021 at 02:03:38PM -0400, Tanner Love wrote:
> From: Tanner Love <tannerlove@xxxxxxxxxx>
> 
> With CONFIG_IRQ_FORCED_THREADING=y, testing the bool force_irqthreads
> could incur a cache line miss in invoke_softirq().
> 
> Replace the test with a static key to avoid the potential cache miss.
> 
> Suggested-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> Signed-off-by: Tanner Love <tannerlove@xxxxxxxxxx>
> Reviewed-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> ---
>  drivers/ide/ide-iops.c    | 7 +++----
>  include/linux/interrupt.h | 4 +++-
>  kernel/irq/manage.c       | 6 +++---
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
> index f2be127ee96e..f86cdb8451e6 100644
> --- a/drivers/ide/ide-iops.c
> +++ b/drivers/ide/ide-iops.c
> @@ -109,7 +109,6 @@ int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
>  	ide_hwif_t *hwif = drive->hwif;
>  	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
>  	unsigned long flags;
> -	bool irqs_threaded = force_irqthreads;
>  	int i;
>  	u8 stat;
>  
> @@ -117,7 +116,7 @@ int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
>  	stat = tp_ops->read_status(hwif);
>  
>  	if (stat & ATA_BUSY) {
> -		if (!irqs_threaded) {
> +		if (!force_irqthreads) {
>  			local_save_flags(flags);
>  			local_irq_enable_in_hardirq();
>  		}
> @@ -133,13 +132,13 @@ int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
>  				if ((stat & ATA_BUSY) == 0)
>  					break;
>  
> -				if (!irqs_threaded)
> +				if (!force_irqthreads)
>  					local_irq_restore(flags);
>  				*rstat = stat;
>  				return -EBUSY;
>  			}
>  		}
> -		if (!irqs_threaded)
> +		if (!force_irqthreads)
>  			local_irq_restore(flags);
>  	}
>  	/*
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 4777850a6dc7..9e676c351f23 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -13,6 +13,7 @@
>  #include <linux/hrtimer.h>
>  #include <linux/kref.h>
>  #include <linux/workqueue.h>
> +#include <linux/jump_label.h>
>  
>  #include <linux/atomic.h>
>  #include <asm/ptrace.h>
> @@ -504,7 +505,8 @@ extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
>  # ifdef CONFIG_PREEMPT_RT
>  #  define force_irqthreads	(true)
>  # else
> -extern bool force_irqthreads;
> +DECLARE_STATIC_KEY_FALSE(force_irqthreads_key);
> +#  define force_irqthreads	(static_branch_unlikely(&force_irqthreads_key))

I like this idiom, though it did make me look twice because I thought
you were missing the static_branch...() calls above. ;)

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

-Kees

>  # endif
>  #else
>  #define force_irqthreads	(0)
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 4c14356543d9..395945e54929 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -25,12 +25,12 @@
>  #include "internals.h"
>  
>  #if defined(CONFIG_IRQ_FORCED_THREADING) && !defined(CONFIG_PREEMPT_RT)
> -__read_mostly bool force_irqthreads;
> -EXPORT_SYMBOL_GPL(force_irqthreads);
> +DEFINE_STATIC_KEY_FALSE(force_irqthreads_key);
> +EXPORT_SYMBOL_GPL(force_irqthreads_key);
>  
>  static int __init setup_forced_irqthreads(char *arg)
>  {
> -	force_irqthreads = true;
> +	static_branch_enable(&force_irqthreads_key);
>  	return 0;
>  }
>  early_param("threadirqs", setup_forced_irqthreads);
> -- 
> 2.32.0.rc0.204.g9fa02ecfa5-goog
> 

-- 
Kees Cook