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

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

[Like Xu]

On 2020/4/16 14:08, Giovanni Gherdovich wrote:
> On Thu, 2020-04-16 at 10:12 +0800, Like Xu wrote:
>> On the Intel SNR processors such as "Intel Atom(R) C6562", the
>> turbo_freq for 4C turbo may be zero which causes a divide by zero
>> exception and blocks the boot process when arch_scale_freq_tick().
>>
>> When one of the preset base_freq or turbo_freq is meaningless,
>> we may disable frequency invariance.
>>
>> Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance")
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>> ---
>>   arch/x86/kernel/smpboot.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index fe3ab9632f3b..741367ce4d14 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1958,6 +1958,9 @@ static bool core_set_max_freq_ratio(u64 *base_freq, u64 *turbo_freq)
>>   	*base_freq = (*base_freq >> 8) & 0xFF;      /* max P state */
>>   	*turbo_freq = (*turbo_freq >> 24) & 0xFF;   /* 4C turbo    */
>>   
>> +	if (*turbo_freq == 0 || *base_freq == 0)
>> +		return false;
>> +
>>   	return true;
>>   }
>>
> 
> 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).

> 
> As per why I'd like to go with 1 core turbo instead of bailing out of freq
> invariance entirely, I've left a comment in the openSUSE bugzilla with some
> details: https://bugzilla.opensuse.org/show_bug.cgi?id=1166664#c35

> The relevant part is:
> 
> :: The fix in this case is to take the 1 core turbo as normalizing factor. The
> :: other choice would be to use the base frequency (no turbo at all), but using
> :: base freq for normalization means that the ratio becomes current_freq / base_freq
> :: which is an over-estimation, which leads the frequency governor to think the
> :: CPU is more loaded than it really is and raise the frequency a bit too
> :: aggressively. This is tolerable in performance-oriented servers but
> :: inappropriate on small machines with 2 cores."
> 
> Regarding base_freq being reported as zero, you're right, that can happen too
> and we've seen it in hypervisors.
> 
> 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.

Thanks,
Like Xu

> 
> 
> Thanks,
> Giovanni Gherdovich
> 

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

[Giovanni Gherdovich]

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.

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.

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.


Thanks,
Giovanni Gherdovich

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

[Like Xu]

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
> 

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

[Giovanni Gherdovich]

On Thu, 2020-04-16 at 20:54 +0800, Like Xu wrote:
> On 2020/4/16 16:40, Giovanni Gherdovich wrote:
> > [...]
> > 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.

No worries, I understand.

> 
> > 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.

Ok.

> 
> > 
> > 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! The model name from intel-family.h is useful!


Giovanni

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

[Like Xu]

On the Intel SNR processors such as "Intel Atom(R) C6562", the
turbo_freq for 4C turbo may be zero which causes a divide by zero
exception and blocks the boot process when arch_scale_freq_tick().

When one of the preset base_freq or turbo_freq is meaningless,
we may disable frequency invariance.

Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance")
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 arch/x86/kernel/smpboot.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index fe3ab9632f3b..741367ce4d14 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1958,6 +1958,9 @@ static bool core_set_max_freq_ratio(u64 *base_freq, u64 *turbo_freq)
 	*base_freq = (*base_freq >> 8) & 0xFF;      /* max P state */
 	*turbo_freq = (*turbo_freq >> 24) & 0xFF;   /* 4C turbo    */
 
+	if (*turbo_freq == 0 || *base_freq == 0)
+		return false;
+
 	return true;
 }
 
-- 
2.21.1