On 15.06.2023 16:07, Jason Andryuk wrote:
> On Thu, Jun 15, 2023 at 9:29 AM Jan Beulich <jbeul...@suse.com> 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
