[patch 3/8] x86/apic: Provide apic_ack_irq()

[patch 3/8] x86/apic: Provide apic_ack_irq()

[Thomas Gleixner]

[-- Attachment #1: x86-apic--Provide-apic_ack-variant.patch --]
[-- Type: text/plain, Size: 1896 bytes --]

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 <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 arch/x86/include/asm/apic.h   |    2 ++
 arch/x86/kernel/apic/vector.c |   10 ++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -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)
 {
 	/*
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -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 = {

Re: [patch 3/8] x86/apic: Provide apic_ack_irq()

[Song Liu]

On Mon, Jun 4, 2018 at 8:33 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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 <tglx@linutronix.de>
> Cc: stable@vger.kernel.org

Tested-by: Song Liu <songliubraving@fb.com>

> ---
>  arch/x86/include/asm/apic.h   |    2 ++
>  arch/x86/kernel/apic/vector.c |   10 ++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -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)
>  {
>         /*
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -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 = {
>
>

Re: [patch 3/8] x86/apic: Provide apic_ack_irq()

[Dou Liyang]

Hi Thomas,

At 06/04/2018 11:33 PM, Thomas Gleixner wrote:
> 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<tglx@linutronix.de>
> Cc:stable@vger.kernel.org
> ---
>   arch/x86/include/asm/apic.h   |    2 ++
>   arch/x86/kernel/apic/vector.c |   10 ++++++++--
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -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)
>   {
>   	/*
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -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

> +		irq_move_irq(irqd);

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

Thanks,
	dou

> +	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 = {

Re: [patch 3/8] x86/apic: Provide apic_ack_irq()

[Thomas Gleixner]

On Tue, 5 Jun 2018, Dou Liyang wrote:
> > +{
> > +	if (unlikely(irqd_is_setaffinity_pending(irqd)))
> 
> Affinity pending is also judged in
> 
> > +		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

Re: [patch 3/8] x86/apic: Provide apic_ack_irq()

[Dou Liyang]

Hi Thomas,

At 06/05/2018 07:41 PM, Thomas Gleixner wrote:
> On Tue, 5 Jun 2018, Dou Liyang wrote:
>>> +{
>>> +	if (unlikely(irqd_is_setaffinity_pending(irqd)))
>>
>> Affinity pending is also judged in
>>
>>> +		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()")

> 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

Re: [patch 3/8] x86/apic: Provide apic_ack_irq()

[Thomas Gleixner]

On Wed, 6 Jun 2018, Dou Liyang wrote:

> Hi Thomas,
> 
> At 06/05/2018 07:41 PM, Thomas Gleixner wrote:
> > On Tue, 5 Jun 2018, Dou Liyang wrote:
> > > > +{
> > > > +	if (unlikely(irqd_is_setaffinity_pending(irqd)))
> > > 
> > > Affinity pending is also judged in
> > > 
> > > > +		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

Re: [patch 3/8] x86/apic: Provide apic_ack_irq()

[Dou Liyang]

Hi Thomas,

At 06/06/2018 04:04 PM, Thomas Gleixner wrote:
> On Wed, 6 Jun 2018, Dou Liyang wrote:
> 
>> Hi Thomas,
>>
>> At 06/05/2018 07:41 PM, Thomas Gleixner wrote:
>>> On Tue, 5 Jun 2018, Dou Liyang wrote:
>>>>> +{
>>>>> +	if (unlikely(irqd_is_setaffinity_pending(irqd)))
>>>>
>>>> Affinity pending is also judged in
>>>>
>>>>> +		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.
> 

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

Thanks,
	dou

> Thanks,
> 
> 	tglx
> 
> 
> 

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

[tip-bot for Thomas Gleixner]

Commit-ID:  d340ebd696f921d3ad01b8c0c29dd38f2ad2bf3e
Gitweb:     https://git.kernel.org/tip/d340ebd696f921d3ad01b8c0c29dd38f2ad2bf3e
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 6 Jun 2018 14:46:59 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
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 <tglx@linutronix.de>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <liu.song.a23@gmail.com>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: stable@vger.kernel.org
Cc: Mike Travis <mike.travis@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tariq Toukan <tariqt@mellanox.com>
Cc: Dou Liyang <douly.fnst@cn.fujitsu.com>
Link: https://lkml.kernel.org/r/20180604162224.471925894@linutronix.de

---
 include/linux/irq.h    | 7 ++++++-
 kernel/irq/migration.c | 5 +----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 65916a305f3d..4e66378f290b 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -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
diff --git a/kernel/irq/migration.c b/kernel/irq/migration.c
index 8b8cecd18cce..def48589ea48 100644
--- a/kernel/irq/migration.c
+++ b/kernel/irq/migration.c
@@ -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;
 

[tip:x86/urgent] x86/apic: Provide apic_ack_irq()

[tip-bot for Thomas Gleixner]

Commit-ID:  c0255770ccdc77ef2184d2a0a2e0cde09d2b44a4
Gitweb:     https://git.kernel.org/tip/c0255770ccdc77ef2184d2a0a2e0cde09d2b44a4
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 4 Jun 2018 17:33:55 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
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 <tglx@linutronix.de>
Tested-by: Song Liu <songliubraving@fb.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <liu.song.a23@gmail.com>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: stable@vger.kernel.org
Cc: Mike Travis <mike.travis@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tariq Toukan <tariqt@mellanox.com>
Link: https://lkml.kernel.org/r/20180604162224.471925894@linutronix.de

---
 arch/x86/include/asm/apic.h   | 2 ++
 arch/x86/kernel/apic/vector.c | 9 +++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 08acd954f00e..74a9e06b6cfd 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -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)
 {
 	/*
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 72b575a0b662..b708f597eee3 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -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,