On 18.02.2025 08:40, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Wednesday, February 12, 2025 12:46 AM
>> To: Penny, Zheng <penny.zh...@amd.com>
>>
>> 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
