[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
> 
> 
>