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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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.

> > --- 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));

And this memcpy copies all the moved entities.

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

Regards,
Jason