Re: [PATCH v4] x86/mwait-idle: re-order state entry/exit code a little

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 18, 2022 at 09:35:10AM +0100, Jan Beulich wrote:
> The initial observation is that unlike the original ACPI idle driver we
> have a 2nd cpu_is_haltable() in here. By making the actual state entry
> conditional, the emitted trace records as well as the subsequent stats
> update are at least misleading in case the state wasn't actually entered.
> Hence they would want moving inside the conditional. At which point the
> cpuidle_get_tick() invocations could (and hence should) move as well.
> cstate_restore_tsc() also isn't needed if we didn't actually enter the
> state.
> 
> This leaves only the errata_c6_workaround() and lapic_timer_off()
> invocations outside the conditional. As a result it looks easier to
> drop the conditional (and come back in sync with the other driver again)
> than to move almost everything into the conditional.
> 
> While there also move the TRACE_6D() out of the IRQ-disabled region.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> Moving the TRACE_6D() may be a little controversial, as this could lead
> to a sequence of trace records not actually representing the sequence of
> events, in case further records get emitted by interrupt handlers. But
> with us now conditionally enabling interrupts around MWAIT, that issue
> exists already anyway.

I think that's OK. We have to priority interrupt latency over trace
readability.

> Unlike said in the earlier outline of the alternative approach,
> errata_c6_workaround() cannot be moved: cpu_has_pending_apic_eoi() needs
> to be called when IRQs are already off.
> ---
> v4: Different approach (and title), as was previously outlined as an
>     alternative.
> v3: Also move cstate_restore_tsc() invocation and split ones to
>     update_idle_stats().
> v2: New.
> 
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -847,26 +847,25 @@ static void mwait_idle(void)
>  
>       update_last_cx_stat(power, cx, before);
>  
> -     if (cpu_is_haltable(cpu)) {
> -             if (cx->irq_enable_early)
> -                     local_irq_enable();
> +     if (cx->irq_enable_early)
> +             local_irq_enable();

Now that I look at this again, we need to be careful with the enabling
interrupts and the interaction with errata_c6_workaround.  Enabling
interrupts here could change the result of the check for pending EOIs,
and thus enter mwait with a condition that could trigger the erratas.
Hopefully CPUIDLE_FLAG_IRQ_ENABLE is only set for C1.

It might be prudent to only allow setting CPUIDLE_FLAG_IRQ_ENABLE for
states <= 2.

Thanks, Roger.