On Wed, Oct 9, 2024 at 6:49 PM Dave Hansen dave.hansen@intel.com wrote:

On 10/9/24 00:20, Zhang Rui wrote:

...

This 12-year-old bug prevents some modern processors from achieving maximum power savings during suspend. For example, Lunar Lake systems gets 0% package C-states during suspend to idle and this causes energy star compliance tests to fail.

Why haven't we noticed or cared for the last 12 years?

Because on the previous platforms the apic_write(APIC_TMICT, 0) in lapic_timer_shutdown() was sufficient even in the TSC deadline mode, or at least no problems with it have ever been reported.

Also, plain language really matters. Is this as simple as: "you close the lid on the laptop and the CPU doesn't power down at all"?

It will power down somewhat, but not as much as to justify suspending the system. It will just drain the battery at a relatively high rate which will also cause the machine to heat up.

Not a good idea to put it into a bag in this state, for instance.

...

According to Intel SDM, for the local APIC timer,

1. "The initial-count register is a read-write register. A write of 0 to the initial-count register effectively stops the local APIC timer, in both one-shot and periodic mode."
2. "In TSC deadline mode, writes to the initial-count register are ignored; and current-count register always reads 0. Instead, timer behavior is controlled using the IA32_TSC_DEADLINE MSR." "In TSC-deadline mode, writing 0 to the IA32_TSC_DEADLINE MSR disarms the local-APIC timer."

Is "stopping" and "disarming" the same thing?

That is my understanding. If you disarm it and it is not armed again, it will be stopped effectively.

Second, while quoting the SDM is great, it would be even better to including the Linux naming for these things. The Linux naming for the APIC registers is completely missing from this changelog. You could say:

    "In TSC deadline mode, writes to the initial-count register
    (APIC_TMICT) are ignored"

which makes it much easier to relate this code:

    apic_write(APIC_TMICT, 0);

back to the SDM language. This is especially true because:

#define APIC_TMICT 0x380

doesn't make it obvious that "ICT" is the "Initial-Count Register". I had to go back to the SDM table to make 100% sure.

This also doesn't ever say which mode the kernel is running in.

...

Stop the TSC Deadline timer in lapic_timer_shutdown() by writing 0 to MSR_IA32_TSC_DEADLINE.

This dances around the problem but never comes out and says it:

    The CPU package does not go into lower power modes (higher
    package C-states) unless all local-APIC timers are disabled.

Plus something to connect the old to the new:

    On older CPUs, setting APIC_TMICT=0 was sufficient for disabling
    the local-APIC timer, no matter the timer mode (deadline, one-
    shot or periodic).  But newer CPUs adhere to the strict letter
    of the law in the SDM and more fully ignore APIC_TMICT when in
    deadline mode.  Those CPUs also don't fully "disable" the timer
    when IA32_TSC_DEADLINE has passed.  They _require_ writing a 0.

Or am I missing something?

No, you are right.

We need a new version of the patch with a better changelog.

...

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index 6513c53c9459..d1006531729a 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -441,6 +441,10 @@ static int lapic_timer_shutdown(struct clock_event_device *evt) v |= (APIC_LVT_MASKED | LOCAL_TIMER_VECTOR); apic_write(APIC_LVTT, v); apic_write(APIC_TMICT, 0);

• if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
  

•         wrmsrl(MSR_IA32_TSC_DEADLINE, 0);
  

• return 0;
  

}

On Wed, Oct 9, 2024 at 7:49 PM Dave Hansen dave.hansen@intel.com wrote:

On 10/9/24 00:20, Zhang Rui wrote:

...

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index 6513c53c9459..d1006531729a 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -441,6 +441,10 @@ static int lapic_timer_shutdown(struct clock_event_device *evt) v |= (APIC_LVT_MASKED | LOCAL_TIMER_VECTOR); apic_write(APIC_LVTT, v); apic_write(APIC_TMICT, 0);

• if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
  

•         wrmsrl(MSR_IA32_TSC_DEADLINE, 0);
  

One last thing, and this is a super nit. We presumably have the actual APIC_LVTT value (v) sitting in a register already. Is there any difference logically between a X86_FEATURE_TSC_DEADLINE_TIMER check and an APIC_LVTT check for APIC_LVT_TIMER_TSCDEADLINE?

I suspect this will generate more compact code:

    if (v & APIC_LVT_TIMER_TSCDEADLINE)
            wrmsrl(MSR_IA32_TSC_DEADLINE, 0);

Does it have any downsides?

I don't see any.

Oh, and how hot is this path? Is this wrmsr() going to matter? I presume it's pretty cheap because it's one of the special architecturally non-serializing WRMSRs.

lapic_timer_shutdown() is called under a raw spin lock in ___tick_broadcast_oneshot_control(), so it better not take too much time or PREEMPT_RT might be unhappy. I'm not sure how often that happens, though.

Also tick_program_event() calls it to stop the tick, but it is assumed that this may take time AFAICS.