[v2,2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible

Commit Message

Julien Grall Dec. 11, 2023, 12:23 p.m. UTC

From: Julien Grall <[e-mail]>

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.

Take the opportunity to change the prototype of timer_irq_works() to
return a bool rather than int (which was already acting as a bool because
only 0/1 could be returned).

Signed-off-by: Julien Grall <[e-mail]>

---

Note that local_irq_restore() cannot be replaced with local_irq_disable()
because the function is not consistently called with IRQs off.

Changes in v2:
    - Return bool rather than int
    - Have a single return path
    - Use 'unsigned int' rather than 'unsigned long' for msec
---
 xen/arch/x86/io_apic.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

Comments

Jan Beulich Dec. 14, 2023, 10:19 a.m. UTC | #1

On 11.12.2023 13:23, Julien Grall wrote:
> From: Julien Grall <[e-mail]>
> 
> 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.

Isn't this more like 40ms (4 ticks 10ms apart)? And really somewhere
between 30 and 40, because the first tick has already partly elapsed?

> Take the opportunity to change the prototype of timer_irq_works() to
> return a bool rather than int (which was already acting as a bool because
> only 0/1 could be returned).
> 
> Signed-off-by: Julien Grall <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>

Jan

Patch

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 238b6c1c2837..c89fbed8d675 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1506,32 +1506,41 @@  static void __init setup_ioapic_ids_from_mpc(void)
  *	- if this function detects that timer IRQs are defunct, then we fall
  *	  back to ISA timer IRQs
  */
-static int __init timer_irq_works(void)
+static bool __init timer_irq_works(void)
 {
     unsigned long t1, flags;
+    /* Wait for maximum 10 ticks */
+    unsigned int msec = (10 * 1000) / HZ;
+    bool works = false;
 
     if ( pit_irq_works )
-        return 1;
+        return true;
 
     t1 = ACCESS_ONCE(pit0_ticks);
 
     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 )
-        return 1;
+    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;
 
-    return 0;
+        works = true;
+        break;
+    }
+
+    local_irq_restore(flags);
+
+    return works;
 }
 
 /*