| Message ID | 20211109195714.7750-5-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor thermal pressure update to avoid code duplication | expand |
On 11/9/21 2:57 PM, Lukasz Luba wrote: > Thermal pressure provides a new API, which allows to use CPU frequency > as an argument. That removes the need of local conversion to capacity. > Use this new API and remove old local conversion code. > > The new arch_update_thermal_pressure() also accepts boost frequencies, > which solves issue in the driver code with wrong reduced capacity > calculation. The reduced capacity was calculated wrongly due to > 'policy->cpuinfo.max_freq' used as a divider. The value present there was > actually the boost frequency. Thus, even a normal maximum frequency value > which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id)) > is not able to remove the capping. Yes, although cpuinfo.max_freq does not reflect the boost frequency unless boost is enabled atleast once. I have sent a patch to fix this. But I agree that using cpuinfo.max_freq has issues you have mentioned in this patch if boost is enabled once. So, for this patch Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org> Warm Regards Thara (She/Her/Hers) > > The second side effect which is solved is that the reduced frequency wasn't > properly translated into the right reduced capacity, > e.g. > boost frequency = 3000MHz (stored in policy->cpuinfo.max_freq) > max normal frequency = 2500MHz (which is 1024 capacity) > 2nd highest frequency = 2000MHz (which translates to 819 capacity) > > Then in a scenario when the 'throttled_freq' max allowed frequency was > 2000MHz the driver translated it into 682 capacity: > capacity = 1024 * 2000 / 3000 = 682 > Then set the pressure value bigger than actually applied by the HW: > max_capacity - capacity => 1024 - 682 = 342 (<- thermal pressure) > Which was causing higher throttling and misleading task scheduler > about available CPU capacity. > A proper calculation in such case should be: > capacity = 1024 * 2000 / 2500 = 819 > 1024 - 819 = 205 (<- thermal pressure) > > This patch relies on the new arch_update_thermal_pressure() handling > correctly such use case (with boost frequencies). > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 0138b2ec406d..248135e5087e 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -275,10 +275,10 @@ static unsigned int qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data) > > static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) > { > - unsigned long max_capacity, capacity, freq_hz, throttled_freq; > struct cpufreq_policy *policy = data->policy; > int cpu = cpumask_first(policy->cpus); > struct device *dev = get_cpu_device(cpu); > + unsigned long freq_hz, throttled_freq; > struct dev_pm_opp *opp; > unsigned int freq; > > @@ -295,17 +295,8 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) > > throttled_freq = freq_hz / HZ_PER_KHZ; > > - /* Update thermal pressure */ > - > - max_capacity = arch_scale_cpu_capacity(cpu); > - capacity = mult_frac(max_capacity, throttled_freq, policy->cpuinfo.max_freq); > - > - /* Don't pass boost capacity to scheduler */ > - if (capacity > max_capacity) > - capacity = max_capacity; > - > - arch_set_thermal_pressure(policy->related_cpus, > - max_capacity - capacity); > + /* Update thermal pressure (the boost frequencies are accepted) */ > + arch_update_thermal_pressure(policy->related_cpus, throttled_freq); > > /* > * In the unlikely case policy is unregistered do not enable >
On 11/15/21 3:57 PM, Thara Gopinath wrote: > > > On 11/9/21 2:57 PM, Lukasz Luba wrote: >> Thermal pressure provides a new API, which allows to use CPU frequency >> as an argument. That removes the need of local conversion to capacity. >> Use this new API and remove old local conversion code. >> >> The new arch_update_thermal_pressure() also accepts boost frequencies, >> which solves issue in the driver code with wrong reduced capacity >> calculation. The reduced capacity was calculated wrongly due to >> 'policy->cpuinfo.max_freq' used as a divider. The value present there was >> actually the boost frequency. Thus, even a normal maximum frequency value >> which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id)) >> is not able to remove the capping. Also I failed to mention that, currently freq_factor is initialized as cpuinfo.max_freq / 1000 which means again all the issues you mentioned below can be hit, if some cpufreq driver decides to set boost at init. I have sent a patch earlier today to fix this. https://lore.kernel.org/linux-arm-msm/20211115201010.68567-1-thara.gopinath@linaro.org/T/#u
Hi Thara, On 11/15/21 11:39 PM, Thara Gopinath wrote: > > > On 11/15/21 3:57 PM, Thara Gopinath wrote: >> >> >> On 11/9/21 2:57 PM, Lukasz Luba wrote: >>> Thermal pressure provides a new API, which allows to use CPU frequency >>> as an argument. That removes the need of local conversion to capacity. >>> Use this new API and remove old local conversion code. >>> >>> The new arch_update_thermal_pressure() also accepts boost frequencies, >>> which solves issue in the driver code with wrong reduced capacity >>> calculation. The reduced capacity was calculated wrongly due to >>> 'policy->cpuinfo.max_freq' used as a divider. The value present there >>> was >>> actually the boost frequency. Thus, even a normal maximum frequency >>> value >>> which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id)) >>> is not able to remove the capping. > > Also I failed to mention that, currently freq_factor is initialized as > cpuinfo.max_freq / 1000 which means again all the issues you mentioned > below can be hit, if some cpufreq driver decides to set boost at init. > I have sent a patch earlier today to fix this. Yes, you are right. > > https://lore.kernel.org/linux-arm-msm/20211115201010.68567-1-thara.gopinath@linaro.org/T/#u > > Looking at the change, it makes sense. I'll try to respond to that patch. Thank you for looking into this issue. Regards, Lukasz
On 11/15/21 8:57 PM, Thara Gopinath wrote: > > > On 11/9/21 2:57 PM, Lukasz Luba wrote: >> Thermal pressure provides a new API, which allows to use CPU frequency >> as an argument. That removes the need of local conversion to capacity. >> Use this new API and remove old local conversion code. >> >> The new arch_update_thermal_pressure() also accepts boost frequencies, >> which solves issue in the driver code with wrong reduced capacity >> calculation. The reduced capacity was calculated wrongly due to >> 'policy->cpuinfo.max_freq' used as a divider. The value present there was >> actually the boost frequency. Thus, even a normal maximum frequency value >> which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id)) >> is not able to remove the capping. > > Yes, although cpuinfo.max_freq does not reflect the boost frequency > unless boost is enabled atleast once. I have sent a patch to fix this. > But I agree that using cpuinfo.max_freq has issues you have mentioned in > this patch if boost is enabled once. > > So, for this patch > > Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org> Thank you for the review! > > Warm Regards > Thara (She/Her/Hers)
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c index 0138b2ec406d..248135e5087e 100644 --- a/drivers/cpufreq/qcom-cpufreq-hw.c +++ b/drivers/cpufreq/qcom-cpufreq-hw.c @@ -275,10 +275,10 @@ static unsigned int qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data) static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) { - unsigned long max_capacity, capacity, freq_hz, throttled_freq; struct cpufreq_policy *policy = data->policy; int cpu = cpumask_first(policy->cpus); struct device *dev = get_cpu_device(cpu); + unsigned long freq_hz, throttled_freq; struct dev_pm_opp *opp; unsigned int freq; @@ -295,17 +295,8 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) throttled_freq = freq_hz / HZ_PER_KHZ; - /* Update thermal pressure */ - - max_capacity = arch_scale_cpu_capacity(cpu); - capacity = mult_frac(max_capacity, throttled_freq, policy->cpuinfo.max_freq); - - /* Don't pass boost capacity to scheduler */ - if (capacity > max_capacity) - capacity = max_capacity; - - arch_set_thermal_pressure(policy->related_cpus, - max_capacity - capacity); + /* Update thermal pressure (the boost frequencies are accepted) */ + arch_update_thermal_pressure(policy->related_cpus, throttled_freq); /* * In the unlikely case policy is unregistered do not enable
Thermal pressure provides a new API, which allows to use CPU frequency as an argument. That removes the need of local conversion to capacity. Use this new API and remove old local conversion code. The new arch_update_thermal_pressure() also accepts boost frequencies, which solves issue in the driver code with wrong reduced capacity calculation. The reduced capacity was calculated wrongly due to 'policy->cpuinfo.max_freq' used as a divider. The value present there was actually the boost frequency. Thus, even a normal maximum frequency value which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id)) is not able to remove the capping. The second side effect which is solved is that the reduced frequency wasn't properly translated into the right reduced capacity, e.g. boost frequency = 3000MHz (stored in policy->cpuinfo.max_freq) max normal frequency = 2500MHz (which is 1024 capacity) 2nd highest frequency = 2000MHz (which translates to 819 capacity) Then in a scenario when the 'throttled_freq' max allowed frequency was 2000MHz the driver translated it into 682 capacity: capacity = 1024 * 2000 / 3000 = 682 Then set the pressure value bigger than actually applied by the HW: max_capacity - capacity => 1024 - 682 = 342 (<- thermal pressure) Which was causing higher throttling and misleading task scheduler about available CPU capacity. A proper calculation in such case should be: capacity = 1024 * 2000 / 2500 = 819 1024 - 819 = 205 (<- thermal pressure) This patch relies on the new arch_update_thermal_pressure() handling correctly such use case (with boost frequencies). Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- drivers/cpufreq/qcom-cpufreq-hw.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)