Re: [PATCH] perf/x86/intel: Restrict period on Haswell

[Thomas Gleixner <tglx@linutronix.de>]

Kan!

On Thu, Aug 15 2024 at 11:39, Kan Liang wrote:
> On 2024-08-14 6:47 p.m., Thomas Gleixner wrote:
>> Now the conclusion of this fun exercise is:
>> 
>>     1) The hardware behaves differently when the perf event happens
>>        concurrently on HT siblings
>
> I think I found a related erratum.

> HSM154. Fixed-Function Performance Counter May Over Count Instructions
> Retired by 32 When Intel® Hyper-Threading Technology is Enabled
>
> Problem: If, while Intel Hyper-Threading Technology is enabled, the
> IA32_FIXED_CTR0 MSR
> (309H) is enabled by setting bits 0 and/or 1 in the
> IA32_PERF_FIXED_CTR_CTRL MSR
> (38DH) before setting bit 32 in the IA32_PERF_GLOBAL_CTRL MSR (38FH) then
> IA32_FIXED_CTR0 may over count by up to 32.
>
> Implication: When this erratum occurs, the fixed-function performance
> counter IA32_FIXED_CTR0 may over count by up to 32.

Sure. That's only explaining half of the problem.

As I demonstrated in the non-contended case even with a count of 2 (I
tried 1 too) the status bit is never set on the second check.

Which is weird, because the number of instructions between setting the
count and re-checking the status MSR is definitely larger than 2 (or 1).

> Workaround: The following sequence avoids this erratum (steps 1 and 2
> are needed if the counter was previously enabled):
> 1. Clear bit 32 in the IA32_PERF_GLOBAL_CTRL MSR (38FH) and clear bits 1
> and 0 in the IA32_PERF_FIXED_CTR_CTRL MSR (38DH).
> 2. Zero the IA32_FIXED_CTR0 MSR.
> 3. Set bit 32 in the IA32_PERF_GLOBAL_CTRL MSR.
> 4. Set bits 0 and/or 1 in the IA32_PERF_FIXED_CTR_CTRL MSR as desired.
>
> It should explains that the issue is gone with the magic number 32 or
> disabling the Hyper-Threading.

It explains only half of it. If you use 32, then the counter is set to
-32 so the overcount of 32 will still bring it to 0, which should set
the status bit, no?

> I also found a related discussion about 9 years ago.
> https://lore.kernel.org/lkml/alpine.DEB.2.11.1505181343090.32481@vincent-weaver-1.umelst.maine.edu/
> Vince tried the workaround but it seems not work.

Let me play with that. :)

>>     2) The frequency estimation algorithm is broken
>
> For the events which occurs frequently, e.g., instructions, cycles, yes,
> the frequency estimation algorithm doesn't work well.
>
> But there are events that may not occur frequently. If a big init period
> is set, it may be impossible to get the required freq for those events.
>
> It's really hard to pick a universal init period that works for all
> events.

I understand that, but especially for RETIRED it's obvious :)

> I'm thinking perf may only calculate/pre-set a init period for the Linux
> defined architectural events, e.g., instructions, cycles, branches,
> cache related events, etc. For the other ARCH specific events, I'm
> afraid the period has to start 1.

Yes, that would be way better than what we have now.

>> 
>>     3) Using a 'limit' guestimate is just papering over the underlying
>>        problems
>
> It's possible that a user set a small number with -c. If the number is
> less than the 'limit', it needs to be adjusted to avoid HW failure.
> I think the 'limit' is still required.

I'm not against the limit per se. I'm against guestimated limits which
are thrown into the code without understanding the underlying problem.

The just paper over it up to the point where they bite back because the
guestimate was off by $N.

Thanks,

        tglx