Re: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2025 05:18, Penny, Zheng wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, April 29, 2025 7:47 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>>
>> On 29.04.2025 12:36, Jan Beulich wrote:
>>> On 14.04.2025 09:40, Penny Zheng wrote:
>>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>>> @@ -71,6 +71,49 @@ unsigned int __initdata cpufreq_xen_cnt = 1;
>>>>
>>>>  static int __init cpufreq_cmdline_parse(const char *s, const char
>>>> *e);
>>>>
>>>> +static bool __init cpufreq_opts_contain(enum cpufreq_xen_opt option)
>>>> +{
>>>> +    unsigned int count = cpufreq_xen_cnt;
>>>> +
>>>> +    while ( count )
>>>> +    {
>>>> +        if ( cpufreq_xen_opts[--count] == option )
>>>> +            return true;
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>> +static int __init handle_cpufreq_cmdline(enum cpufreq_xen_opt
>>>> +option) {
>>>> +    int ret = 0;
>>>> +
>>>> +    if ( cpufreq_opts_contain(option) )
>>>> +    {
>>>> +        const char *cpufreq_opts_str[] = { "CPUFREQ_xen",
>>>> + "CPUFREQ_hwp" };
>>>
>>>         const char *const __initconstrel cpufreq_opts_str[] = {
>>> "CPUFREQ_xen", "CPUFREQ_hwp" };
>>>
>>> (line wrapped suitably, of course) Or maybe even better
>>>
>>>         const char __initconst cpufreq_opts_str[][12] = {
>>> "CPUFREQ_xen", "CPUFREQ_hwp" };
>>>
>>> for the string literals to also end up in .init.rodata.
>>
>> Actually, it didn't even occur to me that there might be a "static" missing 
>> here, too.
> 
> Sorry, I may need more explanation, what is the "static" missing you are 
> referring?

In your code cpufreq_opts_str[] is an automatic variable, which the compiler
needs to emit code for in order to instantiate it on the stack. This can be
avoided if you make the array a static variable, as then all construction
occurs at build time.

>> Plus I'm missing any arrangement for the array slots to remain in sync with 
>> the
>> corresponding enumerators. With that ...
>>
> 
> Thanks for the detailed instructions, learned and I'll change it to
>         const char __initconst cpufreq_opts_str[][4] = { "xen", "hwp" };
> And for in sync with the corresponding enumerators, maybe we shall add 
> comment here,
>         /* The order of cpufreq string literal must be in sync with  the 
> order in "enum cpufreq_xen_opt" */

Instead of a comment I has rather hoping for some use of dedicated array slot
initializers.

Jan