Re: [PATCH v3 1/6] cpufreq: poowernv: Handle throttling due to Pmax capping at chip level

Preeti U Murthy
2015-05-07T12:15:30Z

On 05/07/2015 04:05 PM, Shilpasri G Bhat wrote:
>
>
> On 05/05/2015 02:08 PM, Preeti U Murthy wrote:
>> On 05/05/2015 11:36 AM, Shilpasri G Bhat wrote:
>>> Hi Preeti,
>>>
>>> On 05/05/2015 09:21 AM, Preeti U Murthy wrote:
>>>> Hi Shilpa,
>>>>
>>>> On 05/04/2015 02:24 PM, Shilpasri G Bhat wrote:
>>>>> The On-Chip-Controller(OCC) can throttle cpu frequency by reducing the
>>>>> max allowed frequency for that chip if the chip exceeds its power or
>>>>> temperature limits. As Pmax capping is a chip level condition report
>>>>> this throttling behavior at chip level and also do not set the global
>>>>> 'throttled' on Pmax capping instead set the per-chip throttled
>>>>> variable. Report unthrottling if Pmax is restored after throttling.
>>>>>
>>>>> This patch adds a structure to store chip id and throttled state of
>>>>> the chip.
>>>>>
>>>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>>>>> ---
>>>>> drivers/cpufreq/powernv-cpufreq.c | 59 ++++++++++++++++++++++++++++++++++++---
>>>>> 1 file changed, 55 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>>>>> index ebef0d8..d0c18c9 100644
>>>>> --- a/drivers/cpufreq/powernv-cpufreq.c
>>>>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>>>>> @@ -27,6 +27,7 @@
>>>>> #include <linux/smp.h>
>>>>> #include <linux/of.h>
>>>>> #include <linux/reboot.h>
>>>>> +#include <linux/slab.h>
>>>>>
>>>>> #include <asm/cputhreads.h>
>>>>> #include <asm/firmware.h>
>>>>> @@ -42,6 +43,13 @@
>>>>> static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
>>>>> static bool rebooting, throttled;
>>>>>
>>>>> +static struct chip {
>>>>> + unsigned int id;
>>>>> + bool throttled;
>>>>> +} *chips;
>>>>> +
>>>>> +static int nr_chips;
>>>>> +
>>>>> /*
>>>>> * Note: The set of pstates consists of contiguous integers, the
>>>>> * smallest of which is indicated by powernv_pstate_info.min, the
>>>>> @@ -301,22 +309,33 @@ static inline unsigned int get_nominal_index(void)
>>>>> static void powernv_cpufreq_throttle_check(unsigned int cpu)
>>>>> {
>>>>> unsigned long pmsr;
>>>>> - int pmsr_pmax, pmsr_lp;
>>>>> + int pmsr_pmax, pmsr_lp, i;
>>>>>
>>>>> pmsr = get_pmspr(SPRN_PMSR);
>>>>>
>>>>> + for (i = 0; i < nr_chips; i++)
>>>>> + if (chips[i].id == cpu_to_chip_id(cpu))
>>>>> + break;
>>>>> +
>>>>> /* Check for Pmax Capping */
>>>>> pmsr_pmax = (s8)PMSR_MAX(pmsr);
>>>>> if (pmsr_pmax != powernv_pstate_info.max) {
>>>>> - throttled = true;
>>>>> - pr_info("CPU %d Pmax is reduced to %d\n", cpu, pmsr_pmax);
>>>>> - pr_info("Max allowed Pstate is capped\n");
>>>>> + if (chips[i].throttled)
>>>>> + goto next;
>>>>> + chips[i].throttled = true;
>>>>> + pr_info("CPU %d on Chip %u has Pmax reduced to %d\n", cpu,
>>>>> + chips[i].id, pmsr_pmax);
>>>>> + } else if (chips[i].throttled) {
>>>>> + chips[i].throttled = false;
>>>>
>>>> Is this check on pmax sufficient to indicate that the chip is unthrottled ?
>>>
>>> Unthrottling due to Pmax uncapping here is specific to a chip. So it is
>>> sufficient to decide throttling/unthrottling when OCC is active for that chip.
>>
>> Ok then we can perhaps exit after detecting unthrottling here.
>
> This won't work for older firmwares which do not clear "Frequency control
> enabled bit" on OCC reset cycle. So let us check for remaining two conditions on
> unthrottling as well.

ok.

>
>>>
>>>>
>>>>> + pr_info("CPU %d on Chip %u has Pmax restored to %d\n", cpu,
>>>>> + chips[i].id, pmsr_pmax);
>>>>> }
>>>>>
>>>>> /*
>>>>> * Check for Psafe by reading LocalPstate
>>>>> * or check if Psafe_mode_active is set in PMSR.
>>>>> */
>>>>> +next:
>>>>> pmsr_lp = (s8)PMSR_LP(pmsr);
>>>>> if ((pmsr_lp < powernv_pstate_info.min) ||
>>>>> (pmsr & PMSR_PSAFE_ENABLE)) {
>>>>> @@ -414,6 +433,33 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
>>>>> .attr = powernv_cpu_freq_attr,
>>>>
>>>> What about the situation where although occ is active, this particular
>>>> chip has been throttled and we end up repeatedly reporting "pstate set
>>>> to safe" and "frequency control disabled from OS" ? Should we not have a
>>>> check on (chips[i].throttled) before reporting an anomaly for these two
>>>> scenarios as well just like you have for pmsr_pmax ?
>>>
>>> We will not have "Psafe" and "frequency control disabled" repeatedly printed
>>> because of global variable 'throttled', which is set to true on passing any of
>>> these two conditions.
>>>
>>> It is quite unlikely behavior to have only one chip in "Psafe" or "frequency
>>> control disabled" state. These two conditions are most likely to happen during
>>> an OCC reset cycle which will occur across all chips.
>>
>> Let us then add a comment to indicate that Psafe and frequency control
>> disabled conditions will fail *only if OCC is inactive* and not
>> otherwise and that this is a system wide phenomenon.
>>
>
> I agree that adding a comment here will clear global vs local throttling
> scenarios, but this will contradict the architectural design of OCC wherein it
> can independently go to "Psafe" and "frequency control disabled" state. It is
> the implementation in FSP today that has made the above two states global. My
> point is adding a comment here may be confusing if at all for the future
> firmwares this implementation is changed. Having said that the current patch set
> still seems fit for the newer implementation for the following reason:
> 1) The aim here is to identify any sort of throttling and report it to the user
> with least flooding of error messages, which will happen even if OCC can
> independently reset and restore.
> 2) On unthrottling verify throttling on the chips with the exception of Pmax
> capping is also taken care by this patch set.

Ok as long as we are reporting right, its fine.

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>
> Thanks and Regards,
> Shilpa
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>