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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 07/09/2023 15:28, Jan Beulich wrote:
> 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.)
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