Skip to contentSkip to conversation

x86/apic: Provide apic_ack_irq()

PATCH In review Thomas Gleixner 4 participants 9 messages Jun 4, 2018 – Jun 6, 2018

Subsystems (3)
Stable branch x86 IRQ
lore
Share & tools
lore

Thomas Gleixner

author
#0 lore raw

PATCH 3/8 x86/apic: Provide apic_ack_irq()

apic_ack_edge() is explicitely for handling interrupt affinity cleanup when interrupt remapping is not available or disable.

Remapped interrupts and also some of the platform specific special interrupts, e.g. UV, invoke ack_APIC_irq() directly.

To address the issue of failing an affinity update with -EBUSY the delayed affinity mechanism can be reused, but ack_APIC_irq() does not handle that. Adding this to ack_APIC_irq() is not possible, because that function is also used for exceptions and directly handled interrupts like IPIs.

Create a new function, which just contains the conditional invocation of irq_move_irq() and the final ack_APIC_irq(). Making the invocation of irq_move_irq() conditional avoids the out of line call if the pending bit is not set.

Reuse the new function in apic_ack_edge().

Preparatory change for the real fix

Fixes dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
Signed-off-by Thomas Gleixner <[email protected]>
Cc [email protected]
---
 arch/x86/include/asm/apic.h   |    2 ++
 arch/x86/kernel/apic/vector.c |   10 ++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)
arch/x86/include/asm/apic.h +2 -0
@@ -436,6 +436,8 @@ static inline void apic_set_eoi_write(vo
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+extern void apic_ack_irq(struct irq_data *data);
+
 static inline void ack_APIC_irq(void)
 {
 	/*
arch/x86/kernel/apic/vector.c +8 -2
@@ -809,11 +809,17 @@ static int apic_retrigger_irq(struct irq
 	return 1;
 }
 
+void apic_ack_irq(struct irq_data *irqd)
+{
+	if (unlikely(irqd_is_setaffinity_pending(irqd)))
+		irq_move_irq(irqd);
+	ack_APIC_irq();
+}
+
 void apic_ack_edge(struct irq_data *irqd)
 {
 	irq_complete_move(irqd_cfg(irqd));
-	irq_move_irq(irqd);
-	ack_APIC_irq();
+	apic_ack_irq(irqd);
 }
 
 static struct irq_chip lapic_controller = {
Tested-by on v1 3/8
#1 lore raw
Quoted 16 lines · Thomas Gleixner

apic_ack_edge() is explicitely for handling interrupt affinity cleanup when interrupt remapping is not available or disable.

Remapped interrupts and also some of the platform specific special interrupts, e.g. UV, invoke ack_APIC_irq() directly.

To address the issue of failing an affinity update with -EBUSY the delayed affinity mechanism can be reused, but ack_APIC_irq() does not handle that. Adding this to ack_APIC_irq() is not possible, because that function is also used for exceptions and directly handled interrupts like IPIs.

Create a new function, which just contains the conditional invocation of irq_move_irq() and the final ack_APIC_irq(). Making the invocation of irq_move_irq() conditional avoids the out of line call if the pending bit is not set.

Quoted 7 lines

Reuse the new function in apic_ack_edge().

Preparatory change for the real fix

Fixes dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
Signed-off-by Thomas Gleixner <[email protected]>
Cc [email protected]
Tested-by Song Liu <[email protected]>
Quoted 40 lines
---
 arch/x86/include/asm/apic.h   |    2 ++
 arch/x86/kernel/apic/vector.c |   10 ++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)
arch/x86/include/asm/apic.h +2 -0
@@ -436,6 +436,8 @@ static inline void apic_set_eoi_write(vo
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+extern void apic_ack_irq(struct irq_data *data);
+
 static inline void ack_APIC_irq(void)
 {
        /*
arch/x86/kernel/apic/vector.c +8 -2
@@ -809,11 +809,17 @@ static int apic_retrigger_irq(struct irq
        return 1;
 }
 
+void apic_ack_irq(struct irq_data *irqd)
+{
+       if (unlikely(irqd_is_setaffinity_pending(irqd)))
+               irq_move_irq(irqd);
+       ack_APIC_irq();
+}
+
 void apic_ack_edge(struct irq_data *irqd)
 {
        irq_complete_move(irqd_cfg(irqd));
-       irq_move_irq(irqd);
-       ack_APIC_irq();
+       apic_ack_irq(irqd);
 }
 
 static struct irq_chip lapic_controller = {
#2 lore raw
In reply to Thomas Gleixner

Hi Thomas,

Quoted 42 lines · Thomas Gleixner

apic_ack_edge() is explicitely for handling interrupt affinity cleanup when interrupt remapping is not available or disable.

Remapped interrupts and also some of the platform specific special interrupts, e.g. UV, invoke ack_APIC_irq() directly.

To address the issue of failing an affinity update with -EBUSY the delayed affinity mechanism can be reused, but ack_APIC_irq() does not handle that. Adding this to ack_APIC_irq() is not possible, because that function is also used for exceptions and directly handled interrupts like IPIs.

Create a new function, which just contains the conditional invocation of irq_move_irq() and the final ack_APIC_irq(). Making the invocation of irq_move_irq() conditional avoids the out of line call if the pending bit is not set.

Reuse the new function in apic_ack_edge().

Preparatory change for the real fix

Fixes dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
Signed-off-by Thomas Gleixner<[email protected]>
Cc [email protected]
---
  arch/x86/include/asm/apic.h   |    2 ++
  arch/x86/kernel/apic/vector.c |   10 ++++++++--
  2 files changed, 10 insertions(+), 2 deletions(-)
arch/x86/include/asm/apic.h +2 -0
@@ -436,6 +436,8 @@ static inline void apic_set_eoi_write(vo
 
  #endif /* CONFIG_X86_LOCAL_APIC */
 
+extern void apic_ack_irq(struct irq_data *data);
+
  static inline void ack_APIC_irq(void)
  {
  	/*

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 814

Quoted 6 lines
arch/x86/kernel/apic/vector.c +3 -0
@@ -809,11 +809,17 @@ static int apic_retrigger_irq(struct irq
  	return 1;
  }
 
+void apic_ack_irq(struct irq_data *irqd)
+{
+	if (unlikely(irqd_is_setaffinity_pending(irqd)))

Affinity pending is also judged in

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 815

Quoted 1 line
arch/x86/kernel/apic/vector.c +1 -0
+		irq_move_irq(irqd);

If we can remove the if(...) statement here

Thanks,
dou

Quoted 12 lines
arch/x86/kernel/apic/vector.c +4 -2
+	ack_APIC_irq();
+}
+
  void apic_ack_edge(struct irq_data *irqd)
  {
  	irq_complete_move(irqd_cfg(irqd));
-	irq_move_irq(irqd);
-	ack_APIC_irq();
+	apic_ack_irq(irqd);
  }
 
  static struct irq_chip lapic_controller = {
author
#3 lore raw
Quoted 8 lines · Dou Liyang

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 814

Quoted 2 lines
arch/x86/kernel/apic/vector.c +2 -0
+{
+	if (unlikely(irqd_is_setaffinity_pending(irqd)))

Affinity pending is also judged in

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 815

Quoted 1 line
arch/x86/kernel/apic/vector.c +1 -0
+		irq_move_irq(irqd);

If we can remove the if(...) statement here

That requires to fix all call sites in ia64 and that's why I didn't. But we can make irq_move_irq() an inline function and have the check in the inline.

Thanks,

tglx

#4 lore raw

Hi Thomas,

Quoted 11 lines · Thomas Gleixner
Quoted 8 lines · Dou Liyang

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 814

Quoted 2 lines
arch/x86/kernel/apic/vector.c +2 -0
+{
+	if (unlikely(irqd_is_setaffinity_pending(irqd)))

Affinity pending is also judged in

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 815

Quoted 1 line
arch/x86/kernel/apic/vector.c +1 -0
+		irq_move_irq(irqd);

If we can remove the if(...) statement here

That requires to fix all call sites in ia64 and that's why I didn't. But

I didn't express clearly, I meant remove the if(...) statement from apic_ack_irq(), it doesn't require to fix the call sites in ia64.

+void apic_ack_irq(struct irq_data *irqd)
+{
+ irq_move_irq(irqd);
+ ack_APIC_irq();
+}

BTW, If apic_ack_irq() can accept _any_ irq_data when hierarchical irqdomains are enabled[1]? If it is true, If there is a situation in the original code that we should avoid:

  If the top-level irq_data has the IRQD_SETAFFINITY_PENDING state, but
non-top-level irq_data state not, when using non-top-level irq_data in apic_ack_irq(), we may skip the irq_move_irq() which we should call.

[1] commit 77ed42f18edd("genirq: Prevent crash in irq_move_irq()")

Quoted 3 lines

we can make irq_move_irq() an inline function and have the check in the inline.

I don't know why do we need to make irq_move_irq() an inline function.

And, yes, irq_move_irq() has already had the check

        ...
  	if (likely(!irqd_is_setaffinity_pending(idata)))
		return;
        ...

Thanks,
dou

author
#5 lore raw
Quoted 9 lines · Dou Liyang

Hi Thomas,

Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Dou Liyang

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 814

Quoted 2 lines
arch/x86/kernel/apic/vector.c +2 -0
+{
+	if (unlikely(irqd_is_setaffinity_pending(irqd)))

Affinity pending is also judged in

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 815

Quoted 1 line
arch/x86/kernel/apic/vector.c +1 -0
+		irq_move_irq(irqd);

If we can remove the if(...) statement here

That requires to fix all call sites in ia64 and that's why I didn't. But

I didn't express clearly, I meant remove the if(...) statement from apic_ack_irq(), it doesn't require to fix the call sites in ia64.

I put the check there on purpose as I explained in the changelog:

Making the invocation of irq_move_irq() conditional avoids the out of line call if the pending bit is not set.

Thanks,

tglx

#6 lore raw

Hi Thomas,

Quoted 17 lines · Thomas Gleixner
Quoted 15 lines · Dou Liyang

Hi Thomas,

Quoted 11 lines · Thomas Gleixner
Quoted 8 lines · Dou Liyang

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 814

Quoted 2 lines
arch/x86/kernel/apic/vector.c +2 -0
+{
+	if (unlikely(irqd_is_setaffinity_pending(irqd)))

Affinity pending is also judged in

arch/x86/kernel/apic/vector.c static int apic_retrigger_irq(struct irq · line 815

Quoted 1 line
arch/x86/kernel/apic/vector.c +1 -0
+		irq_move_irq(irqd);

If we can remove the if(...) statement here

That requires to fix all call sites in ia64 and that's why I didn't. But

Quoted 8 lines
Quoted 2 lines

I didn't express clearly, I meant remove the if(...) statement from apic_ack_irq(), it doesn't require to fix the call sites in ia64.

I put the check there on purpose as I explained in the changelog:

Making the invocation of irq_move_irq() conditional avoids the out of line call if the pending bit is not set.

I completely understand now, thank you so much. :-)

Thanks,
dou

Quoted 6 lines

Thanks,

tglx

merge bot
#7 lore raw

[tip:x86/urgent] genirq/migration: Avoid out of line call if pending is not set

In reply to Thomas Gleixner
genirq/migration: Avoid out of line call if pending is not set
Commit-ID:  d340ebd696f921d3ad01b8c0c29dd38f2ad2bf3e
Gitweb:     https://git.kernel.org/tip/d340ebd696f921d3ad01b8c0c29dd38f2ad2bf3e
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Wed, 6 Jun 2018 14:46:59 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 6 Jun 2018 15:18:20 +0200

genirq/migration: Avoid out of line call if pending is not set

The upcoming fix for the -EBUSY return from affinity settings requires to use the irq_move_irq() functionality even on irq remapped interrupts. To avoid the out of line call, move the check for the pending bit into an inline helper.

Preparatory change for the real fix. No functional change.

Fixes dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
Signed-off-by Thomas Gleixner <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Song Liu <[email protected]>
Cc Dmitry Safonov <[email protected]>
Cc [email protected]
Cc Mike Travis <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tariq Toukan <[email protected]>
Cc Dou Liyang <[email protected]>
Link https://lkml.kernel.org/r/[email protected] open here
---
 include/linux/irq.h    | 7 ++++++-
 kernel/irq/migration.c | 5 +----
 2 files changed, 7 insertions(+), 5 deletions(-)
include/linux/irq.h +6 -1
index 65916a305f3d..4e66378f290b 100644
@@ -551,7 +551,12 @@ extern int irq_affinity_online_cpu(unsigned int cpu);
 #endif
 
 #if defined(CONFIG_SMP) && defined(CONFIG_GENERIC_PENDING_IRQ)
-void irq_move_irq(struct irq_data *data);
+void __irq_move_irq(struct irq_data *data);
+static inline void irq_move_irq(struct irq_data *data)
+{
+	if (unlikely(irqd_is_setaffinity_pending(data)))
+		__irq_move_irq(data);
+}
 void irq_move_masked_irq(struct irq_data *data);
 void irq_force_complete_move(struct irq_desc *desc);
 #else
kernel/irq/migration.c +1 -4
index 8b8cecd18cce..def48589ea48 100644
@@ -91,7 +91,7 @@ void irq_move_masked_irq(struct irq_data *idata)
 	cpumask_clear(desc->pending_mask);
 }
 
-void irq_move_irq(struct irq_data *idata)
+void __irq_move_irq(struct irq_data *idata)
 {
 	bool masked;
 

@@ -102,9 +102,6 @@ void irq_move_irq(struct irq_data *idata)
 	 */
 	idata = irq_desc_get_irq_data(irq_data_to_desc(idata));
 
-	if (likely(!irqd_is_setaffinity_pending(idata)))
-		return;
-
 	if (unlikely(irqd_irq_disabled(idata)))
 		return;
merge bot
#8 lore raw
In reply to Thomas Gleixner
x86/apic: Provide apic_ack_irq()
Commit-ID:  c0255770ccdc77ef2184d2a0a2e0cde09d2b44a4
Gitweb:     https://git.kernel.org/tip/c0255770ccdc77ef2184d2a0a2e0cde09d2b44a4
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Mon, 4 Jun 2018 17:33:55 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 6 Jun 2018 15:18:20 +0200

x86/apic: Provide apic_ack_irq()

apic_ack_edge() is explicitely for handling interrupt affinity cleanup when interrupt remapping is not available or disable.

Remapped interrupts and also some of the platform specific special interrupts, e.g. UV, invoke ack_APIC_irq() directly.

To address the issue of failing an affinity update with -EBUSY the delayed affinity mechanism can be reused, but ack_APIC_irq() does not handle that. Adding this to ack_APIC_irq() is not possible, because that function is also used for exceptions and directly handled interrupts like IPIs.

Create a new function, which just contains the conditional invocation of irq_move_irq() and the final ack_APIC_irq().

Reuse the new function in apic_ack_edge().

Preparatory change for the real fix.

Fixes dccfe3147b42 ("x86/vector: Simplify vector move cleanup")
Signed-off-by Thomas Gleixner <[email protected]>
Tested-by Song Liu <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Song Liu <[email protected]>
Cc Dmitry Safonov <[email protected]>
Cc [email protected]
Cc Mike Travis <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tariq Toukan <[email protected]>
Link https://lkml.kernel.org/r/[email protected] open here
---
 arch/x86/include/asm/apic.h   | 2 ++
 arch/x86/kernel/apic/vector.c | 9 +++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)
arch/x86/include/asm/apic.h +2 -0
index 08acd954f00e..74a9e06b6cfd 100644
@@ -436,6 +436,8 @@ static inline void apic_set_eoi_write(void (*eoi_write)(u32 reg, u32 v)) {}
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+extern void apic_ack_irq(struct irq_data *data);
+
 static inline void ack_APIC_irq(void)
 {
 	/*
arch/x86/kernel/apic/vector.c +7 -2
index 72b575a0b662..b708f597eee3 100644
@@ -809,13 +809,18 @@ static int apic_retrigger_irq(struct irq_data *irqd)
 	return 1;
 }
 
-void apic_ack_edge(struct irq_data *irqd)
+void apic_ack_irq(struct irq_data *irqd)
 {
-	irq_complete_move(irqd_cfg(irqd));
 	irq_move_irq(irqd);
 	ack_APIC_irq();
 }
 
+void apic_ack_edge(struct irq_data *irqd)
+{
+	irq_complete_move(irqd_cfg(irqd));
+	apic_ack_irq(irqd);
+}
+
 static struct irq_chip lapic_controller = {
 	.name			= "APIC",
 	.irq_ack		= apic_ack_edge,