Re: [RFT][PATCH v1] cpufreq: ACPI: Set cpuinfo.max_freq directly if max boost is known

[Michael Larabel <Michael@phoronix.com>]

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


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