Re: [PATCH v3 2/2] x86/mwait-idle: squash stats update when not actually entering C-state

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2022 13:42, Roger Pau Monné wrote:
> On Tue, Feb 01, 2022 at 12:37:27PM +0100, Jan Beulich wrote:
>> On 01.02.2022 12:04, Roger Pau Monné wrote:
>>> On Thu, Jan 27, 2022 at 04:13:47PM +0100, Jan Beulich wrote:
>>>> While we don't want to skip calling update_idle_stats(), arrange for it
>>>> to not increment the overall time spent in the state we didn't really
>>>> enter.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> ---
>>>> RFC: If we wanted to also move the tracing, then I think the part ahead
>>>>      of the if() also would need moving. At that point we could as well
>>>>      move update_last_cx_stat(), too, which afaict would allow skipping
>>>>      update_idle_stats() on the "else" path (which therefore would go
>>>>      away). Yet then, with the setting of power->safe_state moved up a
>>>>      little (which imo it should have been anyway) the two
>>>>      cpu_is_haltable() invocations would only have the lapic_timer_off()
>>>>      invocation left in between. This would then seem to call for simply
>>>>      ditching the 2nd one - acpi-idle also doesn't have a 2nd instance.
>>>
>>> It's possible for lapic_timer_off to take a non-trivial amount of time
>>> when virtualized, but it's likely we won't be using mwait in that
>>> case, so not sure it matter much to have the two cpu_is_haltable calls
>>> if there's just a lapic_timer_off between them.
>>>
>>>> TBD: For the tracing I wonder if that really needs to come ahead of the
>>>>      local_irq_enable(). Maybe trace_exit_reason() needs to, but quite
>>>>      certainly TRACE_6D() doesn't.
>>>
>>> Would be good if it could be moved after the local_irq_enable call, as
>>> it's not as trivial as I've expected, and will just add latency to any
>>> pending interrupt waiting to be serviced. FWIW, I haven't spotted a
>>> need to call it with interrupt disabled.
>>
>> Okay, I guess I'll to the larger rework then.
>>
>>>> --- a/xen/arch/x86/cpu/mwait-idle.c
>>>> +++ b/xen/arch/x86/cpu/mwait-idle.c
>>>> @@ -854,17 +854,23 @@ static void mwait_idle(void)
>>>>            mwait_idle_with_hints(cx->address, MWAIT_ECX_INTERRUPT_BREAK);
>>>>  
>>>>            local_irq_disable();
>>>> -  }
>>>>  
>>>> -  after = alternative_call(cpuidle_get_tick);
>>>> +          after = alternative_call(cpuidle_get_tick);
>>>> +
>>>> +          cstate_restore_tsc();
>>>> +
>>>> +          /* Now back in C0. */
>>>> +          update_idle_stats(power, cx, before, after);
>>>> +  } else {
>>>> +          /* Never left C0. */
>>>> +          after = alternative_call(cpuidle_get_tick);
>>>> +          update_idle_stats(power, cx, after, after);
>>>
>>> While adjusting this, could you also modify update_idle_stats to avoid
>>> increasing cx->usage if before == after (or !sleep_ticks). I don't
>>> think it's fine to increase the state counter if we never actually
>>> entered it.
>>
>> I did consider it but then decided against. Even leaving this aspect
>> aside the counter only counts _attempts_ to enter a certain state;
>> the CPU may find reasons to never actually enter it. And what we have
>> when before == after is still an attempt, albeit an unsuccessful one.
> 
> Right, in which case:
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, but ...

> Not sure whether you would like to commit this now and do the lager
> rework as a followup patch. That would be fine by me.

... no, I'd rather do this in a single step. In its current shape the
patch is actually moving us in the opposite direction.

Jan