Jan Beulich
2011-Nov-15 13:14 UTC
[Xen-devel] [PATCH] 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)); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Nov-15 13:19 UTC
Re: [Xen-devel] [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-15 13:27 UTC
Re: [Xen-devel] [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Nov-15 13:35 UTC
Re: [Xen-devel] [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
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. > > JanOk. 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-15 13:43 UTC
Re: [Xen-devel] [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Nov-17 16:12 UTC
Re: [Xen-devel] [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-18 08:31 UTC
Re: [Xen-devel] [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Nov-18 18:01 UTC
Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
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, JanSo 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
Andrew Cooper
2011-Nov-18 18:57 UTC
Re: [PATCH] x86/IO-APIC: refine EOI-ing of migrating level interrupts
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