Messages in this thread | | | Subject | Re: [RFT][PATCH v1] cpufreq: ACPI: Set cpuinfo.max_freq directly if max boost is known | From | Michael Larabel <> | Date | Wed, 17 Feb 2021 07:23:49 -0600 |
| |
On 2/15/21 7:49 PM, Michael Larabel wrote: > > On 2/15/21 1:24 PM, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> Commit 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover >> boost frequencies") attempted to address a performance issue involving >> acpi-cpufreq, the schedutil governor and scale-invariance on x86 by >> extending the frequency tables created by acpi-cpufreq to cover the >> entire range of "turbo" (or "boost") frequencies, but that caused >> frequencies reported via /proc/cpuinfo and the scaling_cur_freq >> attribute in sysfs to change which may confuse users and monitoring >> tools. >> >> For this reason, revert the part of commit 3c55e94c0ade adding the >> extra entry to the frequency table and use the observation that >> in principle cpuinfo.max_freq need not be equal to the maximum >> frequency listed in the frequency table for the given policy. >> >> Namely, modify cpufreq_frequency_table_cpuinfo() to allow cpufreq >> drivers to set their own cpuinfo.max_freq above that frequency and >> change acpi-cpufreq to set cpuinfo.max_freq to the maximum boost >> frequency found via CPPC. >> >> This should be sufficient to let all of the cpufreq subsystem know >> the real maximum frequency of the CPU without changing frequency >> reporting. >> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=211305 >> Fixes: 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover >> boost frequencies") >> Reported-by: Matt McDonald <gardotd426@gmail.com> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> >> Michael, Giovanni, >> >> The fix for the EPYC performance regression that was merged into 5.11 >> introduced >> an undesirable side-effect by distorting the CPU frequency reporting via >> /proc/cpuinfo and scaling_cur_freq (see the BZ link above for details). >> >> The patch below is reported to address this problem and it should >> still allow >> schedutil to achieve desirable performance, because it simply sets >> cpuinfo.max_freq without extending the frequency table of the CPU. >> >> Please test this one and let me know if it adversely affects >> performance. >> >> Thanks! > > > When carrying out tests so far today on an EPYC 7F72 2P and Ryzen 9 > 5900X with workloads seeing impact from the prior patches, everything > is looking good when comparing v5.11 to v5.11 + this patch. Not seeing > any measurable difference on either of those systems as a result of > this patch. > > Running some additional tests and on a few more boxes that should wrap > up tomorrow but at least so far the patch isn't showing any measurable > changes to performance. > > Michael >
Linux 5.11 + this patch is still looking fine on the mix of EPYC (Zen 2) and Ryzen (Zen 2/3) systems tried with the previous workloads. Not seeing any measurable change in performance from this new patch, so it's looking fine on my end.
Michael
Tested-by: Michael Larabel <Michael@phoronix.com>
> >> >> --- >> drivers/cpufreq/acpi-cpufreq.c | 62 >> ++++++++++------------------------------- >> drivers/cpufreq/freq_table.c | 8 ++++- >> 2 files changed, 23 insertions(+), 47 deletions(-) >> >> Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c >> =================================================================== >> --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c >> +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c >> @@ -54,7 +54,6 @@ struct acpi_cpufreq_data { >> unsigned int resume; >> unsigned int cpu_feature; >> unsigned int acpi_perf_cpu; >> - unsigned int first_perf_state; >> cpumask_var_t freqdomain_cpus; >> void (*cpu_freq_write)(struct acpi_pct_register *reg, u32 val); >> u32 (*cpu_freq_read)(struct acpi_pct_register *reg); >> @@ -223,10 +222,10 @@ static unsigned extract_msr(struct cpufr >> perf = to_perf_data(data); >> - cpufreq_for_each_entry(pos, policy->freq_table + >> data->first_perf_state) >> + cpufreq_for_each_entry(pos, policy->freq_table) >> if (msr == perf->states[pos->driver_data].status) >> return pos->frequency; >> - return policy->freq_table[data->first_perf_state].frequency; >> + return policy->freq_table[0].frequency; >> } >> static unsigned extract_freq(struct cpufreq_policy *policy, u32 val) >> @@ -365,7 +364,6 @@ static unsigned int get_cur_freq_on_cpu( >> struct cpufreq_policy *policy; >> unsigned int freq; >> unsigned int cached_freq; >> - unsigned int state; >> pr_debug("%s (%d)\n", __func__, cpu); >> @@ -377,11 +375,7 @@ static unsigned int get_cur_freq_on_cpu( >> if (unlikely(!data || !policy->freq_table)) >> return 0; >> - state = to_perf_data(data)->state; >> - if (state < data->first_perf_state) >> - state = data->first_perf_state; >> - >> - cached_freq = policy->freq_table[state].frequency; >> + cached_freq = >> policy->freq_table[to_perf_data(data)->state].frequency; >> freq = extract_freq(policy, get_cur_val(cpumask_of(cpu), data)); >> if (freq != cached_freq) { >> /* >> @@ -680,7 +674,6 @@ static int acpi_cpufreq_cpu_init(struct >> struct cpuinfo_x86 *c = &cpu_data(cpu); >> unsigned int valid_states = 0; >> unsigned int result = 0; >> - unsigned int state_count; >> u64 max_boost_ratio; >> unsigned int i; >> #ifdef CONFIG_SMP >> @@ -795,28 +788,8 @@ static int acpi_cpufreq_cpu_init(struct >> goto err_unreg; >> } >> - state_count = perf->state_count + 1; >> - >> - max_boost_ratio = get_max_boost_ratio(cpu); >> - if (max_boost_ratio) { >> - /* >> - * Make a room for one more entry to represent the highest >> - * available "boost" frequency. >> - */ >> - state_count++; >> - valid_states++; >> - data->first_perf_state = valid_states; >> - } else { >> - /* >> - * If the maximum "boost" frequency is unknown, ask the arch >> - * scale-invariance code to use the "nominal" performance for >> - * CPU utilization scaling so as to prevent the schedutil >> - * governor from selecting inadequate CPU frequencies. >> - */ >> - arch_set_max_freq_ratio(true); >> - } >> - >> - freq_table = kcalloc(state_count, sizeof(*freq_table), GFP_KERNEL); >> + freq_table = kcalloc(perf->state_count + 1, sizeof(*freq_table), >> + GFP_KERNEL); >> if (!freq_table) { >> result = -ENOMEM; >> goto err_unreg; >> @@ -851,27 +824,25 @@ static int acpi_cpufreq_cpu_init(struct >> } >> freq_table[valid_states].frequency = CPUFREQ_TABLE_END; >> + max_boost_ratio = get_max_boost_ratio(cpu); >> if (max_boost_ratio) { >> - unsigned int state = data->first_perf_state; >> - unsigned int freq = freq_table[state].frequency; >> + unsigned int freq = freq_table[0].frequency; >> /* >> * Because the loop above sorts the freq_table entries in the >> * descending order, freq is the maximum frequency in the >> table. >> * Assume that it corresponds to the CPPC nominal frequency >> and >> - * use it to populate the frequency field of the extra "boost" >> - * frequency entry. >> + * use it to set cpuinfo.max_freq. >> */ >> - freq_table[0].frequency = freq * max_boost_ratio >> >> SCHED_CAPACITY_SHIFT; >> + policy->cpuinfo.max_freq = freq * max_boost_ratio >> >> SCHED_CAPACITY_SHIFT; >> + } else { >> /* >> - * The purpose of the extra "boost" frequency entry is to make >> - * the rest of cpufreq aware of the real maximum frequency, but >> - * the way to request it is the same as for the >> first_perf_state >> - * entry that is expected to cover the entire range of "boost" >> - * frequencies of the CPU, so copy the driver_data value from >> - * that entry. >> + * If the maximum "boost" frequency is unknown, ask the arch >> + * scale-invariance code to use the "nominal" performance for >> + * CPU utilization scaling so as to prevent the schedutil >> + * governor from selecting inadequate CPU frequencies. >> */ >> - freq_table[0].driver_data = freq_table[state].driver_data; >> + arch_set_max_freq_ratio(true); >> } >> policy->freq_table = freq_table; >> @@ -947,8 +918,7 @@ static void acpi_cpufreq_cpu_ready(struc >> { >> struct acpi_processor_performance *perf = >> per_cpu_ptr(acpi_perf_data, >> policy->cpu); >> - struct acpi_cpufreq_data *data = policy->driver_data; >> - unsigned int freq = >> policy->freq_table[data->first_perf_state].frequency; >> + unsigned int freq = policy->freq_table[0].frequency; >> if (perf->states[0].core_frequency * 1000 != freq) >> pr_warn(FW_WARN "P-state 0 is not max freq\n"); >> Index: linux-pm/drivers/cpufreq/freq_table.c >> =================================================================== >> --- linux-pm.orig/drivers/cpufreq/freq_table.c >> +++ linux-pm/drivers/cpufreq/freq_table.c >> @@ -52,7 +52,13 @@ int cpufreq_frequency_table_cpuinfo(stru >> } >> policy->min = policy->cpuinfo.min_freq = min_freq; >> - policy->max = policy->cpuinfo.max_freq = max_freq; >> + policy->max = max_freq; >> + /* >> + * If the driver has set its own cpuinfo.max_freq above >> max_freq, leave >> + * it as is. >> + */ >> + if (policy->cpuinfo.max_freq < max_freq) >> + policy->max = policy->cpuinfo.max_freq = max_freq; >> if (policy->min == ~0) >> return -EINVAL; >> >> >>
| |