Re: [PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible
On 15.09.2023 16:00, Julien Grall wrote: > Hi Jan, > > On 07/09/2023 15:28, Jan Beulich wrote: >> On 18.08.2023 15:44, Julien Grall wrote: >>> From: Julien Grall >>> >>> Currently timer_irq_works() will wait the full 100ms before checking >>> that pit0_ticks has been incremented at least 4 times. >>> >>> However, the bulk of the BIOS/platform should not have a buggy timer. >>> So waiting for the full 100ms is a bit harsh. >>> >>> Rework the logic to only wait until 100ms passed or we saw more than >>> 4 ticks. So now, in the good case, this will reduce the wait time >>> to ~50ms. >>> >>> Signed-off-by: Julien Grall >> >> In principle this is all fine. There's a secondary aspect though which >> may call for a slight rework of the patch. >> >>> --- a/xen/arch/x86/io_apic.c >>> +++ b/xen/arch/x86/io_apic.c >>> @@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void) >>> static int __init timer_irq_works(void) >>> { >>> unsigned long t1, flags; >>> +/* Wait for maximum 10 ticks */ >>> +unsigned long msec = (10 * 1000) / HZ; >> >> (Minor remark: I don't think this needs to be unsigned long; unsigned >> in will suffice.) > > You are right. I can switch to unsigned int. > >> >>> @@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void) >>> >>> local_save_flags(flags); >>> local_irq_enable(); >>> -/* Let ten ticks pass... */ >>> -mdelay((10 * 1000) / HZ); >>> -local_irq_restore(flags); >>> >>> -/* >>> - * Expect a few ticks at least, to be sure some possible >>> - * glue logic does not lock up after one or two first >>> - * ticks in a non-ExtINT mode. Also the local APIC >>> - * might have cached one ExtINT interrupt. Finally, at >>> - * least one tick may be lost due to delays. >>> - */ >>> -if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 ) >>> +while ( msec-- ) >>> +{ >>> +mdelay(1); >>> +/* >>> + * Expect a few ticks at least, to be sure some possible >>> + * glue logic does not lock up after one or two first >>> + * ticks in a non-ExtINT mode. Also the local APIC >>> + * might have cached one ExtINT interrupt. Finally, at >>> + * least one tick may be lost due to delays. >>> + */ >>> +if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 ) >>> +continue; >>> + >>> +local_irq_restore(flags); >>> return 1; >>> +} >>> + >>> +local_irq_restore(flags); >>> >>> return 0; >>> } >> >> While Andrew has a patch pending (not sure why it didn't go in yet) >> to simplify local_irq_restore(), and while further it shouldn't really >> need using here (local_irq_disable() ought to be fine) > > Skimming through the code, the last call of timer_irq_works() in > check_timer() happens after the interrupts masking state have been restored: > > local_irq_restore(flags); > > if ( timer_irq_works() ) >... > > > So I think timer_irq_works() can be called with interrupts enabled and > therefore we can't use local_irq_disable(). Hmm, yes, you're right. That's inconsistent, but dealing with that is a separate task. Jan
Re: [PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible
Hi Jan, On 07/09/2023 15:28, Jan Beulich wrote: On 18.08.2023 15:44, Julien Grall wrote: From: Julien Grall Currently timer_irq_works() will wait the full 100ms before checking that pit0_ticks has been incremented at least 4 times. However, the bulk of the BIOS/platform should not have a buggy timer. So waiting for the full 100ms is a bit harsh. Rework the logic to only wait until 100ms passed or we saw more than 4 ticks. So now, in the good case, this will reduce the wait time to ~50ms. Signed-off-by: Julien Grall In principle this is all fine. There's a secondary aspect though which may call for a slight rework of the patch. --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void) static int __init timer_irq_works(void) { unsigned long t1, flags; +/* Wait for maximum 10 ticks */ +unsigned long msec = (10 * 1000) / HZ; (Minor remark: I don't think this needs to be unsigned long; unsigned in will suffice.) You are right. I can switch to unsigned int. @@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void) local_save_flags(flags); local_irq_enable(); -/* Let ten ticks pass... */ -mdelay((10 * 1000) / HZ); -local_irq_restore(flags); -/* - * Expect a few ticks at least, to be sure some possible - * glue logic does not lock up after one or two first - * ticks in a non-ExtINT mode. Also the local APIC - * might have cached one ExtINT interrupt. Finally, at - * least one tick may be lost due to delays. - */ -if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 ) +while ( msec-- ) +{ +mdelay(1); +/* + * Expect a few ticks at least, to be sure some possible + * glue logic does not lock up after one or two first + * ticks in a non-ExtINT mode. Also the local APIC + * might have cached one ExtINT interrupt. Finally, at + * least one tick may be lost due to delays. + */ +if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 ) +continue; + +local_irq_restore(flags); return 1; +} + +local_irq_restore(flags); return 0; } While Andrew has a patch pending (not sure why it didn't go in yet) to simplify local_irq_restore(), and while further it shouldn't really need using here (local_irq_disable() ought to be fine) Skimming through the code, the last call of timer_irq_works() in check_timer() happens after the interrupts masking state have been restored: local_irq_restore(flags); if ( timer_irq_works() ) ... So I think timer_irq_works() can be called with interrupts enabled and therefore we can't use local_irq_disable(). I can see that you don't want to make such an adjustment here. But then I'd prefer if we got away with just a single instance, adjusting the final return statement accordingly (easiest would likely be to go from the value of "msec"). I was thinking to use 'msec > 0' as a condition to determine if the test passed. However, it would consider a failure if it tooks 10ms for the test to pass. I will see if I can rework the loop. Cheers, -- Julien Grall
Re: [PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible
On 18.08.2023 15:44, Julien Grall wrote: > From: Julien Grall > > Currently timer_irq_works() will wait the full 100ms before checking > that pit0_ticks has been incremented at least 4 times. > > However, the bulk of the BIOS/platform should not have a buggy timer. > So waiting for the full 100ms is a bit harsh. > > Rework the logic to only wait until 100ms passed or we saw more than > 4 ticks. So now, in the good case, this will reduce the wait time > to ~50ms. > > Signed-off-by: Julien Grall In principle this is all fine. There's a secondary aspect though which may call for a slight rework of the patch. > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void) > static int __init timer_irq_works(void) > { > unsigned long t1, flags; > +/* Wait for maximum 10 ticks */ > +unsigned long msec = (10 * 1000) / HZ; (Minor remark: I don't think this needs to be unsigned long; unsigned in will suffice.) > @@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void) > > local_save_flags(flags); > local_irq_enable(); > -/* Let ten ticks pass... */ > -mdelay((10 * 1000) / HZ); > -local_irq_restore(flags); > > -/* > - * Expect a few ticks at least, to be sure some possible > - * glue logic does not lock up after one or two first > - * ticks in a non-ExtINT mode. Also the local APIC > - * might have cached one ExtINT interrupt. Finally, at > - * least one tick may be lost due to delays. > - */ > -if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 ) > +while ( msec-- ) > +{ > +mdelay(1); > +/* > + * Expect a few ticks at least, to be sure some possible > + * glue logic does not lock up after one or two first > + * ticks in a non-ExtINT mode. Also the local APIC > + * might have cached one ExtINT interrupt. Finally, at > + * least one tick may be lost due to delays. > + */ > +if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 ) > +continue; > + > +local_irq_restore(flags); > return 1; > +} > + > +local_irq_restore(flags); > > return 0; > } While Andrew has a patch pending (not sure why it didn't go in yet) to simplify local_irq_restore(), and while further it shouldn't really need using here (local_irq_disable() ought to be fine), I can see that you don't want to make such an adjustment here. But then I'd prefer if we got away with just a single instance, adjusting the final return statement accordingly (easiest would likely be to go from the value of "msec"). Jan
[PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible
From: Julien Grall Currently timer_irq_works() will wait the full 100ms before checking that pit0_ticks has been incremented at least 4 times. However, the bulk of the BIOS/platform should not have a buggy timer. So waiting for the full 100ms is a bit harsh. Rework the logic to only wait until 100ms passed or we saw more than 4 ticks. So now, in the good case, this will reduce the wait time to ~50ms. Signed-off-by: Julien Grall --- xen/arch/x86/io_apic.c | 30 +++--- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index 4875bb97003f..0bd774962a68 100644 --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void) static int __init timer_irq_works(void) { unsigned long t1, flags; +/* Wait for maximum 10 ticks */ +unsigned long msec = (10 * 1000) / HZ; if ( no_timer_check ) return 1; @@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void) local_save_flags(flags); local_irq_enable(); -/* Let ten ticks pass... */ -mdelay((10 * 1000) / HZ); -local_irq_restore(flags); -/* - * Expect a few ticks at least, to be sure some possible - * glue logic does not lock up after one or two first - * ticks in a non-ExtINT mode. Also the local APIC - * might have cached one ExtINT interrupt. Finally, at - * least one tick may be lost due to delays. - */ -if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 ) +while ( msec-- ) +{ +mdelay(1); +/* + * Expect a few ticks at least, to be sure some possible + * glue logic does not lock up after one or two first + * ticks in a non-ExtINT mode. Also the local APIC + * might have cached one ExtINT interrupt. Finally, at + * least one tick may be lost due to delays. + */ +if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 ) +continue; + +local_irq_restore(flags); return 1; +} + +local_irq_restore(flags); return 0; } -- 2.40.1