[PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4554 bytes --]

Rather than going through all IO-APICs and calling io_apic_eoi_vector()
for the vector in question, just use eoi_IO_APIC_irq().

This in turn allows to eliminate quite a bit of other code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -69,10 +69,6 @@ int __read_mostly nr_ioapics;
 
 #define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
 
-#define io_apic_eoi_vector(apic, vector) io_apic_eoi((apic), (vector), -1)
-#define io_apic_eoi_pin(apic, pin) io_apic_eoi((apic), -1, (pin))
-
-
 /*
  * This is performance-critical, we want to do it O(1)
  *
@@ -213,21 +209,18 @@ static void ioapic_write_entry(
     spin_unlock_irqrestore(&ioapic_lock, flags);
 }
 
-/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
- * it should be worked out using the other.  This function expect that the
- * ioapic_lock is taken, and interrupts are disabled (or there is a good reason
- * not to), and that if both pin and vector are passed, that they refer to the
+/* EOI an IO-APIC entry.  Vector may be -1, indicating that it should be
+ * worked out using the pin.  This function expects that the ioapic_lock is
+ * being held, and interrupts are disabled (or there is a good reason not
+ * to), and that if both pin and vector are passed, that they refer to the
  * same redirection entry in the IO-APIC. */
 static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
 {
-    /* Ensure some useful information is passed in */
-    BUG_ON( (vector == -1 && pin == -1) );
-    
     /* Prefer the use of the EOI register if available */
     if ( ioapic_has_eoi_reg(apic) )
     {
         /* If vector is unknown, read it from the IO-APIC */
-        if ( vector == -1 )
+        if ( vector == IRQ_VECTOR_UNASSIGNED )
             vector = __ioapic_read_entry(apic, pin, TRUE).vector;
 
         *(IO_APIC_BASE(apic)+16) = vector;
@@ -239,42 +232,6 @@ static void __io_apic_eoi(unsigned int a
         struct IO_APIC_route_entry entry;
         bool_t need_to_unmask = 0;
 
-        /* If pin is unknown, search for it */
-        if ( pin == -1 )
-        {
-            unsigned int p;
-            for ( p = 0; p < nr_ioapic_entries[apic]; ++p )
-            {
-                entry = __ioapic_read_entry(apic, p, TRUE);
-                if ( entry.vector == vector )
-                {
-                    pin = p;
-                    /* break; */
-
-                    /* Here should be a break out of the loop, but at the 
-                     * Xen code doesn't actually prevent multiple IO-APIC
-                     * entries being assigned the same vector, so EOI all
-                     * pins which have the correct vector.
-                     *
-                     * Remove the following code when the above assertion
-                     * is fulfilled. */
-                    __io_apic_eoi(apic, vector, p);
-                }
-            }
-            
-            /* If search fails, nothing to do */
-
-            /* if ( pin == -1 ) */
-
-            /* Because the loop wasn't broken out of (see comment above),
-             * all relevant pins have been EOI, so we can always return.
-             * 
-             * Re-instate the if statement above when the Xen logic has been
-             * fixed.*/
-
-            return;
-        }
-
         entry = __ioapic_read_entry(apic, pin, TRUE);
 
         if ( ! entry.mask )
@@ -301,17 +258,6 @@ static void __io_apic_eoi(unsigned int a
     }
 }
 
-/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
- * it should be worked out using the other.  This function disables interrupts
- * and takes the ioapic_lock */
-static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
-{
-    unsigned int flags;
-    spin_lock_irqsave(&ioapic_lock, flags);
-    __io_apic_eoi(apic, vector, pin);
-    spin_unlock_irqrestore(&ioapic_lock, flags);
-}
-
 /*
  * Saves all the IO-APIC RTE's
  */
@@ -1693,11 +1639,7 @@ static void end_level_ioapic_irq(struct 
 
     /* Manually EOI the old vector if we are moving to the new */
     if ( vector && i != vector )
-    {
-        int ioapic;
-        for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi_vector(ioapic, i);
-    }
+        eoi_IO_APIC_irq(desc);
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
 



[-- Attachment #2: x86-ioapic-EOI-after-migration.patch --]
[-- Type: text/plain, Size: 4611 bytes --]

x86/IO-APIC: refine EOI-ing of migrating level interrupts

Rather than going through all IO-APICs and calling io_apic_eoi_vector()
for the vector in question, just use eoi_IO_APIC_irq().

This in turn allows to eliminate quite a bit of other code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -69,10 +69,6 @@ int __read_mostly nr_ioapics;
 
 #define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
 
-#define io_apic_eoi_vector(apic, vector) io_apic_eoi((apic), (vector), -1)
-#define io_apic_eoi_pin(apic, pin) io_apic_eoi((apic), -1, (pin))
-
-
 /*
  * This is performance-critical, we want to do it O(1)
  *
@@ -213,21 +209,18 @@ static void ioapic_write_entry(
     spin_unlock_irqrestore(&ioapic_lock, flags);
 }
 
-/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
- * it should be worked out using the other.  This function expect that the
- * ioapic_lock is taken, and interrupts are disabled (or there is a good reason
- * not to), and that if both pin and vector are passed, that they refer to the
+/* EOI an IO-APIC entry.  Vector may be -1, indicating that it should be
+ * worked out using the pin.  This function expects that the ioapic_lock is
+ * being held, and interrupts are disabled (or there is a good reason not
+ * to), and that if both pin and vector are passed, that they refer to the
  * same redirection entry in the IO-APIC. */
 static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
 {
-    /* Ensure some useful information is passed in */
-    BUG_ON( (vector == -1 && pin == -1) );
-    
     /* Prefer the use of the EOI register if available */
     if ( ioapic_has_eoi_reg(apic) )
     {
         /* If vector is unknown, read it from the IO-APIC */
-        if ( vector == -1 )
+        if ( vector == IRQ_VECTOR_UNASSIGNED )
             vector = __ioapic_read_entry(apic, pin, TRUE).vector;
 
         *(IO_APIC_BASE(apic)+16) = vector;
@@ -239,42 +232,6 @@ static void __io_apic_eoi(unsigned int a
         struct IO_APIC_route_entry entry;
         bool_t need_to_unmask = 0;
 
-        /* If pin is unknown, search for it */
-        if ( pin == -1 )
-        {
-            unsigned int p;
-            for ( p = 0; p < nr_ioapic_entries[apic]; ++p )
-            {
-                entry = __ioapic_read_entry(apic, p, TRUE);
-                if ( entry.vector == vector )
-                {
-                    pin = p;
-                    /* break; */
-
-                    /* Here should be a break out of the loop, but at the 
-                     * Xen code doesn't actually prevent multiple IO-APIC
-                     * entries being assigned the same vector, so EOI all
-                     * pins which have the correct vector.
-                     *
-                     * Remove the following code when the above assertion
-                     * is fulfilled. */
-                    __io_apic_eoi(apic, vector, p);
-                }
-            }
-            
-            /* If search fails, nothing to do */
-
-            /* if ( pin == -1 ) */
-
-            /* Because the loop wasn't broken out of (see comment above),
-             * all relevant pins have been EOI, so we can always return.
-             * 
-             * Re-instate the if statement above when the Xen logic has been
-             * fixed.*/
-
-            return;
-        }
-
         entry = __ioapic_read_entry(apic, pin, TRUE);
 
         if ( ! entry.mask )
@@ -301,17 +258,6 @@ static void __io_apic_eoi(unsigned int a
     }
 }
 
-/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
- * it should be worked out using the other.  This function disables interrupts
- * and takes the ioapic_lock */
-static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
-{
-    unsigned int flags;
-    spin_lock_irqsave(&ioapic_lock, flags);
-    __io_apic_eoi(apic, vector, pin);
-    spin_unlock_irqrestore(&ioapic_lock, flags);
-}
-
 /*
  * Saves all the IO-APIC RTE's
  */
@@ -1693,11 +1639,7 @@ static void end_level_ioapic_irq(struct 
 
     /* Manually EOI the old vector if we are moving to the new */
     if ( vector && i != vector )
-    {
-        int ioapic;
-        for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
-            io_apic_eoi_vector(ioapic, i);
-    }
+        eoi_IO_APIC_irq(desc);
 
     v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Andrew Cooper]

On 15/11/11 13:14, Jan Beulich wrote:
> Rather than going through all IO-APICs and calling io_apic_eoi_vector()
> for the vector in question, just use eoi_IO_APIC_irq().
>
> This in turn allows to eliminate quite a bit of other code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -69,10 +69,6 @@ int __read_mostly nr_ioapics;
>  
>  #define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
>  
> -#define io_apic_eoi_vector(apic, vector) io_apic_eoi((apic), (vector), -1)
> -#define io_apic_eoi_pin(apic, pin) io_apic_eoi((apic), -1, (pin))
> -
> -
>  /*
>   * This is performance-critical, we want to do it O(1)
>   *
> @@ -213,21 +209,18 @@ static void ioapic_write_entry(
>      spin_unlock_irqrestore(&ioapic_lock, flags);
>  }
>  
> -/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
> - * it should be worked out using the other.  This function expect that the
> - * ioapic_lock is taken, and interrupts are disabled (or there is a good reason
> - * not to), and that if both pin and vector are passed, that they refer to the
> +/* EOI an IO-APIC entry.  Vector may be -1, indicating that it should be
> + * worked out using the pin.  This function expects that the ioapic_lock is
> + * being held, and interrupts are disabled (or there is a good reason not
> + * to), and that if both pin and vector are passed, that they refer to the
>   * same redirection entry in the IO-APIC. */
>  static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
>  {
> -    /* Ensure some useful information is passed in */
> -    BUG_ON( (vector == -1 && pin == -1) );
> -    
>      /* Prefer the use of the EOI register if available */
>      if ( ioapic_has_eoi_reg(apic) )
>      {
>          /* If vector is unknown, read it from the IO-APIC */
> -        if ( vector == -1 )
> +        if ( vector == IRQ_VECTOR_UNASSIGNED )

Quick style query:  I consider IRQ_VECTOR_UNASSIGNED logically different
from passing -1 in as a value for vector, even though they are the are
the same value.  Is it sensible to mix them?

~Andrew

>              vector = __ioapic_read_entry(apic, pin, TRUE).vector;
>  
>          *(IO_APIC_BASE(apic)+16) = vector;
> @@ -239,42 +232,6 @@ static void __io_apic_eoi(unsigned int a
>          struct IO_APIC_route_entry entry;
>          bool_t need_to_unmask = 0;
>  
> -        /* If pin is unknown, search for it */
> -        if ( pin == -1 )
> -        {
> -            unsigned int p;
> -            for ( p = 0; p < nr_ioapic_entries[apic]; ++p )
> -            {
> -                entry = __ioapic_read_entry(apic, p, TRUE);
> -                if ( entry.vector == vector )
> -                {
> -                    pin = p;
> -                    /* break; */
> -
> -                    /* Here should be a break out of the loop, but at the 
> -                     * Xen code doesn't actually prevent multiple IO-APIC
> -                     * entries being assigned the same vector, so EOI all
> -                     * pins which have the correct vector.
> -                     *
> -                     * Remove the following code when the above assertion
> -                     * is fulfilled. */
> -                    __io_apic_eoi(apic, vector, p);
> -                }
> -            }
> -            
> -            /* If search fails, nothing to do */
> -
> -            /* if ( pin == -1 ) */
> -
> -            /* Because the loop wasn't broken out of (see comment above),
> -             * all relevant pins have been EOI, so we can always return.
> -             * 
> -             * Re-instate the if statement above when the Xen logic has been
> -             * fixed.*/
> -
> -            return;
> -        }
> -
>          entry = __ioapic_read_entry(apic, pin, TRUE);
>  
>          if ( ! entry.mask )
> @@ -301,17 +258,6 @@ static void __io_apic_eoi(unsigned int a
>      }
>  }
>  
> -/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
> - * it should be worked out using the other.  This function disables interrupts
> - * and takes the ioapic_lock */
> -static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
> -{
> -    unsigned int flags;
> -    spin_lock_irqsave(&ioapic_lock, flags);
> -    __io_apic_eoi(apic, vector, pin);
> -    spin_unlock_irqrestore(&ioapic_lock, flags);
> -}
> -
>  /*
>   * Saves all the IO-APIC RTE's
>   */
> @@ -1693,11 +1639,7 @@ static void end_level_ioapic_irq(struct 
>  
>      /* Manually EOI the old vector if we are moving to the new */
>      if ( vector && i != vector )
> -    {
> -        int ioapic;
> -        for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
> -            io_apic_eoi_vector(ioapic, i);
> -    }
> +        eoi_IO_APIC_irq(desc);
>  
>      v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
>  
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Jan Beulich]

>>> On 15.11.11 at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 15/11/11 13:14, Jan Beulich wrote:
>>      if ( ioapic_has_eoi_reg(apic) )
>>      {
>>          /* If vector is unknown, read it from the IO-APIC */
>> -        if ( vector == -1 )
>> +        if ( vector == IRQ_VECTOR_UNASSIGNED )
> 
> Quick style query:  I consider IRQ_VECTOR_UNASSIGNED logically different
> from passing -1 in as a value for vector, even though they are the are
> the same value.  Is it sensible to mix them?

I view it quite the other way around: One should explicitly pass
IRQ_VECTOR_UNASSIGNED when passing a literal value (which
currently doesn't happen anyway. Primarily because passing
desc->arch.vector or desc->arch.old_vector could happen to also
hold this very value.

Jan

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Andrew Cooper]

On 15/11/11 13:27, Jan Beulich wrote:
>>>> On 15.11.11 at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 15/11/11 13:14, Jan Beulich wrote:
>>>      if ( ioapic_has_eoi_reg(apic) )
>>>      {
>>>          /* If vector is unknown, read it from the IO-APIC */
>>> -        if ( vector == -1 )
>>> +        if ( vector == IRQ_VECTOR_UNASSIGNED )
>> Quick style query:  I consider IRQ_VECTOR_UNASSIGNED logically different
>> from passing -1 in as a value for vector, even though they are the are
>> the same value.  Is it sensible to mix them?
> I view it quite the other way around: One should explicitly pass
> IRQ_VECTOR_UNASSIGNED when passing a literal value (which
> currently doesn't happen anyway. Primarily because passing
> desc->arch.vector or desc->arch.old_vector could happen to also
> hold this very value.
>
> Jan

Ok.

Do you want any other patches to be tested on the problem server?

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Jan Beulich]

>>> On 15.11.11 at 14:35, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 15/11/11 13:27, Jan Beulich wrote:
>>>>> On 15.11.11 at 14:19, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 15/11/11 13:14, Jan Beulich wrote:
>>>>      if ( ioapic_has_eoi_reg(apic) )
>>>>      {
>>>>          /* If vector is unknown, read it from the IO-APIC */
>>>> -        if ( vector == -1 )
>>>> +        if ( vector == IRQ_VECTOR_UNASSIGNED )
>>> Quick style query:  I consider IRQ_VECTOR_UNASSIGNED logically different
>>> from passing -1 in as a value for vector, even though they are the are
>>> the same value.  Is it sensible to mix them?
>> I view it quite the other way around: One should explicitly pass
>> IRQ_VECTOR_UNASSIGNED when passing a literal value (which
>> currently doesn't happen anyway. Primarily because passing
>> desc->arch.vector or desc->arch.old_vector could happen to also
>> hold this very value.
>>
>> Jan
> 
> Ok.
> 
> Do you want any other patches to be tested on the problem server?

While not directly related, throwing in "x86/IRQ: prevent vector
sharing within IO-APICs" (which I want to apply only after this one)
would certainly be good.

Plus of course the experimental, yet to be written, use of
desc->arch.old_vector in end_level_io_apic_irq() (which anyway
should be done only after a good run with the above included).

Jan

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Andrew Cooper]

On 15/11/11 13:14, Jan Beulich wrote:
> Rather than going through all IO-APICs and calling io_apic_eoi_vector()
> for the vector in question, just use eoi_IO_APIC_irq().
>
> This in turn allows to eliminate quite a bit of other code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Tested-and-acked-by: Andrew Cooper <andrew.cooper3@citrix.com> (tested
via backport to xen-4.1.x)

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -69,10 +69,6 @@ int __read_mostly nr_ioapics;
>  
>  #define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
>  
> -#define io_apic_eoi_vector(apic, vector) io_apic_eoi((apic), (vector), -1)
> -#define io_apic_eoi_pin(apic, pin) io_apic_eoi((apic), -1, (pin))
> -
> -
>  /*
>   * This is performance-critical, we want to do it O(1)
>   *
> @@ -213,21 +209,18 @@ static void ioapic_write_entry(
>      spin_unlock_irqrestore(&ioapic_lock, flags);
>  }
>  
> -/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
> - * it should be worked out using the other.  This function expect that the
> - * ioapic_lock is taken, and interrupts are disabled (or there is a good reason
> - * not to), and that if both pin and vector are passed, that they refer to the
> +/* EOI an IO-APIC entry.  Vector may be -1, indicating that it should be
> + * worked out using the pin.  This function expects that the ioapic_lock is
> + * being held, and interrupts are disabled (or there is a good reason not
> + * to), and that if both pin and vector are passed, that they refer to the
>   * same redirection entry in the IO-APIC. */
>  static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
>  {
> -    /* Ensure some useful information is passed in */
> -    BUG_ON( (vector == -1 && pin == -1) );
> -    
>      /* Prefer the use of the EOI register if available */
>      if ( ioapic_has_eoi_reg(apic) )
>      {
>          /* If vector is unknown, read it from the IO-APIC */
> -        if ( vector == -1 )
> +        if ( vector == IRQ_VECTOR_UNASSIGNED )
>              vector = __ioapic_read_entry(apic, pin, TRUE).vector;
>  
>          *(IO_APIC_BASE(apic)+16) = vector;
> @@ -239,42 +232,6 @@ static void __io_apic_eoi(unsigned int a
>          struct IO_APIC_route_entry entry;
>          bool_t need_to_unmask = 0;
>  
> -        /* If pin is unknown, search for it */
> -        if ( pin == -1 )
> -        {
> -            unsigned int p;
> -            for ( p = 0; p < nr_ioapic_entries[apic]; ++p )
> -            {
> -                entry = __ioapic_read_entry(apic, p, TRUE);
> -                if ( entry.vector == vector )
> -                {
> -                    pin = p;
> -                    /* break; */
> -
> -                    /* Here should be a break out of the loop, but at the 
> -                     * Xen code doesn't actually prevent multiple IO-APIC
> -                     * entries being assigned the same vector, so EOI all
> -                     * pins which have the correct vector.
> -                     *
> -                     * Remove the following code when the above assertion
> -                     * is fulfilled. */
> -                    __io_apic_eoi(apic, vector, p);
> -                }
> -            }
> -            
> -            /* If search fails, nothing to do */
> -
> -            /* if ( pin == -1 ) */
> -
> -            /* Because the loop wasn't broken out of (see comment above),
> -             * all relevant pins have been EOI, so we can always return.
> -             * 
> -             * Re-instate the if statement above when the Xen logic has been
> -             * fixed.*/
> -
> -            return;
> -        }
> -
>          entry = __ioapic_read_entry(apic, pin, TRUE);
>  
>          if ( ! entry.mask )
> @@ -301,17 +258,6 @@ static void __io_apic_eoi(unsigned int a
>      }
>  }
>  
> -/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
> - * it should be worked out using the other.  This function disables interrupts
> - * and takes the ioapic_lock */
> -static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
> -{
> -    unsigned int flags;
> -    spin_lock_irqsave(&ioapic_lock, flags);
> -    __io_apic_eoi(apic, vector, pin);
> -    spin_unlock_irqrestore(&ioapic_lock, flags);
> -}
> -
>  /*
>   * Saves all the IO-APIC RTE's
>   */
> @@ -1693,11 +1639,7 @@ static void end_level_ioapic_irq(struct 
>  
>      /* Manually EOI the old vector if we are moving to the new */
>      if ( vector && i != vector )
> -    {
> -        int ioapic;
> -        for (ioapic = 0; ioapic < nr_ioapics; ioapic++)
> -            io_apic_eoi_vector(ioapic, i);
> -    }
> +        eoi_IO_APIC_irq(desc);
>  
>      v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
>  
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Jan Beulich]

>>> On 17.11.11 at 17:12, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 15/11/11 13:14, Jan Beulich wrote:
>> Rather than going through all IO-APICs and calling io_apic_eoi_vector()
>> for the vector in question, just use eoi_IO_APIC_irq().
>>
>> This in turn allows to eliminate quite a bit of other code.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Tested-and-acked-by: Andrew Cooper <andrew.cooper3@citrix.com> (tested
> via backport to xen-4.1.x)

Now that this is in, could you try (again on the offending system)
whether adding e.g. a WARN_ON(vector != desc->arch.old_vector)
prior to the just added call to eoi_IO_APIC_irq() (but inside the
surrounding if()) would ever trigger (obviously you'd want to make
sure that the code path actually gets executed at all - perhaps
counting and printing the count once in a while would be the easiest
thing to do)?

If it does, we obviously need to stay with passing in vector. If not,
we'd need to do another round of code inspection to determine
whether indeed there's no race when relying on just the stored
data.

Thanks, Jan

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Andrew Cooper]

On 18/11/11 08:31, Jan Beulich wrote:
> Now that this is in, could you try (again on the offending system)
> whether adding e.g. a WARN_ON(vector != desc->arch.old_vector)
> prior to the just added call to eoi_IO_APIC_irq() (but inside the
> surrounding if()) would ever trigger (obviously you'd want to make
> sure that the code path actually gets executed at all - perhaps
> counting and printing the count once in a while would be the easiest
> thing to do)?
>
> If it does, we obviously need to stay with passing in vector. If not,
> we'd need to do another round of code inspection to determine
> whether indeed there's no race when relying on just the stored
> data.
>
> Thanks, Jan

So long as you also check for arch.old_vector != IRQ_UNASSIGNED_VECTOR,
this appears to be fine.

I will sort out a patch to change this behavior

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts

[Andrew Cooper]

On 18/11/11 18:01, Andrew Cooper wrote:
> On 18/11/11 08:31, Jan Beulich wrote:
>> Now that this is in, could you try (again on the offending system)
>> whether adding e.g. a WARN_ON(vector != desc->arch.old_vector)
>> prior to the just added call to eoi_IO_APIC_irq() (but inside the
>> surrounding if()) would ever trigger (obviously you'd want to make
>> sure that the code path actually gets executed at all - perhaps
>> counting and printing the count once in a while would be the easiest
>> thing to do)?
>>
>> If it does, we obviously need to stay with passing in vector. If not,
>> we'd need to do another round of code inspection to determine
>> whether indeed there's no race when relying on just the stored
>> data.
>>
>> Thanks, Jan
> So long as you also check for arch.old_vector != IRQ_UNASSIGNED_VECTOR,
> this appears to be fine.
>
> I will sort out a patch to change this behavior
>

Wait actually not.  It turns out that there is some race condition which
causes this assertion not to hold.  Over the space of 2 hours with
16guests and dom0 each trying to stress their storage over a line level
interrupt, there have been 5 cases where vector != old_vector.

I presume it is some race condition where the scheduler is attempting to
move IRQs between PCPUs while they are already in a half moved state.

I will attempt to work out what is causing this race condition, but I
have some more important bugs to deal with at the moment.

I guess we can do with the kudge involving having the lapic vector
passed into hw_irq_handler.end until the race condition is identified.

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com