[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4] x86/mwait-idle: re-order state entry/exit code a little
On 22.02.2022 10:08, Roger Pau Monné wrote: > 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> Thanks. >> --- 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. Well, the justification for enabling was the low exit time. I don't expect states > 2 to satisfy this criteria, so I think we're okay without further precautions added right away. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |