Re: [PATCH v4 04/15] xen/sysctl: Nest cpufreq scaling options

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.06.2023 16:07, Jason Andryuk wrote:
> On Thu, Jun 15, 2023 at 9:29 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 14.06.2023 20:02, Jason Andryuk wrote:
>>> --- a/tools/include/xenctrl.h
>>> +++ b/tools/include/xenctrl.h
>>> @@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para {
>>>      uint32_t cpuinfo_cur_freq;
>>>      uint32_t cpuinfo_max_freq;
>>>      uint32_t cpuinfo_min_freq;
>>> -    uint32_t scaling_cur_freq;
>>> -
>>> -    char scaling_governor[CPUFREQ_NAME_LEN];
>>> -    uint32_t scaling_max_freq;
>>> -    uint32_t scaling_min_freq;
>>> -
>>> -    /* for specific governor */
>>>      union {
>>> -        xc_userspace_t userspace;
>>> -        xc_ondemand_t ondemand;
>>> +        struct {
>>> +            uint32_t scaling_cur_freq;
>>> +
>>> +            char scaling_governor[CPUFREQ_NAME_LEN];
>>> +            uint32_t scaling_max_freq;
>>> +            uint32_t scaling_min_freq;
>>> +
>>> +            /* for specific governor */
>>> +            union {
>>> +                xc_userspace_t userspace;
>>> +                xc_ondemand_t ondemand;
>>> +            } u;
>>> +        } s;
>>>      } u;
>>
>> There's no comment in the header that this needs to mirror the sysctl
>> struct. Does it really need changing?
> 
> Since this matched the other structure, I kept them in sync.  The
> cppc/hwp data needs to be represented somehow, and it gets introduced
> in the same way for both later.  If this doesn't get the new nested
> struct, then maybe fields could be placed into the single union.  That
> would grow the overall struct and have unused fields for hwp.

I guess I need to leave this to the maintainers then. Still ...

>>> --- a/tools/libs/ctrl/xc_pm.c
>>> +++ b/tools/libs/ctrl/xc_pm.c
>>> @@ -265,15 +265,10 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
>>>          user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq;
>>>          user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq;
>>>          user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq;
>>> -        user_para->scaling_cur_freq = sys_para->scaling_cur_freq;
>>> -        user_para->scaling_max_freq = sys_para->scaling_max_freq;
>>> -        user_para->scaling_min_freq = sys_para->scaling_min_freq;
>>>          user_para->turbo_enabled    = sys_para->turbo_enabled;
>>>
>>>          memcpy(user_para->scaling_driver,
>>>                  sys_para->scaling_driver, CPUFREQ_NAME_LEN);
>>> -        memcpy(user_para->scaling_governor,
>>> -                sys_para->scaling_governor, CPUFREQ_NAME_LEN);
>>
>> Did you really mean to remove the copying of these 4 entities, rather
>> than simply change the way the fields are accessed?
> 
> Yes, it was intentional.
> 
> The immediate following lines are:
>         /* copy to user_para no matter what cpufreq governor */
>         BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) !=
>              sizeof(((struct xen_get_cpufreq_para *)0)->u));
> 
>         memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u));

... this suggests that some matching is intended, yet it's not clear
to me why then the hole struct-s aren't assumed to be matching / made
match.

> And this memcpy copies all the moved entities.

Right, I should have gone to the source instead of going just from
patch context, sorry.

> I suppose the comment could change to "...no matter which cpufreq driver".

Yeah, well, it really would be driver/governor then, I guess.

Jan