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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Jun 15, 2023 at 10:29 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

The tools version replaces struct xen_$foo with xc_$foo typedefs.
Maybe it's just to enforce the typedefs?

Regards,
Jason