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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2023 15:44, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> 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 <jgrall@xxxxxxxxxx>

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