Re: [PATCH v2 05/11] xen/x86: introduce a new amd cppc driver for cpufreq scaling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.02.2025 08:40, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, February 12, 2025 12:46 AM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>>
>> On 06.02.2025 09:32, Penny Zheng wrote:
>>> +static int amd_cppc_khz_to_perf(const struct amd_cppc_drv_data *data,
>>> +unsigned int freq, uint8_t *perf)
>>
>> Overlong line again. Please sort throughout the series.
>>
>>> +{
>>> +    const struct xen_processor_cppc *cppc_data = data->cppc_data;
>>> +    uint64_t mul, div, offset = 0, res;
>>> +
>>> +    if ( freq == (cppc_data->nominal_freq * 1000) )
>>
>> There's no comment anywhere what the units of the values are. Therefore the
>> multiplication by 1000 here leaves me wondering why consistent units aren't 
>> used in
>> the first place. (From the name of the function I might guess that "freq" is 
>> in kHz,
>> and then perhaps ->{min,max,nominal}_freq are in MHz.
>> Then for the foreseeable future we're hopefully safe here wrt overflow.)
> 
> These conversion functions are designed in the first place for *ondemand* 
> governor, which
> reports performance as CPU frequencies. In generic governor->target() 
> functions, we are always
> take freq in KHz, but in CPPC ACPI spec, the frequency is read in Mhz from 
> register...

That's all fine, but it wants reflecting in our sources somehow. Perhaps
simply by either naming the variables/fields accordingly (see how we e.g.
have a cpu_khz global variable, rather than it being named e.g. cpu_freq)
or by at least adding brief comments to their declarations.

>>> +    {
>>> +        *perf = data->caps.nominal_perf;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if ( freq == (cppc_data->lowest_freq * 1000) )
>>> +    {
>>> +        *perf = data->caps.lowest_perf;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if ( (cppc_data->lowest_freq) && (cppc_data->nominal_freq) )
>>
>> Why the inner parentheses?
>>
>>> +    {
>>> +        mul = data->caps.nominal_perf - data->caps.lowest_perf;
>>> +        div = cppc_data->nominal_freq - cppc_data->lowest_freq;
>>> +        /*
>>> +         * We don't need to convert to kHz for computing offset and can
>>> +         * directly use nominal_freq and lowest_freq as the division
>>> +         * will remove the frequency unit.
>>> +         */
>>> +        div = div ?: 1;
>>> +        offset = data->caps.nominal_perf - (mul *
>>> + cppc_data->nominal_freq) / div;
>>
>> I fear I can't convince myself that the subtraction can't ever underflow.
>> With
>>
>> O = offset
>> Pn = data->caps.nominal_perf
>> Pl = data->caps.lowest_perf
>> Fn = cppc_data->nominal_freq
>> Fl = cppc_data->lowest_freq
>>
>> the above becomes
>>
>> O = Pn - ((Pn - Pl) * Fn) / (Fn - Fl)
>>
>> and your assumption is O >= 0 (and for inputs: Fn >= Fl and Pn >= Pl). That 
>> for me
>> transforms to
>>
>> (Pn - Pl) * Fn <= Pn * (Fn - Fl)
>>
>> and further
>>
>> -(Pl * Fn) <= -(Pn * Fl)
>>
>> or
>>
>> Pn * Fl <= Pl * Fn
>>
>> and I don't see why this would always hold. Yet if there can be underflow, I 
>> wonder
>> whether the calculation is actually correct. Or, ...
> 
> Because we are assuming that in normal circumstances, when F==0, P is the 
> offset value, and
> It shall be an non-smaller-than-zero value, tbh, ==0 is more logical fwit
> So if it is underflow, I might think the hardware itself is malfunctional.

Why so? The more that I continued ...

>>> +    }
>>> +    else
>>> +    {
>>> +        /* Read Processor Max Speed(mhz) as anchor point */
>>> +        mul = data->caps.highest_perf;
>>> +        div = this_cpu(max_freq_mhz);
>>> +        if ( !div )
>>> +            return -EINVAL;
>>> +    }
>>> +
>>> +    res = offset + (mul * freq) / (div * 1000);
>>
>> ... considering that a negative offset here isn't really an issue, as long 
>> as the rhs of
>> the addition is large enough, is offset perhaps meant to be a signed 
>> quantity (and
>> considering it's in principle an [abstract] perf value, it doesn't even need 
>> to be a 64-
>> bit one, i.e. perhaps one of the cases where plain int is appropriate to 
>> use)?

... my explanation here, including the outline of an approach to deal with
this.

Jan