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

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

[Tanner Love]

From: Tanner Love <tannerlove@google.com>

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@google.com>
Signed-off-by: Tanner Love <tannerlove@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 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))
 # 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

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@google.com>
> 
> 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@google.com>
> Signed-off-by: Tanner Love <tannerlove@google.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
>  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@chromium.org>

-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

[tip: irq/core] genirq: Change force_irqthreads to a static key

[tip-bot2 for Tanner Love]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     af5b7fe6bb77ac775d446e2f25f013d5df551e9a
Gitweb:        https://git.kernel.org/tip/af5b7fe6bb77ac775d446e2f25f013d5df551e9a
Author:        Tanner Love <tannerlove@google.com>
AuthorDate:    Wed, 02 Jun 2021 14:03:38 -04:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 10 Aug 2021 16:06:55 +02:00

genirq: Change force_irqthreads to a static key

With CONFIG_IRQ_FORCED_THREADING=y, testing the boolean force_irqthreads
could incur a cache line miss in invoke_softirq() and other places.

Replace the test with a static key to avoid the potential cache miss.

[ tglx: Dropped the IDE part, removed the export and updated blk-mq ]

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Tanner Love <tannerlove@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20210602180338.3324213-1-tannerlove.kernel@gmail.com
---
 block/blk-mq.c            |  2 +-
 include/linux/interrupt.h |  8 +++++---
 kernel/irq/manage.c       |  9 ++++-----
 kernel/softirq.c          |  2 +-
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2c4ac51..572d8ab 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -606,7 +606,7 @@ static inline bool blk_mq_complete_need_ipi(struct request *rq)
 	 * This is probably worse than completing the request on a different
 	 * cache domain.
 	 */
-	if (force_irqthreads)
+	if (force_irqthreads())
 		return false;
 
 	/* same CPU or cache domain?  Complete locally */
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 2ed65b0..1f22a30 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>
@@ -474,12 +475,13 @@ extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
 
 #ifdef CONFIG_IRQ_FORCED_THREADING
 # ifdef CONFIG_PREEMPT_RT
-#  define force_irqthreads	(true)
+#  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))
 # endif
 #else
-#define force_irqthreads	(0)
+#define force_irqthreads()	(false)
 #endif
 
 #ifndef local_softirq_pending
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 766468a..bb3c51d 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -25,12 +25,11 @@
 #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);
 
 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);
@@ -1260,8 +1259,8 @@ static int irq_thread(void *data)
 	irqreturn_t (*handler_fn)(struct irq_desc *desc,
 			struct irqaction *action);
 
-	if (force_irqthreads && test_bit(IRQTF_FORCED_THREAD,
-					&action->thread_flags))
+	if (force_irqthreads() && test_bit(IRQTF_FORCED_THREAD,
+					   &action->thread_flags))
 		handler_fn = irq_forced_thread_fn;
 	else
 		handler_fn = irq_thread_fn;
diff --git a/kernel/softirq.c b/kernel/softirq.c
index f3a0121..322b65d 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -422,7 +422,7 @@ static inline void invoke_softirq(void)
 	if (ksoftirqd_running(local_softirq_pending()))
 		return;
 
-	if (!force_irqthreads || !__this_cpu_read(ksoftirqd)) {
+	if (!force_irqthreads() || !__this_cpu_read(ksoftirqd)) {
 #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK
 		/*
 		 * We can safely execute softirq on the current stack if

[tip: irq/core] genirq: Change force_irqthreads to a static key

[tip-bot2 for Tanner Love]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     91cc470e797828d779cd4c1efbe8519bcb358bae
Gitweb:        https://git.kernel.org/tip/91cc470e797828d779cd4c1efbe8519bcb358bae
Author:        Tanner Love <tannerlove@google.com>
AuthorDate:    Wed, 02 Jun 2021 14:03:38 -04:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 10 Aug 2021 22:50:07 +02:00

genirq: Change force_irqthreads to a static key

With CONFIG_IRQ_FORCED_THREADING=y, testing the boolean force_irqthreads
could incur a cache line miss in invoke_softirq() and other places.

Replace the test with a static key to avoid the potential cache miss.

[ tglx: Dropped the IDE part, removed the export and updated blk-mq ]

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Tanner Love <tannerlove@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20210602180338.3324213-1-tannerlove.kernel@gmail.com

---
 block/blk-mq.c            |  2 +-
 include/linux/interrupt.h |  8 +++++---
 kernel/irq/manage.c       | 11 +++++------
 kernel/softirq.c          |  2 +-
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2c4ac51..572d8ab 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -606,7 +606,7 @@ static inline bool blk_mq_complete_need_ipi(struct request *rq)
 	 * This is probably worse than completing the request on a different
 	 * cache domain.
 	 */
-	if (force_irqthreads)
+	if (force_irqthreads())
 		return false;
 
 	/* same CPU or cache domain?  Complete locally */
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 2ed65b0..1f22a30 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>
@@ -474,12 +475,13 @@ extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
 
 #ifdef CONFIG_IRQ_FORCED_THREADING
 # ifdef CONFIG_PREEMPT_RT
-#  define force_irqthreads	(true)
+#  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))
 # endif
 #else
-#define force_irqthreads	(0)
+#define force_irqthreads()	(false)
 #endif
 
 #ifndef local_softirq_pending
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 766468a..34a66c4 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -25,12 +25,11 @@
 #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);
 
 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);
@@ -1260,8 +1259,8 @@ static int irq_thread(void *data)
 	irqreturn_t (*handler_fn)(struct irq_desc *desc,
 			struct irqaction *action);
 
-	if (force_irqthreads && test_bit(IRQTF_FORCED_THREAD,
-					&action->thread_flags))
+	if (force_irqthreads() && test_bit(IRQTF_FORCED_THREAD,
+					   &action->thread_flags))
 		handler_fn = irq_forced_thread_fn;
 	else
 		handler_fn = irq_thread_fn;
@@ -1322,7 +1321,7 @@ EXPORT_SYMBOL_GPL(irq_wake_thread);
 
 static int irq_setup_forced_threading(struct irqaction *new)
 {
-	if (!force_irqthreads)
+	if (!force_irqthreads())
 		return 0;
 	if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
 		return 0;
diff --git a/kernel/softirq.c b/kernel/softirq.c
index f3a0121..322b65d 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -422,7 +422,7 @@ static inline void invoke_softirq(void)
 	if (ksoftirqd_running(local_softirq_pending()))
 		return;
 
-	if (!force_irqthreads || !__this_cpu_read(ksoftirqd)) {
+	if (!force_irqthreads() || !__this_cpu_read(ksoftirqd)) {
 #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK
 		/*
 		 * We can safely execute softirq on the current stack if