Re: [PATCH] x86, smpboot: Disable frequency invariance when it's unsupported

[Like Xu <like.xu@linux.intel.com>]

On 2020/4/16 16:40, Giovanni Gherdovich wrote:
> On Thu, 2020-04-16 at 15:01 +0800, Like Xu wrote:
>> On 2020/4/16 14:08, Giovanni Gherdovich wrote:
>>> [...]
>>> I've just sent fixes for these two problems here:
>>> https://lore.kernel.org/lkml/20200416054745.740-1-ggherdovich@suse.cz/
>>
>> Hence the "less than 4 cores" comment is weird for C6562
>> but the use of "1C turbo" looks good to me.
> 
> Right, your C6562 has 24 cores, (I think) it doesn't support turbo at all,
> declares 1C turbo equal to the base frequency and all other turbo ratios (2C,
> 4C etc) as zero.
> 
> The commit message of the fix I sent doesn't describe exactly your situation
> but the patch addresses your case nonetheless. Some more comments below.
> 
> On Thu, 2020-04-16 at 15:01 +0800, Like Xu wrote:
>> On 2020/4/16 14:08, Giovanni Gherdovich wrote:
>>> [...]
>>> Hello Like Xu,
>>>
>>> thanks for reporting this and for the patch. My preferred solution for when
>>> the 4 cores turbo freq is detected as zero would be to look for the 1 core turbo
>>> frequency, as we're likely on a machine with less than 4 cores. Is that the
>>> case on your Atom C6562? I couldn't find it on ark.intel.com.
>>
>> The Atom C6562 is "24 cores" based on
>> https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/atom-p5900-product-brief.pdf
>>
>> #define MSR_PLATFORM_INFO		0x000000ce
>>
>> the value for this msr is 80820f9801600
>>
>> #define MSR_TURBO_RATIO_LIMIT		0x000001ad
>>
>> the value for this msr is 16
>>
>> I know you didn't test your feature on this platform,
>> but combinations of other various values are also possible
>> (unless it's made clear in the specification).
> 
> That's an interesting CPU; let me indulge in a couple of comments/questions
> for my own curiosity.
>>From the document you link, the product name in the Intel catalogue seems to
> be Atom P5962B. Apparently it belongs to the "P Series" just launched:
> https://ark.intel.com/content/www/us/en/ark/products/series/202693/intel-atom-processor-p-series.html
> and your product brief suggests it's meant for installation in 5G base stations.
> 
> 1) Can you share the output of "turbostat --interval 1 sleep 0"? I'm
>     interested in the headers of the output, where all the various pm-related
>     MSRs are decoded.
> 

I couldn't disclose more information about this.

> 2) Despite not being in the Intel SDM, I was under the assumption that all
>     Intel CPUs declare the "all-cores turbo" frequency, but it's not the case
>     for this one. Eg: if you have 24 cores, somewhere in your MSRs I'd expect
>     to find "24C turbo" (or even "30C turbo", anything greater or equal than 24).
>     My understanding from
>     https://ark.intel.com/content/www/us/en/ark/products/202682/intel-atom-processor-p5962b-27m-cache-2-20-ghz.html
>     is that this CPU doesn't support turbo boost at all; in other CPUs without
>     turbo I've seen MSRs saying the all-cores turbo freq is equal to the base
>     freq (for compatibility I suppose). Here MSR_TURBO_RATIO_LIMIT says that 1C
>     turbo is the same as base frequency (2.2GHz), but turbo for larger sets of
>     cores is declared as zero, which I find a little odd.

That's odd and we could only rely on the Intel specification
about the assumption "Intel CPUs declare the all-cores turbo frequency"
and I may report this issue if something does mismatch.

> 
> 3) The parsing of MSRs in the frequency invariance code is modeled after
>     turbostat, and classifies CPUs in 5 groups: Atom up to Goldmont, Atom from
>     Goldmont onwards, Xeon Phi, Xeon Scalable Processors onwards and "generic
>     Core". As you've already found out from where your panic happens, your Atom
>     falls into the "generic Core" category (function core_set_max_freq_ratio()),
>     but given that it's an Atom and it's been released this very quarter I'd
>     have guessed it to behave like a Goldmont. Something for me to keep in mind.

It's INTEL_FAM6_ATOM_TREMONT or INTEL_FAM6_ATOM_TREMONT_D.

Thanks,
Like Xu

> 
> 
> Thanks,
> Giovanni Gherdovich
>